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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Linus,

On Tue, Sep 4, 2018 at 2:50 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> The SPI chipselect are assumed to be active low in the current
> binding, so when we want to use GPIO descriptors and handle
> the active low/high semantics in gpiolib, we need a special
> parsing quirk to deal with this.
>
> We check for the property "cs-active-high" and if that is

There are no users of this property => "spi-cs-high" (everywhere!)?

> NOT present we assume the CS line is active low.
>
> If the line is tagged as active low in the device tree and
> has no "cs-active-high" property all is fine, the device
> tree and the SPI bindings are in agreement.

OK.

> 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.  The latter already has a standard way to specify
polarity (the second cell).
So this has the potential of breaking things, and deserves a warning.

However, on second thought, the "cs-gpios" property is a property of the
SPI controller, so it can be assumed to follow the SPI standard (active
low) convention.
While "spi-cs-high" is a property of the SPI device on the SPI bus, and
thus inverts the standard behavior.
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

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).

Looks like the current expectation is that all two (or three) should agree?
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).

> If the line is tagged as active low with the second cell flag
> AND tagged with "cs-active-high" the SPI active high property
> takes precedence and we print a warning.

OK, makes sense.

>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: linux-spi@xxxxxxxxxxxxxxx
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> This will be merged as a precursor to a series switching
> over to using GPIO descriptors for chip select handling
> in the SPI subsystem. Currently no descriptors are used
> for chip selects so the impact will be zero until we start
> switching drivers over.

drivers/spi/spi-pxa2xx.c and drivers/spi/spi-sh-msiof.c do use descriptors
for chip selects.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds