Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647618 > unrolled thread

[PATCH 2/2] spi: use gpio_desc instead of numeric gpio

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Chris Packham <chris.packham@alliedtelesis.co.nz> - 2017-05-23 06:10 +0200
    Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-23 20:30 +0200
      Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Chris Packham <Chris.Packham@alliedtelesis.co.nz> - 2017-05-23 22:50 +0200
        Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Chris Packham <Chris.Packham@alliedtelesis.co.nz> - 2017-05-24 07:50 +0200
          Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Mark Brown <broonie@kernel.org> - 2017-05-24 19:10 +0200
      Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Mark Brown <broonie@kernel.org> - 2017-05-24 19:10 +0200
    Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio kbuild test robot <lkp@intel.com> - 2017-05-24 04:30 +0200

#1647618 — [PATCH 2/2] spi: use gpio_desc instead of numeric gpio

By using a gpio_desc and gpiod_set_value() instead of a numeric gpio and
gpio_set_value() the gpio flags are taken into account. This is useful
when using a gpio chip-select to supplement a controllers native
chip-select.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    My specific use-case is I have a board that uses the spi-orion driver but
    only has one CS pin available. In order to access two spi slave devices the
    board has a 1-of-2 decoder/demultiplexer which is driven via a gpio.
    
    The problem is that for one of the 2 slave devices the gpio level required
    is opposite to the chip-select so I can't simply specify "spi-cs-high".
    With this change I can flag the gpio as active low and the gpio subsystem
    takes care of the additional inversion required.

 drivers/spi/spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6f87fec409b5..b39c0f9956dd 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -725,7 +725,10 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
 		enable = !enable;
 
 	if (gpio_is_valid(spi->cs_gpio)) {
-		gpio_set_value(spi->cs_gpio, !enable);
+		struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
+
+		if (gpio)
+			gpiod_set_value(gpio, !enable);
 		/* Some SPI masters need both GPIO CS & slave_select */
 		if ((spi->master->flags & SPI_MASTER_GPIO_SS) &&
 		    spi->master->set_cs)
-- 
2.13.0

[toc] | [next] | [standalone]

#1648324

On Tue, May 23, 2017 at 7:03 AM, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
> By using a gpio_desc and gpiod_set_value() instead of a numeric gpio and
> gpio_set_value() the gpio flags are taken into account. This is useful
> when using a gpio chip-select to supplement a controllers native
> chip-select.

I think would be better to move everything in SPI core to GPIO descriptors.

>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     My specific use-case is I have a board that uses the spi-orion driver but
>     only has one CS pin available. In order to access two spi slave devices the
>     board has a 1-of-2 decoder/demultiplexer which is driven via a gpio.
>
>     The problem is that for one of the 2 slave devices the gpio level required
>     is opposite to the chip-select so I can't simply specify "spi-cs-high".
>     With this change I can flag the gpio as active low and the gpio subsystem
>     takes care of the additional inversion required.
>
>  drivers/spi/spi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 6f87fec409b5..b39c0f9956dd 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -725,7 +725,10 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
>                 enable = !enable;
>
>         if (gpio_is_valid(spi->cs_gpio)) {
> -               gpio_set_value(spi->cs_gpio, !enable);
> +               struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
> +
> +               if (gpio)
> +                       gpiod_set_value(gpio, !enable);
>                 /* Some SPI masters need both GPIO CS & slave_select */
>                 if ((spi->master->flags & SPI_MASTER_GPIO_SS) &&
>                     spi->master->set_cs)
> --
> 2.13.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1648622

Hi Andy,

On 24/05/17 06:28, Andy Shevchenko wrote:
> On Tue, May 23, 2017 at 7:03 AM, Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
>> By using a gpio_desc and gpiod_set_value() instead of a numeric gpio and
>> gpio_set_value() the gpio flags are taken into account. This is useful
>> when using a gpio chip-select to supplement a controllers native
>> chip-select.
>
> I think would be better to move everything in SPI core to GPIO descriptors.

I did consider it but it's a big change and I don't have access a lot of 
gear to test on (maybe 2 or 3 SoCs with a SPI host controller and the 
same SPI-NOR chips).

I can give it a try. Perhaps converting the spi core structures over and 
leaving the slaves using numeric gpios. Then later converting the slaves.

>
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      My specific use-case is I have a board that uses the spi-orion driver but
>>      only has one CS pin available. In order to access two spi slave devices the
>>      board has a 1-of-2 decoder/demultiplexer which is driven via a gpio.
>>
>>      The problem is that for one of the 2 slave devices the gpio level required
>>      is opposite to the chip-select so I can't simply specify "spi-cs-high".
>>      With this change I can flag the gpio as active low and the gpio subsystem
>>      takes care of the additional inversion required.
>>
>>   drivers/spi/spi.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 6f87fec409b5..b39c0f9956dd 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -725,7 +725,10 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
>>                  enable = !enable;
>>
>>          if (gpio_is_valid(spi->cs_gpio)) {
>> -               gpio_set_value(spi->cs_gpio, !enable);
>> +               struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
>> +
>> +               if (gpio)
>> +                       gpiod_set_value(gpio, !enable);
>>                  /* Some SPI masters need both GPIO CS & slave_select */
>>                  if ((spi->master->flags & SPI_MASTER_GPIO_SS) &&
>>                      spi->master->set_cs)
>> --
>> 2.13.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

[toc] | [prev] | [next] | [standalone]

#1649134

On 24/05/17 08:43, Chris Packham wrote:
> Hi Andy,
> 
> On 24/05/17 06:28, Andy Shevchenko wrote:
>> On Tue, May 23, 2017 at 7:03 AM, Chris Packham
>> <chris.packham@alliedtelesis.co.nz> wrote:
>>> By using a gpio_desc and gpiod_set_value() instead of a numeric gpio and
>>> gpio_set_value() the gpio flags are taken into account. This is useful
>>> when using a gpio chip-select to supplement a controllers native
>>> chip-select.
>>
>> I think would be better to move everything in SPI core to GPIO descriptors.
> 
> I did consider it but it's a big change and I don't have access a lot of
> gear to test on (maybe 2 or 3 SoCs with a SPI host controller and the
> same SPI-NOR chips).
> 
> I can give it a try. Perhaps converting the spi core structures over and
> leaving the slaves using numeric gpios. Then later converting the slaves.
> 

OK so I've had a look at this. The changes to change struct spi_master 
to use gpio_desc in drivers/spi/spi-gpio.c are pretty straight forward 
(in-line patch below, apologies for MUA wrapping).

There are a few SPI host drivers that deal with cs_gpios directly. 
spi-mt65xx.c and spi-davinci.c would be trivial to update. spi-ep93xx.c 
and spi-imx.c are harder because they handle non-dt platforms.

Changing struct spi_device to use gpio_desc would be wider reaching but 
probably automatable s/gpio_set_value/gpiod_set_value/.

I'm a little too nervous about messing up spi-ep93xx.c and spi-ep93xx.c 
to push ahead with changing all of SPI core. I'd be happy to help out 
someone with access to platforms using those.

-- 8< --
index b39c0f9956dd..80a96d3daa45 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -40,6 +40,7 @@
  #include <linux/ioport.h>
  #include <linux/acpi.h>
  #include <linux/highmem.h>
+#include <linux/gpio/consumer.h>

  #define CREATE_TRACE_POINTS
  #include <trace/events/spi.h>
@@ -531,8 +532,8 @@ int spi_add_device(struct spi_device *spi)
                 goto done;
         }

-       if (master->cs_gpios)
-               spi->cs_gpio = master->cs_gpios[spi->chip_select];
+       if (master->cs_gpios[spi->chip_select])
+               spi->cs_gpio = 
desc_to_gpio(master->cs_gpios[spi->chip_select]);

         /* Drivers may modify this initial i/o setup, but will
          * normally rely on the device being setup.  Devices
@@ -1878,7 +1879,8 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
  #ifdef CONFIG_OF
  static int of_spi_register_master(struct spi_master *master)
  {
-       int nb, i, *cs;
+       int nb, i;
+       struct gpio_desc **cs;
         struct device_node *np = master->dev.of_node;

         if (!np)
@@ -1894,7 +1896,7 @@ static int of_spi_register_master(struct 
spi_master *master)
                 return nb;

         cs = devm_kzalloc(&master->dev,
-                         sizeof(int) * master->num_chipselect,
+                         sizeof(*master->cs_gpios) * 
master->num_chipselect,
                           GFP_KERNEL);
         master->cs_gpios = cs;

@@ -1902,10 +1904,16 @@ static int of_spi_register_master(struct 
spi_master *master)
                 return -ENOMEM;

         for (i = 0; i < master->num_chipselect; i++)
-               cs[i] = -ENOENT;
+               cs[i] = NULL;

-       for (i = 0; i < nb; i++)
-               cs[i] = of_get_named_gpio(np, "cs-gpios", i);
+       for (i = 0; i < nb; i++) {
+               struct gpio_desc *gpio;
+
+               gpio = devm_gpiod_get_index(&master->dev, "cs", i,
+                                           GPIOD_OUT_HIGH);
+               if (!IS_ERR(gpio))
+                       cs[i] = gpio;
+       }

         return 0;
  }
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 935bd2854ff1..46960ca62d5b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -556,7 +556,7 @@ struct spi_master {
                            struct spi_message *message);

         /* gpio chip select */
-       int                     *cs_gpios;
+       struct gpio_desc        **cs_gpios;

         /* statistics */
         struct spi_statistics   statistics;
-- >8 --

>>
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>       My specific use-case is I have a board that uses the spi-orion driver but
>>>       only has one CS pin available. In order to access two spi slave devices the
>>>       board has a 1-of-2 decoder/demultiplexer which is driven via a gpio.
>>>
>>>       The problem is that for one of the 2 slave devices the gpio level required
>>>       is opposite to the chip-select so I can't simply specify "spi-cs-high".
>>>       With this change I can flag the gpio as active low and the gpio subsystem
>>>       takes care of the additional inversion required.
>>>
>>>    drivers/spi/spi.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>> index 6f87fec409b5..b39c0f9956dd 100644
>>> --- a/drivers/spi/spi.c
>>> +++ b/drivers/spi/spi.c
>>> @@ -725,7 +725,10 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
>>>                   enable = !enable;
>>>
>>>           if (gpio_is_valid(spi->cs_gpio)) {
>>> -               gpio_set_value(spi->cs_gpio, !enable);
>>> +               struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
>>> +
>>> +               if (gpio)
>>> +                       gpiod_set_value(gpio, !enable);
>>>                   /* Some SPI masters need both GPIO CS & slave_select */
>>>                   if ((spi->master->flags & SPI_MASTER_GPIO_SS) &&
>>>                       spi->master->set_cs)
>>> --
>>> 2.13.0
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1649776

[Multipart message — attachments visible in raw view] — view raw

On Wed, May 24, 2017 at 05:42:38AM +0000, Chris Packham wrote:

> I'm a little too nervous about messing up spi-ep93xx.c and spi-ep93xx.c 
> to push ahead with changing all of SPI core. I'd be happy to help out 
> someone with access to platforms using those.

Well, if you try writing the patch blind we can hopefully find someone
to test it separately?

[toc] | [prev] | [next] | [standalone]

#1649781

[Multipart message — attachments visible in raw view] — view raw

On Tue, May 23, 2017 at 09:28:22PM +0300, Andy Shevchenko wrote:
> On Tue, May 23, 2017 at 7:03 AM, Chris Packham

> > By using a gpio_desc and gpiod_set_value() instead of a numeric gpio and
> > gpio_set_value() the gpio flags are taken into account. This is useful
> > when using a gpio chip-select to supplement a controllers native
> > chip-select.

> I think would be better to move everything in SPI core to GPIO descriptors.

Yes, this sort of bodge is just far too ugly and I'd not trust it to
continue to work.  

[toc] | [prev] | [next] | [standalone]

#1649065

[Multipart message — attachments visible in raw view] — view raw

Hi Chris,

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Packham/spi-orion-Handle-GPIO-chip-selects/20170524-074032
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc 

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi.c: In function 'spi_set_cs':
>> drivers/spi/spi.c:728:28: error: implicit declaration of function 'gpio_to_desc' [-Werror=implicit-function-declaration]
      struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
                               ^~~~~~~~~~~~
>> drivers/spi/spi.c:728:28: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
>> drivers/spi/spi.c:731:4: error: implicit declaration of function 'gpiod_set_value' [-Werror=implicit-function-declaration]
       gpiod_set_value(gpio, !enable);
       ^~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/gpio_to_desc +728 drivers/spi/spi.c

   722	static void spi_set_cs(struct spi_device *spi, bool enable)
   723	{
   724		if (spi->mode & SPI_CS_HIGH)
   725			enable = !enable;
   726	
   727		if (gpio_is_valid(spi->cs_gpio)) {
 > 728			struct gpio_desc *gpio = gpio_to_desc(spi->cs_gpio);
   729	
   730			if (gpio)
 > 731				gpiod_set_value(gpio, !enable);
   732			/* Some SPI masters need both GPIO CS & slave_select */
   733			if ((spi->master->flags & SPI_MASTER_GPIO_SS) &&
   734			    spi->master->set_cs)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web