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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
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>,
	"Andrew Lunn" <andrew@lunn.ch>, "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: Mon, 27 Nov 2023 15:19:52 +0100	[thread overview]
Message-ID: <CAMuHMdUYEwMuxJ2Xx=KRVKneRT-e+uHz8LE1JVY5zLDkWksqKw@mail.gmail.com> (raw)
In-Reply-To: <20231125184422.12315-1-krzysztof.kozlowski@linaro.org>

Hi Krzysztof,

On Sat, Nov 25, 2023 at 7:44 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> Document preferred coding style for Devicetree sources (DTS and DTSI),
> to bring consistency among all (sub)architectures and ease in reviews.
>
> Cc: Andrew Davis <afd@ti.com>
> cc: Andrew Lunn <andrew@lunn.ch>
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Chen-Yu Tsai <wens@kernel.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Rafał Miłecki <zajec5@gmail.com>
> Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Merging idea: Rob/DT bindings
>
> Changes in v3
> =============
> 1. should->shall (Angelo)
> 2. Comments // -> /* (Angelo, Michal)
> 3. Use imaginary example in "Order of Properties in Device Node"
>    (Angelo)
> 4. Added paragraphs for three sections with justifications of chosen
>    style.
> 5. Allow two style of ordering overrides in board DTS: alphabetically or
>    by order of DTSI (Rob).
> 6. I did not incorporate feedback about, due to lack of consensus and my
>    disagreement:
>    a. SoM being DTS without DTSI in "Organizing DTSI and DTS"

Thanks for the update!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst

> +       /* SoC DTSI */
> +
> +       / {
> +               cpus {
> +                       /* ... */
> +               };
> +
> +               psci {
> +                       /* ... */
> +               };
> +
> +               soc@ {

"soc@" is invalid, that should be "soc".

As the "soc" node is special, you may want to elaborate:

                compatible = "simple-bus";
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

> +                       dma: dma-controller@10000 {
> +                               /* ... */
> +                       };
> +
> +                       clk: clock-controller@80000 {
> +                               /* ... */
> +                       };
> +               };
> +       };
> +
> +       /* Board DTS - alphabetical order */
> +
> +       &clk {
> +               /* ... */
> +       };
> +
> +       &dma {
> +               /* ... */
> +       };
> +
> +       /* Board DTS - alternative order, keep as DTSI */
> +
> +       &dma {
> +               /* ... */
> +       };
> +
> +       &clk {
> +               /* ... */
> +       };

IMO that alternative order is hard to review: you need to have multiple
files open.  It will also make validation hard, as you can only validate
the end result, not individual files.

Anyway, this is already quite usable so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip