linux-kernel - Re: [PATCH v1] spi: fix client driver breakages when using 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]

Date:   Wed, 11 Nov 2020 12:33:27 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Sven Van Asbroeck <thesven73@...il.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        Simon Han <z.han@...bus.com>, Lukas Wunner <lukas@...ner.de>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] spi: fix client driver breakages when using GPIO
 descriptors

On Wed, Nov 11, 2020 at 02:05:19AM +0100, Linus Walleij wrote:
> On Mon, Nov 9, 2020 at 3:41 PM Sven Van Asbroeck <thesven73@...il.com> wrote:

> > I don't disagree. Fact is that after the imx cspi bus driver was converted
> > to gpio descriptors, most spi client drivers broke. It would be great if this
> > could be fixed. Any method that the community can find a consensus on,
> > would be great :)

> I think your patch is the quick fix.

> I would say that anything that has:

> spi->mode = ...

> is essentially broken.

This is not clear to me, most of these settings are things that are
constant for the device so it's not clear that they should be being set
by the device tree in the first place.  The idea that the chip select
might be being inverted like it is by this whole gpiolib/DT/new binding
thing is breaking expectations too.

> The core sets up vital things in .mode from e.g. device tree in
> of_spi_parse_dt():

>         /* Mode (clock phase/polarity/etc.) */
>         if (of_property_read_bool(nc, "spi-cpha"))
>                 spi->mode |= SPI_CPHA;
>         if (of_property_read_bool(nc, "spi-cpol"))
>                 spi->mode |= SPI_CPOL;
>         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;
>         if (of_property_read_bool(nc, "spi-cs-high"))
>                 spi->mode |= SPI_CS_HIGH;

> All this gets overwritten and ignored when a client just assigns mode
> like that. Not just SPI_CS_HIGH. I doubt things are different
> with ACPI.

OTOH most of these are things the device driver should just get right
without needing any input from DT, there's a few where there's plausible
options (eg, you can imagine pin strap configuration for 3 wire mode)
so generally it's not clear how many of these make sense for anything
other than spidev.  This binding all predates my involvement so I don't
know the thought process here.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)