ST DTS lint #90611
ST DTS lint #90611
Conversation
github-actions
bot
requested review from
djiatsaf-st,
erwango,
etienne-lms,
FRASTM,
gautierg-st,
GeorgeCGV,
HoZHel,
marwaiehm-st and
mathieuchopstm
| @@ -117,6 +117,7 @@ arduino_i2c: &i2c1 {}; | |||
| &spi1_miso_pa6 &spi1_mosi_pa7>; | |||
| pinctrl-names = "default"; | |||
| status = "okay"; | |||
|
|
|||
There was a problem hiding this comment.
from the specs, I guess we should rather have:
pinctrl-names = "default";
+ cs-gpios = <&gpioa 15 GPIO_ACTIVE_LOW>;
status = "okay";
- cs-gpios = <&gpioa 15 GPIO_ACTIVE_LOW>;
lora: lora@0 {There was a problem hiding this comment.
This exists in a bunch of places. I think it falls into the category of splitting into paragraphs for readabillity. It's nice to have the cs-gpios right before the spi device sub-nodes because they go together logically
There was a problem hiding this comment.
I must say I don't understand why you don't want to change this while still change other status properties placement.
(2 other occurrences).
There was a problem hiding this comment.
Did I fix all the inconsistancies?
|
@etienne-lms All good? |
There was a problem hiding this comment.
Aside the 3 comments below, the changes here are consistent.
(edited) the 3 comments are here, here and here.
@erwango, what do you think about such changes?
The DTS spec talks about a "preferred" order that is addressed in these changes (in https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node). It's not a strong recommendation.
The following order of properties in device nodes is preferred:
- “compatible”
- “reg”
- “ranges”
- Standard/common properties (defined by common bindings, e.g. without vendor-prefixes)
- Vendor-specific properties
- “status” (if applicable)
- Child nodes, where each node is preceded with a blank line
There are a few changes in this P-R that I'm fully in favor of: rules 7. (sub nodes always after properties). As for the other, I'm not sure. That said, I must admit that always expecting the status property at this end is simpler, to know whether it's present or not, especially in case there are a lot of properties.
| height = <320>; | ||
| pixel-format = <PANEL_PIXEL_FORMAT_RGB_565>; | ||
| width = <240>; |
There was a problem hiding this comment.
width is better placed next to height.
(several occurrences)
There was a problem hiding this comment.
I agree but how would you arrange it? Separate block?
There was a problem hiding this comment.
def-back-color-green = <0xFF>;
def-back-color-blue = <0xFF>;
display-controller = <&ili9341>;
ext-sdram = <&sdram2>;
height = <320>;
+ width = <240>;
pixel-format = <PANEL_PIXEL_FORMAT_RGB_565>;
- width = <240>;
display-timings {There was a problem hiding this comment.
A bit weird but all right...
| @@ -117,6 +117,7 @@ arduino_i2c: &i2c1 {}; | |||
| &spi1_miso_pa6 &spi1_mosi_pa7>; | |||
| pinctrl-names = "default"; | |||
| status = "okay"; | |||
|
|
|||
There was a problem hiding this comment.
I must say I don't understand why you don't want to change this while still change other status properties placement.
(2 other occurrences).
|
|
||
| mx25lm51245: ospi-nor-flash@90000000 { | ||
| status = "okay"; |
There was a problem hiding this comment.
Equivalent change in boards/st/stm32l562e_dk/stm32l562e_dk_common.dtsi is missing.
There was a problem hiding this comment.
Missed it. Will fix
According to the guidelines the `reg` property should always come before any other property other than `compatible`. Link: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html Signed-off-by: Yishai Jaffe <[email protected]>
According to the guidelines the `status` property should always be the last property listed. Link: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html Signed-off-by: Yishai Jaffe <[email protected]>
|
There was a problem hiding this comment.
Thanks for the effort and care provided to these files.
This being said, I'm a bit concerned by the spread of the change and:
- the probability it will create churn to anyone that has PR/branch/patch on those files
- the fact that this change is only a one time thing and will be hard to maintain (one going and next PR should be reviewed with the same requirements) and not enforced by linting script/checker, so won't be likely enforced in future
- applied only on part of the files (st boards), so most probably, contamination from other areas will happen some day.
For all these reasons, I'd suggest to drop 2 parts to minimize the spread of the change:
- status moves
- label moves
I hope you understand my concern.
yishai1999
added
area: Devicetree
area: Coding Guidelines
Fixed two issues according to the device tree guidelines https://docs.kernel.org/devicetree/bindings/dts-coding-style.html:
regproperty should be second only to thecompatibleproperty within a node. This was not the case in many partition nodes definitions.statusproperty should be the last property in a node.