| Message ID | 20181215233823.1042-2-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SPI CS using GPIO descriptors | expand |
On Sun, 16 Dec 2018 00:38:17 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > This augments the SPI core to optionally use GPIO descriptors > for chip select on a per-master-driver opt-in basis. > > Drivers using this will rely on the SPI core to look up > GPIO descriptors associated with the device, such as > when using device tree or board files with GPIO descriptor > tables. > > When getting descriptors from the device tree, this will in > turn activate the code in gpiolib that was > added in commit 6953c57ab172 > ("gpio: of: Handle SPI chipselect legacy bindings") > which means that these descriptors are aware of the active > low semantics that is the default for SPI CS GPIO lines > and we can assume that all of these are "active high" and > thus assign SPI_CS_HIGH to all CS lines on the DT path. > > The previously used gpio_set_value() would call down into > gpiod_set_raw_value() and ignore the polarity inversion > semantics. > > It seems like many drivers go to great lengths to set up the > CS GPIO line as non-asserted, respecting SPI_CS_HIGH. We pull > this out of the SPI drivers and into the core, and by simply > requesting the line as GPIOD_OUT_LOW when retrieveing it from > the device and relying on the gpiolib to handle any inversion > semantics. This way a lot of code can be simplified and > removed in each converted driver. > > The end goal after dealing with each driver in turn, is to > delete the non-descriptor path (of_spi_register_master() for > example) and let the core deal with only descriptors. > > The different SPI drivers have complex interactions with the > core so we cannot simply change them all over, we need to use > a stepwise, bisectable approach so that each driver can be > converted and fixed in isolation. > > Cc: Linuxarm <linuxarm@huawei.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Looks good to me. A couple of trivial comments inline. Thanks, Jonathan > --- > Jay, I think this patch should cover your ACPI usecase > as well, as in subject "spi: add ACPI support for SPI controller > chip select lines(cs-gpios)" > --- > drivers/spi/spi.c | 105 ++++++++++++++++++++++++++++++++++++---- > include/linux/spi/spi.h | 23 +++++++-- > 2 files changed, 114 insertions(+), 14 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 6ca59406b0b7..05e73290cdf3 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -21,6 +21,7 @@ > #include <linux/spi/spi.h> > #include <linux/spi/spi-mem.h> > #include <linux/of_gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/pm_runtime.h> > #include <linux/pm_domain.h> > #include <linux/property.h> > @@ -509,6 +510,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr) > spi->dev.bus = &spi_bus_type; > spi->dev.release = spidev_release; > spi->cs_gpio = -ENOENT; > + spi->cs_gpiod = NULL; spi is kzalloc'd above and this is a fairly obvious default after allocation. So I would say no point in initializing it. There are other elements that aren't explicitly initialized so local precedent seems to suggest not doing so.. > > spin_lock_init(&spi->statistics.lock); > > @@ -580,7 +582,10 @@ int spi_add_device(struct spi_device *spi) > goto done; > } > > - if (ctlr->cs_gpios) > + /* Descriptors take precedence */ > + if (ctlr->cs_gpiods) > + spi->cs_gpiod = ctlr->cs_gpiods[spi->chip_select]; > + else if (ctlr->cs_gpios) > spi->cs_gpio = ctlr->cs_gpios[spi->chip_select]; > > /* Drivers may modify this initial i/o setup, but will > @@ -774,10 +779,20 @@ static void spi_set_cs(struct spi_device *spi, bool enable) > if (spi->mode & SPI_CS_HIGH) > enable = !enable; > > - if (gpio_is_valid(spi->cs_gpio)) { > - /* Honour the SPI_NO_CS flag */ > - if (!(spi->mode & SPI_NO_CS)) > - gpio_set_value(spi->cs_gpio, !enable); > + if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) { > + /* > + * Honour the SPI_NO_CS flag and invert the enable line, as > + * active low is default for SPI. Execution paths that handle > + * polarity inversion in gpiolib (such as device tree) will > + * enforce active high using the SPI_CS_HIGH resulting in a > + * double inversion through the code above. > + */ > + if (!(spi->mode & SPI_NO_CS)) { > + if (spi->cs_gpiod) > + gpiod_set_value(spi->cs_gpiod, !enable); > + else > + gpio_set_value(spi->cs_gpio, !enable); > + } > /* Some SPI masters need both GPIO CS & slave_select */ > if ((spi->controller->flags & SPI_MASTER_GPIO_SS) && > spi->controller->set_cs) > @@ -1599,13 +1614,21 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > spi->mode |= SPI_CPHA; > if (of_property_read_bool(nc, "spi-cpol")) > spi->mode |= SPI_CPOL; > - if (of_property_read_bool(nc, "spi-cs-high")) > - spi->mode |= SPI_CS_HIGH; > if (of_property_read_bool(nc, "spi-3wire")) > spi->mode |= SPI_3WIRE; > if (of_property_read_bool(nc, "spi-lsb-first")) > spi->mode |= SPI_LSB_FIRST; > > + /* > + * For descriptors associated with the device, polarity inversion is > + * handled in the gpiolib, so all chip selects are "active high" in > + * the logical sense, the gpiolib will invert the line if need be. > + */ > + if (ctlr->use_gpio_descriptors) > + spi->mode |= SPI_CS_HIGH; > + else if (of_property_read_bool(nc, "spi-cs-high")) > + spi->mode |= SPI_CS_HIGH; > + > /* Device DUAL/QUAD mode */ > if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { > switch (value) { > @@ -2115,6 +2138,60 @@ static int of_spi_register_master(struct spi_controller *ctlr) > } > #endif > > +/** > + * spi_get_gpio_descs() - grab chip select GPIOs for the master > + * @ctlr: The SPI master to grab GPIO descriptors for > + */ > +static int spi_get_gpio_descs(struct spi_controller *ctlr) > +{ > + int nb, i; > + struct gpio_desc **cs; > + struct device *dev = &ctlr->dev; > + > + nb = gpiod_count(dev, "cs"); > + ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); > + > + /* No GPIOs at all is fine, else return the error */ > + if (nb == 0 || nb == -ENOENT) > + return 0; > + else if (nb < 0) > + return nb; > + > + cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs), > + GFP_KERNEL); > + if (!cs) > + return -ENOMEM; > + ctlr->cs_gpiods = cs; > + > + for (i = 0; i < nb; i++) { > + /* > + * Most chipselects are active low, the inverted > + * semantics are handled by special quirks in gpiolib, > + * so initializing them GPIOD_OUT_LOW here means > + * "unasserted", in most cases the will drive the physical this will > + * line high. > + */ > + cs[i] = devm_gpiod_get_index_optional(dev, "cs", i, > + GPIOD_OUT_LOW); > + > + if (cs[i]) { > + /* > + * If we find a CS GPIO, name it after the device and > + * chip select line. > + */ > + char *gpioname; > + > + gpioname = devm_kasprintf(dev, GFP_KERNEL, "%s CS%d", > + dev_name(dev), i); > + if (!gpioname) > + return -ENOMEM; > + gpiod_set_consumer_name(cs[i], gpioname); > + } > + } > + > + return 0; > +} > + > static int spi_controller_check_ops(struct spi_controller *ctlr) > { > /* > @@ -2177,9 +2254,16 @@ int spi_register_controller(struct spi_controller *ctlr) > return status; > > if (!spi_controller_is_slave(ctlr)) { > - status = of_spi_register_master(ctlr); > - if (status) > - return status; > + if (ctlr->use_gpio_descriptors) { > + status = spi_get_gpio_descs(ctlr); > + if (status) > + return status; > + } else { > + /* Legacy code path for GPIOs from DT */ > + status = of_spi_register_master(ctlr); > + if (status) > + return status; > + } > } > > /* even if it's just one always-selected device, there must > @@ -2891,6 +2975,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) > * cs_change is set for each transfer. > */ > if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) || > + spi->cs_gpiod || > gpio_is_valid(spi->cs_gpio))) { > size_t maxsize; > int ret; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 6be77fa5ab90..68d77cfc8bb7 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -12,6 +12,7 @@ > #include <linux/kthread.h> > #include <linux/completion.h> > #include <linux/scatterlist.h> > +#include <linux/gpio/consumer.h> > > struct dma_chan; > struct property_entry; > @@ -116,7 +117,10 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats, > * @modalias: Name of the driver to use with this device, or an alias > * for that name. This appears in the sysfs "modalias" attribute > * for driver coldplugging, and in uevents used for hotplugging > - * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when > + * @cs_gpio: LEGACY: gpio number of the chipselect line (optional, -ENOENT when > + * not using a GPIO line) use cs_gpiod in new drivers by opting in on > + * the spi_master. > + * @cs_gpiod: gpio descriptor of the chipselect line (optional, NULL when > * not using a GPIO line) > * > * @statistics: statistics for the spi_device > @@ -160,7 +164,8 @@ struct spi_device { > void *controller_data; > char modalias[SPI_NAME_SIZE]; > const char *driver_override; > - int cs_gpio; /* chip select gpio */ > + int cs_gpio; /* LEGACY: chip select gpio */ > + struct gpio_desc *cs_gpiod; /* chip select gpio desc */ > > /* the statistics */ > struct spi_statistics statistics; > @@ -373,9 +378,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * controller has native support for memory like operations. > * @unprepare_message: undo any work done by prepare_message(). > * @slave_abort: abort the ongoing transfer request on an SPI slave controller > - * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS > - * number. Any individual value may be -ENOENT for CS lines that > + * @cs_gpios: LEGACY: array of GPIO descs to use as chip select lines; one per > + * CS number. Any individual value may be -ENOENT for CS lines that > + * are not GPIOs (driven by the SPI controller itself). Use the cs_gpiods > + * in new drivers. > + * @cs_gpiods: Array of GPIO descs to use as chip select lines; one per CS > + * number. Any individual value may be NULL for CS lines that > * are not GPIOs (driven by the SPI controller itself). > + * @use_gpio_descriptors: Turns on the code in the SPI core to parse and grab > + * GPIO descriptors rather than using global GPIO numbers grabbed by the > + * driver. This will fill in @cs_gpiods and @cs_gpios should not be used, > + * and SPI devices will have the cs_gpiod assigned rather than cs_gpio. > * @statistics: statistics for the spi_controller > * @dma_tx: DMA transmit channel > * @dma_rx: DMA receive channel > @@ -554,6 +567,8 @@ struct spi_controller { > > /* gpio chip select */ > int *cs_gpios; > + struct gpio_desc **cs_gpiods; > + bool use_gpio_descriptors; > > /* statistics */ > struct spi_statistics statistics;
On 2018/12/16 7:38, Linus Walleij wrote: > This augments the SPI core to optionally use GPIO descriptors > for chip select on a per-master-driver opt-in basis. > > Drivers using this will rely on the SPI core to look up > GPIO descriptors associated with the device, such as > when using device tree or board files with GPIO descriptor > tables. > > When getting descriptors from the device tree, this will in > turn activate the code in gpiolib that was > added in commit 6953c57ab172 > ("gpio: of: Handle SPI chipselect legacy bindings") > which means that these descriptors are aware of the active > low semantics that is the default for SPI CS GPIO lines > and we can assume that all of these are "active high" and > thus assign SPI_CS_HIGH to all CS lines on the DT path. > > The previously used gpio_set_value() would call down into > gpiod_set_raw_value() and ignore the polarity inversion > semantics. > > It seems like many drivers go to great lengths to set up the > CS GPIO line as non-asserted, respecting SPI_CS_HIGH. We pull > this out of the SPI drivers and into the core, and by simply > requesting the line as GPIOD_OUT_LOW when retrieveing it from > the device and relying on the gpiolib to handle any inversion > semantics. This way a lot of code can be simplified and > removed in each converted driver. > > The end goal after dealing with each driver in turn, is to > delete the non-descriptor path (of_spi_register_master() for > example) and let the core deal with only descriptors. > > The different SPI drivers have complex interactions with the > core so we cannot simply change them all over, we need to use > a stepwise, bisectable approach so that each driver can be > converted and fixed in isolation. > > Cc: Linuxarm <linuxarm@huawei.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > Jay, I think this patch should cover your ACPI usecase > as well, as in subject "spi: add ACPI support for SPI controller > chip select lines(cs-gpios)" > --- Thank you Linus Yes, this patch cover my ACPI usecase already. I have tested the patch on my board, and it works well. > drivers/spi/spi.c | 105 ++++++++++++++++++++++++++++++++++++---- > include/linux/spi/spi.h | 23 +++++++-- > 2 files changed, 114 insertions(+), 14 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 6ca59406b0b7..05e73290cdf3 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -21,6 +21,7 @@ > #include <linux/spi/spi.h> > #include <linux/spi/spi-mem.h> > #include <linux/of_gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/pm_runtime.h> > #include <linux/pm_domain.h> > #include <linux/property.h> > @@ -509,6 +510,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr) > spi->dev.bus = &spi_bus_type; > spi->dev.release = spidev_release; > spi->cs_gpio = -ENOENT; > + spi->cs_gpiod = NULL; > > spin_lock_init(&spi->statistics.lock); > > @@ -580,7 +582,10 @@ int spi_add_device(struct spi_device *spi) > goto done; > } > > - if (ctlr->cs_gpios) > + /* Descriptors take precedence */ > + if (ctlr->cs_gpiods) > + spi->cs_gpiod = ctlr->cs_gpiods[spi->chip_select]; > + else if (ctlr->cs_gpios) > spi->cs_gpio = ctlr->cs_gpios[spi->chip_select]; > > /* Drivers may modify this initial i/o setup, but will > @@ -774,10 +779,20 @@ static void spi_set_cs(struct spi_device *spi, bool enable) > if (spi->mode & SPI_CS_HIGH) > enable = !enable; > > - if (gpio_is_valid(spi->cs_gpio)) { > - /* Honour the SPI_NO_CS flag */ > - if (!(spi->mode & SPI_NO_CS)) > - gpio_set_value(spi->cs_gpio, !enable); > + if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) { > + /* > + * Honour the SPI_NO_CS flag and invert the enable line, as > + * active low is default for SPI. Execution paths that handle > + * polarity inversion in gpiolib (such as device tree) will > + * enforce active high using the SPI_CS_HIGH resulting in a > + * double inversion through the code above. > + */ > + if (!(spi->mode & SPI_NO_CS)) { > + if (spi->cs_gpiod) > + gpiod_set_value(spi->cs_gpiod, !enable); > + else > + gpio_set_value(spi->cs_gpio, !enable); > + } > /* Some SPI masters need both GPIO CS & slave_select */ > if ((spi->controller->flags & SPI_MASTER_GPIO_SS) && > spi->controller->set_cs) > @@ -1599,13 +1614,21 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, > spi->mode |= SPI_CPHA; > if (of_property_read_bool(nc, "spi-cpol")) > spi->mode |= SPI_CPOL; > - if (of_property_read_bool(nc, "spi-cs-high")) > - spi->mode |= SPI_CS_HIGH; > if (of_property_read_bool(nc, "spi-3wire")) > spi->mode |= SPI_3WIRE; > if (of_property_read_bool(nc, "spi-lsb-first")) > spi->mode |= SPI_LSB_FIRST; > > + /* > + * For descriptors associated with the device, polarity inversion is > + * handled in the gpiolib, so all chip selects are "active high" in > + * the logical sense, the gpiolib will invert the line if need be. > + */ > + if (ctlr->use_gpio_descriptors) > + spi->mode |= SPI_CS_HIGH; > + else if (of_property_read_bool(nc, "spi-cs-high")) > + spi->mode |= SPI_CS_HIGH; > + > /* Device DUAL/QUAD mode */ > if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { > switch (value) { > @@ -2115,6 +2138,60 @@ static int of_spi_register_master(struct spi_controller *ctlr) > } > #endif > > +/** > + * spi_get_gpio_descs() - grab chip select GPIOs for the master > + * @ctlr: The SPI master to grab GPIO descriptors for > + */ > +static int spi_get_gpio_descs(struct spi_controller *ctlr) > +{ > + int nb, i; > + struct gpio_desc **cs; > + struct device *dev = &ctlr->dev; > + > + nb = gpiod_count(dev, "cs"); > + ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); > + > + /* No GPIOs at all is fine, else return the error */ > + if (nb == 0 || nb == -ENOENT) > + return 0; > + else if (nb < 0) > + return nb; > + > + cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs), > + GFP_KERNEL); > + if (!cs) > + return -ENOMEM; > + ctlr->cs_gpiods = cs; > + > + for (i = 0; i < nb; i++) { > + /* > + * Most chipselects are active low, the inverted > + * semantics are handled by special quirks in gpiolib, > + * so initializing them GPIOD_OUT_LOW here means > + * "unasserted", in most cases the will drive the physical > + * line high. > + */ > + cs[i] = devm_gpiod_get_index_optional(dev, "cs", i, > + GPIOD_OUT_LOW); > + > + if (cs[i]) { > + /* > + * If we find a CS GPIO, name it after the device and > + * chip select line. > + */ > + char *gpioname; > + > + gpioname = devm_kasprintf(dev, GFP_KERNEL, "%s CS%d", > + dev_name(dev), i); > + if (!gpioname) > + return -ENOMEM; > + gpiod_set_consumer_name(cs[i], gpioname); > + } > + } > + > + return 0; > +} > + > static int spi_controller_check_ops(struct spi_controller *ctlr) > { > /* > @@ -2177,9 +2254,16 @@ int spi_register_controller(struct spi_controller *ctlr) > return status; > > if (!spi_controller_is_slave(ctlr)) { > - status = of_spi_register_master(ctlr); > - if (status) > - return status; > + if (ctlr->use_gpio_descriptors) { > + status = spi_get_gpio_descs(ctlr); > + if (status) > + return status; > + } else { > + /* Legacy code path for GPIOs from DT */ > + status = of_spi_register_master(ctlr); > + if (status) > + return status; > + } > } > > /* even if it's just one always-selected device, there must > @@ -2891,6 +2975,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) > * cs_change is set for each transfer. > */ > if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) || > + spi->cs_gpiod || > gpio_is_valid(spi->cs_gpio))) { > size_t maxsize; > int ret; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 6be77fa5ab90..68d77cfc8bb7 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -12,6 +12,7 @@ > #include <linux/kthread.h> > #include <linux/completion.h> > #include <linux/scatterlist.h> > +#include <linux/gpio/consumer.h> > > struct dma_chan; > struct property_entry; > @@ -116,7 +117,10 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats, > * @modalias: Name of the driver to use with this device, or an alias > * for that name. This appears in the sysfs "modalias" attribute > * for driver coldplugging, and in uevents used for hotplugging > - * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when > + * @cs_gpio: LEGACY: gpio number of the chipselect line (optional, -ENOENT when > + * not using a GPIO line) use cs_gpiod in new drivers by opting in on > + * the spi_master. > + * @cs_gpiod: gpio descriptor of the chipselect line (optional, NULL when > * not using a GPIO line) > * > * @statistics: statistics for the spi_device > @@ -160,7 +164,8 @@ struct spi_device { > void *controller_data; > char modalias[SPI_NAME_SIZE]; > const char *driver_override; > - int cs_gpio; /* chip select gpio */ > + int cs_gpio; /* LEGACY: chip select gpio */ > + struct gpio_desc *cs_gpiod; /* chip select gpio desc */ > > /* the statistics */ > struct spi_statistics statistics; > @@ -373,9 +378,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * controller has native support for memory like operations. > * @unprepare_message: undo any work done by prepare_message(). > * @slave_abort: abort the ongoing transfer request on an SPI slave controller > - * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS > - * number. Any individual value may be -ENOENT for CS lines that > + * @cs_gpios: LEGACY: array of GPIO descs to use as chip select lines; one per > + * CS number. Any individual value may be -ENOENT for CS lines that > + * are not GPIOs (driven by the SPI controller itself). Use the cs_gpiods > + * in new drivers. > + * @cs_gpiods: Array of GPIO descs to use as chip select lines; one per CS > + * number. Any individual value may be NULL for CS lines that > * are not GPIOs (driven by the SPI controller itself). > + * @use_gpio_descriptors: Turns on the code in the SPI core to parse and grab > + * GPIO descriptors rather than using global GPIO numbers grabbed by the > + * driver. This will fill in @cs_gpiods and @cs_gpios should not be used, > + * and SPI devices will have the cs_gpiod assigned rather than cs_gpio. > * @statistics: statistics for the spi_controller > * @dma_tx: DMA transmit channel > * @dma_rx: DMA receive channel > @@ -554,6 +567,8 @@ struct spi_controller { > > /* gpio chip select */ > int *cs_gpios; > + struct gpio_desc **cs_gpiods; > + bool use_gpio_descriptors; > > /* statistics */ > struct spi_statistics statistics; >
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 6ca59406b0b7..05e73290cdf3 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -21,6 +21,7 @@ #include <linux/spi/spi.h> #include <linux/spi/spi-mem.h> #include <linux/of_gpio.h> +#include <linux/gpio/consumer.h> #include <linux/pm_runtime.h> #include <linux/pm_domain.h> #include <linux/property.h> @@ -509,6 +510,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr) spi->dev.bus = &spi_bus_type; spi->dev.release = spidev_release; spi->cs_gpio = -ENOENT; + spi->cs_gpiod = NULL; spin_lock_init(&spi->statistics.lock); @@ -580,7 +582,10 @@ int spi_add_device(struct spi_device *spi) goto done; } - if (ctlr->cs_gpios) + /* Descriptors take precedence */ + if (ctlr->cs_gpiods) + spi->cs_gpiod = ctlr->cs_gpiods[spi->chip_select]; + else if (ctlr->cs_gpios) spi->cs_gpio = ctlr->cs_gpios[spi->chip_select]; /* Drivers may modify this initial i/o setup, but will @@ -774,10 +779,20 @@ static void spi_set_cs(struct spi_device *spi, bool enable) if (spi->mode & SPI_CS_HIGH) enable = !enable; - if (gpio_is_valid(spi->cs_gpio)) { - /* Honour the SPI_NO_CS flag */ - if (!(spi->mode & SPI_NO_CS)) - gpio_set_value(spi->cs_gpio, !enable); + if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) { + /* + * Honour the SPI_NO_CS flag and invert the enable line, as + * active low is default for SPI. Execution paths that handle + * polarity inversion in gpiolib (such as device tree) will + * enforce active high using the SPI_CS_HIGH resulting in a + * double inversion through the code above. + */ + if (!(spi->mode & SPI_NO_CS)) { + if (spi->cs_gpiod) + gpiod_set_value(spi->cs_gpiod, !enable); + else + gpio_set_value(spi->cs_gpio, !enable); + } /* Some SPI masters need both GPIO CS & slave_select */ if ((spi->controller->flags & SPI_MASTER_GPIO_SS) && spi->controller->set_cs) @@ -1599,13 +1614,21 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, spi->mode |= SPI_CPHA; if (of_property_read_bool(nc, "spi-cpol")) spi->mode |= SPI_CPOL; - if (of_property_read_bool(nc, "spi-cs-high")) - spi->mode |= SPI_CS_HIGH; if (of_property_read_bool(nc, "spi-3wire")) spi->mode |= SPI_3WIRE; if (of_property_read_bool(nc, "spi-lsb-first")) spi->mode |= SPI_LSB_FIRST; + /* + * For descriptors associated with the device, polarity inversion is + * handled in the gpiolib, so all chip selects are "active high" in + * the logical sense, the gpiolib will invert the line if need be. + */ + if (ctlr->use_gpio_descriptors) + spi->mode |= SPI_CS_HIGH; + else if (of_property_read_bool(nc, "spi-cs-high")) + spi->mode |= SPI_CS_HIGH; + /* Device DUAL/QUAD mode */ if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { switch (value) { @@ -2115,6 +2138,60 @@ static int of_spi_register_master(struct spi_controller *ctlr) } #endif +/** + * spi_get_gpio_descs() - grab chip select GPIOs for the master + * @ctlr: The SPI master to grab GPIO descriptors for + */ +static int spi_get_gpio_descs(struct spi_controller *ctlr) +{ + int nb, i; + struct gpio_desc **cs; + struct device *dev = &ctlr->dev; + + nb = gpiod_count(dev, "cs"); + ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); + + /* No GPIOs at all is fine, else return the error */ + if (nb == 0 || nb == -ENOENT) + return 0; + else if (nb < 0) + return nb; + + cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs), + GFP_KERNEL); + if (!cs) + return -ENOMEM; + ctlr->cs_gpiods = cs; + + for (i = 0; i < nb; i++) { + /* + * Most chipselects are active low, the inverted + * semantics are handled by special quirks in gpiolib, + * so initializing them GPIOD_OUT_LOW here means + * "unasserted", in most cases the will drive the physical + * line high. + */ + cs[i] = devm_gpiod_get_index_optional(dev, "cs", i, + GPIOD_OUT_LOW); + + if (cs[i]) { + /* + * If we find a CS GPIO, name it after the device and + * chip select line. + */ + char *gpioname; + + gpioname = devm_kasprintf(dev, GFP_KERNEL, "%s CS%d", + dev_name(dev), i); + if (!gpioname) + return -ENOMEM; + gpiod_set_consumer_name(cs[i], gpioname); + } + } + + return 0; +} + static int spi_controller_check_ops(struct spi_controller *ctlr) { /* @@ -2177,9 +2254,16 @@ int spi_register_controller(struct spi_controller *ctlr) return status; if (!spi_controller_is_slave(ctlr)) { - status = of_spi_register_master(ctlr); - if (status) - return status; + if (ctlr->use_gpio_descriptors) { + status = spi_get_gpio_descs(ctlr); + if (status) + return status; + } else { + /* Legacy code path for GPIOs from DT */ + status = of_spi_register_master(ctlr); + if (status) + return status; + } } /* even if it's just one always-selected device, there must @@ -2891,6 +2975,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) * cs_change is set for each transfer. */ if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) || + spi->cs_gpiod || gpio_is_valid(spi->cs_gpio))) { size_t maxsize; int ret; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 6be77fa5ab90..68d77cfc8bb7 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -12,6 +12,7 @@ #include <linux/kthread.h> #include <linux/completion.h> #include <linux/scatterlist.h> +#include <linux/gpio/consumer.h> struct dma_chan; struct property_entry; @@ -116,7 +117,10 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats, * @modalias: Name of the driver to use with this device, or an alias * for that name. This appears in the sysfs "modalias" attribute * for driver coldplugging, and in uevents used for hotplugging - * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when + * @cs_gpio: LEGACY: gpio number of the chipselect line (optional, -ENOENT when + * not using a GPIO line) use cs_gpiod in new drivers by opting in on + * the spi_master. + * @cs_gpiod: gpio descriptor of the chipselect line (optional, NULL when * not using a GPIO line) * * @statistics: statistics for the spi_device @@ -160,7 +164,8 @@ struct spi_device { void *controller_data; char modalias[SPI_NAME_SIZE]; const char *driver_override; - int cs_gpio; /* chip select gpio */ + int cs_gpio; /* LEGACY: chip select gpio */ + struct gpio_desc *cs_gpiod; /* chip select gpio desc */ /* the statistics */ struct spi_statistics statistics; @@ -373,9 +378,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * controller has native support for memory like operations. * @unprepare_message: undo any work done by prepare_message(). * @slave_abort: abort the ongoing transfer request on an SPI slave controller - * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS - * number. Any individual value may be -ENOENT for CS lines that + * @cs_gpios: LEGACY: array of GPIO descs to use as chip select lines; one per + * CS number. Any individual value may be -ENOENT for CS lines that + * are not GPIOs (driven by the SPI controller itself). Use the cs_gpiods + * in new drivers. + * @cs_gpiods: Array of GPIO descs to use as chip select lines; one per CS + * number. Any individual value may be NULL for CS lines that * are not GPIOs (driven by the SPI controller itself). + * @use_gpio_descriptors: Turns on the code in the SPI core to parse and grab + * GPIO descriptors rather than using global GPIO numbers grabbed by the + * driver. This will fill in @cs_gpiods and @cs_gpios should not be used, + * and SPI devices will have the cs_gpiod assigned rather than cs_gpio. * @statistics: statistics for the spi_controller * @dma_tx: DMA transmit channel * @dma_rx: DMA receive channel @@ -554,6 +567,8 @@ struct spi_controller { /* gpio chip select */ int *cs_gpios; + struct gpio_desc **cs_gpiods; + bool use_gpio_descriptors; /* statistics */ struct spi_statistics statistics;
This augments the SPI core to optionally use GPIO descriptors for chip select on a per-master-driver opt-in basis. Drivers using this will rely on the SPI core to look up GPIO descriptors associated with the device, such as when using device tree or board files with GPIO descriptor tables. When getting descriptors from the device tree, this will in turn activate the code in gpiolib that was added in commit 6953c57ab172 ("gpio: of: Handle SPI chipselect legacy bindings") which means that these descriptors are aware of the active low semantics that is the default for SPI CS GPIO lines and we can assume that all of these are "active high" and thus assign SPI_CS_HIGH to all CS lines on the DT path. The previously used gpio_set_value() would call down into gpiod_set_raw_value() and ignore the polarity inversion semantics. It seems like many drivers go to great lengths to set up the CS GPIO line as non-asserted, respecting SPI_CS_HIGH. We pull this out of the SPI drivers and into the core, and by simply requesting the line as GPIOD_OUT_LOW when retrieveing it from the device and relying on the gpiolib to handle any inversion semantics. This way a lot of code can be simplified and removed in each converted driver. The end goal after dealing with each driver in turn, is to delete the non-descriptor path (of_spi_register_master() for example) and let the core deal with only descriptors. The different SPI drivers have complex interactions with the core so we cannot simply change them all over, we need to use a stepwise, bisectable approach so that each driver can be converted and fixed in isolation. Cc: Linuxarm <linuxarm@huawei.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Jay, I think this patch should cover your ACPI usecase as well, as in subject "spi: add ACPI support for SPI controller chip select lines(cs-gpios)" --- drivers/spi/spi.c | 105 ++++++++++++++++++++++++++++++++++++---- include/linux/spi/spi.h | 23 +++++++-- 2 files changed, 114 insertions(+), 14 deletions(-) -- 2.19.2