Re: [PATCH] spi: meson-spicc: Use GPIO descriptors

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Dec 5, 2019 at 10:12 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
> On 05/12/2019 09:39, Linus Walleij wrote:
> > Instead of grabbing GPIOs using the legacy interface and
> > handling them in the setup callback, just let the core
> > grab and use the GPIOs using descriptors.
> >
> > Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > Cc: Sunny Luo <sunny.luo@xxxxxxxxxxx>
> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> >  drivers/spi/spi-meson-spicc.c | 25 ++-----------------------
> >  1 file changed, 2 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> > index f3f10443f9e2..7f5680fe2568 100644
> > --- a/drivers/spi/spi-meson-spicc.c
> > +++ b/drivers/spi/spi-meson-spicc.c
> > @@ -19,7 +19,6 @@
> >  #include <linux/types.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/reset.h>
> > -#include <linux/gpio.h>
> >
> >  /*
> >   * The Meson SPICC controller could support DMA based transfers, but is not
> > @@ -467,35 +466,14 @@ static int meson_spicc_unprepare_transfer(struct spi_master *master)
> >
> >  static int meson_spicc_setup(struct spi_device *spi)
> >  {
> > -     int ret = 0;
> > -
> >       if (!spi->controller_state)
> >               spi->controller_state = spi_master_get_devdata(spi->master);
> > -     else if (gpio_is_valid(spi->cs_gpio))
> > -             goto out_gpio;
> > -     else if (spi->cs_gpio == -ENOENT)
> > -             return 0;
> > -
> > -     if (gpio_is_valid(spi->cs_gpio)) {
> > -             ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
> > -             if (ret) {
> > -                     dev_err(&spi->dev, "failed to request cs gpio\n");
> > -                     return ret;
> > -             }
> > -     }
> > -
> > -out_gpio:
> > -     ret = gpio_direction_output(spi->cs_gpio,
> > -                     !(spi->mode & SPI_CS_HIGH));
> >
> > -     return ret;
> > +     return 0;
> >  }
> >
> >  static void meson_spicc_cleanup(struct spi_device *spi)
> >  {
> > -     if (gpio_is_valid(spi->cs_gpio))
> > -             gpio_free(spi->cs_gpio);
> > -
> >       spi->controller_state = NULL;
> >  }
> >
> > @@ -564,6 +542,7 @@ static int meson_spicc_probe(struct platform_device *pdev)
> >       master->prepare_message = meson_spicc_prepare_message;
> >       master->unprepare_transfer_hardware = meson_spicc_unprepare_transfer;
> >       master->transfer_one = meson_spicc_transfer_one;
> > +     master->use_gpio_descriptors = true;
> >
> >       /* Setup max rate according to the Meson GX datasheet */
> >       if ((rate >> 2) > SPICC_MAX_FREQ)
> >
>
> Hmm, I did this because the SPI core was buggy, so I assume it has been fixed ?
>
> gpio_request/free was not done by the core, thus needing to be done in the setup/cleanup callback.

Yes and no. I convert to using descriptors which are devm managed
resources.

If you use the legacy API for requesting GPIO (by number) which is
what happens without ->use_gpio_descriptors then this need for
the driver to request the GPIOs still exist.

Here we solve two problems at the same time:

- Get rid of the need to explicitly request the lines
- Convert to use descriptors

Isn't it great.

Yours,
Linus Walleij