Message ID | 20231116181218.18886-1-krzysztof.kozlowski@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6359:6513:b0:164:83eb:24d7 with SMTP id sk19csp45402rwb; Thu, 16 Nov 2023 10:12:49 -0800 (PST) X-Google-Smtp-Source: AGHT+IEOlfZInjlVxWSGYu5h7l6/bOwLaRWgw44plqjQ9qkk9Q+wS6silhEMW0zohBuyqJQhsF7R X-Received: by 2002:a05:6808:6406:b0:3b6:b9db:ea1c with SMTP id fg6-20020a056808640600b003b6b9dbea1cmr20641253oib.42.1700158369419; Thu, 16 Nov 2023 10:12:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700158369; cv=none; d=google.com; s=arc-20160816; b=c/yK0rp+5fPDpcIgRYU6ZEsAT4EqizyMx6NVGJkM2sk7+PVRksX9XIJZqZYF/mK7Eo W39QQKtVy5rIDcPhWsh4Wynab6OUDAdY8DiPj/MSUCIPLQ+DwoK2cTywt2zXIb667eAq x8r70p/PwI3PxIIfrenDVQhNw6wU/iDY+Vyr+ajyf3EmRKm/rLXOezcnDJ8nBO0PW3kf Kx+99q9+nPZgYYiV46RJ9Dc6YAF6unRNSYnU+Rg8CHy4CeX1HmpHGcQOhUA+3e5BDNIC CO8Mr42cz088p8W30w8FqRIWCGimkRL4RuPFwkQAjh6aHjjRJxTdB/opw7azADgo9+YI 1P2g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=qsPp3Fy1desywxNmmnzNYZYsXy6qDHsAAEUlLMwI4gw=; fh=WH9mNsJARTnJcdvmsiEqwvVaEAT83OgyZuVgYB+6y2A=; b=qFYAeRcunkuToTqUeTJjRlaPMXaSTu5Y/AG7JPoqkUiSD0YDUIfjORRxxMxpUkbPku aT5ww7Ib3rf4Zo847OTtAr3lEmX55b6XkhcC06fnIZnokbfzhcvL/q1V4t0UVmnWlAYV ypG0SbEO1q/yA7xipRSdwGjPJo2LeFNR7LB5g2lmfU7B/HWWfUFYmUsBj1wcO8EkBdgZ HKqEgevonbyxDF4L2GuTpnCpb5+4hBdh73lqbfmo52Qn+tNpqAu4z3iCt8/adkJiLJdB Mpc+rzS/nUNvpDZWfgSESzvSm/WXI3ubhIeJfOWJ6o1e1z2ROk3GsT4dadyHIO4Odhda mLyg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="DY7/6Zww"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from groat.vger.email (groat.vger.email. [2620:137:e000::3:5]) by mx.google.com with ESMTPS id bz41-20020a056a02062900b005be052b03a9si13100683pgb.354.2023.11.16.10.12.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Nov 2023 10:12:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="DY7/6Zww"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id C50DA824686D; Thu, 16 Nov 2023 10:12:46 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345425AbjKPSMa (ORCPT <rfc822;jaysivo@gmail.com> + 30 others); Thu, 16 Nov 2023 13:12:30 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42336 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231327AbjKPSM1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 16 Nov 2023 13:12:27 -0500 Received: from mail-qk1-x734.google.com (mail-qk1-x734.google.com [IPv6:2607:f8b0:4864:20::734]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AA8F9D5D for <linux-kernel@vger.kernel.org>; Thu, 16 Nov 2023 10:12:22 -0800 (PST) Received: by mail-qk1-x734.google.com with SMTP id af79cd13be357-778a20df8c3so66719885a.3 for <linux-kernel@vger.kernel.org>; Thu, 16 Nov 2023 10:12:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1700158342; x=1700763142; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=qsPp3Fy1desywxNmmnzNYZYsXy6qDHsAAEUlLMwI4gw=; b=DY7/6ZwwgPWrUqyXx4wamMC64sG+Lz4ucPWt66uTyG+AcjOvdO0ICEI9UIihs9IAxE aLa2pFgVq0bV61DsQmgDrTBg5wL33sfwZCkvlPE18G19NnOG2kSdtOTNp5tqaHCBdYIc E3nChv0X6tCbJ9U4dpS9DrlVtNiUKFZRcr7VcpC35B+SqGYvG1jqdv6NhB0YhPtJsK54 1H3plTCAZtBd4KniF9UqHiI4OU3p84TV+kLrrYbFn3DFwpWUMTFos3560Xsp7HLyp6IC t0nf0EPJU/sBcAG93jRsU6HAK8QSv4bRiJMoxaAgvsHgBwT7IWRMawAwBnTNtzcq7ymO fkqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700158342; x=1700763142; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=qsPp3Fy1desywxNmmnzNYZYsXy6qDHsAAEUlLMwI4gw=; b=eARUUHD+g7ZxtbPaleEkV3D37BhxOSvdrT4zSrX728rpFk/OXRpWx4Yzu34/h6eXHU uUwvJqxkEmF4w3YsNFaZTmCtmbebwYaydQVdfU8qkPbcuIqgdo6quuBA0hw8nTkk65hb I+BpI/J84iVw5bNHgeYMjTa/65EFGNqTjQ4FX6LQ8gQ7a/Ir4TuVWIxywfDRnCM9+7iE YCEBDH/XxD0ekjEomiGbuKGApcETFQEcgaoLHpKz8ecReX0gBAilXHp7fKpt72s9PdS0 Q9ak/wm05QhuC2N5e3XCS9Mq3ZnaHlcDC1dCdwmP65Q4O31/bUKwhGi4PUH4gNoVsBF6 l/cw== X-Gm-Message-State: AOJu0YyP2HljQSOGvTQflPgeSaHNDb4owpFH9Of2hGy6sWO10yio/1gx /L3fn156Dz26rBKLJDIWtikI9qhO2lbcEPn61dA= X-Received: by 2002:a05:620a:191b:b0:772:63d9:5264 with SMTP id bj27-20020a05620a191b00b0077263d95264mr5460176qkb.59.1700158341764; Thu, 16 Nov 2023 10:12:21 -0800 (PST) Received: from krzk-bin.. ([12.161.6.169]) by smtp.gmail.com with ESMTPSA id i8-20020a05620a0a0800b00774652483b7sm4461715qka.33.2023.11.16.10.12.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Nov 2023 10:12:21 -0800 (PST) From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Arnd Bergmann <arnd@arndb.de>, Bjorn Andersson <andersson@kernel.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Heiko Stuebner <heiko@sntech.de>, Konrad Dybcio <konrad.dybcio@linaro.org>, Matthias Brugger <matthias.bgg@gmail.com>, Michal Simek <michal.simek@amd.com>, Neil Armstrong <neil.armstrong@linaro.org>, Nishanth Menon <nm@ti.com>, Olof Johansson <olof@lixom.net> Subject: [PATCH] docs: dt-bindings: add DTS Coding Style document Date: Thu, 16 Nov 2023 19:12:18 +0100 Message-Id: <20231116181218.18886-1-krzysztof.kozlowski@linaro.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 16 Nov 2023 10:12:46 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782745262459404769 X-GMAIL-MSGID: 1782745262459404769 |
Series |
docs: dt-bindings: add DTS Coding Style document
|
|
Commit Message
Krzysztof Kozlowski
Nov. 16, 2023, 6:12 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Heiko Stuebner <heiko@sntech.de>
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>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Merging idea: Rob/DT bindings
---
Documentation/devicetree/bindings/index.rst | 1 +
.../devicetree/bindings/writing-dts.rst | 137 ++++++++++++++++++
2 files changed, 138 insertions(+)
create mode 100644 Documentation/devicetree/bindings/writing-dts.rst
Comments
Hi Krzysztof, Am Donnerstag, 16. November 2023, 19:12:18 CET schrieb Krzysztof Kozlowski: > Document preferred coding style for Devicetree sources (DTS and DTSI), > to bring consistency among all (sub)architectures and ease in reviews. > > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > +Order of Properties in Device Node > +---------------------------------- > + > +Following order of properties in device nodes is preferred: > + > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties I guess the only thing I do have questions about is the part > +4. All properties with values > +5. Boolean properties Is there a rationale for it? Because with it things like regulator-* properties then end up in two different blocks. For all the rest I fully agree :-) Thanks Heiko > +6. status (if applicable) > +7. Child nodes > + > +The "status" property is by default "okay", thus it can be omitted. > + > +Example:: > + > + // SoC DTSI > + > + usb_1_hsphy: phy@88e3000 { > + compatible = "qcom,sm8550-snps-eusb2-phy"; > + reg = <0x0 0x088e3000 0x0 0x154>; > + #phy-cells = <0>; > + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>; > + status = "disabled"; > + }; > + > + // Board DTS > + > + &usb_1_hsphy { > + clocks = <&tcsr TCSR_USB2_CLKREF_EN>; > + clock-names = "ref"; > + status = "okay"; > + }; > +
On 16/11/2023 20:26, Heiko Stuebner wrote: > Hi Krzysztof, > Am Donnerstag, 16. November 2023, 19:12:18 CET schrieb Krzysztof Kozlowski: >> Document preferred coding style for Devicetree sources (DTS and DTSI), >> to bring consistency among all (sub)architectures and ease in reviews. >> >> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Bjorn Andersson <andersson@kernel.org> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Heiko Stuebner <heiko@sntech.de> >> 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> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> > >> +Order of Properties in Device Node >> +---------------------------------- >> + >> +Following order of properties in device nodes is preferred: >> + >> +1. compatible >> +2. reg >> +3. ranges >> +4. All properties with values >> +5. Boolean properties > > I guess the only thing I do have questions about is the part > >> +4. All properties with values >> +5. Boolean properties > > Is there a rationale for it? Because with it things like regulator-* > properties then end up in two different blocks. Good point. It is only a matter of style that this: foo { compatible = "foo"; reg = <0x1>; clocks = <&clk>; wakeup-source; key-autorepeat; } looks better to me than: foo { compatible = "foo"; reg = <0x1>; key-autorepeat; wakeup-source; clocks = <&clk>; } But you have good point that similar properties should be usually grouped together. About which regulator properties are you thinking now? You mean the supplies or the provider? Best regards, Krzysztof
Am Donnerstag, 16. November 2023, 20:51:24 CET schrieb Krzysztof Kozlowski: > On 16/11/2023 20:26, Heiko Stuebner wrote: > > Hi Krzysztof, > > Am Donnerstag, 16. November 2023, 19:12:18 CET schrieb Krzysztof Kozlowski: > >> Document preferred coding style for Devicetree sources (DTS and DTSI), > >> to bring consistency among all (sub)architectures and ease in reviews. > >> > >> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > >> Cc: Arnd Bergmann <arnd@arndb.de> > >> Cc: Bjorn Andersson <andersson@kernel.org> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> Cc: Heiko Stuebner <heiko@sntech.de> > >> 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> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> > > > >> +Order of Properties in Device Node > >> +---------------------------------- > >> + > >> +Following order of properties in device nodes is preferred: > >> + > >> +1. compatible > >> +2. reg > >> +3. ranges > >> +4. All properties with values > >> +5. Boolean properties > > > > I guess the only thing I do have questions about is the part > > > >> +4. All properties with values > >> +5. Boolean properties > > > > Is there a rationale for it? Because with it things like regulator-* > > properties then end up in two different blocks. > > Good point. It is only a matter of style that this: > > foo { > compatible = "foo"; > reg = <0x1>; > clocks = <&clk>; > wakeup-source; > key-autorepeat; > } > > looks better to me than: > > > foo { > compatible = "foo"; > reg = <0x1>; > key-autorepeat; > wakeup-source; > clocks = <&clk>; > } > > But you have good point that similar properties should be usually > grouped together. > > About which regulator properties are you thinking now? You mean the > supplies or the provider? I was thinking about the provider. There are regulator-min-microvolt = <>; and friends, but also regulator-boot-on; I guess I would just go with 1. compatible 2. reg 3. ranges 4. All other properties 5. status (if applicable) 6. Child nodes aka grouping the old 4+5 together. The difference is probably minimal but doesn't create corner cases and you don't need to know if a property has a value or is boolean when looking for it. Heiko
On 16/11/2023 21:03, Heiko Stuebner wrote: >>> I guess the only thing I do have questions about is the part >>> >>>> +4. All properties with values >>>> +5. Boolean properties >>> >>> Is there a rationale for it? Because with it things like regulator-* >>> properties then end up in two different blocks. >> >> Good point. It is only a matter of style that this: >> >> foo { >> compatible = "foo"; >> reg = <0x1>; >> clocks = <&clk>; >> wakeup-source; >> key-autorepeat; >> } >> >> looks better to me than: >> >> >> foo { >> compatible = "foo"; >> reg = <0x1>; >> key-autorepeat; >> wakeup-source; >> clocks = <&clk>; >> } >> >> But you have good point that similar properties should be usually >> grouped together. >> >> About which regulator properties are you thinking now? You mean the >> supplies or the provider? > > I was thinking about the provider. There are > regulator-min-microvolt = <>; > and friends, but also > regulator-boot-on; These are in regulator provider nodes and above guideline would keep logical order: regulator-name = "vdd_kfc"; regulator-min-microvolt = <800000>; regulator-max-microvolt = <1500000>; regulator-always-on; regulator-boot-on; regulator-state-mem { regulator-off-in-suspend; }; What exactly would be here misordered? > > > I guess I would just go with > > 1. compatible > 2. reg > 3. ranges > 4. All other properties > 5. status (if applicable) > 6. Child nodes > > aka grouping the old 4+5 together. The difference is probably minimal > but doesn't create corner cases and you don't need to know if a property > has a value or is boolean when looking for it. Best regards, Krzysztof
Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: > On 16/11/2023 21:03, Heiko Stuebner wrote: > > >>> I guess the only thing I do have questions about is the part > >>> > >>>> +4. All properties with values > >>>> +5. Boolean properties > >>> > >>> Is there a rationale for it? Because with it things like regulator-* > >>> properties then end up in two different blocks. > >> > >> Good point. It is only a matter of style that this: > >> > >> foo { > >> compatible = "foo"; > >> reg = <0x1>; > >> clocks = <&clk>; > >> wakeup-source; > >> key-autorepeat; > >> } > >> > >> looks better to me than: > >> > >> > >> foo { > >> compatible = "foo"; > >> reg = <0x1>; > >> key-autorepeat; > >> wakeup-source; > >> clocks = <&clk>; > >> } > >> > >> But you have good point that similar properties should be usually > >> grouped together. > >> > >> About which regulator properties are you thinking now? You mean the > >> supplies or the provider? > > > > I was thinking about the provider. There are > > regulator-min-microvolt = <>; > > and friends, but also > > regulator-boot-on; > > These are in regulator provider nodes and above guideline would keep > logical order: > > regulator-name = "vdd_kfc"; > regulator-min-microvolt = <800000>; > regulator-max-microvolt = <1500000>; > regulator-always-on; > regulator-boot-on; > > regulator-state-mem { > regulator-off-in-suspend; > }; > > What exactly would be here misordered? going with the vcc5v0_host regulator of the rk3588-quartzpro64 and +1. compatible +2. reg +3. ranges +4. All properties with values +5. Boolean properties +6. status (if applicable) +7. Child nodes we'd end up with vcc5v0_host: vcc5v0-host-regulator { /* 1. */ compatible = "regulator-fixed"; /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; pinctrl-names = "default"; pinctrl-0 = <&vcc5v0_host_en>; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; regulator-name = "vcc5v0_host"; vin-supply = <&vcc5v0_usb>; /* 5. */ enable-active-high; regulator-always-on; regulator-boot-on; }; which I find somewhat counter-intuitive ;-) . Heiko
On Thu, Nov 16, 2023 at 12:12 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. Thanks for doing this. > > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Merging idea: Rob/DT bindings > --- > Documentation/devicetree/bindings/index.rst | 1 + > .../devicetree/bindings/writing-dts.rst | 137 ++++++++++++++++++ > 2 files changed, 138 insertions(+) > create mode 100644 Documentation/devicetree/bindings/writing-dts.rst Perhaps dts-coding-style.rst After adding writing-schema, I realized the difference between writing-schema and writing-bindings isn't all that clear. I never got around to renaming things. > > diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst > index d9002a3a0abb..975449be4862 100644 > --- a/Documentation/devicetree/bindings/index.rst > +++ b/Documentation/devicetree/bindings/index.rst > @@ -5,5 +5,6 @@ > > ABI > writing-bindings > + writing-dts > writing-schema > submitting-patches > diff --git a/Documentation/devicetree/bindings/writing-dts.rst b/Documentation/devicetree/bindings/writing-dts.rst > new file mode 100644 > index 000000000000..10c477ec1eed > --- /dev/null > +++ b/Documentation/devicetree/bindings/writing-dts.rst > @@ -0,0 +1,137 @@ > +.. SPDX-License-Identifier: GPL-2.0 > +.. _writingdts: > + > +=================================================== > +Writing Devicetree Sources (DTS) - 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 > +--------------------------- > + > +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 should use lowercase hex, without leading zeros (padding). Strictly speaking, the unit-address is whatever a bus defines, but yes, by default, that is the format. > + > +4. Hex values in properties, e.g. "reg", should use lowercase hex. Any 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. > + > +2. Nodes without unit addresses should be ordered alpha-numerically. > + > +3. When extending nodes in board DTS via &label, the entries should be ordered > + alpha-numerically. Or matching the original node definition order? > + > +Example:: > + > + // SoC DTSI > + > + \ { / { > + cpus { > + // ... > + }; > + > + psci { > + // ... > + }; > + > + soc@ { > + dma: dma-controller@10000 { > + // ... > + }; > + > + clk: clock-controller@80000 { > + // ... > + }; > + }; > + }; > + > + // Board DTS > + > + &clk { > + // ... > + }; > + > + &dma { > + // ... > + }; > + > + > +Order of Properties in Device Node > +---------------------------------- > + > +Following order of properties in device nodes is preferred: > + > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties I make this like schemas, standard/common properties first, then vendor specific properties. > +6. status (if applicable) > +7. Child nodes > + > +The "status" property is by default "okay", thus it can be omitted. > + > +Example:: > + > + // SoC DTSI > + > + usb_1_hsphy: phy@88e3000 { > + compatible = "qcom,sm8550-snps-eusb2-phy"; > + reg = <0x0 0x088e3000 0x0 0x154>; > + #phy-cells = <0>; > + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>; > + status = "disabled"; > + }; > + > + // Board DTS > + > + &usb_1_hsphy { > + clocks = <&tcsr TCSR_USB2_CLKREF_EN>; > + clock-names = "ref"; > + status = "okay"; > + }; > + > + > +Indentation > +----------- > + > +1. Use indentation according to :ref:`codingstyle`. > +2. For arrays spanning across lines, preferred is to align the continued > + entries with opening < from first line. > + > +Example:: > + > + thermal-sensor@c271000 { > + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; > + reg = <0x0 0x0c271000 0x0 0x1000>, > + <0x0 0x0c222000 0x0 0x1000>; You should cover the <> style too, meaning <> around each logical entry. > + }; > -- > 2.34.1 >
On Thu, Nov 16, 2023 at 07:12:18PM +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. This is a good idea IMO, thanks for writing it. Should we also mention in this about the expected breakdown of things between $soc.dtsi and $soc-$board.dtsi? There's commonly confusion about things like oscillators and where they belong - particularly when the SoC mandates that these oscillators be of a single frequency. It may seem obvious to us what goes where, but certainly contributors frequently get this wrong. Cheers, Conor. > > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Merging idea: Rob/DT bindings > --- > Documentation/devicetree/bindings/index.rst | 1 + > .../devicetree/bindings/writing-dts.rst | 137 ++++++++++++++++++ > 2 files changed, 138 insertions(+) > create mode 100644 Documentation/devicetree/bindings/writing-dts.rst > > diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst > index d9002a3a0abb..975449be4862 100644 > --- a/Documentation/devicetree/bindings/index.rst > +++ b/Documentation/devicetree/bindings/index.rst > @@ -5,5 +5,6 @@ > > ABI > writing-bindings > + writing-dts > writing-schema > submitting-patches > diff --git a/Documentation/devicetree/bindings/writing-dts.rst b/Documentation/devicetree/bindings/writing-dts.rst > new file mode 100644 > index 000000000000..10c477ec1eed > --- /dev/null > +++ b/Documentation/devicetree/bindings/writing-dts.rst > @@ -0,0 +1,137 @@ > +.. SPDX-License-Identifier: GPL-2.0 > +.. _writingdts: > + > +=================================================== > +Writing Devicetree Sources (DTS) - 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 > +--------------------------- > + > +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 should use lowercase hex, without leading zeros (padding). > + > +4. Hex values in properties, e.g. "reg", should use lowercase hex. Any 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. > + > +2. Nodes without unit addresses should be ordered alpha-numerically. > + > +3. When extending nodes in board DTS via &label, the entries should be ordered > + alpha-numerically. > + > +Example:: > + > + // SoC DTSI > + > + \ { > + cpus { > + // ... > + }; > + > + psci { > + // ... > + }; > + > + soc@ { > + dma: dma-controller@10000 { > + // ... > + }; > + > + clk: clock-controller@80000 { > + // ... > + }; > + }; > + }; > + > + // Board DTS > + > + &clk { > + // ... > + }; > + > + &dma { > + // ... > + }; > + > + > +Order of Properties in Device Node > +---------------------------------- > + > +Following order of properties in device nodes is preferred: > + > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties > +6. status (if applicable) > +7. Child nodes > + > +The "status" property is by default "okay", thus it can be omitted. > + > +Example:: > + > + // SoC DTSI > + > + usb_1_hsphy: phy@88e3000 { > + compatible = "qcom,sm8550-snps-eusb2-phy"; > + reg = <0x0 0x088e3000 0x0 0x154>; > + #phy-cells = <0>; > + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>; > + status = "disabled"; > + }; > + > + // Board DTS > + > + &usb_1_hsphy { > + clocks = <&tcsr TCSR_USB2_CLKREF_EN>; > + clock-names = "ref"; > + status = "okay"; > + }; > + > + > +Indentation > +----------- > + > +1. Use indentation according to :ref:`codingstyle`. > +2. For arrays spanning across lines, preferred is to align the continued > + entries with opening < from first line. > + > +Example:: > + > + thermal-sensor@c271000 { > + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; > + reg = <0x0 0x0c271000 0x0 0x1000>, > + <0x0 0x0c222000 0x0 0x1000>; > + }; > -- > 2.34.1 >
Hi Krzysztof, On Thu, Nov 16, 2023 at 7:12 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. > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Thanks for your patch! > --- a/Documentation/devicetree/bindings/index.rst > +++ b/Documentation/devicetree/bindings/index.rst > +Order of Nodes > +-------------- > + > +1. Nodes within any bus, thus using unit addresses for children, shall be > + ordered incrementally by unit address. In Renesas DTS files, we have an (unwritten) additional rule: when there are multiple nodes of the same type, they do not follow the global ordering by unit-address, but are grouped together. E.g. the second and any subsequent serial ports are always listed immediately after the first serial port. > +2. Nodes without unit addresses should be ordered alpha-numerically. > + > +3. When extending nodes in board DTS via &label, the entries should be ordered > + alpha-numerically. > +Order of Properties in Device Node > +---------------------------------- > + > +Following order of properties in device nodes is preferred: > + > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties > +6. status (if applicable) There must be a lank line between properties and child nodes? And once we have a documented coding style, the next long-standing item from our wishlist would be a script to sort the contents of a DTS file according to the rules ;-) Gr{oetje,eeting}s, Geert
On 11/16/23 2:33 PM, Heiko Stuebner wrote: > Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: >> On 16/11/2023 21:03, Heiko Stuebner wrote: >> >>>>> I guess the only thing I do have questions about is the part >>>>> >>>>>> +4. All properties with values >>>>>> +5. Boolean properties >>>>> >>>>> Is there a rationale for it? Because with it things like regulator-* >>>>> properties then end up in two different blocks. >>>> >>>> Good point. It is only a matter of style that this: >>>> >>>> foo { >>>> compatible = "foo"; >>>> reg = <0x1>; >>>> clocks = <&clk>; >>>> wakeup-source; >>>> key-autorepeat; >>>> } >>>> >>>> looks better to me than: >>>> >>>> >>>> foo { >>>> compatible = "foo"; >>>> reg = <0x1>; >>>> key-autorepeat; >>>> wakeup-source; >>>> clocks = <&clk>; >>>> } >>>> >>>> But you have good point that similar properties should be usually >>>> grouped together. >>>> >>>> About which regulator properties are you thinking now? You mean the >>>> supplies or the provider? >>> >>> I was thinking about the provider. There are >>> regulator-min-microvolt = <>; >>> and friends, but also >>> regulator-boot-on; >> >> These are in regulator provider nodes and above guideline would keep >> logical order: >> >> regulator-name = "vdd_kfc"; >> regulator-min-microvolt = <800000>; >> regulator-max-microvolt = <1500000>; >> regulator-always-on; >> regulator-boot-on; >> >> regulator-state-mem { >> regulator-off-in-suspend; >> }; >> >> What exactly would be here misordered? > > going with the vcc5v0_host regulator of the rk3588-quartzpro64 and > > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties > +6. status (if applicable) > +7. Child nodes > > we'd end up with > > vcc5v0_host: vcc5v0-host-regulator { > /* 1. */ compatible = "regulator-fixed"; > /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > pinctrl-names = "default"; > pinctrl-0 = <&vcc5v0_host_en>; > regulator-min-microvolt = <5000000>; > regulator-max-microvolt = <5000000>; > regulator-name = "vcc5v0_host"; > vin-supply = <&vcc5v0_usb>; > /* 5. */ enable-active-high; > regulator-always-on; > regulator-boot-on; > }; > How about grouping like properties (defined in the same schema), then sorting within that group. Would also allow for defining where to add spacing. 1. compatible 2. reg 3. ranges 4. All property groups 4.1 Properties with values 4.2 Boolean properties 4.3 Separating space 6. status (if applicable) 7. Child nodes Your node then would look like we expect: vcc5v0_host: vcc5v0-host-regulator { /* 1 */ compatible = "regulator-fixed"; /* 4.1 */ pinctrl-names = "default"; /* 4.1 */ pinctrl-0 = <&vcc5v0_host_en>; /* 4.3 */ /* 4.1 */ regulator-min-microvolt = <5000000>; /* 4.1 */ regulator-max-microvolt = <5000000>; /* 4.1 */ regulator-name = "vcc5v0_host"; /* 4.2 */ regulator-always-on; /* 4.2 */ regulator-boot-on; /* 4.2 */ enable-active-high; /* 4.3 */ /* 4.1 */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; ... }; Andrew > which I find somewhat counter-intuitive ;-) . > > > Heiko > >
Am Freitag, 17. November 2023, 15:03:38 CET schrieb Andrew Davis: > On 11/16/23 2:33 PM, Heiko Stuebner wrote: > > Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: > >> On 16/11/2023 21:03, Heiko Stuebner wrote: > > going with the vcc5v0_host regulator of the rk3588-quartzpro64 and > > > > +1. compatible > > +2. reg > > +3. ranges > > +4. All properties with values > > +5. Boolean properties > > +6. status (if applicable) > > +7. Child nodes > > > > we'd end up with > > > > vcc5v0_host: vcc5v0-host-regulator { > > /* 1. */ compatible = "regulator-fixed"; > > /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > > pinctrl-names = "default"; > > pinctrl-0 = <&vcc5v0_host_en>; > > regulator-min-microvolt = <5000000>; > > regulator-max-microvolt = <5000000>; > > regulator-name = "vcc5v0_host"; > > vin-supply = <&vcc5v0_usb>; > > /* 5. */ enable-active-high; > > regulator-always-on; > > regulator-boot-on; > > }; > > > > How about grouping like properties (defined in the same schema), > then sorting within that group. Would also allow for defining > where to add spacing. > > 1. compatible > 2. reg > 3. ranges > 4. All property groups > 4.1 Properties with values > 4.2 Boolean properties > 4.3 Separating space > 6. status (if applicable) > 7. Child nodes > > Your node then would look like we expect: > > vcc5v0_host: vcc5v0-host-regulator { > /* 1 */ compatible = "regulator-fixed"; > > /* 4.1 */ pinctrl-names = "default"; > /* 4.1 */ pinctrl-0 = <&vcc5v0_host_en>; > /* 4.3 */ > /* 4.1 */ regulator-min-microvolt = <5000000>; > /* 4.1 */ regulator-max-microvolt = <5000000>; > /* 4.1 */ regulator-name = "vcc5v0_host"; > /* 4.2 */ regulator-always-on; > /* 4.2 */ regulator-boot-on; > /* 4.2 */ enable-active-high; > /* 4.3 */ > /* 4.1 */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > ... > }; I'm really not sure about adding big sets of rules. In the above example you'd also need to define which schema has a higher priority? ;-) When I started with Rockchip stuff, I also had some fancy way of sorting elements in mind that was really intuitive to myself :-) . Over time I realized that it was quite complex - especially when I had to explain it to people. There are definite advantages for having compatible + reg + status in fixed positions, as it helps going over a whole dt to spot the huge mistakes (accidentially disabled, wrong address), but for the rest a simple alphabetical sorting is easiest to explain to people :-) . And alphabetic elements are also easier on my eyes. I just think having a short clean set of rules like Krzysztof proposed, is easier to follow and "enforce" and also most likely doesn't deter people from contributing, if mainline work is not their main occupation. Heiko
On 11/17/23 1:38 PM, Heiko Stübner wrote: > Am Freitag, 17. November 2023, 15:03:38 CET schrieb Andrew Davis: >> On 11/16/23 2:33 PM, Heiko Stuebner wrote: >>> Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: >>>> On 16/11/2023 21:03, Heiko Stuebner wrote: >>> going with the vcc5v0_host regulator of the rk3588-quartzpro64 and >>> >>> +1. compatible >>> +2. reg >>> +3. ranges >>> +4. All properties with values >>> +5. Boolean properties >>> +6. status (if applicable) >>> +7. Child nodes >>> >>> we'd end up with >>> >>> vcc5v0_host: vcc5v0-host-regulator { >>> /* 1. */ compatible = "regulator-fixed"; >>> /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; >>> pinctrl-names = "default"; >>> pinctrl-0 = <&vcc5v0_host_en>; >>> regulator-min-microvolt = <5000000>; >>> regulator-max-microvolt = <5000000>; >>> regulator-name = "vcc5v0_host"; >>> vin-supply = <&vcc5v0_usb>; >>> /* 5. */ enable-active-high; >>> regulator-always-on; >>> regulator-boot-on; >>> }; >>> >> >> How about grouping like properties (defined in the same schema), >> then sorting within that group. Would also allow for defining >> where to add spacing. >> >> 1. compatible >> 2. reg >> 3. ranges >> 4. All property groups >> 4.1 Properties with values >> 4.2 Boolean properties >> 4.3 Separating space >> 6. status (if applicable) >> 7. Child nodes >> >> Your node then would look like we expect: >> >> vcc5v0_host: vcc5v0-host-regulator { >> /* 1 */ compatible = "regulator-fixed"; >> >> /* 4.1 */ pinctrl-names = "default"; >> /* 4.1 */ pinctrl-0 = <&vcc5v0_host_en>; >> /* 4.3 */ >> /* 4.1 */ regulator-min-microvolt = <5000000>; >> /* 4.1 */ regulator-max-microvolt = <5000000>; >> /* 4.1 */ regulator-name = "vcc5v0_host"; >> /* 4.2 */ regulator-always-on; >> /* 4.2 */ regulator-boot-on; >> /* 4.2 */ enable-active-high; >> /* 4.3 */ >> /* 4.1 */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; >> ... >> }; > > I'm really not sure about adding big sets of rules. > In the above example you'd also need to define which schema has a higher > priority? ;-) > > > When I started with Rockchip stuff, I also had some fancy way of sorting > elements in mind that was really intuitive to myself :-) . > Over time I realized that it was quite complex - especially when I had to > explain it to people. > > There are definite advantages for having compatible + reg + status in > fixed positions, as it helps going over a whole dt to spot the huge > mistakes (accidentially disabled, wrong address), but for the rest a > simple alphabetical sorting is easiest to explain to people :-) . > > And alphabetic elements are also easier on my eyes. > +1 for starting with compatible/reg/status that we would like to see in the same spot in each node. Not so sure on plain alphabetical. That has the same issue you pointed out with splitting value vs boolean properties, related properties would end up not grouped. Some like regulator- with the same prefix will, but think -gpios that is a postfix, they would be scattered about. How about just enforcing ordering on the couple common property we care about seeing and everything else left free-hand as it today? Andrew > > I just think having a short clean set of rules like Krzysztof proposed, > is easier to follow and "enforce" and also most likely doesn't deter > people from contributing, if mainline work is not their main occupation. > > > Heiko > > >
Am Freitag, 17. November 2023, 20:54:05 CET schrieb Andrew Davis: > On 11/17/23 1:38 PM, Heiko Stübner wrote: > > Am Freitag, 17. November 2023, 15:03:38 CET schrieb Andrew Davis: > >> On 11/16/23 2:33 PM, Heiko Stuebner wrote: > >>> Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: > >>>> On 16/11/2023 21:03, Heiko Stuebner wrote: > >>> going with the vcc5v0_host regulator of the rk3588-quartzpro64 and > >>> > >>> +1. compatible > >>> +2. reg > >>> +3. ranges > >>> +4. All properties with values > >>> +5. Boolean properties > >>> +6. status (if applicable) > >>> +7. Child nodes > >>> > >>> we'd end up with > >>> > >>> vcc5v0_host: vcc5v0-host-regulator { > >>> /* 1. */ compatible = "regulator-fixed"; > >>> /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > >>> pinctrl-names = "default"; > >>> pinctrl-0 = <&vcc5v0_host_en>; > >>> regulator-min-microvolt = <5000000>; > >>> regulator-max-microvolt = <5000000>; > >>> regulator-name = "vcc5v0_host"; > >>> vin-supply = <&vcc5v0_usb>; > >>> /* 5. */ enable-active-high; > >>> regulator-always-on; > >>> regulator-boot-on; > >>> }; > >>> > >> > >> How about grouping like properties (defined in the same schema), > >> then sorting within that group. Would also allow for defining > >> where to add spacing. > >> > >> 1. compatible > >> 2. reg > >> 3. ranges > >> 4. All property groups > >> 4.1 Properties with values > >> 4.2 Boolean properties > >> 4.3 Separating space > >> 6. status (if applicable) > >> 7. Child nodes > >> > >> Your node then would look like we expect: > >> > >> vcc5v0_host: vcc5v0-host-regulator { > >> /* 1 */ compatible = "regulator-fixed"; > >> > >> /* 4.1 */ pinctrl-names = "default"; > >> /* 4.1 */ pinctrl-0 = <&vcc5v0_host_en>; > >> /* 4.3 */ > >> /* 4.1 */ regulator-min-microvolt = <5000000>; > >> /* 4.1 */ regulator-max-microvolt = <5000000>; > >> /* 4.1 */ regulator-name = "vcc5v0_host"; > >> /* 4.2 */ regulator-always-on; > >> /* 4.2 */ regulator-boot-on; > >> /* 4.2 */ enable-active-high; > >> /* 4.3 */ > >> /* 4.1 */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > >> ... > >> }; > > > > I'm really not sure about adding big sets of rules. > > In the above example you'd also need to define which schema has a higher > > priority? ;-) > > > > > > When I started with Rockchip stuff, I also had some fancy way of sorting > > elements in mind that was really intuitive to myself :-) . > > Over time I realized that it was quite complex - especially when I had to > > explain it to people. > > > > There are definite advantages for having compatible + reg + status in > > fixed positions, as it helps going over a whole dt to spot the huge > > mistakes (accidentially disabled, wrong address), but for the rest a > > simple alphabetical sorting is easiest to explain to people :-) . > > > > And alphabetic elements are also easier on my eyes. > > > > +1 for starting with compatible/reg/status that we would like to see > in the same spot in each node. > > Not so sure on plain alphabetical. That has the same issue you pointed out > with splitting value vs boolean properties, related properties would end up > not grouped. Some like regulator- with the same prefix will, but think -gpios > that is a postfix, they would be scattered about. > > How about just enforcing ordering on the couple common property we care about > seeing and everything else left free-hand as it today? Sounds like a very sensible idea :-) . Especially as the sorting of individual properties is just a tiny part of Krzysztof's document, and all the other parts in it are way more important anyway. Heiko
On 16.11.2023 19:12, 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- [...] > +Order of Nodes > +-------------- > + > +1. Nodes within any bus, thus using unit addresses for children, shall be > + ordered incrementally by unit address. > + > +2. Nodes without unit addresses should be ordered alpha-numerically. I largely agree with all this given our fight for some level of tidyness across linux-msm, but there's one specific case where I think it would make sense to break this ordering, and that's GPIO states: &pinctrl { xyz-active-state { pins ="gpio0"; ... }; abc-active-state { pins ="gpio1"; ... }; qwe-active-state { pins ="gpio2"; ... }; }; looks so much more readable to me than &pinctrl { abc-active-state { pins ="gpio1"; ... }; qwe-active-state { pins ="gpio2"; ... }; xyz-active-state { pins ="gpio0"; ... }; }; Konrad
On 16.11.2023 21:44, Rob Herring wrote: > On Thu, Nov 16, 2023 at 12:12 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. > > Thanks for doing this. > >> >> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Bjorn Andersson <andersson@kernel.org> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Heiko Stuebner <heiko@sntech.de> >> 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> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- [...] >> +Example:: >> + >> + thermal-sensor@c271000 { >> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; >> + reg = <0x0 0x0c271000 0x0 0x1000>, >> + <0x0 0x0c222000 0x0 0x1000>; > > You should cover the <> style too, meaning <> around each logical entry. FWIW this matters because e.g. https://github.com/devicetree-org/devicetree-specification/issues/66 Konrad
On 16.11.2023 19:12, 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Heiko Stuebner <heiko@sntech.de> > 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- Another thing that was suggested by at least one person to me is that with a formalized ordering system in place AND using dt-bindings, devicetrees could largely be reduced to a set of structs thrown into some sort of an ASLC-alike (meaning: you fill out structs with the necessary data, like reg, name etc. and the computer infers the rest and creates a nice & stylish output for you). Konrad
Hi Heiko, On Fri, Nov 17, 2023 at 8:38 PM Heiko Stübner <heiko@sntech.de> wrote: > Am Freitag, 17. November 2023, 15:03:38 CET schrieb Andrew Davis: > > On 11/16/23 2:33 PM, Heiko Stuebner wrote: > > > Am Donnerstag, 16. November 2023, 21:23:20 CET schrieb Krzysztof Kozlowski: > > >> On 16/11/2023 21:03, Heiko Stuebner wrote: > > > going with the vcc5v0_host regulator of the rk3588-quartzpro64 and > > > > > > +1. compatible > > > +2. reg > > > +3. ranges > > > +4. All properties with values > > > +5. Boolean properties > > > +6. status (if applicable) > > > +7. Child nodes > > > > > > we'd end up with > > > > > > vcc5v0_host: vcc5v0-host-regulator { > > > /* 1. */ compatible = "regulator-fixed"; > > > /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > > > pinctrl-names = "default"; > > > pinctrl-0 = <&vcc5v0_host_en>; > > > regulator-min-microvolt = <5000000>; > > > regulator-max-microvolt = <5000000>; > > > regulator-name = "vcc5v0_host"; > > > vin-supply = <&vcc5v0_usb>; > > > /* 5. */ enable-active-high; > > > regulator-always-on; > > > regulator-boot-on; > > > }; > > > > > > > How about grouping like properties (defined in the same schema), > > then sorting within that group. Would also allow for defining > > where to add spacing. > > > > 1. compatible > > 2. reg > > 3. ranges > > 4. All property groups > > 4.1 Properties with values > > 4.2 Boolean properties > > 4.3 Separating space > > 6. status (if applicable) > > 7. Child nodes > > > > Your node then would look like we expect: > > > > vcc5v0_host: vcc5v0-host-regulator { > > /* 1 */ compatible = "regulator-fixed"; > > > > /* 4.1 */ pinctrl-names = "default"; > > /* 4.1 */ pinctrl-0 = <&vcc5v0_host_en>; > > /* 4.3 */ > > /* 4.1 */ regulator-min-microvolt = <5000000>; > > /* 4.1 */ regulator-max-microvolt = <5000000>; > > /* 4.1 */ regulator-name = "vcc5v0_host"; > > /* 4.2 */ regulator-always-on; > > /* 4.2 */ regulator-boot-on; > > /* 4.2 */ enable-active-high; > > /* 4.3 */ > > /* 4.1 */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > > ... > > }; > > I'm really not sure about adding big sets of rules. > In the above example you'd also need to define which schema has a higher > priority? ;-) > > > When I started with Rockchip stuff, I also had some fancy way of sorting > elements in mind that was really intuitive to myself :-) . > Over time I realized that it was quite complex - especially when I had to > explain it to people. Feel familiar. > There are definite advantages for having compatible + reg + status in > fixed positions, as it helps going over a whole dt to spot the huge > mistakes (accidentially disabled, wrong address), but for the rest a > simple alphabetical sorting is easiest to explain to people :-) . > > And alphabetic elements are also easier on my eyes. Alphabetical does break logical ordering and grouping. Apart from compatible/reg order, on Renesas SoCs we usually also have the clocks/resets/power-domains block. > I just think having a short clean set of rules like Krzysztof proposed, > is easier to follow and "enforce" and also most likely doesn't deter > people from contributing, if mainline work is not their main occupation. And in reality, most of this is created by copy-and-corrupt^Wmodify... Gr{oetje,eeting}s, Geert
On 16/11/2023 21:33, Heiko Stuebner wrote: >> regulator-state-mem { >> regulator-off-in-suspend; >> }; >> >> What exactly would be here misordered? > > going with the vcc5v0_host regulator of the rk3588-quartzpro64 and > > +1. compatible > +2. reg > +3. ranges > +4. All properties with values > +5. Boolean properties > +6. status (if applicable) > +7. Child nodes > > we'd end up with > > vcc5v0_host: vcc5v0-host-regulator { > /* 1. */ compatible = "regulator-fixed"; > /* 4. */ gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>; > pinctrl-names = "default"; > pinctrl-0 = <&vcc5v0_host_en>; > regulator-min-microvolt = <5000000>; > regulator-max-microvolt = <5000000>; > regulator-name = "vcc5v0_host"; > vin-supply = <&vcc5v0_usb>; > /* 5. */ enable-active-high; > regulator-always-on; > regulator-boot-on; > }; > > which I find somewhat counter-intuitive ;-) . Yes, good point. Best regards, Krzysztof
On 16/11/2023 22:27, Conor Dooley wrote: > On Thu, Nov 16, 2023 at 07:12:18PM +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. > > This is a good idea IMO, thanks for writing it. > > Should we also mention in this about the expected breakdown of things > between $soc.dtsi and $soc-$board.dtsi? There's commonly confusion about > things like oscillators and where they belong - particularly when the > SoC mandates that these oscillators be of a single frequency. > It may seem obvious to us what goes where, but certainly contributors > frequently get this wrong. Sure, I can add a chapter about DTS/DTSI split. Best regards, Krzysztof
On 18/11/2023 00:53, Konrad Dybcio wrote: > On 16.11.2023 19:12, 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Bjorn Andersson <andersson@kernel.org> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Heiko Stuebner <heiko@sntech.de> >> 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> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- > [...] > >> +Order of Nodes >> +-------------- >> + >> +1. Nodes within any bus, thus using unit addresses for children, shall be >> + ordered incrementally by unit address. >> + >> +2. Nodes without unit addresses should be ordered alpha-numerically. > I largely agree with all this given our fight for some level of > tidyness across linux-msm, but there's one specific case where I think > it would make sense to break this ordering, and that's GPIO states: > > > &pinctrl { > xyz-active-state { > pins ="gpio0"; > ... > }; > > abc-active-state { > pins ="gpio1"; > ... > }; > > qwe-active-state { > pins ="gpio2"; > ... > }; > }; > > looks so much more readable to me than True. I'll mention exception for this and Renesas. Best regards, Krzysztof
On 16/11/2023 22:32, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Thu, Nov 16, 2023 at 7:12 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. > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Thanks for your patch! > >> --- a/Documentation/devicetree/bindings/index.rst >> +++ b/Documentation/devicetree/bindings/index.rst > >> +Order of Nodes >> +-------------- >> + >> +1. Nodes within any bus, thus using unit addresses for children, shall be >> + ordered incrementally by unit address. > > In Renesas DTS files, we have an (unwritten) additional rule: when there > are multiple nodes of the same type, they do not follow the global > ordering by unit-address, but are grouped together. > E.g. the second and any subsequent serial ports are always listed > immediately after the first serial port. OK, I'll add here some exception paragraph. pin-state nodes also need one. > >> +2. Nodes without unit addresses should be ordered alpha-numerically. >> + >> +3. When extending nodes in board DTS via &label, the entries should be ordered >> + alpha-numerically. > > >> +Order of Properties in Device Node >> +---------------------------------- >> + >> +Following order of properties in device nodes is preferred: >> + >> +1. compatible >> +2. reg >> +3. ranges >> +4. All properties with values >> +5. Boolean properties >> +6. status (if applicable) > > There must be a lank line between properties and child nodes? +1 > > And once we have a documented coding style, the next long-standing item > from our wishlist would be a script to sort the contents of a DTS file > according to the rules ;-) Yeah... Best regards, Krzysztof
On 16/11/2023 21:44, Rob Herring wrote: >> >> Merging idea: Rob/DT bindings >> --- >> Documentation/devicetree/bindings/index.rst | 1 + >> .../devicetree/bindings/writing-dts.rst | 137 ++++++++++++++++++ >> 2 files changed, 138 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/writing-dts.rst > > Perhaps dts-coding-style.rst Ack > > After adding writing-schema, I realized the difference between > writing-schema and writing-bindings isn't all that clear. I never got > around to renaming things. > >> >> diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst >> index d9002a3a0abb..975449be4862 100644 >> --- a/Documentation/devicetree/bindings/index.rst >> +++ b/Documentation/devicetree/bindings/index.rst >> @@ -5,5 +5,6 @@ >> >> ABI >> writing-bindings >> + writing-dts >> writing-schema >> submitting-patches >> diff --git a/Documentation/devicetree/bindings/writing-dts.rst b/Documentation/devicetree/bindings/writing-dts.rst >> new file mode 100644 >> index 000000000000..10c477ec1eed >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/writing-dts.rst >> @@ -0,0 +1,137 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> +.. _writingdts: >> + >> +=================================================== >> +Writing Devicetree Sources (DTS) - 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 >> +--------------------------- >> + >> +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 should use lowercase hex, without leading zeros (padding). > > Strictly speaking, the unit-address is whatever a bus defines, but > yes, by default, that is the format. > >> + >> +4. Hex values in properties, e.g. "reg", should use lowercase hex. Any 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. >> + >> +2. Nodes without unit addresses should be ordered alpha-numerically. >> + >> +3. When extending nodes in board DTS via &label, the entries should be ordered >> + alpha-numerically. > > Or matching the original node definition order? If there is any sub-arch using such style, then sure, why not. Otherwise if we do not have such examples, I find that a bit tricky to implement: for patches adding new board, one needs to check the original DTSI for order. > >> + >> +Example:: >> + >> + // SoC DTSI >> + >> + \ { > > / { > >> + cpus { >> + // ... >> + }; >> + >> + psci { >> + // ... >> + }; >> + >> + soc@ { >> + dma: dma-controller@10000 { >> + // ... >> + }; >> + >> + clk: clock-controller@80000 { >> + // ... >> + }; >> + }; >> + }; >> + >> + // Board DTS >> + >> + &clk { >> + // ... >> + }; >> + >> + &dma { >> + // ... >> + }; >> + >> + >> +Order of Properties in Device Node >> +---------------------------------- >> + >> +Following order of properties in device nodes is preferred: >> + >> +1. compatible >> +2. reg >> +3. ranges > >> +4. All properties with values >> +5. Boolean properties > > I make this like schemas, standard/common properties first, then > vendor specific properties. Sure. > >> +6. status (if applicable) >> +7. Child nodes >> + >> +The "status" property is by default "okay", thus it can be omitted. >> + >> +Example:: >> + >> + // SoC DTSI >> + >> + usb_1_hsphy: phy@88e3000 { >> + compatible = "qcom,sm8550-snps-eusb2-phy"; >> + reg = <0x0 0x088e3000 0x0 0x154>; >> + #phy-cells = <0>; >> + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>; >> + status = "disabled"; >> + }; >> + >> + // Board DTS >> + >> + &usb_1_hsphy { >> + clocks = <&tcsr TCSR_USB2_CLKREF_EN>; >> + clock-names = "ref"; >> + status = "okay"; >> + }; >> + >> + >> +Indentation >> +----------- >> + >> +1. Use indentation according to :ref:`codingstyle`. >> +2. For arrays spanning across lines, preferred is to align the continued >> + entries with opening < from first line. >> + >> +Example:: >> + >> + thermal-sensor@c271000 { >> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; >> + reg = <0x0 0x0c271000 0x0 0x1000>, >> + <0x0 0x0c222000 0x0 0x1000>; > > You should cover the <> style too, meaning <> around each logical entry. Ack. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/index.rst b/Documentation/devicetree/bindings/index.rst index d9002a3a0abb..975449be4862 100644 --- a/Documentation/devicetree/bindings/index.rst +++ b/Documentation/devicetree/bindings/index.rst @@ -5,5 +5,6 @@ ABI writing-bindings + writing-dts writing-schema submitting-patches diff --git a/Documentation/devicetree/bindings/writing-dts.rst b/Documentation/devicetree/bindings/writing-dts.rst new file mode 100644 index 000000000000..10c477ec1eed --- /dev/null +++ b/Documentation/devicetree/bindings/writing-dts.rst @@ -0,0 +1,137 @@ +.. SPDX-License-Identifier: GPL-2.0 +.. _writingdts: + +=================================================== +Writing Devicetree Sources (DTS) - 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 +--------------------------- + +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 should use lowercase hex, without leading zeros (padding). + +4. Hex values in properties, e.g. "reg", should use lowercase hex. Any 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. + +2. Nodes without unit addresses should be ordered alpha-numerically. + +3. When extending nodes in board DTS via &label, the entries should be ordered + alpha-numerically. + +Example:: + + // SoC DTSI + + \ { + cpus { + // ... + }; + + psci { + // ... + }; + + soc@ { + dma: dma-controller@10000 { + // ... + }; + + clk: clock-controller@80000 { + // ... + }; + }; + }; + + // Board DTS + + &clk { + // ... + }; + + &dma { + // ... + }; + + +Order of Properties in Device Node +---------------------------------- + +Following order of properties in device nodes is preferred: + +1. compatible +2. reg +3. ranges +4. All properties with values +5. Boolean properties +6. status (if applicable) +7. Child nodes + +The "status" property is by default "okay", thus it can be omitted. + +Example:: + + // SoC DTSI + + usb_1_hsphy: phy@88e3000 { + compatible = "qcom,sm8550-snps-eusb2-phy"; + reg = <0x0 0x088e3000 0x0 0x154>; + #phy-cells = <0>; + resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>; + status = "disabled"; + }; + + // Board DTS + + &usb_1_hsphy { + clocks = <&tcsr TCSR_USB2_CLKREF_EN>; + clock-names = "ref"; + status = "okay"; + }; + + +Indentation +----------- + +1. Use indentation according to :ref:`codingstyle`. +2. For arrays spanning across lines, preferred is to align the continued + entries with opening < from first line. + +Example:: + + thermal-sensor@c271000 { + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2"; + reg = <0x0 0x0c271000 0x0 0x1000>, + <0x0 0x0c222000 0x0 0x1000>; + };