Re: [PATCH v2] gpiolib: of: Only apply the SPI CS quirk to SPI buses

[Miquel Raynal]

Hello Maciej,

On 19/08/2026 at 17:55:10 +02, Maciej Andrzejewski ICEYE <maciej.andrzejewski@xxxxxxxxxxx> wrote:

> The legacy SPI chip-select polarity quirk in of_gpio_flags_quirks() is
> keyed on nothing but the property name "cs-gpios". That name is not
> exclusive to SPI: nand-controller.yaml documents the very same property
> for NAND controllers, and rawnand_dt_parse_gpio_cs() requests those
> lines with gpiod_count(dev, "cs"), which gpiolib expands to "cs-gpios".
> A NAND controller therefore has SPI chip-select semantics forced onto
> its chip selects, and any chip node whose first reg cell matches a GPIO
> index is silently flipped to active low. The NAND core requests the
> descriptors GPIOD_OUT_HIGH and drivers assert with a logical 0, so the
> inversion leaves the die permanently deselected.
>
> The example in nand-controller.yaml is itself affected: it pairs a
> native chip select with a GPIO one and gives the second chip a reg of 1,
> which trips the quirk whenever CONFIG_SPI_MASTER is enabled. In-tree the
> collision is real but latent. 36 board trees, all Atmel/Microchip at91,
> give their nand@3 controller a cs-gpios line, and they escape only
> because the sole child of those nodes is a partitions container with no
> reg for the quirk to match against.

:')

> Device tree carries no bus type marker, so identify the bus from two
> hints. Properties of an SPI peripheral are namespaced with "spi-"
> (spi-max-frequency, spi-cpol, spi-cs-high and the rest of
> spi-peripheral-props.yaml), whereas a NAND chip node carries only reg,
> nand-* and its partition table. That is a convention rather than a
> guarantee, since compatible and reg are the only properties
> spi-controller.yaml makes mandatory for a peripheral, so let the
> controller settle the remaining cases: spi-controller.yaml constrains
> the controller nodename to ^spi(@.*|-[0-9]+)?$, which makes the name the
> one bus marker every conforming controller has to carry. A peripheral
> that carries nothing but compatible and reg therefore keeps its active
> low default through its parent. Skip the quirk only when neither test
> matches.
>
> Both tests were scored over every board device tree in the kernel,
> expanded with the same cpp and scripts/dtc pipeline the build uses, at
> v7.2-rc7. All 3620 trees under arch/*/boot/dts expand; 503 of them hold
> at least one GPIO chip select that can reach the quirk, 765 such chip
> selects in total. 761 are matched by both tests, and none are matched by
> neither, so no in-tree board changes behaviour.
>
> One needs the property scan on its own: psc@11400 on ac14xx, a
> fsl,mpc5121-psc-spi named after the hardware block rather than the bus,
> whose m25p128@0 child carries spi-max-frequency.
>
> Three need the nodename on its own, all peripherals with no "spi-"
> property of any kind:
>
>   - panel@0 under the spi-gpio controller on rk3566-anbernic-rg503
>   - can@0, an mcp251xfd, under ecspi3 on imx8mn-vhip4-evalboard-v1
>   - spi@1 under ecspi1 on imx53-ppd
>
> The two tests are complementary, so both are needed to keep every
> in-tree board working. A tree that names its controller after the
> hardware block and gives it a peripheral with no "spi-" property would
> still lose the quirk. No in-tree board does, and such a controller is
> already outside the nodename pattern spi-controller.yaml requires.

Sounds okay to me.

> of_gpio_spi_cs_get_count() in this file identifies SPI controllers with
> of_device_is_compatible() instead, but it only has to name three legacy
> controllers whose bindings are closed. An allow-list here would have to
> name every SPI controller binding in the tree, 71 distinct compatible
> strings among the candidates alone, and grow with every new one.

That would be impossible to maintain indeed.

>
> Signed-off-by: Maciej Andrzejewski ICEYE <maciej.andrzejewski@xxxxxxxxxxx>

I believe "ICEYE" is your company which backed this work, in which case
you should use "(ICEYE)" instead.

Also, this probably deserves a Fixes and Cc: stable set of tags.

> ---
> Changes in v2:
> - Squashed the two patches into one. The property scan and the nodename
>   test are complementary, so patch 1 alone stopped applying the quirk to
>   the three peripherals that carry no "spi-" property, which would have
>   broken those boards for anyone bisecting through the series. Reported
>   by an automated review of v1.
> - Folded the two helpers into a single of_gpio_is_spi_chipselect().
> - Said outright that a peripheral with nothing but compatible and reg
>   keeps its active low default through the controller nodename.
> - Rebased; drivers/gpio/gpiolib-of.c is unchanged since v7.2-rc7, so the
>   diff itself is the v1 pair verbatim.
>
> v1: https://lore.kernel.org/r/20260810141629.81650-1-maciej.andrzejewski@xxxxxxxxxxx
>
>  drivers/gpio/gpiolib-of.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 940b566946ce..d73114166f6b 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -340,6 +340,28 @@ static void of_gpio_set_polarity_by_property(const struct device_node *np,
>  	}
>  }
>  
> +/*
> + * The legacy SPI chip select binding below is keyed on a property name that
> + * other subsystems reuse for the same purpose, notably NAND controllers (see
> + * Documentation/devicetree/bindings/mtd/nand-controller.yaml), whose chip
> + * selects carry no SPI polarity semantics. Device tree has no bus type
> + * marker, so take two hints. Properties of an SPI peripheral are namespaced
> + * with "spi-", and a peripheral that declares none is covered by the
> + * controller, whose nodename spi-controller.yaml constrains to
> + * ^spi(@.*|-[0-9]+)?$.
> + */
> +static bool of_gpio_is_spi_chipselect(const struct device_node *np,
> +				      const struct device_node *child)
> +{

This check will be done for each child, whereas we should have to do it
only once for a given controller node. Plus, as soon as one of the
children has a spi- property, it is enough for enabling the quirk for
all the children whereas in this implementation, only one child is used
at a time to derive the bus type, which means in theory you could have
one child node that gets the quirk and another child node of the same
parent which does not.

This is not a show stopper, but I believe this can be slightly
enhanced,if that does not make the code too complex.

With the comments on the tags and SoB addressed, you can nevertheless
add my:

Reviewed-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

Thanks,
Miquèl