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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <3FA1D050-3BD5-4A97-9D83-520CCF75D147@goldelico.com>
Date:   Wed, 9 Dec 2020 19:13:39 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        linux-gpio@...r.kernel.org,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Lukas Wunner <lukas@...ner.de>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Andreas Kemnade <andreas@...nade.info>,
        Maxime Ripard <maxime@...no.tech>
Subject: Re: [PATCH] spi: dt-bindings: clarify CS behavior for spi-cs-high and gpio descriptors

> Am 09.12.2020 um 18:36 schrieb Sven Van Asbroeck <thesven73@...il.com>:
> 
> On Wed, Dec 9, 2020 at 4:57 AM H. Nikolaus Schaller <hns@...delico.com> 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