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

["Heiko Stübner" <heiko@sntech.de>]

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