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

[Neil Armstrong <narmstrong@xxxxxxxxxxxx>]

On 13/12/2019 11:08, Linus Walleij wrote:
> 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.

Bingo !

LGTM !

Acked-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>

> 
> Yours,
> Linus Walleij
>