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

[Doug Anderson]

Hi,

On Sat, Oct 1, 2022 at 3:01 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 30/09/2022 22:12, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> The pin configuration (done with generic pin controller helpers and
> >> as expressed by bindings) requires children nodes with either:
> >> 1. "pins" property and the actual configuration,
> >> 2. another set of nodes with above point.
> >>
> >> The qup_spi2_default pin configuration used second method - with a
> >> "pinmux" child.
> >>
> >> Fixes: 8d23a0040475 ("arm64: dts: qcom: db845c: add Low speed expansion i2c and spi nodes")
> >> Cc: <stable@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >>
> >> ---
> >>
> >> Not tested on hardware.
> >> ---
> >>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> 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.


> >     pins = "gpio27", "gpio28", "gpio29", "gpio30";
> >     drive-strength = <16>;
> >   };
> > };
> >
> > We've since moved away from this to a less cumbersome approach, but
> > for "older" boards like db845c we should probably match the existing
> > convention, or have a flag day and change all sdm845 boards over to
> > the new convention.
>
> That's what my next patchset from yesterday was doing. Unifying the
> bindings with modern bindings and converting DTS to match them.
>
> https://lore.kernel.org/linux-devicetree/20220930200529.331223-1-krzysztof.kozlowski@xxxxxxxxxx/T/#t

I wasn't CCed on that patch series. A few things jump out as not quite
right to me. I'll try to do a review.

-Doug