Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck <thesven73@xxxxxxxxx>]

Mike, Linus, Andy, XinHao,

On Fri, May 7, 2021 at 5:33 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> But I think Andy's approach is the best.

I too like Andy's approach. It would fix XinHao's use case (acpi) and
not break OF.

But, we have to be careful not to put work-around on top of
work-around, and complicate the code too much. Sometimes when logic
gets hard to follow, there's an opportunity to refactor and simplify.
Perhaps if we put our heads together here, we can find that
opportunity? Let's try?

For reasons explained below, the gpiod OF code moves the SPI
chip-select inverting logic from the SPI mode flags to the gpiods. If
I understand XinHao/Andy's problem correctly, this breaks ACPI
chip-selects, because the ACPI gpio code has no way to know that it's
part of a SPI bus, and apply quirks. Does that sound right so far?

If we were able to store the polarity in the SPI mode flag, then we
could refactor very elegantly:

1. Simplify Linus's OF gpio quirks, so:
- they print warnings in case of polarity conflicts
- but no longer change the gpiod polarity (i.e. they just keep what
was specified in OF)

2. Drive the gpiod chip select using the raw value, ignoring the
built-in polarity, which treats it the same as a gpio_xxx. Nice!

in driver/spi/spi.c:
+       /*
+        * invert the enable line, as active low is
+        * default for SPI.
+        */
        if (spi->cs_gpiod)
-               /* polarity handled by gpiolib */
-               gpiod_set_value_cansleep(spi->cs_gpiod,
-                                        enable1);
+               gpiod_set_raw_value_cansleep(spi->cs_gpiod, !enable);
        else
-               /*
-                * invert the enable line, as active low is
-                * default for SPI.
-                */
                gpio_set_value_cansleep(spi->cs_gpio, !enable);

Andy/XinHao, is it correct that this will fix the ACPI case?
Example: enable ACPI CS when SPI_CS_HIGH:
        enable = true
        mode & SPI_CS_HIGH => invert enable => false
        gpiod_set_raw_value_cansleep(!enable => true)
        ACPI gpiod: always active high
        chip select goes high.

Now we get to the tricky bit. Storing the polarity in the SPI mode
breaks a lot of OF spi client drivers. Why? Hardware designers love to
put chip-selects behind inverting gates. This is quite common in case
a voltage domain shift is needed - a single transistor will work, but
is inverting. So depending on the hardware topology (OF), sometimes
client device X has SPI_CS_HIGH set, sometimes it doesn't.

That would all be fine, but... a common pattern in SPI client drivers is this:

drivers/net/phy/spi_ks8995.c:
        spi->mode = SPI_MODE_0;
        spi->bits_per_word = 8;
        err = spi_setup(spi);

In its zeal to specify the correct mode, the driver "plows" right over
the SPI_CS_HIGH mode flag. That'll break stuff.

If there was some way to prevent this from happening, we could make
our code a lot simpler. So I'd like to reach out to Mike Brown to hear
his opinion.

In case of a SPI bus described by OF or ACPI, the mode flags have
already been filled out, so there should be no need for the
initialization in the driver? Could we perhaps replace the pattern
with the following code?

        spi->mode = spi->mode ? : SPI_MODE_0;
        spi->bits_per_word = 8;
        err = spi_setup(spi);

I am not sure in which cases the spi->mode has not been filled out
yet. I live mostly in the OF world, so I'm a bit myopic here.

Even if Mike Brown agrees to change the pattern, it still means lots
of changes to spi client drivers, all over the place. So in terms of
stability, Andy's solution might be preferable.

Looking forward to hearing your opinions,
Sven