| Message ID | 20231125184422.12315-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] docs: dt-bindings: add DTS Coding Style document | expand |
On 25.11.2023 19:44, Krzysztof Kozlowski wrote: > Document preferred coding style for Devicetree sources (DTS and DTSI), > to bring consistency among all (sub)architectures and ease in reviews. > > Cc: Andrew Davis <afd@ti.com> > cc: Andrew Lunn <andrew@lunn.ch> > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Chen-Yu Tsai <wens@kernel.org> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: Michal Simek <michal.simek@amd.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Nishanth Menon <nm@ti.com> > Cc: Olof Johansson <olof@lixom.net> > Cc: Rafał Miłecki <zajec5@gmail.com> > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > Acked-by: Heiko Stuebner <heiko@sntech.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- There's still a couple of comments being resolved, but even as-is, I agree with the contents and want to thank you for doing this. Acked-by: Konrad Dybcio <konradybcio@kernel.org> Konrad
On 25/11/2023 20:33, Jonathan Corbet wrote: > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > >> Document preferred coding style for Devicetree sources (DTS and DTSI), >> to bring consistency among all (sub)architectures and ease in reviews. > > One little nit: > >> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst >> new file mode 100644 >> index 000000000000..e374bec0f555 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst >> @@ -0,0 +1,194 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> +.. _dtscodingstyle: > > There is no need to put a label at the top of a document like that, I'd > just take it out. OK Best regards, Krzysztof
On 25/11/2023 20:47, Andrew Lunn wrote: >> +===================================== >> +Devicetree Sources (DTS) Coding Style >> +===================================== >> + >> +When writing Devicetree Sources (DTS) please observe below guidelines. They >> +should be considered complementary to any rules expressed already in Devicetree >> +Specification and dtc compiler (including W=1 and W=2 builds). >> + >> +Individual architectures and sub-architectures can add additional rules, making >> +the style stricter. > > It would be nice to add a pointer where such rules are documented. Subsystem profile or any other place. The generic doc should not point to specific ones. > >> +Naming and Valid Characters >> +--------------------------- >> + >> +Devicetree specification allows broader range of characters in node and >> +property names, but for code readability the choice shall be narrowed. >> + >> +1. Node and property names are allowed to use only: >> + >> + * lowercase characters: [a-z] >> + * digits: [0-9] >> + * dash: - >> + >> +2. Labels are allowed to use only: >> + >> + * lowercase characters: [a-z] >> + * digits: [0-9] >> + * underscore: _ >> + >> +3. Unit addresses shall use lowercase hex, without leading zeros (padding). >> + >> +4. Hex values in properties, e.g. "reg", shall use lowercase hex. The address >> + part can be padded with leading zeros. >> + >> +Example:: >> + >> + gpi_dma2: dma-controller@800000 { > > Not the best of example. Upper case 8 does not exist, as far as i > known. Sure, this was taken from DTS, but I can bring here some fake address to illustrate :) > >> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma"; >> + reg = <0x0 0x00800000 0x0 0x60000>; > > Maybe introduce some [a-f] in the example reg? > >> +Order of Nodes >> +-------------- >> + >> +1. Nodes within any bus, thus using unit addresses for children, shall be >> + ordered incrementally by unit address. >> + Alternatively for some sub-architectures, nodes of the same type can be >> + grouped together (e.g. all I2C controllers one after another even if this >> + breaks unit address ordering). >> + >> +2. Nodes without unit addresses shall be ordered alpha-numerically by the node >> + name. For a few types of nodes, they can be ordered by the main property >> + (e.g. pin configuration states ordered by value of "pins" property). >> + >> +3. When extending nodes in the board DTS via &label, the entries shall be >> + ordered either alpha-numerically or by keeping the order from DTSI (choice >> + depending on sub-architecture). > > Are these sub-architecture choices documented somewhere? Can you > include a hint which they are? This is a generic document, so it does not point to all possible variations per each architecture or subarch. Just like Linux Coding style does not cover all differences between subsystems. > >> +Example:: >> + >> + /* SoC DTSI */ >> + >> + / { > > Dumb question. Does this open { indicate the start of a bus? > >> + cpus { >> + /* ... */ >> + }; >> + >> + psci { >> + /* ... */ >> + }; > > If that does indicate a bus, the nodes above are ordered > alpha-numerically, according to 2). They are ordered. c is before p. p is before s. > >> + >> + soc@ { > > This has a unit address, even if its missing, so should be sorted by > 1). And it is sorted... > > Should there be something in the coding style that 2) comes before 1) > on the bus? And if that is true, don't you think it would make sense > to swap 1) and 2) in the description above? The root node is a bit special, but other than that mixing nodes with and without unit address is discouraged practice. > >> + dma: dma-controller@10000 { >> + /* ... */ >> + }; >> + >> + clk: clock-controller@80000 { >> + /* ... */ >> + }; >> + }; >> + }; >> + >> + /* Board DTS - alphabetical order */ >> + >> + &clk { >> + /* ... */ >> + }; >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + /* Board DTS - alternative order, keep as DTSI */ >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + &clk { >> + /* ... */ >> + }; > > Do you imaging there will ever be a checkpatch for DT files? The > second alternative seems pretty difficult to check for with tools. You Rob pointed out that it is possible. > need to include all the .dtsi files to determine the ordered tree, > then flatten it to get the properties order. Should we discourage this > alternative? Please respond to Rob in v2 in such case. > >> +Indentation >> +----------- >> + >> +1. Use indentation according to :ref:`codingstyle`. >> +2. For arrays spanning across lines, it is preferred to align the continued >> + entries with opening < from the first line. >> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses) >> + shall be enclosed in <>. >> + >> +Example:: >> + >> + thermal-sensor@c271000 { >> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; >> + reg = <0x0 0x0c271000 0x0 0x1000>, >> + <0x0 0x0c222000 0x0 0x1000>; >> + }; > > I'm not sure i understand this. Is this example correct? > > gpio-fan,speed-map = <0 0 > 3000 1 > 6000 2>; > > It exists a lot in todays files. Depends on the binidng. Is it matrix? If yes, then it is not correct. > > >> +The DTSI and DTS files shall be organized in a way representing the common >> +(and re-usable) parts of the hardware. Typically this means organizing DTSI >> +and DTS files into several files: >> + >> +1. DTSI with contents of the entire SoC (without nodes for hardware not present >> + on the SoC). > > Maybe point out that SoC DTSI files can by hierarchical when there is > a family of SoCs. You often have one .DTSI file for all the common > parts of a family. And then each member of the family has a .dtsi file > which includes the core, and then adds properties for that member of > the family. It's not really a coding style issue. We are going way to deep how people should organize their source code. The only thing here I care is to properly differentiate between SoC, SoM and board parts. Best regards, Krzysztof
On Sun, Nov 26, 2023 at 11:38:38AM +0100, Krzysztof Kozlowski wrote: > On 25/11/2023 20:47, Andrew Lunn wrote: > >> +===================================== > >> +Devicetree Sources (DTS) Coding Style > >> +===================================== > >> + > >> +When writing Devicetree Sources (DTS) please observe below guidelines. They > >> +should be considered complementary to any rules expressed already in Devicetree > >> +Specification and dtc compiler (including W=1 and W=2 builds). > >> + > >> +Individual architectures and sub-architectures can add additional rules, making > >> +the style stricter. > > > > It would be nice to add a pointer where such rules are documented. > > Subsystem profile or any other place. The generic doc should not point > to specific ones. That is not so friendly for a developer. A reviewer points out that a file is not consistent with the coding style. So they go away and fix it, as described here. They then get a second review which say, no, you to do X, Y and Z, despite them actually following the coding style. Maybe add to the paragraph above: These further restrictions are voluntary, until added to this document. This should encourage those architectures to document their coding style. > The root node is a bit special, but other than that mixing nodes with > and without unit address is discouraged practice. If the root node is special, maybe it needs a few rules of its own? All properties without an address come first, then properties with addresses. Sorting within these classes follow the normal rules already stated? > >> +Indentation > >> +----------- > >> + > >> +1. Use indentation according to :ref:`codingstyle`. > >> +2. For arrays spanning across lines, it is preferred to align the continued > >> + entries with opening < from the first line. > >> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses) > >> + shall be enclosed in <>. > >> + > >> +Example:: > >> + > >> + thermal-sensor@c271000 { > >> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; > >> + reg = <0x0 0x0c271000 0x0 0x1000>, > >> + <0x0 0x0c222000 0x0 0x1000>; > >> + }; > > > > I'm not sure i understand this. Is this example correct? > > > > gpio-fan,speed-map = <0 0 > > 3000 1 > > 6000 2>; > > > > It exists a lot in todays files. > > Depends on the binidng. Is it matrix? If yes, then it is not correct. It seems to me, rules 2 and 3 should be swapped. You can only align the <, if you have <. So logically, the rule about having < should come first. Andrew
Hi Krzysztof, On Sat, Nov 25, 2023 at 7:44 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > Document preferred coding style for Devicetree sources (DTS and DTSI), > to bring consistency among all (sub)architectures and ease in reviews. > > Cc: Andrew Davis <afd@ti.com> > cc: Andrew Lunn <andrew@lunn.ch> > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Chen-Yu Tsai <wens@kernel.org> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: Michal Simek <michal.simek@amd.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Nishanth Menon <nm@ti.com> > Cc: Olof Johansson <olof@lixom.net> > Cc: Rafał Miłecki <zajec5@gmail.com> > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > Acked-by: Heiko Stuebner <heiko@sntech.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Merging idea: Rob/DT bindings > > Changes in v3 > ============= > 1. should->shall (Angelo) > 2. Comments // -> /* (Angelo, Michal) > 3. Use imaginary example in "Order of Properties in Device Node" > (Angelo) > 4. Added paragraphs for three sections with justifications of chosen > style. > 5. Allow two style of ordering overrides in board DTS: alphabetically or > by order of DTSI (Rob). > 6. I did not incorporate feedback about, due to lack of consensus and my > disagreement: > a. SoM being DTS without DTSI in "Organizing DTSI and DTS" Thanks for the update! > --- /dev/null > +++ b/Documentation/devicetree/bindings/dts-coding-style.rst > + /* SoC DTSI */ > + > + / { > + cpus { > + /* ... */ > + }; > + > + psci { > + /* ... */ > + }; > + > + soc@ { "soc@" is invalid, that should be "soc". As the "soc" node is special, you may want to elaborate: compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; ranges; > + dma: dma-controller@10000 { > + /* ... */ > + }; > + > + clk: clock-controller@80000 { > + /* ... */ > + }; > + }; > + }; > + > + /* Board DTS - alphabetical order */ > + > + &clk { > + /* ... */ > + }; > + > + &dma { > + /* ... */ > + }; > + > + /* Board DTS - alternative order, keep as DTSI */ > + > + &dma { > + /* ... */ > + }; > + > + &clk { > + /* ... */ > + }; IMO that alternative order is hard to review: you need to have multiple files open. It will also make validation hard, as you can only validate the end result, not individual files. Anyway, this is already quite usable so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Sun, Nov 26, 2023 at 04:53:40PM +0200, Laurent Pinchart wrote: > Hi Krzysztof, > > On Sun, Nov 26, 2023 at 11:32:17AM +0100, Krzysztof Kozlowski wrote: > > On 25/11/2023 20:37, Laurent Pinchart wrote: > > > On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: > > >> Document preferred coding style for Devicetree sources (DTS and DTSI), > > >> to bring consistency among all (sub)architectures and ease in reviews. > > >> + > > >> +Naming and Valid Characters > > >> +--------------------------- > > >> + > > >> +Devicetree specification allows broader range of characters in node and > > > > > > s/Devicetree specification/The Devicetree specification/ > > > s/broader range/a broad range/ > > > > Ack, but I really expect people finish with grammar and style fixes at > > v3. Please point the language things now or just let it go. > > v3 is the first version that ended up in my inbox. I haven't noticed any > other spelling or grammar error in this patch, but I can't guarantee I > won't see any in new text that gets introduced in a future version :-) > > > >> +property names, but for code readability the choice shall be narrowed. > > >> + > > >> +1. Node and property names are allowed to use only: > > >> + > > >> + * lowercase characters: [a-z] > > >> + * digits: [0-9] > > >> + * dash: - > > >> + > > >> +2. Labels are allowed to use only: > > >> + > > >> + * lowercase characters: [a-z] > > >> + * digits: [0-9] > > >> + * underscore: _ > > >> + > > >> +3. Unit addresses shall use lowercase hex, without leading zeros (padding). Unless a bus defines differently, unit addresses shall ... > > > > > > I'm curious, what's the reason for this ? I think it makes the sources > > > less readable. If the rule is "just" because that's how DT sources are > > > written today and it would be too complicated to change that, that's > > > fine with me. > > > > Warning (unit_address_format): /cpus/cpu@0100: unit name should not have > > leading 0s > > > > And we fixed all or most of DTS some time ago. > > It's the current practice in DT sources and I understand it won't get > changed, but I was more curious about the rationale behind that. I put the dtc check in, but the rational predates me. The OF PCI bus supplement from the 1990s defines that. My only rationale to check it was to be consistent. We don't check "the unit-address is lowercase hex" because technically the bus defines the format. Leading 0s was as much as I could get David G to agree to check without regard to bus type. Rob
Hello Krzysztof, On 2023-11-25 19:44, Krzysztof Kozlowski wrote: > Document preferred coding style for Devicetree sources (DTS and DTSI), > to bring consistency among all (sub)architectures and ease in reviews. > > Cc: Andrew Davis <afd@ti.com> > cc: Andrew Lunn <andrew@lunn.ch> > Cc: AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Chen-Yu Tsai <wens@kernel.org> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > Cc: Michal Simek <michal.simek@amd.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Nishanth Menon <nm@ti.com> > Cc: Olof Johansson <olof@lixom.net> > Cc: Rafał Miłecki <zajec5@gmail.com> > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > Acked-by: Heiko Stuebner <heiko@sntech.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Merging idea: Rob/DT bindings > > Changes in v3 > ============= > 1. should->shall (Angelo) > 2. Comments // -> /* (Angelo, Michal) > 3. Use imaginary example in "Order of Properties in Device Node" > (Angelo) > 4. Added paragraphs for three sections with justifications of chosen > style. > 5. Allow two style of ordering overrides in board DTS: alphabetically > or > by order of DTSI (Rob). > 6. I did not incorporate feedback about, due to lack of consensus and > my > disagreement: > a. SoM being DTS without DTSI in "Organizing DTSI and DTS" I went through the language of the entire patch, after the notice that the v4 would no longer accept language improvements. My wording- and grammar-related suggestions are available inline below. > Changes in v2 > ============= > 1. Hopefully incorporate entire feedback from comments: > a. Fix \ { => / { (Rob) > b. Name: dts-coding-style (Rob) > c. Exceptions for ordering nodes by name for Renesas and pinctrl > (Geert, > Konrad) > d. Ordering properties by common/vendor (Rob) > e. Array entries in <> (Rob) > > 2. New chapter: Organizing DTSI and DTS > > 3. Several grammar fixes (missing articles) > > Cc: linux-rockchip@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-amlogic@lists.infradead.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-arm-msm@vger.kernel.org > Cc: workflows@vger.kernel.org > Cc: linux-doc@vger.kernel.org > --- > .../devicetree/bindings/dts-coding-style.rst | 194 ++++++++++++++++++ > Documentation/devicetree/bindings/index.rst | 1 + > 2 files changed, 195 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/dts-coding-style.rst > > diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst > b/Documentation/devicetree/bindings/dts-coding-style.rst > new file mode 100644 > index 000000000000..e374bec0f555 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dts-coding-style.rst > @@ -0,0 +1,194 @@ > +.. SPDX-License-Identifier: GPL-2.0 > +.. _dtscodingstyle: > + > +===================================== > +Devicetree Sources (DTS) Coding Style > +===================================== > + > +When writing Devicetree Sources (DTS) please observe below guidelines. > They The sentence above should be replaced with: "The following guidelines are to be followed when writing Devicetree Source (DTS) files." > +should be considered complementary to any rules expressed already in > Devicetree > +Specification and dtc compiler (including W=1 and W=2 builds). A definite article ("the") should be added before "Devicetree Specification" and "dtc". Also, "Specification" in "Devicetree Specification" should be capitalized. > + > +Individual architectures and sub-architectures can add additional > rules, making > +the style stricter. "Sub-architectures" should be replaced with "subarchitectures". "Can add" should be replaced with "can define". "Style" should be replaced with "coding style". > + > +Naming and Valid Characters > +--------------------------- > + > +Devicetree specification allows broader range of characters in node > and > +property names, but for code readability the choice shall be narrowed. The definite article ("the") should be added before "Devicetree Specification", and "specification" should be capitalised. As already suggested, "broader range" should be replaced with "a broad range". "But for code readability the choice shall be narrowed" should be replaced with "but this coding style narrows the range down to achieve better code readability". > + > +1. Node and property names are allowed to use only: "Are allowed to" should be replaced with "can". After "only", "the following characters" should be added. > + > + * lowercase characters: [a-z] > + * digits: [0-9] > + * dash: - List items should be capitalized, i.e. "Lowercase characters" should be used instead of "lowercase characters", etc. > + > +2. Labels are allowed to use only: "Are allowed to" should be replaced with "can". After "only", "the following characters" should be added. > + > + * lowercase characters: [a-z] > + * digits: [0-9] > + * underscore: _ List items should be capitalized, i.e. "Lowercase characters" should be used instead of "lowercase characters", etc. > + > +3. Unit addresses shall use lowercase hex, without leading zeros > (padding). "Lowercase hex" should be replaced with "lowercase hexadecimal digits". > + > +4. Hex values in properties, e.g. "reg", shall use lowercase hex. The > address > + part can be padded with leading zeros. "Hex values" should be replaced with "Hexadecimal values". "Lowercase hex" should be replaced with "lowercase hexadecimal digits". > + > +Example:: > + > + gpi_dma2: dma-controller@800000 { > + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma"; > + reg = <0x0 0x00800000 0x0 0x60000>; > + } > + > +Order of Nodes > +-------------- > + > +1. Nodes within any bus, thus using unit addresses for children, shall > be "Within" should be replaced "on". > + ordered incrementally by unit address. Should be replaced with "ordered by unit address in ascending order". > + Alternatively for some sub-architectures, nodes of the same type > can be > + grouped together (e.g. all I2C controllers one after another even > if this > + breaks unit address ordering). "Sub-architectures" should be replaced with "subarchitectures". The "(e.g. ...)" form should be replaced with ", e.g. ...". > + > +2. Nodes without unit addresses shall be ordered alpha-numerically by > the node > + name. For a few types of nodes, they can be ordered by the main > property > + (e.g. pin configuration states ordered by value of "pins" > property). "Alpha-numerically" should be replaced with "alphabetically". "Types of nodes" should be replaced with "node types". The "(e.g. ...)" form should be replaced with ", e.g. ...". > + > +3. When extending nodes in the board DTS via &label, the entries shall > be > + ordered either alpha-numerically or by keeping the order from DTSI > (choice > + depending on sub-architecture). "Alpha-numerically" should be replaced with "alphabetically". "Sub-architecture" should be replaced with "subarchitecture". "(Choice depending on sub-architecture)" should be replaced with ", where the choice depends on the subarchitecture". > + > +Above ordering rules are easy to enforce during review, reduce chances > of > +conflicts for simultaneous additions (new nodes) to a file and help in > +navigating through the DTS source. "Above" should be replaced with "The above-described". "(New nodes)" should be replaced with "of new nodes". > + > +Example:: > + > + /* SoC DTSI */ > + > + / { > + cpus { > + /* ... */ > + }; > + > + psci { > + /* ... */ > + }; > + > + soc@ { > + dma: dma-controller@10000 { > + /* ... */ > + }; > + > + clk: clock-controller@80000 { > + /* ... */ > + }; > + }; > + }; > + > + /* Board DTS - alphabetical order */ > + > + &clk { > + /* ... */ > + }; > + > + &dma { > + /* ... */ > + }; > + > + /* Board DTS - alternative order, keep as DTSI */ > + > + &dma { > + /* ... */ > + }; > + > + &clk { > + /* ... */ > + }; > + > +Order of Properties in Device Node > +---------------------------------- > + > +Following order of properties in device nodes is preferred: "Following" should be replaced with "The following". > + > +1. compatible > +2. reg > +3. ranges > +4. Standard/common properties (defined by common bindings, e.g. > without > + vendor-prefixes) > +5. Vendor-specific properties > +6. status (if applicable) > +7. Child nodes, where each node is preceded with a blank line > + > +The "status" property is by default "okay", thus it can be omitted. > + > +Above order follows approach: The last sentence should be replaced with "The above-described ordering follows this approach:". > + > +1. Most important properties start the node: compatible then bus > addressing to > + match unit address. > +2. Each node will have common properties in similar place. > +3. Status is the last information to annotate that device node is or > is not > + finished (board resources are needed). > + > +Example:: > + > + /* SoC DTSI */ > + > + device_node: device-class@6789abc { > + compatible = "vendor,device"; > + reg = <0x0 0x06789abc 0x0 0xa123>; > + ranges = <0x0 0x0 0x06789abc 0x1000>; > + #dma-cells = <1>; > + clocks = <&clock_controller 0>, <&clock_controller 1>; > + clock-names = "bus", "host"; > + vendor,custom-property = <2>; > + status = "disabled"; > + > + child_node: child-class@100 { > + reg = <0x100 0x200>; > + /* ... */ > + }; > + }; > + > + /* Board DTS */ > + > + &device_node { > + vdd-supply = <&board_vreg1>; > + status = "okay"; > + } > + > +Indentation > +----------- > + > +1. Use indentation according to :ref:`codingstyle`. > +2. For arrays spanning across lines, it is preferred to align the > continued > + entries with opening < from the first line. > +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO > addresses) > + shall be enclosed in <>. The "(e.g. ...)" form should be replaced with ", e.g. ...,". > + > +Example:: > + > + thermal-sensor@c271000 { > + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; > + reg = <0x0 0x0c271000 0x0 0x1000>, > + <0x0 0x0c222000 0x0 0x1000>; > + }; > + > +Organizing DTSI and DTS > +----------------------- > + > +The DTSI and DTS files shall be organized in a way representing the > common > +(and re-usable) parts of the hardware. Typically this means > organizing DTSI The "(and re-usable)" form should be replaced with ", reusable". "The hardware" should be replaced with "hardware". A comma should be added after "Typically". > +and DTS files into several files: > + > +1. DTSI with contents of the entire SoC (without nodes for hardware > not present > + on the SoC). > +2. If applicable: DTSI with common or re-usable parts of the hardware > (e.g. > + entire System-on-Module). > +3. DTS representing the board. The "(...)" forms should be replaced with ", ...". Periods at the ends of list items should be deleted, because those aren't real, complete sentences. > + > +Hardware components which are present on the board shall be placed in > the "Which" should be replaced with "that". > +board DTS, not in the SoC or SoM DTSI. A partial exception is a > common > +external reference SoC-input clock, which could be coded as a > fixed-clock in "SoC-input" should be replaced with "SoC input". > +the SoC DTSI with its frequency provided by each board DTS. > diff --git a/Documentation/devicetree/bindings/index.rst > b/Documentation/devicetree/bindings/index.rst > index d9002a3a0abb..cc1fbdc05657 100644 > --- a/Documentation/devicetree/bindings/index.rst > +++ b/Documentation/devicetree/bindings/index.rst > @@ -4,6 +4,7 @@ > :maxdepth: 1 > > ABI > + dts-coding-style > writing-bindings > writing-schema > submitting-patches
On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: > Document preferred coding style for Devicetree sources (DTS and DTSI), > to bring consistency among all (sub)architectures and ease in reviews. Thank Krzysztof, we had most of this collected as BKM in some internal documents and it's great to see the effort to consolidate this and add it to the kernel documentation. > --- > +Following order of properties in device nodes is preferred: > + > +1. compatible > +2. reg > +3. ranges > +4. Standard/common properties (defined by common bindings, e.g. without > + vendor-prefixes) > +5. Vendor-specific properties > +6. status (if applicable) > +7. Child nodes, where each node is preceded with a blank line On point 4, do you have a more explicit way to define what is an actual standard/common property? You mention the vendor-prefixes as an example, is this just an example or this is the whole definition? What would be the order for this for example (from an existing DTS file)? reg_sdhc1_vmmc: regulator-sdhci1 { compatible = "regulator-fixed"; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_sd1_pwr_en>; enable-active-high; gpio = <&main_gpio0 29 GPIO_ACTIVE_HIGH>; off-on-delay-us = <100000>; regulator-max-microvolt = <3300000>; regulator-min-microvolt = <3300000>; regulator-name = "+V3.3_SD"; startup-delay-us = <2000>; }; I guess the point that is not obvious to me here is where do we want pinctrl. I like it at position between 3 and 4, the rationale is that is a very frequent property and this way it will be in a similar place for every node. Francesco
Hi Francesco, On Wed, Nov 29, 2023 at 8:29 AM Francesco Dolcini <francesco@dolcini.it> wrote: > On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: > > Document preferred coding style for Devicetree sources (DTS and DTSI), > > to bring consistency among all (sub)architectures and ease in reviews. > > Thank Krzysztof, we had most of this collected as BKM in some internal > documents and it's great to see the effort to consolidate this and add > it to the kernel documentation. > > > --- > > +Following order of properties in device nodes is preferred: > > + > > +1. compatible > > +2. reg > > +3. ranges > > +4. Standard/common properties (defined by common bindings, e.g. without > > + vendor-prefixes) > > +5. Vendor-specific properties > > +6. status (if applicable) > > +7. Child nodes, where each node is preceded with a blank line > > On point 4, do you have a more explicit way to define what is an actual > standard/common property? You mention the vendor-prefixes as an example, > is this just an example or this is the whole definition? I think there are three classes of standard properties: 1. Device Tree Specification (from devicetree.org) 2. dt-schema 3. Common subsystem bindings (Documentation/devicetree/bindings/) (may be moved to 2). > What would be the order for this for example (from an existing DTS file)? > > reg_sdhc1_vmmc: regulator-sdhci1 { > compatible = "regulator-fixed"; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_sd1_pwr_en>; > enable-active-high; > gpio = <&main_gpio0 29 GPIO_ACTIVE_HIGH>; > off-on-delay-us = <100000>; > regulator-max-microvolt = <3300000>; > regulator-min-microvolt = <3300000>; > regulator-name = "+V3.3_SD"; > startup-delay-us = <2000>; > }; > > I guess the point that is not obvious to me here is where do we want > pinctrl. I like it at position between 3 and 4, the rationale is that is > a very frequent property and this way it will be in a similar place for > every node. The pinctrl properties are only present in board DTS files, not in SoC DTSi files. There are two classes of them: 1. Extension of on-SoC devices, where they are added to already existing nodes, defined in the SoC DTSi files, e.g. (from the same existing DTS file): &cpsw3g { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_rgmii1>; status = "disabled"; }; 2. Pure board devices, in new nodes (e.g. your regulator example). These are less common, so I don't even know from the top of my mind when I last added one, and where ;-) I'd guess after all standard properties? Gr{oetje,eeting}s, Geert
On 26/11/2023 18:48, Andrew Lunn wrote: > On Sun, Nov 26, 2023 at 11:38:38AM +0100, Krzysztof Kozlowski wrote: >> On 25/11/2023 20:47, Andrew Lunn wrote: >>>> +===================================== >>>> +Devicetree Sources (DTS) Coding Style >>>> +===================================== >>>> + >>>> +When writing Devicetree Sources (DTS) please observe below guidelines. They >>>> +should be considered complementary to any rules expressed already in Devicetree >>>> +Specification and dtc compiler (including W=1 and W=2 builds). >>>> + >>>> +Individual architectures and sub-architectures can add additional rules, making >>>> +the style stricter. >>> >>> It would be nice to add a pointer where such rules are documented. >> >> Subsystem profile or any other place. The generic doc should not point >> to specific ones. > > That is not so friendly for a developer. A reviewer points out that a > file is not consistent with the coding style. So they go away and fix Then it is poor reviewer. If reviewer does not mention specific issues to fix or specific style to use, but just "coding style", then he has no right to expect some other output. > it, as described here. They then get a second review which say, no, > you to do X, Y and Z, despite them actually following the coding > style. > > Maybe add to the paragraph above: > > These further restrictions are voluntary, until added to this > document. "can add" already expresses this. > > This should encourage those architectures to document their coding > style. > >> The root node is a bit special, but other than that mixing nodes with >> and without unit address is discouraged practice. > > If the root node is special, maybe it needs a few rules of its own? > All properties without an address come first, then properties with > addresses. Sorting within these classes follow the normal rules > already stated? This document ought to be simple at the beginning. Also, root node has only nodes without addresses and soc@ node. > >>>> +Indentation >>>> +----------- >>>> + >>>> +1. Use indentation according to :ref:`codingstyle`. >>>> +2. For arrays spanning across lines, it is preferred to align the continued >>>> + entries with opening < from the first line. >>>> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses) >>>> + shall be enclosed in <>. >>>> + >>>> +Example:: >>>> + >>>> + thermal-sensor@c271000 { >>>> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; >>>> + reg = <0x0 0x0c271000 0x0 0x1000>, >>>> + <0x0 0x0c222000 0x0 0x1000>; >>>> + }; >>> >>> I'm not sure i understand this. Is this example correct? >>> >>> gpio-fan,speed-map = <0 0 >>> 3000 1 >>> 6000 2>; >>> >>> It exists a lot in todays files. >> >> Depends on the binidng. Is it matrix? If yes, then it is not correct. > > It seems to me, rules 2 and 3 should be swapped. You can only align > the <, if you have <. So logically, the rule about having < should > come first. Hm, sure, I'll reorder them. Best regards, Krzysztof
On 27/11/2023 15:19, Geert Uytterhoeven wrote: >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst > >> + /* SoC DTSI */ >> + >> + / { >> + cpus { >> + /* ... */ >> + }; >> + >> + psci { >> + /* ... */ >> + }; >> + >> + soc@ { > > "soc@" is invalid, that should be "soc". soc@0 is valid. > > As the "soc" node is special, you may want to elaborate: > > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; > ranges; but then we go to missing address/size cells in root node. Your comment is in general correct, but what you propose here is not a coding style, but DTS correctness and I only wanted to show the order of nodes. dtc already enforces the proper unit addresses, ranges and cells. > >> + dma: dma-controller@10000 { >> + /* ... */ >> + }; >> + >> + clk: clock-controller@80000 { >> + /* ... */ >> + }; >> + }; >> + }; >> + >> + /* Board DTS - alphabetical order */ >> + >> + &clk { >> + /* ... */ >> + }; >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + /* Board DTS - alternative order, keep as DTSI */ >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + &clk { >> + /* ... */ >> + }; > > IMO that alternative order is hard to review: you need to have multiple > files open. It will also make validation hard, as you can only validate > the end result, not individual files. Rob commented on this - tools (will) solve the issue. :) > > Anyway, this is already quite usable so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > Best regards, Krzysztof
On 29/11/2023 08:29, Francesco Dolcini wrote: > On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: >> Document preferred coding style for Devicetree sources (DTS and DTSI), >> to bring consistency among all (sub)architectures and ease in reviews. > > Thank Krzysztof, we had most of this collected as BKM in some internal > documents and it's great to see the effort to consolidate this and add > it to the kernel documentation. > >> --- >> +Following order of properties in device nodes is preferred: >> + >> +1. compatible >> +2. reg >> +3. ranges >> +4. Standard/common properties (defined by common bindings, e.g. without >> + vendor-prefixes) >> +5. Vendor-specific properties >> +6. status (if applicable) >> +7. Child nodes, where each node is preceded with a blank line > > On point 4, do you have a more explicit way to define what is an actual > standard/common property? You mention the vendor-prefixes as an example, > is this just an example or this is the whole definition? The actual definition is: defined by common bindings, which are: meta-schemas and schemas in dtschema, and common bindings per subsystem (e.g. leds/common.yaml). Lack of vendor-prefix is I think 99% accurate in this matter, but there are some "linux," ones. > > What would be the order for this for example (from an existing DTS file)? > > reg_sdhc1_vmmc: regulator-sdhci1 { > compatible = "regulator-fixed"; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_sd1_pwr_en>; > enable-active-high; > gpio = <&main_gpio0 29 GPIO_ACTIVE_HIGH>; > off-on-delay-us = <100000>; > regulator-max-microvolt = <3300000>; > regulator-min-microvolt = <3300000>; > regulator-name = "+V3.3_SD"; > startup-delay-us = <2000>; > }; > > I guess the point that is not obvious to me here is where do we want > pinctrl. I like it at position between 3 and 4, the rationale is that is > a very frequent property and this way it will be in a similar place for > every node. Order here is correct but all of them are generic properties, thus this coding style does not define ordering within. Best regards, Krzysztof
On 28/11/2023 21:00, Dragan Simic wrote: >> 5. Allow two style of ordering overrides in board DTS: alphabetically >> or >> by order of DTSI (Rob). >> 6. I did not incorporate feedback about, due to lack of consensus and >> my >> disagreement: >> a. SoM being DTS without DTSI in "Organizing DTSI and DTS" > > I went through the language of the entire patch, after the notice that > the v4 would no longer accept language improvements. My wording- and > grammar-related suggestions are available inline below. Thanks. I want to finish this at some point and it might not happen if grammar fixes will be coming every patch revision. Then after we finish review, new feedback will appear about using British or American spelling (which reminds me old quote/email about which variant of English is most popular in Linux kernel: the incorrect one). > >> Changes in v2 >> ============= >> 1. Hopefully incorporate entire feedback from comments: >> a. Fix \ { => / { (Rob) >> b. Name: dts-coding-style (Rob) >> c. Exceptions for ordering nodes by name for Renesas and pinctrl >> (Geert, >> Konrad) >> d. Ordering properties by common/vendor (Rob) >> e. Array entries in <> (Rob) >> >> 2. New chapter: Organizing DTSI and DTS >> >> 3. Several grammar fixes (missing articles) >> >> Cc: linux-rockchip@lists.infradead.org >> Cc: linux-mediatek@lists.infradead.org >> Cc: linux-samsung-soc@vger.kernel.org >> Cc: linux-amlogic@lists.infradead.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-arm-msm@vger.kernel.org >> Cc: workflows@vger.kernel.org >> Cc: linux-doc@vger.kernel.org >> --- >> .../devicetree/bindings/dts-coding-style.rst | 194 ++++++++++++++++++ >> Documentation/devicetree/bindings/index.rst | 1 + >> 2 files changed, 195 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/dts-coding-style.rst >> >> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst >> b/Documentation/devicetree/bindings/dts-coding-style.rst >> new file mode 100644 >> index 000000000000..e374bec0f555 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst >> @@ -0,0 +1,194 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> +.. _dtscodingstyle: >> + >> +===================================== >> +Devicetree Sources (DTS) Coding Style >> +===================================== >> + >> +When writing Devicetree Sources (DTS) please observe below guidelines. >> They > > The sentence above should be replaced with: "The following guidelines > are to be followed when writing Devicetree Source (DTS) files." Are you sure? It's passive and I was taught it is discouraged for writing. See for example: https://www.hamilton.edu/academics/centers/writing/seven-sins-of-writing/1 > >> +should be considered complementary to any rules expressed already in >> Devicetree >> +Specification and dtc compiler (including W=1 and W=2 builds). > > A definite article ("the") should be added before "Devicetree ack > Specification" and "dtc". Also, "Specification" in "Devicetree > Specification" should be capitalized. It was. > >> + >> +Individual architectures and sub-architectures can add additional >> rules, making >> +the style stricter. > > "Sub-architectures" should be replaced with "subarchitectures". "Can A hint, you can write such review feedback as: s/sub-architectures/subarchitectures/ BTW, my language spelling points "subarchitectures" as mistake, but sure, ack. > add" should be replaced with "can define". "Style" should be replaced > with "coding style". ack > >> + >> +Naming and Valid Characters >> +--------------------------- >> + >> +Devicetree specification allows broader range of characters in node >> and >> +property names, but for code readability the choice shall be narrowed. > > The definite article ("the") should be added before "Devicetree > Specification", and "specification" should be capitalised. As already > suggested, "broader range" should be replaced with "a broad range". > "But for code readability the choice shall be narrowed" should be > replaced with "but this coding style narrows the range down to achieve > better code readability". Ack > >> + >> +1. Node and property names are allowed to use only: > > "Are allowed to" should be replaced with "can". After "only", "the > following characters" should be added. ack > >> + >> + * lowercase characters: [a-z] >> + * digits: [0-9] >> + * dash: - > > List items should be capitalized, i.e. "Lowercase characters" should be > used instead of "lowercase characters", etc. ack > >> + >> +2. Labels are allowed to use only: > > "Are allowed to" should be replaced with "can". After "only", "the > following characters" should be added. > ack >> + >> + * lowercase characters: [a-z] >> + * digits: [0-9] >> + * underscore: _ > > List items should be capitalized, i.e. "Lowercase characters" should be > used instead of "lowercase characters", etc. > ack >> + >> +3. Unit addresses shall use lowercase hex, without leading zeros >> (padding). > > "Lowercase hex" should be replaced with "lowercase hexadecimal digits". > >> + >> +4. Hex values in properties, e.g. "reg", shall use lowercase hex. The >> address >> + part can be padded with leading zeros. > > "Hex values" should be replaced with "Hexadecimal values". "Lowercase > hex" should be replaced with "lowercase hexadecimal digits". ack, but that's quite picky. We are (software) engineers so we are supposed to know the slang. > >> + >> +Example:: >> + >> + gpi_dma2: dma-controller@800000 { >> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma"; >> + reg = <0x0 0x00800000 0x0 0x60000>; >> + } >> + >> +Order of Nodes >> +-------------- >> + >> +1. Nodes within any bus, thus using unit addresses for children, shall >> be > > "Within" should be replaced "on". ack > >> + ordered incrementally by unit address. > > Should be replaced with "ordered by unit address in ascending order". ack > >> + Alternatively for some sub-architectures, nodes of the same type >> can be >> + grouped together (e.g. all I2C controllers one after another even >> if this >> + breaks unit address ordering). > > "Sub-architectures" should be replaced with "subarchitectures". The > "(e.g. ...)" form should be replaced with ", e.g. ...". ack > >> + >> +2. Nodes without unit addresses shall be ordered alpha-numerically by >> the node >> + name. For a few types of nodes, they can be ordered by the main >> property >> + (e.g. pin configuration states ordered by value of "pins" >> property). > > "Alpha-numerically" should be replaced with "alphabetically". Are you sure? Does alphabetical order include numbers? > "Types of > nodes" should be replaced with "node types". The "(e.g. ...)" form > should be replaced with ", e.g. ...". ack > >> + >> +3. When extending nodes in the board DTS via &label, the entries shall >> be >> + ordered either alpha-numerically or by keeping the order from DTSI >> (choice >> + depending on sub-architecture). > > "Alpha-numerically" should be replaced with "alphabetically". Similar concern > "Sub-architecture" should be replaced with "subarchitecture". "(Choice > depending on sub-architecture)" should be replaced with ", where the > choice depends on the subarchitecture". ack > >> + >> +Above ordering rules are easy to enforce during review, reduce chances >> of >> +conflicts for simultaneous additions (new nodes) to a file and help in >> +navigating through the DTS source. > > "Above" should be replaced with "The above-described". "(New nodes)" > should be replaced with "of new nodes". ack > >> + >> +Example:: >> + >> + /* SoC DTSI */ >> + >> + / { >> + cpus { >> + /* ... */ >> + }; >> + >> + psci { >> + /* ... */ >> + }; >> + >> + soc@ { >> + dma: dma-controller@10000 { >> + /* ... */ >> + }; >> + >> + clk: clock-controller@80000 { >> + /* ... */ >> + }; >> + }; >> + }; >> + >> + /* Board DTS - alphabetical order */ >> + >> + &clk { >> + /* ... */ >> + }; >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + /* Board DTS - alternative order, keep as DTSI */ >> + >> + &dma { >> + /* ... */ >> + }; >> + >> + &clk { >> + /* ... */ >> + }; >> + >> +Order of Properties in Device Node >> +---------------------------------- >> + >> +Following order of properties in device nodes is preferred: > > "Following" should be replaced with "The following". ack > >> + >> +1. compatible >> +2. reg >> +3. ranges >> +4. Standard/common properties (defined by common bindings, e.g. >> without >> + vendor-prefixes) >> +5. Vendor-specific properties >> +6. status (if applicable) >> +7. Child nodes, where each node is preceded with a blank line >> + >> +The "status" property is by default "okay", thus it can be omitted. >> + >> +Above order follows approach: > > The last sentence should be replaced with "The above-described ordering > follows this approach:". ack > >> + >> +1. Most important properties start the node: compatible then bus >> addressing to >> + match unit address. >> +2. Each node will have common properties in similar place. >> +3. Status is the last information to annotate that device node is or >> is not >> + finished (board resources are needed). >> + >> +Example:: >> + >> + /* SoC DTSI */ >> + >> + device_node: device-class@6789abc { >> + compatible = "vendor,device"; >> + reg = <0x0 0x06789abc 0x0 0xa123>; >> + ranges = <0x0 0x0 0x06789abc 0x1000>; >> + #dma-cells = <1>; >> + clocks = <&clock_controller 0>, <&clock_controller 1>; >> + clock-names = "bus", "host"; >> + vendor,custom-property = <2>; >> + status = "disabled"; >> + >> + child_node: child-class@100 { >> + reg = <0x100 0x200>; >> + /* ... */ >> + }; >> + }; >> + >> + /* Board DTS */ >> + >> + &device_node { >> + vdd-supply = <&board_vreg1>; >> + status = "okay"; >> + } >> + >> +Indentation >> +----------- >> + >> +1. Use indentation according to :ref:`codingstyle`. >> +2. For arrays spanning across lines, it is preferred to align the >> continued >> + entries with opening < from the first line. >> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO >> addresses) >> + shall be enclosed in <>. > > The "(e.g. ...)" form should be replaced with ", e.g. ...,". ack > >> + >> +Example:: >> + >> + thermal-sensor@c271000 { >> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; >> + reg = <0x0 0x0c271000 0x0 0x1000>, >> + <0x0 0x0c222000 0x0 0x1000>; >> + }; >> + >> +Organizing DTSI and DTS >> +----------------------- >> + >> +The DTSI and DTS files shall be organized in a way representing the >> common >> +(and re-usable) parts of the hardware. Typically this means >> organizing DTSI > > The "(and re-usable)" form should be replaced with ", reusable". "The > hardware" should be replaced with "hardware". A comma should be added > after "Typically". ack > >> +and DTS files into several files: >> + >> +1. DTSI with contents of the entire SoC (without nodes for hardware >> not present >> + on the SoC). >> +2. If applicable: DTSI with common or re-usable parts of the hardware >> (e.g. >> + entire System-on-Module). >> +3. DTS representing the board. > > The "(...)" forms should be replaced with ", ...". Periods at the ends > of list items should be deleted, because those aren't real, complete > sentences. ack > >> + >> +Hardware components which are present on the board shall be placed in >> the > > "Which" should be replaced with "that". ack > >> +board DTS, not in the SoC or SoM DTSI. A partial exception is a >> common >> +external reference SoC-input clock, which could be coded as a >> fixed-clock in > > "SoC-input" should be replaced with "SoC input". ack, thanks! Best regards, Krzysztof
On Wed, Nov 29, 2023 at 11:19:15AM +0100, Krzysztof Kozlowski wrote: > On 29/11/2023 08:29, Francesco Dolcini wrote: > > On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: > >> --- > >> +Following order of properties in device nodes is preferred: > >> + > >> +1. compatible > >> +2. reg > >> +3. ranges > >> +4. Standard/common properties (defined by common bindings, e.g. without > >> + vendor-prefixes) ... > > On point 4, do you have a more explicit way to define what is an actual > > standard/common property? You mention the vendor-prefixes as an example, > > is this just an example or this is the whole definition? > > The actual definition is: defined by common bindings, which are: > meta-schemas and schemas in dtschema, and common bindings per subsystem > (e.g. leds/common.yaml). > > Lack of vendor-prefix is I think 99% accurate in this matter, but there > are some "linux," ones. Got it, thanks! I would suggest to incorporate in some way this additional explanation in v4. Francesco
On 2023-11-29 11:43, Krzysztof Kozlowski wrote: > On 28/11/2023 21:00, Dragan Simic wrote: >> >> I went through the language of the entire patch, after the notice that >> the v4 would no longer accept language improvements. My wording- and >> grammar-related suggestions are available inline below. > > Thanks. I want to finish this at some point and it might not happen if > grammar fixes will be coming every patch revision. Then after we finish > review, new feedback will appear about using British or American > spelling (which reminds me old quote/email about which variant of > English is most popular in Linux kernel: the incorrect one). Ah, that's a good one. :) Basically, both English variants should be fine, but a single document should obviously use only one variant. >>> +===================================== >>> +Devicetree Sources (DTS) Coding Style >>> +===================================== >>> + >>> +When writing Devicetree Sources (DTS) please observe below >>> guidelines. >>> They >> >> The sentence above should be replaced with: "The following guidelines >> are to be followed when writing Devicetree Source (DTS) files." > > Are you sure? It's passive and I was taught it is discouraged for > writing. See for example: > https://www.hamilton.edu/academics/centers/writing/seven-sins-of-writing/1 Hmm, you're right, passive voice is usually not the best choice. Here's my take two for the suggested replacement sentence, which is actually a simplified version: "This document contains the guidelines for writing Devicetree Source (DTS) files." >>> +should be considered complementary to any rules expressed already in >>> Devicetree >>> +Specification and dtc compiler (including W=1 and W=2 builds). >> >> A definite article ("the") should be added before "Devicetree > > ack > >> Specification" and "dtc". Also, "Specification" in "Devicetree >> Specification" should be capitalized. > > It was. Oh, sorry, I see now. IIRC, it wasn't capitalized in some places, so I made a mistake here. >>> + >>> +Individual architectures and sub-architectures can add additional >>> rules, making >>> +the style stricter. >> >> "Sub-architectures" should be replaced with "subarchitectures". "Can > > A hint, you can write such review feedback as: > s/sub-architectures/subarchitectures/ Sure, but I specifically wanted to be less terse, as a way to be respectful. > BTW, my language spelling points "subarchitectures" as mistake, but > sure, ack. Using hyphens or not is almost always debatable, but modern English in general leans toward not using them. >>> +3. Unit addresses shall use lowercase hex, without leading zeros >>> (padding). >> >> "Lowercase hex" should be replaced with "lowercase hexadecimal >> digits". >> >>> + >>> +4. Hex values in properties, e.g. "reg", shall use lowercase hex. >>> The >>> address >>> + part can be padded with leading zeros. >> >> "Hex values" should be replaced with "Hexadecimal values". "Lowercase >> hex" should be replaced with "lowercase hexadecimal digits". > > ack, but that's quite picky. We are (software) engineers so we are > supposed to know the slang. Sure, but this document is of a bit formal nature, so using slightly more formal language can only be helpful. >>> +2. Nodes without unit addresses shall be ordered alpha-numerically >>> by >>> the node >>> + name. For a few types of nodes, they can be ordered by the main >>> property >>> + (e.g. pin configuration states ordered by value of "pins" >>> property). >> >> "Alpha-numerically" should be replaced with "alphabetically". > > Are you sure? Does alphabetical order include numbers? That's a good question, which also crossed my mind while writing the suggestions down. A more correct word would be "lexicographically", with something like ", with the already defined valid characters making the symbol set and the ACSII character set defining the ordering, " serving as an additional explanation. This would be a rather formal, but also very precise definition of the applied ordering. >>> +3. When extending nodes in the board DTS via &label, the entries >>> shall >>> be >>> + ordered either alpha-numerically or by keeping the order from >>> DTSI >>> (choice >>> + depending on sub-architecture). >> >> "Alpha-numerically" should be replaced with "alphabetically". > > Similar concern I agree. We could use "lexicographically" instead, with the precise definition already established earlier in the document. >>> +board DTS, not in the SoC or SoM DTSI. A partial exception is a >>> common >>> +external reference SoC-input clock, which could be coded as a >>> fixed-clock in >> >> "SoC-input" should be replaced with "SoC input". > > ack, thanks! Thank you once again for working on this document!
On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: > +Indentation > +----------- > + > +1. Use indentation according to :ref:`codingstyle`. One thing Jonathan mentioned before to me was to drop this :ref: stuff. | > +:ref:`devicetree-abi` more information on the ABI. | | ...can just be written as "Please see | Documentation/devicetree/bindings/ABI.rst". The cross-reference link | will be generated as expected, and readers of the plain-text docs don't | have to go grepping to find the reference. https://lore.kernel.org/all/87bki23rbx.fsf@meer.lwn.net/ Apparently the doc generation scripting can automagically do the right thing here: https://docs.kernel.org/process/maintainer-soc.html#devicetree-abi-stability Cheers, Conor.
On 01/12/2023 17:46, Conor Dooley wrote: > On Sat, Nov 25, 2023 at 07:44:22PM +0100, Krzysztof Kozlowski wrote: >> +Indentation >> +----------- >> + >> +1. Use indentation according to :ref:`codingstyle`. > > One thing Jonathan mentioned before to me was to drop this :ref: stuff. > | > +:ref:`devicetree-abi` more information on the ABI. > | > | ...can just be written as "Please see > | Documentation/devicetree/bindings/ABI.rst". The cross-reference link > | will be generated as expected, and readers of the plain-text docs don't > | have to go grepping to find the reference. > https://lore.kernel.org/all/87bki23rbx.fsf@meer.lwn.net/ > Sure, indeed it's better for plain-text readers. Best regards, Krzysztof
Just a brief reminder about my suggestions below, which seemingly didn't find their way into the v4. At least the first one, which improves the opening sentence, is worth including, IMHO. On 2023-11-29 12:37, Dragan Simic wrote: > On 2023-11-29 11:43, Krzysztof Kozlowski wrote: >> On 28/11/2023 21:00, Dragan Simic wrote: >>> >>> I went through the language of the entire patch, after the notice >>> that >>> the v4 would no longer accept language improvements. My wording- and >>> grammar-related suggestions are available inline below. >> >> Thanks. I want to finish this at some point and it might not happen if >> grammar fixes will be coming every patch revision. Then after we >> finish >> review, new feedback will appear about using British or American >> spelling (which reminds me old quote/email about which variant of >> English is most popular in Linux kernel: the incorrect one). > > Ah, that's a good one. :) Basically, both English variants should be > fine, but a single document should obviously use only one variant. > >>>> +===================================== >>>> +Devicetree Sources (DTS) Coding Style >>>> +===================================== >>>> + >>>> +When writing Devicetree Sources (DTS) please observe below >>>> guidelines. >>>> They >>> >>> The sentence above should be replaced with: "The following guidelines >>> are to be followed when writing Devicetree Source (DTS) files." >> >> Are you sure? It's passive and I was taught it is discouraged for >> writing. See for example: >> https://www.hamilton.edu/academics/centers/writing/seven-sins-of-writing/1 > > Hmm, you're right, passive voice is usually not the best choice. > Here's my take two for the suggested replacement sentence, which is > actually a simplified version: > > "This document contains the guidelines for writing Devicetree Source > (DTS) files." > >>>> +should be considered complementary to any rules expressed already >>>> in >>>> Devicetree >>>> +Specification and dtc compiler (including W=1 and W=2 builds). >>> >>> A definite article ("the") should be added before "Devicetree >> >> ack >> >>> Specification" and "dtc". Also, "Specification" in "Devicetree >>> Specification" should be capitalized. >> >> It was. > > Oh, sorry, I see now. IIRC, it wasn't capitalized in some places, so > I made a mistake here. > >>>> + >>>> +Individual architectures and sub-architectures can add additional >>>> rules, making >>>> +the style stricter. >>> >>> "Sub-architectures" should be replaced with "subarchitectures". "Can >> >> A hint, you can write such review feedback as: >> s/sub-architectures/subarchitectures/ > > Sure, but I specifically wanted to be less terse, as a way to be > respectful. > >> BTW, my language spelling points "subarchitectures" as mistake, but >> sure, ack. > > Using hyphens or not is almost always debatable, but modern English in > general leans toward not using them. > >>>> +3. Unit addresses shall use lowercase hex, without leading zeros >>>> (padding). >>> >>> "Lowercase hex" should be replaced with "lowercase hexadecimal >>> digits". >>> >>>> + >>>> +4. Hex values in properties, e.g. "reg", shall use lowercase hex. >>>> The >>>> address >>>> + part can be padded with leading zeros. >>> >>> "Hex values" should be replaced with "Hexadecimal values". >>> "Lowercase >>> hex" should be replaced with "lowercase hexadecimal digits". >> >> ack, but that's quite picky. We are (software) engineers so we are >> supposed to know the slang. > > Sure, but this document is of a bit formal nature, so using slightly > more formal language can only be helpful. > >>>> +2. Nodes without unit addresses shall be ordered alpha-numerically >>>> by >>>> the node >>>> + name. For a few types of nodes, they can be ordered by the main >>>> property >>>> + (e.g. pin configuration states ordered by value of "pins" >>>> property). >>> >>> "Alpha-numerically" should be replaced with "alphabetically". >> >> Are you sure? Does alphabetical order include numbers? > > That's a good question, which also crossed my mind while writing the > suggestions down. A more correct word would be "lexicographically", > with something like ", with the already defined valid characters > making the symbol set and the ACSII character set defining the > ordering, " serving as an additional explanation. > > This would be a rather formal, but also very precise definition of the > applied ordering. > >>>> +3. When extending nodes in the board DTS via &label, the entries >>>> shall >>>> be >>>> + ordered either alpha-numerically or by keeping the order from >>>> DTSI >>>> (choice >>>> + depending on sub-architecture). >>> >>> "Alpha-numerically" should be replaced with "alphabetically". >> >> Similar concern > > I agree. We could use "lexicographically" instead, with the precise > definition already established earlier in the document. > >>>> +board DTS, not in the SoC or SoM DTSI. A partial exception is a >>>> common >>>> +external reference SoC-input clock, which could be coded as a >>>> fixed-clock in >>> >>> "SoC-input" should be replaced with "SoC input". >> >> ack, thanks! > > Thank you once again for working on this document!
On 03/12/2023 21:12, Dragan Simic wrote: > Just a brief reminder about my suggestions below, which seemingly didn't > find their way into the v4. At least the first one, which improves the > opening sentence, is worth including, IMHO. > I applied almost all your suggestions, except few which I disagreed with in my replies. Best regards, Krzysztof
On 2023-12-04 16:11, Krzysztof Kozlowski wrote: > On 03/12/2023 21:12, Dragan Simic wrote: >> Just a brief reminder about my suggestions below, which seemingly >> didn't >> find their way into the v4. At least the first one, which improves >> the >> opening sentence, is worth including, IMHO. >> > > I applied almost all your suggestions, except few which I disagreed > with > in my replies. Sure, but I got no response from you after I replied and offered different versions of the few suggestions you disagreed with. I mean, none of those are too important, except the one about the opening sentence, which is currently a bit awkward, so it might be good to have it improved as suggested. It's the opening sentence after all. :)
diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst b/Documentation/devicetree/bindings/dts-coding-style.rst new file mode 100644 index 000000000000..e374bec0f555 --- /dev/null +++ b/Documentation/devicetree/bindings/dts-coding-style.rst @@ -0,0 +1,194 @@ +.. SPDX-License-Identifier: GPL-2.0 +.. _dtscodingstyle: + +===================================== +Devicetree Sources (DTS) Coding Style +===================================== + +When writing Devicetree Sources (DTS) please observe below guidelines. They +should be considered complementary to any rules expressed already in Devicetree +Specification and dtc compiler (including W=1 and W=2 builds). + +Individual architectures and sub-architectures can add additional rules, making +the style stricter. + +Naming and Valid Characters +--------------------------- + +Devicetree specification allows broader range of characters in node and +property names, but for code readability the choice shall be narrowed. + +1. Node and property names are allowed to use only: + + * lowercase characters: [a-z] + * digits: [0-9] + * dash: - + +2. Labels are allowed to use only: + + * lowercase characters: [a-z] + * digits: [0-9] + * underscore: _ + +3. Unit addresses shall use lowercase hex, without leading zeros (padding). + +4. Hex values in properties, e.g. "reg", shall use lowercase hex. The address + part can be padded with leading zeros. + +Example:: + + gpi_dma2: dma-controller@800000 { + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma"; + reg = <0x0 0x00800000 0x0 0x60000>; + } + +Order of Nodes +-------------- + +1. Nodes within any bus, thus using unit addresses for children, shall be + ordered incrementally by unit address. + Alternatively for some sub-architectures, nodes of the same type can be + grouped together (e.g. all I2C controllers one after another even if this + breaks unit address ordering). + +2. Nodes without unit addresses shall be ordered alpha-numerically by the node + name. For a few types of nodes, they can be ordered by the main property + (e.g. pin configuration states ordered by value of "pins" property). + +3. When extending nodes in the board DTS via &label, the entries shall be + ordered either alpha-numerically or by keeping the order from DTSI (choice + depending on sub-architecture). + +Above ordering rules are easy to enforce during review, reduce chances of +conflicts for simultaneous additions (new nodes) to a file and help in +navigating through the DTS source. + +Example:: + + /* SoC DTSI */ + + / { + cpus { + /* ... */ + }; + + psci { + /* ... */ + }; + + soc@ { + dma: dma-controller@10000 { + /* ... */ + }; + + clk: clock-controller@80000 { + /* ... */ + }; + }; + }; + + /* Board DTS - alphabetical order */ + + &clk { + /* ... */ + }; + + &dma { + /* ... */ + }; + + /* Board DTS - alternative order, keep as DTSI */ + + &dma { + /* ... */ + }; + + &clk { + /* ... */ + }; + +Order of Properties in Device Node +---------------------------------- + +Following order of properties in device nodes is preferred: + +1. compatible +2. reg +3. ranges +4. Standard/common properties (defined by common bindings, e.g. without + vendor-prefixes) +5. Vendor-specific properties +6. status (if applicable) +7. Child nodes, where each node is preceded with a blank line + +The "status" property is by default "okay", thus it can be omitted. + +Above order follows approach: + +1. Most important properties start the node: compatible then bus addressing to + match unit address. +2. Each node will have common properties in similar place. +3. Status is the last information to annotate that device node is or is not + finished (board resources are needed). + +Example:: + + /* SoC DTSI */ + + device_node: device-class@6789abc { + compatible = "vendor,device"; + reg = <0x0 0x06789abc 0x0 0xa123>; + ranges = <0x0 0x0 0x06789abc 0x1000>; + #dma-cells = <1>; + clocks = <&clock_controller 0>, <&clock_controller 1>; + clock-names = "bus", "host"; + vendor,custom-property = <2>; + status = "disabled"; + + child_node: child-class@100 { + reg = <0x100 0x200>; + /* ... */ + }; + }; + + /* Board DTS */ + + &device_node { + vdd-supply = <&board_vreg1>; + status = "okay"; + } + +Indentation +----------- + +1. Use indentation according to :ref:`codingstyle`. +2. For arrays spanning across lines, it is preferred to align the continued + entries with opening < from the first line. +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO addresses) + shall be enclosed in <>. + +Example:: + + thermal-sensor@c271000 { + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; + reg = <0x0 0x0c271000 0x0 0x1000>, + <0x0 0x0c222000 0x0 0x1000>; + }; + +Organizing DTSI and DTS +----------------------- + +The DTSI and DTS files shall be organized in a way representing the common +(and re-usable) parts of the hardware. Typically this means organizing DTSI +and DTS files into several files: + +1. DTSI with contents of the entire SoC (without nodes for hardware not present + on the SoC). +2. If applicable: DTSI with common or re-usable parts of the hardware (e.g. + entire System-on-Module). +3. DTS representing the board. + +Hardware components which are present on the board shall be placed in the +board DTS, not in the SoC or SoM DTSI. A partial exception is a common +external reference SoC-input clock, which could be coded as a fixed-clock in +the SoC DTSI with its frequency provided by each board DTS. diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst index d9002a3a0abb..cc1fbdc05657 100644 --- a/Documentation/devicetree/bindings/index.rst +++ b/Documentation/devicetree/bindings/index.rst @@ -4,6 +4,7 @@ :maxdepth: 1 ABI + dts-coding-style writing-bindings writing-schema submitting-patches