[RFC] edtlib: enhance for pin controller, clock controller, partitions, special nodes #22255

b0661 · 2020-01-28T07:18:42Z

A set of edtlib.py enhancements that proved useful to implement an OOT pin controller driver, clock controller driver, flash driver, gpio leds driver (using code generation).

This makes edtlib more complete. It should also simplify defines generation in gen_defines.py, especially for pin controllers and clock controllers.

Comments welcome.

dts: edtlib: enable bindings for special nodes without compatibles

Associate a default compatible to special nodes that by specification
do not have or can not have a compatible.
The default compatible allows to use bindings for nodes that otherwise
can not be controlled by a binding.

Default compatible for special DTS nodes:
  - '/aliases' node                       : 'aliases'
  - '/chosen' node                        : 'chosen'
  - pin controller pinctrl state node     : 'pinctrl-state'
  - pin controller pin configuration node : 'pincfg'
  - fixed partition node                    : 'fixed-partition'
  - gpio led node                              : 'gpio-led'

Default compatibles are only associated if no compatible is given.

dts: edtlib: allow binding override by binding file name

Allow to override a binding by file name. Take the first file
given by the binding directories provided to edtlib.

This allows applications and out of tree drivers to amend
a binding already given by Zephyr.

Warn in case there are duplicate binding file names.

dts: edtlib: allow extraction of gpio-ranges properties

Handling of 'gpio-ranges' is a must to implement pin controllers.

gpio-ranges properties expect a phandle value list.
Extend _standard_phandle_val_list() to also work on gpio-ranges.

dts: edtlib: extract pinctrl state and pin configuration nodes

Extract the pin controller child nodes for pinctrl state and pin
configuration.

Extract gpio ranges controlled by the pin controller.

Provide a PinCfg object that represents the pin configuration
data for easy access.

Extend the Node class with properties for:

  pinctrl_states:
    A list with the Node instances for the 'pinctrl-state' children of
    a pin controller node. The list is empty if the node does not have any
    pinctrl state children.

  pincfgs:
    A list of PinCfg objects for the 'pinctrl-state' node. The list is
    empty if the node is not a 'pinctrl-state' node.

  pinctrl_gpio_ranges:
    A list of ControllerAndData objects of the gpio ranges a pin controller
    controls. The list is empty if the node is not a pin controller or no
    'gpio-ranges' are defined by the gpio nodes.

  pin_controller:
    The pin controller for the node. Only meaningful for nodes representing
    pinctrl states.

dts: edtlib: extract clock inputs and clock outputs

Extract clock inputs ('clocks') and clock outputs ('clock-output-names')
properties.

Extend the Node class with properties for:

  clocks:
    A list of ControllerAndData objects for the clock inputs on the
    node, sorted by index. The list is empty if the node does not have a
    clocks property.

  clock_outputs:
    A list of ControllerAndData objects for the clock outputs on the
    node, sorted by index. The list is empty if the node does not have a
    \#clock-cells property.

dts: edtlib: extract partitions nodes

Extract the partitions child node of flash nodes.

Extend the Node class with properties for:

  partitions:
    A list of Partition objects of the partitions of the 'partitions'
    node of a flash. Only meaningful for nodes representing a flash -
    otherwise an empty list.

dts: edtlib: extract gpio led nodes

Extract the gpio led child node of gpio leds controller nodes.

Extend the Node class with properties for:

  gpio_leds:
    A list of ControllerAndData objects of the leds a gpio leds
    controller controls. The list is empty if the node is not a
    gpio leds controller or it does not have gpio led children.

dts: edtlib: issue same warning only once

Repeating the same warning is just an annoyance.

Issue a warning only on first occurence. Suppress all following
warnings having the identical warning text.

zephyrbot · 2020-01-28T07:21:13Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:1489: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "in-dir" appears un-documented -- check ./dts/bindings/
#1489: FILE: scripts/dts/test-multidir.dts:14:
+		compatible = "in-dir";

-:1493: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "in-dir" appears un-documented -- check ./dts/bindings/
#1493: FILE: scripts/dts/test-multidir.dts:17:
+		compatible = "in-dir";

-:1526: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "clock-provider" appears un-documented -- check ./dts/bindings/
#1526: FILE: scripts/dts/test.dts:26:
+			compatible = "clock-provider";

-:1531: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "clock-consumer" appears un-documented -- check ./dts/bindings/
#1531: FILE: scripts/dts/test.dts:31:
+			compatible = "clock-consumer";

-:1562: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "gpio-two-cell" appears un-documented -- check ./dts/bindings/
#1562: FILE: scripts/dts/test.dts:62:
+			compatible = "gpio-two-cell";

-:1629: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "pinctrl-gpio" appears un-documented -- check ./dts/bindings/
#1629: FILE: scripts/dts/test.dts:304:
+				compatible = "pinctrl-gpio";

-:1631: WARNING:LONG_LINE: line over 80 characters
#1631: FILE: scripts/dts/test.dts:306:
+				gpio-ranges = <&{/pinctrl/pincontroller} 0 0 16>;

-:1635: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "pinctrl-gpio" appears un-documented -- check ./dts/bindings/
#1635: FILE: scripts/dts/test.dts:310:
+				compatible = "pinctrl-gpio";

-:1637: WARNING:LONG_LINE: line over 80 characters
#1637: FILE: scripts/dts/test.dts:312:
+				gpio-ranges = <&{/pinctrl/pincontroller} 0 16 16>;

- total: 0 errors, 9 warnings, 1707 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

ulfalizer · 2020-01-28T07:22:00Z

scripts/dts/edtlib.py

Quick look, but this function looks pretty long and hard to follow.

Could you try splitting it up into some smaller functions, so that each functions is like 30 lines maximum? Maybe some common stuff could be factored out at the same time.

Split. Did not reach 30 lines. Where do get this limit from?

Just some number I pulled out that feels like the right ballpark. Short and focused functions with few parameters are considered a good practice at least. See e.g. the Linux kernel coding style.

Alright to have it a bit longer if it's just a big switch or whatever with only one or two levels of indent, because then it's more like many small functions.

Good idea to go through PEP 8 once too. Check this section for how to format continuation lines (e.g. for function parameters).

Strive for having few parameters too, and for all combinations of parameters to make sense (i.e., no dependencies between parameters, if it can be avoided).

Might be a bit busy next week, but will look more at the logic.

Not saying I'm perfect re. that stuff. Think it might be possible to shorten some of this stuff though, and have some simple helpers with just a few parameters.

ulfalizer · 2020-01-28T07:23:15Z

scripts/dts/edtlib.py

Ditto here. See if it can be split up a bit.

Split. Did not reach 30 lines.

ulfalizer · 2020-01-28T07:24:25Z

Will need to add tests in testedtlib.py too.

ulfalizer · 2020-01-28T07:38:58Z

scripts/dts/edtlib.py

Could make it a bit easier to read like this:

if "clock-output-names" in node.props: clock_output_names = node.props["clock-output-names"].to_strings() else: clock_output_names = []

I try to avoid the "set a variable to a value that might be wrong, do a check, and then overwrite it with the right value" pattern. Gets confusing when scanning for where a variable is set.

Changed programming style to be in line.

Instead of catching DeviceTreeCheck exceptions from test(e)dtlib.py all the way up in main(), catch them in the DeviceTreeCheck test itself, and report them with ComplianceTest.error(). That way, other tests (e.g. pylint) can still run and report to GitHub when there are internal errors in test(e)dtlib.py. Motivated by zephyrproject-rtos/zephyr#22255. Signed-off-by: Ulf Magnusson <[email protected]>

ulfalizer · 2020-01-28T08:06:40Z

Improved the CI script so that it still runs other tests when there's an internal error in test(e)dtlib.py: zephyrproject-rtos/ci-tools#127. Sorry about that.

There's these issues reported by pylint in the meantime (would normally get reported automatically by CI):

ERROR   : Test pylint failed: ************* Module edtlib
scripts/dts/edtlib.py:1571:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1572:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1574:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1575:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1576:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1577:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1578:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1580:0: W0311: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
scripts/dts/edtlib.py:1581:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1582:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1584:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1585:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1586:0: W0311: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
scripts/dts/edtlib.py:1588:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1589:0: W0311: Bad indentation. Found 24 spaces, expected 20 (bad-indentation)
scripts/dts/edtlib.py:1591:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1592:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1594:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1595:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1596:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1600:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1601:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1602:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1603:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1604:0: W0311: Bad indentation. Found 20 spaces, expected 16 (bad-indentation)
scripts/dts/edtlib.py:1606:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1608:0: W0311: Bad indentation. Found 16 spaces, expected 12 (bad-indentation)
scripts/dts/edtlib.py:1362:11: C0113: Consider changing "not 'partitions' in self.children" to "'partitions' not in self.children" (unneeded-not)
scripts/dts/edtlib.py:2547:8: W0612: Unused variable 'partition_name' (unused-variable)

Can run pylint manually like this after cloning down the ci-tools repo.

$ pylint --rcfile=ci-tools/scripts/pylintrc scripts/dts/edtlib.py

ulfalizer · 2020-01-28T08:56:29Z

scripts/dts/edtlib.py

I think we should avoid magic hardcoded strings like these that "piggyback" on dictionaries used for other stuff. Better to use regular attributes.

Maybe a new class could be added.

New class 'Partition' used.

ulfalizer · 2020-01-28T08:57:24Z

scripts/dts/edtlib.py

Better to have this as part of the EDT class, since it's possible to have multiple EDT instances. The library itself has no global state.

Moved to EDT. Made some collateral moves necessary as _warn now needs EDT.

Instead of catching DeviceTreeCheck exceptions from test(e)dtlib.py all the way up in main(), catch them in the DeviceTreeCheck test itself, and report them with ComplianceTest.error(). That way, other tests (e.g. pylint) can still run and report to GitHub when there are internal errors in test(e)dtlib.py. Motivated by zephyrproject-rtos/zephyr#22255. Signed-off-by: Ulf Magnusson <[email protected]>

Extract the gpio led child node of gpio leds controller nodes. Extend the Node class with properties for: gpio_leds: A list of ControllerAndData objects of the leds a gpio leds controller controls. The list is empty if the node is not a gpio leds controller or it does not have gpio led children. Signed-off-by: Bobby Noelte <[email protected]>

Tests for edtlib features: - enable bindings for special nodes without compatibles - allow binding override by binding file name - allow extraction of gpio-ranges properties - extract pinctrl state and pin configuration nodes - extract clock inputs and clock outputs - extract partitions nodes - extract gpio led nodes Signed-off-by: Bobby Noelte <[email protected]>

b0661 · 2020-09-16T05:17:32Z

Stale too long

b0661 requested review from galak and ulfalizer as code owners January 28, 2020 07:18

ulfalizer reviewed Jan 28, 2020

View reviewed changes

ulfalizer mentioned this pull request Jan 28, 2020

check_compliance.py: Don't error out for internal test(e)dtlib errors zephyrproject-rtos/ci-tools#127

Merged

ulfalizer reviewed Jan 28, 2020

View reviewed changes

b0661 mentioned this pull request Jan 29, 2020

[RFC] DT: emit ordinals #22292

Closed

galak added the area: Devicetree label Jan 29, 2020

b0661 force-pushed the pr_edtlib_enhance branch from fcb3fa5 to a8dd73a Compare January 29, 2020 20:20

b0661 requested review from MaureenHelm, SebastianBoe, alexanderwachter, andrewboie, carlescufi, dcpleung, erwango, ldts, mnkp, nashif, pabigot, rsalveti, superna9999 and tbursztyka as code owners January 29, 2020 20:20

b0661 added 2 commits February 22, 2020 08:56

b0661 force-pushed the pr_edtlib_enhance branch from 4ec1d4e to c761f92 Compare February 22, 2020 08:06

This was referenced Mar 19, 2020

dts: stm32: Introduce linux like pinctrl bindings #20386

Closed

dts: stm32: Introduce linux like pinctrl DT bindings #23592

Closed

b0661 mentioned this pull request Apr 30, 2020

devicetree: Update generation/API #24769

Closed

github-actions bot added area: Devicetree has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020

galak removed the area: Device Tree label Jul 1, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020

b0661 mentioned this pull request Sep 2, 2020

edtlib: support inferring binding from node content #27937

Merged

nashif added the Stale label Sep 4, 2020

b0661 closed this Sep 16, 2020

[RFC] edtlib: enhance for pin controller, clock controller, partitions, special nodes #22255

[RFC] edtlib: enhance for pin controller, clock controller, partitions, special nodes #22255

Uh oh!

Conversation

b0661 commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dts: edtlib: enable bindings for special nodes without compatibles

dts: edtlib: allow binding override by binding file name

dts: edtlib: allow extraction of gpio-ranges properties

dts: edtlib: extract pinctrl state and pin configuration nodes

dts: edtlib: extract clock inputs and clock outputs

dts: edtlib: extract partitions nodes

dts: edtlib: extract gpio led nodes

dts: edtlib: issue same warning only once

Uh oh!

zephyrbot commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

checkpatch (informational only, not a failure)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer commented Jan 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b0661 commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

b0661 commented Jan 28, 2020 •

edited

Loading

zephyrbot commented Jan 28, 2020 •

edited

Loading

ulfalizer Feb 1, 2020 •

edited

Loading

ulfalizer commented Jan 28, 2020 •

edited

Loading