Re: [RFC][PATCH v1] spi: Introduce custom GPIO chip select for slave devices

[Rob Herring <robh@xxxxxxxxxx>]

On Tue, Mar 26, 2019 at 11:29:18PM +0200, Andy Shevchenko wrote:
> The SPI hardware interface requires to have clock, data, and chip select
> lines to communicate with a certain chip on the bus.
> 
> Historically the chip select lines were related to the SPI controller
> itself ("native" mode), however there is no technical obstacles to make
> any controllable pin as chip select. And thus no limitation to the
> amount of chip select pins should be put in such case.
> 
> As a result here is a proposal to logically split "native" and GPIO chip
> select pins and make it possible for slave device to choose which pin it
> will use.

Doesn't the h/w design choose?

> 
> Rationale points here are:
> - it's actually not a business of the SPI controller to know what
> resource might be used as a chip select except "native" ones

Not the business of the slave to know either.

> - some of the controllers would like to request GPIOs as soon as the
> controller itself is being registered, which is an artificial limitation
> of what GPIOs and when can be used as CS pins

Fix the controller driver implementation then?

> - as a continuation of previous point the CS pin can't be reused at
> runtime until entire controller will gone, the easy example that can
> come up is MMC SPI where we can multiplex GPIO CS based on card
> detection
> - (I dunno if DT overlays allow to expand amount of CS pins at run-time)

In theory yes, but the problem with much of it is whether the kernel can 
deal with new or changing properties (hint: it can't).

> - last, but not least, is a firmware, that can't be modified by user on
> a board where some pins still available for general purpose use and user
> would like to re-use them for custom SPI slave device
> 
> The split itself is simple an introduction of the "spi-cs-gpios"
> property for the slave device. SPI core will handle this for all
> supported controllers.
> 
> Known issues / TODO list:
> - needs flag to handle such cases to avoid freeing controller owned
> GPIO descriptors
> - clean up is not correct in error path
> - device tree support

I'm not seeing anything new we can't already accomplish with DT 
bindings.

> - split patch to device tree bindings and main part
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> - it is sent as RFC to gather opinions
> - it has been tested on x86 hardware with 74x164 SPI GPIO expander connected
> 
>  .../devicetree/bindings/spi/spi-bus.txt        |  5 ++++-
>  drivers/spi/spi.c                              | 18 +++++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
> index 1f6e86f787ef..7f5ff64efc7d 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -69,6 +69,8 @@ All slave nodes can contain the following optional properties:
>  		    phase (CPHA) mode.
>  - spi-cs-high     - Empty property indicating device requires chip select
>  		    active high.
> +- spi-cs-gpios    - Custom chip select for this device, when provided reg
> +		    property must be 0.

Having multiple nodes at address 0 is not valid in DT. So this wouldn't 
work it CS0 is used or you wanted to do this with multiple slaves.

>  - spi-3wire       - Empty property indicating device requires 3-wire mode.
>  - spi-lsb-first   - Empty property indicating device requires LSB first mode.
>  - spi-tx-bus-width - The bus width (number of data wires) that is used for MOSI.
> @@ -86,7 +88,8 @@ only 1 (SINGLE), 2 (DUAL) and 4 (QUAD).
>  Dual/Quad mode is not allowed when 3-wire mode is used.
>  
>  If a gpio chipselect is used for the SPI slave the gpio number will be passed
> -via the SPI master node cs-gpios property.
> +via the SPI master node cs-gpios property or, when it's not possible, via
> +spi-cs-gpios property in slave configuration.
>  
>  SPI example for an MPC5200 SPI bus:
>  	spi@f00 {
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 93986f879b09..111e9705eb96 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -535,7 +535,8 @@ static int spi_dev_check(struct device *dev, void *data)
>  	struct spi_device *new_spi = data;
>  
>  	if (spi->controller == new_spi->controller &&
> -	    spi->chip_select == new_spi->chip_select)
> +	    spi->chip_select == new_spi->chip_select &&
> +	    !new_spi->cs_gpiod)
>  		return -EBUSY;
>  	return 0;
>  }
> @@ -580,7 +581,9 @@ int spi_add_device(struct spi_device *spi)
>  	}
>  
>  	/* Descriptors take precedence */
> -	if (ctlr->cs_gpiods)
> +	if (spi->cs_gpiod)
> +		dev_dbg(dev, "chipselect is provided by slave\n");
> +	else 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];
> @@ -693,8 +696,11 @@ void spi_unregister_device(struct spi_device *spi)
>  		of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
>  		of_node_put(spi->dev.of_node);
>  	}
> -	if (ACPI_COMPANION(&spi->dev))
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		gpiod_put(spi->cs_gpiod);
> +		spi->cs_gpiod = NULL;
>  		acpi_device_clear_enumerated(ACPI_COMPANION(&spi->dev));
> +	}
>  	device_unregister(&spi->dev);
>  }
>  EXPORT_SYMBOL_GPL(spi_unregister_device);
> @@ -1910,6 +1916,12 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
>  	if (spi->irq < 0)
>  		spi->irq = acpi_dev_gpio_irq_get(adev, 0);
>  
> +	spi->cs_gpiod = gpiod_get_optional(&spi->dev, "spi-cs", GPIOD_ASIS);
> +	if (IS_ERR(spi->cs_gpiod)) {
> +		spi_dev_put(spi);
> +		return AE_OK;
> +	}
> +
>  	acpi_device_set_enumerated(adev);
>  
>  	adev->power.flags.ignore_parent = true;
> -- 
> 2.20.1
>