Re: [PATCH] spi: dt-bindings: clarify CS behavior for spi-cs-high and gpio descriptors

[H. Nikolaus Schaller]

> Am 09.12.2020 um 18:36 schrieb Sven Van Asbroeck <thesven73@xxxxxxxxx>:
> 
> On Wed, Dec 9, 2020 at 4:57 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>> 
>> +
>> +      device node     | cs-gpio       | CS pin state active | Note
>> +      ================+===============+=====================+=====
>> +      spi-cs-high     | -             | H                   |
>> +      -               | -             | L                   |
>> +      spi-cs-high     | ACTIVE_HIGH   | H                   |
>> +      -               | ACTIVE_HIGH   | L                   | 1
>> +      spi-cs-high     | ACTIVE_LOW    | H                   | 2
>> +      -               | ACTIVE_LOW    | L                   |
>> +
> 
> Doesn't this table simply say:
> - specify   'spi-cs-high' for an active-high chip select
> - leave out 'spi-cs-high' for an active-low  chip select
> - the gpio active high/active low consumer flags are ignored
> ?

Yes it does, but I don't know if it is what we want to have. Linus confirmed
and you also seem to agree. Let's wait for other verdicts.

This is also what made me wonder if that is really intended because then
the whole discussion about the cs-gpio-flags and inversion and the fixes
would not have been needed. The current code and fixes are all about
not ignoring the flags...

And I am sure the code would be much simpler than currently for treating
special cases. Code would simply be: make any spi driver look at the gpio
descriptor and undo any inversion that gpiod_set_value() will do in
gpiod_set_value_nocheck() so that we can really control the physical
state by spi-cs-high instead of the logical gpio activity.

Something like:

static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
{
	struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);

	/* set initial clock line level */
	if (is_active)
		gpiod_set_value_cansleep(spi_gpio->sck, spi->mode & SPI_CPOL);

	/* Drive chip select line, if we have one */

	if (spi_gpio->cs_gpios) {
		struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];

		/* check if gpiod_set_value_nocheck() will invert */
		if (test_bit(FLAG_ACTIVE_LOW, &cs->flags)
			is_active = !is_active;

		/* SPI chip selects are normally active-low */	
		gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);
	}
}

There would be no need to detect spi-cs-high etc. in gpio-lib or
elsewhere. Only for printing warnings as suggested by Notes 1 and 2.

> 
> If so, then I would simply document it that way.
> Simple is beautiful.

Firstly, I would only think about collapsing the table if we agree that
it is correct. Beauty is IMHO not a reason to declare the table to be
correct.

Secondly, please imagine some reader of a device tree who finds

	cs-gpios = <&gpio 7 ACTIVE_LOW>;
	spi-cs-high;

Documentation should work well and be helpful especially in such a case.
Otherwise you don't need documentation.

Saying that the gpio flags are ignored would be helpful but a full table
with Notes and recommendations how to resolve is even more helpful and
unambiguous - even if it tells the same.

BR and thanks,
Nikolaus