diff mbox series

spi: meson-spicc: Use GPIO descriptors

Message ID 20191205083915.27650-1-linus.walleij@linaro.org
State Accepted
Commit cd8fb859a84c1c4f985a582b79050116d2c30c47
Headers show
Series spi: meson-spicc: Use GPIO descriptors | expand

Commit Message

Linus Walleij Dec. 5, 2019, 8:39 a.m. UTC 
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@baylibre.com>
Cc: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/spi/spi-meson-spicc.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

-- 
2.23.0

Comments

Neil Armstrong Dec. 5, 2019, 9:12 a.m. UTC | #1 
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@baylibre.com>

> Cc: Sunny Luo <sunny.luo@amlogic.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

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

Neil
Linus Walleij Dec. 13, 2019, 10:08 a.m. UTC | #2 
On Thu, Dec 5, 2019 at 10:12 AM Neil Armstrong <narmstrong@baylibre.com> 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@baylibre.com>

> > Cc: Sunny Luo <sunny.luo@amlogic.com>

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> > ---

> >  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
Neil Armstrong Dec. 13, 2019, 10:11 a.m. UTC | #3 
On 13/12/2019 11:08, Linus Walleij wrote:
> On Thu, Dec 5, 2019 at 10:12 AM Neil Armstrong <narmstrong@baylibre.com> 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@baylibre.com>

>>> Cc: Sunny Luo <sunny.luo@amlogic.com>

>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>>> ---

>>>  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@baylibre.com>


> 

> Yours,

> Linus Walleij

>
diff mbox series

Patch

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)