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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"AngeloGioacchino Del Regno"
	<angelogioacchino.delregno@collabora.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, "Andrew Davis" <afd@ti.com>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Chen-Yu Tsai" <wens@kernel.org>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	"Michal Simek" <michal.simek@amd.com>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Nishanth Menon" <nm@ti.com>, "Olof Johansson" <olof@lixom.net>,
	"Rafał Miłecki" <zajec5@gmail.com>,
	linux-rockchip@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org,
	linux-amlogic@lists.infradead.org, linux-arm-msm@vger.kernel.org,
	workflows@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v3] docs: dt-bindings: add DTS Coding Style document
Date: Wed, 29 Nov 2023 11:06:12 +0100	[thread overview]
Message-ID: <ae9ef5a6-bb66-4b15-ad6a-fc71206e2795@linaro.org> (raw)
In-Reply-To: <4fa1e860-48bd-456b-ab0b-88215c2b8d1b@lunn.ch>

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