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

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

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