Re: [PATCH 1/7 v1] spi: Optionally use GPIO descriptors for CS GPIOs

[Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>]

On Sun, 16 Dec 2018 00:38:17 +0100
Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

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;