[v3] docs: dt-bindings: add DTS Coding Style document

Commit Message

Krzysztof Kozlowski Nov. 25, 2023, 6:44 p.m. UTC

Document preferred coding style for Devicetree sources (DTS and DTSI),
to bring consistency among all (sub)architectures and ease in reviews.

Cc: Andrew Davis <[e-mail]>
cc: Andrew Lunn <[e-mail]>
Cc: AngeloGioacchino Del Regno <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Bjorn Andersson <[e-mail]>
Cc: Chen-Yu Tsai <[e-mail]>
Cc: Dmitry Baryshkov <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Heiko Stuebner <[e-mail]>
Cc: Jonathan Corbet <[e-mail]>
Cc: Konrad Dybcio <[e-mail]>
Cc: Matthias Brugger <[e-mail]>
Cc: Michal Simek <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Nishanth Menon <[e-mail]>
Cc: Olof Johansson <[e-mail]>
Cc: Rafał Miłecki <[e-mail]>
Acked-by: Neil Armstrong <[e-mail]>
Acked-by: Heiko Stuebner <[e-mail]>
Signed-off-by: Krzysztof Kozlowski <[e-mail]>

---

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"

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: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 .../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

Comments

Jonathan Corbet Nov. 25, 2023, 7:33 p.m. UTC | #1

Krzysztof Kozlowski <[e-mail]> 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.

Thanks,

jon

Laurent Pinchart Nov. 25, 2023, 7:37 p.m. UTC | #2

Hi Krzysztof,

Thank you for the patch.

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.
> 
> Cc: Andrew Davis <[e-mail]>
> cc: Andrew Lunn <[e-mail]>
> Cc: AngeloGioacchino Del Regno <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Heiko Stuebner <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Konrad Dybcio <[e-mail]>
> Cc: Matthias Brugger <[e-mail]>
> Cc: Michal Simek <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Cc: Olof Johansson <[e-mail]>
> Cc: Rafał Miłecki <[e-mail]>
> Acked-by: Neil Armstrong <[e-mail]>
> Acked-by: Heiko Stuebner <[e-mail]>
> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
> 
> ---
> 
> 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"
> 
> 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: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  .../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
> +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

s/Devicetree specification/The Devicetree specification/
s/broader range/a broad range/

> +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).

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.

> +
> +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.

I'm looking forward to discussing how to organize overlays. That
discussion should be separate though, or this patch will never get
merged :-)

Reviewed-by: Laurent Pinchart <[e-mail]>

> 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

Andrew Lunn Nov. 25, 2023, 7:47 p.m. UTC | #3

> +=====================================
> +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.

> +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. 

> +		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?

> +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).

> +
> +		soc@ {

This has a unit address, even if its missing, so should be sorted by
1).

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?

> +			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
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?

> +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.


> +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.

The word 'entire' probably gives the wrong idea about this.

    Andrew

Konrad Dybcio Nov. 25, 2023, 10:24 p.m. UTC | #4

[...]
>> +
>> +3. Unit addresses shall use lowercase hex, without leading zeros (padding).
> 
> 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.
One more thing not mentioned is "no 0x prefix" (the unit address is *always*
interpreted as hex).

Lowercase hex seems to be (in my experience?) the consensus for everything
except preprocessor defines across the spectrum

No leading zeroes.. I guess it was just eye-pleasing for people that have
been doing devicetree to date, myself included.

Konrad

Konrad Dybcio Nov. 25, 2023, 10:26 p.m. UTC | #5

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 <[e-mail]>
> cc: Andrew Lunn <[e-mail]>
> Cc: AngeloGioacchino Del Regno <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Heiko Stuebner <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Konrad Dybcio <[e-mail]>
> Cc: Matthias Brugger <[e-mail]>
> Cc: Michal Simek <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Cc: Olof Johansson <[e-mail]>
> Cc: Rafał Miłecki <[e-mail]>
> Acked-by: Neil Armstrong <[e-mail]>
> Acked-by: Heiko Stuebner <[e-mail]>
> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
> 
> ---
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 <[e-mail]>

Konrad

Krzysztof Kozlowski Nov. 26, 2023, 10:28 a.m. UTC | #6

On 25/11/2023 20:33, Jonathan Corbet wrote:
> Krzysztof Kozlowski <[e-mail]> 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

Krzysztof Kozlowski Nov. 26, 2023, 10:32 a.m. UTC | #7

On 25/11/2023 20:37, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> Thank you for the patch.
> 
> 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.
>>
>> Cc: Andrew Davis <[e-mail]>
>> cc: Andrew Lunn <[e-mail]>
>> Cc: AngeloGioacchino Del Regno <[e-mail]>
>> Cc: Arnd Bergmann <[e-mail]>
>> Cc: Bjorn Andersson <[e-mail]>
>> Cc: Chen-Yu Tsai <[e-mail]>
>> Cc: Dmitry Baryshkov <[e-mail]>
>> Cc: Geert Uytterhoeven <[e-mail]>
>> Cc: Heiko Stuebner <[e-mail]>
>> Cc: Jonathan Corbet <[e-mail]>
>> Cc: Konrad Dybcio <[e-mail]>
>> Cc: Matthias Brugger <[e-mail]>
>> Cc: Michal Simek <[e-mail]>
>> Cc: Neil Armstrong <[e-mail]>
>> Cc: Nishanth Menon <[e-mail]>
>> Cc: Olof Johansson <[e-mail]>
>> Cc: Rafał Miłecki <[e-mail]>
>> Acked-by: Neil Armstrong <[e-mail]>
>> Acked-by: Heiko Stuebner <[e-mail]>
>> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
>>
>> ---
>>
>> 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"
>>
>> 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: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>  .../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
>> +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
> 
> 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.

> 
>> +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).
> 
> 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.

> 
>> +
>> +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.
> 
> I'm looking forward to discussing how to organize overlays. That
> discussion should be separate though, or this patch will never get
> merged :-)
> 
> Reviewed-by: Laurent Pinchart <[e-mail]>

Thanks

Best regards,
Krzysztof

Krzysztof Kozlowski Nov. 26, 2023, 10:38 a.m. UTC | #8

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

Laurent Pinchart Nov. 26, 2023, 2:53 p.m. UTC | #9

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.
> >>
> >> Cc: Andrew Davis <[e-mail]>
> >> cc: Andrew Lunn <[e-mail]>
> >> Cc: AngeloGioacchino Del Regno <[e-mail]>
> >> Cc: Arnd Bergmann <[e-mail]>
> >> Cc: Bjorn Andersson <[e-mail]>
> >> Cc: Chen-Yu Tsai <[e-mail]>
> >> Cc: Dmitry Baryshkov <[e-mail]>
> >> Cc: Geert Uytterhoeven <[e-mail]>
> >> Cc: Heiko Stuebner <[e-mail]>
> >> Cc: Jonathan Corbet <[e-mail]>
> >> Cc: Konrad Dybcio <[e-mail]>
> >> Cc: Matthias Brugger <[e-mail]>
> >> Cc: Michal Simek <[e-mail]>
> >> Cc: Neil Armstrong <[e-mail]>
> >> Cc: Nishanth Menon <[e-mail]>
> >> Cc: Olof Johansson <[e-mail]>
> >> Cc: Rafał Miłecki <[e-mail]>
> >> Acked-by: Neil Armstrong <[e-mail]>
> >> Acked-by: Heiko Stuebner <[e-mail]>
> >> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
> >>
> >> ---
> >>
> >> 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"
> >>
> >> 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: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> ---
> >>  .../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
> >> +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
> > 
> > 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).
> > 
> > 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.

> >> +
> >> +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.
> > 
> > I'm looking forward to discussing how to organize overlays. That
> > discussion should be separate though, or this patch will never get
> > merged :-)
> > 
> > Reviewed-by: Laurent Pinchart <[e-mail]>

Andrew Lunn Nov. 26, 2023, 5:48 p.m. UTC | #10

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

Geert Uytterhoeven Nov. 27, 2023, 2:19 p.m. UTC | #11

Hi Krzysztof,

On Sat, Nov 25, 2023 at 7:44 PM Krzysztof Kozlowski
<[e-mail]> 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 <[e-mail]>
> cc: Andrew Lunn <[e-mail]>
> Cc: AngeloGioacchino Del Regno <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Heiko Stuebner <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Konrad Dybcio <[e-mail]>
> Cc: Matthias Brugger <[e-mail]>
> Cc: Michal Simek <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Cc: Olof Johansson <[e-mail]>
> Cc: Rafał Miłecki <[e-mail]>
> Acked-by: Neil Armstrong <[e-mail]>
> Acked-by: Heiko Stuebner <[e-mail]>
> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
>
> ---
>
> 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 <[e-mail]>

Gr{oetje,eeting}s,

                        Geert

Rob Herring (Arm) Nov. 27, 2023, 9:55 p.m. UTC | #12

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

Dragan Simic Nov. 28, 2023, 8 p.m. UTC | #13

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 <[e-mail]>
> cc: Andrew Lunn <[e-mail]>
> Cc: AngeloGioacchino Del Regno 
> <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Heiko Stuebner <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Konrad Dybcio <[e-mail]>
> Cc: Matthias Brugger <[e-mail]>
> Cc: Michal Simek <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Cc: Olof Johansson <[e-mail]>
> Cc: Rafał Miłecki <[e-mail]>
> Acked-by: Neil Armstrong <[e-mail]>
> Acked-by: Heiko Stuebner <[e-mail]>
> Signed-off-by: Krzysztof Kozlowski <[e-mail]>
> 
> ---
> 
> 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: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  .../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

Francesco Dolcini Nov. 29, 2023, 7:29 a.m. UTC | #14

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

Geert Uytterhoeven Nov. 29, 2023, 8:47 a.m. UTC | #15

Hi Francesco,

On Wed, Nov 29, 2023 at 8:29 AM Francesco Dolcini <[e-mail]> 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

Krzysztof Kozlowski Nov. 29, 2023, 10:06 a.m. UTC | #16

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

Krzysztof Kozlowski Nov. 29, 2023, 10:16 a.m. UTC | #17

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 <[e-mail]>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Best regards,
Krzysztof

Krzysztof Kozlowski Nov. 29, 2023, 10:19 a.m. UTC | #18

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

Krzysztof Kozlowski Nov. 29, 2023, 10:43 a.m. UTC | #19

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: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>  .../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

Francesco Dolcini Nov. 29, 2023, 11:16 a.m. UTC | #20

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

Dragan Simic Nov. 29, 2023, 11:37 a.m. UTC | #21

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!

Conor Dooley Dec. 1, 2023, 4:46 p.m. UTC | #22

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/[e-mail]/

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.

Krzysztof Kozlowski Dec. 2, 2023, 1:04 p.m. UTC | #23

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/[e-mail]/
> 

Sure, indeed it's better for plain-text readers.

Best regards,
Krzysztof

Dragan Simic Dec. 3, 2023, 8:12 p.m. UTC | #24

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!

Krzysztof Kozlowski Dec. 4, 2023, 3:11 p.m. UTC | #25

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

Dragan Simic Dec. 4, 2023, 4:19 p.m. UTC | #26

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. :)

Patch

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