[1/4,v3] spi: Support high CS when using descriptors

All controllers using GPIO descriptors can by definition
support high CS connections, so just enforce this when
registering an SPI controller.

This fixes a regression where controllers were missing
SPI_CS_HIGH, the drivers would fail like this:

spi spi0.0: setup: unsupported mode bits 4
cdns-spi fd0b0000.spi: can't setup spi0.0, status -22

This is because as using descriptors moves the CS inversion
logic over to gpiolib, all such controllers are registered
with CS active high.

Cc: Jan Kotas <jank@cadence.com>
Reported-by: Jan Kotas <jank@cadence.com>
Tested-by: Jan Kotas <jank@cadence.com>
Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v3:
- Collected Jan's Tested-by
---
 drivers/spi/spi.c | 5 +++++
 1 file changed, 5 insertions(+)

Hi Linus,

On Wed, Jan 16, 2019 at 7:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> All controllers using GPIO descriptors can by definition
> support high CS connections, so just enforce this when
> registering an SPI controller.

But that is guaranteed to be true only for chip selects handled by a GPIO,
right?
Native chip selects may still not support SPI_CS_HIGH, depending
on the controller.
Before, the bad_bits check in spi_setup() would detect this, and return
an error. After, this will fail silently.

I agree configuring the system like this is a mistake by the integrator,
to be detected during integration testing.

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2336,6 +2336,11 @@ int spi_register_controller(struct spi_controller *ctlr)
>                         status = spi_get_gpio_descs(ctlr);
>                         if (status)
>                                 return status;
> +                       /*
> +                        * A controller using GPIO descriptors always
> +                        * supports SPI_CS_HIGH if need be.
> +                        */
> +                       ctlr->mode_bits |= SPI_CS_HIGH;
>                 } else {
>                         /* Legacy code path for GPIOs from DT */
>                         status = of_spi_register_master(ctlr);

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

On Wed, Apr 03, 2019 at 10:34:58AM +0200, Geert Uytterhoeven wrote:
> On Wed, Jan 16, 2019 at 7:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > All controllers using GPIO descriptors can by definition
> > support high CS connections, so just enforce this when
> > registering an SPI controller.

> But that is guaranteed to be true only for chip selects handled by a GPIO,
> right?
> Native chip selects may still not support SPI_CS_HIGH, depending
> on the controller.
> Before, the bad_bits check in spi_setup() would detect this, and return
> an error. After, this will fail silently.

> I agree configuring the system like this is a mistake by the integrator,
> to be detected during integration testing.

Yeah, it's kind of unfortunate that the flags are set at the controller
level rather than at the chip select level as especially in this case
it's likely that capabilities will diverge.

On Wed, Apr 3, 2019 at 3:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Jan 16, 2019 at 7:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > All controllers using GPIO descriptors can by definition
> > support high CS connections, so just enforce this when
> > registering an SPI controller.
>
> But that is guaranteed to be true only for chip selects handled by a GPIO,
> right?
> Native chip selects may still not support SPI_CS_HIGH, depending
> on the controller.
> Before, the bad_bits check in spi_setup() would detect this, and return
> an error. After, this will fail silently.

Yes but only for systems that use descriptors all the way. So dealing
with this is part of the process of converting to using descriptors.
(Thus the other patches in this series.)

Do we have some hardware that supports only active low native
CS but also want to use GPIOs? Because then maybe I should
take a stab at that in particular. If I keep converting drivers to this
then I will hit my head into it sooner or later I suppose.

Yours,
Linus Walleij

Hi Linus,

On Wed, Apr 3, 2019 at 6:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 3, 2019 at 3:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jan 16, 2019 at 7:27 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > All controllers using GPIO descriptors can by definition
> > > support high CS connections, so just enforce this when
> > > registering an SPI controller.
> >
> > But that is guaranteed to be true only for chip selects handled by a GPIO,
> > right?
> > Native chip selects may still not support SPI_CS_HIGH, depending
> > on the controller.
> > Before, the bad_bits check in spi_setup() would detect this, and return
> > an error. After, this will fail silently.
>
> Yes but only for systems that use descriptors all the way. So dealing
> with this is part of the process of converting to using descriptors.
> (Thus the other patches in this series.)

Well, if the it worked before (no error), it should work after the conversion.
The error is handy for new (future) users.

> Do we have some hardware that supports only active low native
> CS but also want to use GPIOs? Because then maybe I should
> take a stab at that in particular. If I keep converting drivers to this
> then I will hit my head into it sooner or later I suppose.

There may be hardware like that.  The integrator should take care of
it[*].

[*] Until we manage to describe functional relations in DT instead of explicit
    GPIO/native <whatever>/IRQn relations. After that
      - drivers can switch from native to GPIO chip select automatically, when
        some native feature is missing,
      - the system can choose to use spi-gpio or i2c-gpio if no driver is
        available for the hardware SPI or I2C controller on the same pins,
      - the system can pinmux between a GPIO with interrupt functionality
        or a real interrupt controller pin, depending on availability.

Gr{oetje,eeting}s,

                        Geert