Re: [PATCH] gpio: of: Handle SPI chipselect legacy bindings

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Sep 5, 2018 at 9:30 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:

> > We check for the property "cs-active-high" and if that is
>
> There are no users of this property => "spi-cs-high" (everywhere!)?

Sorry for not drinking coffee before patching :/

> > If the line is tagged as active high in the device tree with
> > the second cell flag and has no "cs-active-high" property we
> > enforce active low semantics (as this is the exception we can
> > just tag on the flag).
>
> My initial feeling said "spi-cs-high" is meant for native chipselects, not
> for GPIO chipselects.

I will try to not touch the native chipselects right now, I see that
as a SPI problem not a GPIO problem. Those indeed assume
active low as the default as well.

> The latter already has a standard way to specify
> polarity (the second cell).

Exactly and that is what I want to get to.

AFAICT what all drivers are doing
(including the two that actually use descriptors for cs) is get the
GPIO, assume it is set up as active high (i.e. no flag) then look at
spi->mode & SPI_CS_HIGH to determine if the line is instead
active high and in that case explicitly drive it high inside the SPI
master driver.

So I want to pull this into the gpiolib core and get rid of this
complexity from the SPI masters.

> So this has the potential of breaking things, and deserves a warning.

Hm I don't know about that, assuming it is active low and only
make it high on demand is what all drivers seem to be doing
right now, so there will be some warnings for things that was
business as usual before.

But indeed it is nicer if the flags agree. What about dev_info()?

> Hence what if both tagged active high with the second cell flag, and with
> "cs-active-high"?
> Cfr. arch/arm/boot/dts/imx27-phytec-phycore-som.dtsi
> arch/arm/boot/dts/imx51-babbage.dts
> arch/arm/boot/dts/imx51-digi-connectcore-som.dtsi
> arch/arm/boot/dts/imx51-zii-scu2-mezz.dts
> arch/arm/boot/dts/imx6q-evi.dts
> arch/arm/boot/dts/imx51-zii-rdu1.dts
> arch/arm/boot/dts/vf610-zii-dev-rev-b.dts

Active high is fine, because what all drivers do as I state
above is simply assume that no matter where the GPIO came
from, whether device tree or board file.

Then the SPI core code inverse the polarity internally.

So I want to move that semantic over to gpiolib.

> The last two are even more interesting, as they use spi-gpio, so there
> are 3 flags
> impacting chip select polarity (gpio-sck cell 2, cs-gpios cell 2,
> cs-active-high).

Currently I am dealing with the two latter, cs-gpios and
cs-active-high.

Maybe I should look into the clock as well. Probably possible
to deal with in separate patches.

> Looks like the current expectation is that all two (or three) should agree?

I think they all assume that the GPIO line is tagged as active
high and all polarity inversion lives inside the SPI core and
drivers.

> That complicates DT overlays adding an SPI device to the bus, as
> "cs-active-high" is a property of the device added, while modifying
> "cs-gpios" means touching the controller's property (which in the case of
> DT overlays used for connectors may or may not be present, as the connector
> is the abstraction layer, and the underlying SPI controller may use native
> or GPIO chip selects).

The more I look at device tree overlays the less I believe in
it. But I'm a dissident. I don't want to distance myself from
the people driving the effort so I try to be diplomatic and see
if I can get it working anyways. The concept seems simple,
the devil is in the details.

I guess I work with the systems we have and try to make
reasonable API work so that these things can get a reasonable
encapsulation.

Yours,
Linus Walleij