From: Chris Packham <Chris.Packham@alliedtelesis.co.nz> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Mark Brown <broonie@kernel.org>, linux-spi <linux-spi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Date: Wed, 24 May 2017 05:42:38 +0000 [thread overview] Message-ID: <1be4374ca1b1422fb2b14298bf21ae9d@svr-chch-ex1.atlnz.lc> (raw) In-Reply-To: ca3b3f5247624a35b8203472a53df79a@svr-chch-ex1.atlnz.lc 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 >> >> >> > > >
WARNING: multiple messages have this Message-ID (diff)
From: Chris Packham <Chris.Packham-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org> To: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, linux-spi <linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Date: Wed, 24 May 2017 05:42:38 +0000 [thread overview] Message-ID: <1be4374ca1b1422fb2b14298bf21ae9d@svr-chch-ex1.atlnz.lc> (raw) In-Reply-To: ca3b3f5247624a35b8203472a53df79a@svr-chch-ex1.atlnz.lc 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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org> 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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org> >>> --- >>> >>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-05-24 5:42 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-23 4:03 [PATCH 1/2] spi: orion: Handle GPIO chip-selects Chris Packham 2017-05-23 4:03 ` Chris Packham 2017-05-23 4:03 ` [PATCH 2/2] spi: use gpio_desc instead of numeric gpio Chris Packham 2017-05-23 18:28 ` Andy Shevchenko 2017-05-23 20:43 ` Chris Packham 2017-05-24 5:42 ` Chris Packham [this message] 2017-05-24 5:42 ` Chris Packham 2017-05-24 17:02 ` Mark Brown 2017-05-24 17:02 ` Mark Brown 2017-05-24 17:00 ` Mark Brown 2017-05-24 17:00 ` Mark Brown 2017-05-24 2:23 ` kbuild test robot 2017-05-24 2:23 ` kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1be4374ca1b1422fb2b14298bf21ae9d@svr-chch-ex1.atlnz.lc \ --to=chris.packham@alliedtelesis.co.nz \ --cc=andy.shevchenko@gmail.com \ --cc=broonie@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.