Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Doug Anderson]

Hi,

On Mon, Oct 3, 2022 at 10:57 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 03/10/2022 17:40, Doug Anderson wrote:
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> index 132417e2d11e..a157eab66dee 100644
> >>>> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> @@ -1123,7 +1123,9 @@ &wifi {
> >>>>
> >>>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
> >>>>  &qup_spi2_default {
> >>>> -       drive-strength = <16>;
> >>>> +       pinmux {
> >>>> +               drive-strength = <16>;
> >>>> +       };
> >>>
> >>> The convention on Qualcomm boards of this era is that muxing (setting
> >>> the function) is done under a "pinmux" node and, unless some of the
> >>> pins need to be treated differently like for the UARTs, configuration
> >>> (bias, drive strength, etc) is done under a "pinconf" subnode.
> >>
> >> Yes, although this was not expressed in bindings.
> >>
> >>> I
> >>> believe that the "pinconf" subnode also needs to replicate the list of
> >>> pins, or at least that's what we did everywhere else on sdm845 /
> >>> sc7180.
> >>
> >> Yes.
> >>
> >>>
> >>> Thus to match conventions, I assume you'd do:
> >>>
> >>> &qup_spi2_default {
> >>>   pinconf {
> >>
> >> No, because I want a convention of all pinctrl bindings and drivers, not
> >> convention of old pinctrl ones. The new ones are already moved or being
> >> moved to "-state" and "-pins". In the same time I am also unifying the
> >> requirement of "function" property - enforcing it in each node, thus
> >> "pinconf" will not be valid anymore.
> >
> > Regardless of where we want to end up, it feels like as of ${SUBJECT}
> > patch this should match existing conventions in this file. If a later
> > patch wants to change the conventions in this file then it can, but
> > having just this one patch leaving things in an inconsistent state
> > isn't great IMO...
> >
> > If this really has to be one-off then the subnode shouldn't be called
> > "pinmux". A subnode called "pinmux" implies that it just has muxing
> > information in it. After your patch this is called "pinmux" but has
> > _configuration_ in it.
> >
>
> It is a poor argument to keep some convention which is both
> undocumented, not kept in this file and known only to some folks
> (although that's effect of lack of documentation). Even the bindings do
> not say it should be "pinconf" but they mention "config" in example. The
> existing sdm845.dts uses config - so why now there should be "pinconf"?
> By this "convention" we have both "pinmux" and "mux", perfect. Several
> other pins do not have pinmux/mux/config at all.
>
> This convention was never implemented, so there is nothing to keep/match.
>
> Changing it to "config" (because this is the most used "convention" in
> the file and bindings) would also mean to add useless "pins" which will
> be in next patch removed.

I certainly won't make the argument that the old convention was great
or even that consistently followed. That's why it changed. ...but to
me it's more that a patch should stand on its own and not only make
sense in the context of future patches. After applying ${SUBJECT}
patch you end up with a node called "pinmux" that has more than just
muxing information in it. That seems less than ideal.

-Doug