Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 04 Feb 17:31 CST 2021, Doug Anderson wrote:
> On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote:
[..]
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > index 3cea28058a91..03015174ec06 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > @@ -3046,6 +3046,25 @@ config {
> > >                               };
> > >                       };
> > >
> > > +                     qup_spi0_cs_gpio: qup-spi0-cs-gpio {
> >
> > There might be others who need the same states, but I would prefer if we
> > move this to the device's dts.
> 
> This is opposite to what Stephen requested, though it was in a review
> on our gerrit and not on lists [1].  :-P
> 
> It definitely feels like a 6 of one half dozen of the other.  Unless
> you're dead set on moving them to the board dts my bias would be
> towards keeping consistent with what was done on sc7180.  If you
> really want this moved to the board file we should do it for sc7180
> too.
> 

What I dislike is the fact that we have a huge amount of these unused in
the platform.dtsi, but let's align with what I agreed to on
sc7180...Sorry for the short memory.

> 
> > > +                             mux {
> >
> > Rather than splitting the properties in {mux, cs, config} I think it
> > makes more sense to split them in {spi, cs} or something like that.
> 
> In general pinconf doesn't belong in the SoC dts file.  If there's no
> reason to change it seems like this should match what sc7180 did.
> 

Right, but I still would prefer the pinctrl state to be split by
function/pins, rather than pinmux vs pinconf. That way it's natural to
add pinconf properties to the various functional parts (i.e. bias or
drive-strength for the spi pins vs cs).

Do you have any concerns with this?

Regards,
Bjorn

> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi