Re: [PATCH 1/1] arm64: dts: qcom: sc7280: Add device tree for herobrine evoker

[Doug Anderson <dianders@chromium.org>]

Hi,

On Tue, Aug 30, 2022 at 9:50 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/08/2022 19:10, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Aug 30, 2022 at 2:33 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >>> +};
> >>> +
> >>> +/*
> >>> + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES
> >>
> >> What does it mean and why it's SCREAMING?
> >>> + *
> >>> + * Sort order matches the order in the parent files (parents before children).
> >>
> >> Why? Sorting should be rather alphabetical.
> >
> > We've had this discussion on the lists in the past. See, for instance:
> >
> > https://lore.kernel.org/r/CAD=FV=U2C1W+JHWyGRfyRB=WiPKLYvtjO90UDoJ9p+Xwe09+ow@mail.gmail.com/
>
> Good explanation, such sorting rule is quite nice. The part about
> regulators is still a bit confusing, I guess it is about some other
> pieces in some other place?

Yeah, we originally started with the regulator sorting of "parents
above children" long ago when it helped avoid some cases of
-EPROBE_DEFER in Linux. The -EPROBE_DEFER isn't a reason these days,
but when I looked back at it I decided that I quite liked "parents
above children" and that it matched my mental model.

Specifically, take a look at
"/sys/kernel/debug/regulator/regulator_summary". Parent regulators are
listed above child regulators because it makes the most sense to think
of the regulator tree. Obviously we can only do this in the dts for
regulators that are separate nodes and not ones provided by a big
PMIC, but we often end up with quite a few of those in the end.

In "child" device trees that are overriding a single regulator (like
evoker) the comment is a bit nonsensical, of course. I'd be OK with
removing the "Sort order matches the order in the parent files
(parents before children)." in the evoker device tree since there's
really only one regulator in this section. The only downside would be
that when someone adds that second regulator then they might not know
the sort ordering. ...so I would be fine keeping it too...


> But isn't this kind of obvious from
> including other DTSI?

Isn't what kind of obvious from including the other DTSI? That the
sort order should match the sort order of the parent for this section?
It wasn't obvious to me. Since there are usually just a few regulators
that referenced like this it seemed like it might be easiest to just
alphabetize them in the child device trees. ...but I settled on
thinking that matching the parent was marginally better. Since I
debated it myself I decided it was probably better to comment so
others understood the sort order...

-Doug