Re: [PATCH RFC] Remove "spi-cs-high" property for GPIO-based chipselects

[Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>]

On Sun, Dec 01, 2013 at 11:59 +0400, Alexander Shiyan wrote:
> 
> This patch takes the CS active level from the GPIO bindings,
> so we remove the special property "spi-cs-high" for chipselects
> that use GPIO.

I'm afraid this description does not reflect what the patch does.
This is not saying that the patch is wrong, but suggests that the
description may be in need of an update.

Whether there is some gain in the change is a different matter,
see below.


> Signed-off-by: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
> ---
>  drivers/spi/spi.c       | 28 ++++++++++++++++++----------
>  include/linux/spi/spi.h |  2 ++
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 98f4b77..d2ba1ec 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
>  		goto done;
>  	}
>  
> -	if (master->cs_gpios)
> +	if (master->cs_gpios) {
>  		spi->cs_gpio = master->cs_gpios[spi->chip_select];
> +		if (!(master->cs_gpios_flags[spi->chip_select] &
> +		      OF_GPIO_ACTIVE_LOW))
> +			spi->mode |= SPI_CS_HIGH;
> +	}
>  
>  	/* Drivers may modify this initial i/o setup, but will
>  	 * normally rely on the device being setup.  Devices

OK, so you accept GPIO flags as an _additional_ source of
information, to derive "CS high" attributes in the SPI subsystem
in the (potential) absence of explicit specs in the device tree.

I'm not quite certain whether the GPIO pin's "not being inverted"
(not being low-active, the de-facto default for a GPIO pin) and
the SPI chip select's being high-active really are the same
thing.  Unless I'm missing something, this is quite a change in
semantics.  (Should this automatic deriving attributes emit a
message as well, to raise awareness?  Are we spamming users then,
or do these go unnoticed und ignored?)

Does this patch _force_ those who use GPIOs for SPI CS to adjust
their GPIO configuration in device trees?  Or make SPI
communication fail if the device tree won't get adjusted?  Should
you adjust users as well, and put a big fat remark into the
documentation?

A quick 'git grep spi-cs-high' reveals just one in-tree user, but
I'd expect quite a lot out-of-tree users which assume "regular,
straight semantics".


> @@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct spi_master *master)
>  			spi->mode |= SPI_CPHA;
>  		if (of_find_property(nc, "spi-cpol", NULL))
>  			spi->mode |= SPI_CPOL;
> -		if (of_find_property(nc, "spi-cs-high", NULL))
> -			spi->mode |= SPI_CS_HIGH;
>  		if (of_find_property(nc, "spi-3wire", NULL))
>  			spi->mode |= SPI_3WIRE;
> +		if (of_find_property(nc, "spi-cs-high", NULL)) {
> +			if (master->cs_gpios)
> +				dev_notice(&master->dev,
> +					"\"spi-cs-high\" property is obsolete.\n");
> +			spi->mode |= SPI_CS_HIGH;
> +		}
>  
>  		/* Device DUAL/QUAD mode */
>  		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {

Here you keep the evaluation of the 'spi-cs-high' property.
Which is a good thing, and should not go away as Mark has
suggested, at least not without proper period of migration.

You introduce a diagnostic message if SPI CS is high-active (and
this spec comes from the SPI node) and the CS is backed by a GPIO
pin.

Considering that you cannot remove the "CS high-active" attribute
in general, and need to keep the 'spi-cs-high' property for those
cases where the signal is not driven by means of GPIO, and keep
backwards compatibility for GPIO backed signals -- is there
really any gain in the change?

I'm afraid that forcing users to specify the same "this SPI
slave's CS is high-active" information in different ways
depending on whether the signal is generated by GPIO or by other
means is not a good thing.  I'm even questioning the benefit of
introducing the optional second source of this information by
deriving it from the GPIO flags.  Am I missing something?

[ it's just a nit that hou move the 'spi-cs-high' evaluation in
the patch while I fail to see the motivation of the move, it's
neither logical grouping nor alphabetical sorting ]


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
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