[PATCH 0/2] gpiolib: of: Scope the legacy SPI CS quirk to SPI

[PATCH 0/2] gpiolib: of: Scope the legacy SPI CS quirk to SPI

[Maciej Andrzejewski ICEYE]

of_gpio_flags_quirks() decides whether to apply SPI chip-select polarity
semantics purely from the property name "cs-gpios". Nothing about that
name is SPI specific. nand-controller.yaml documents it for NAND
controllers too, and rawnand_dt_parse_gpio_cs() asks for those lines
with gpiod_count(dev, "cs"), which gpiolib expands to "cs-gpios". The
result is that a NAND chip node whose first reg cell happens to equal a
GPIO index gets flipped to active low, and since the NAND core requests
the descriptors GPIOD_OUT_HIGH and drivers assert with a logical 0, the
die is never selected.

We hit this on an Arasan controller with a GPIO CS1 and worked around it
downstream by putting spi-cs-high on the NAND chip node, which is
plainly the wrong property to need. These two patches narrow the quirk
instead, so that it only fires on an SPI bus.

Device tree has no bus type marker, so the series uses two tests, one
per patch, and applies the quirk if either matches:

  1. the matched child has a property namespaced with "spi-"
  2. the controller nodename has the "spi" prefix that
     spi-controller.yaml requires

Testing
-------

Both tests were scored against every board device tree in the kernel.
Each .dts under arch/*/boot/dts was expanded with the same cpp and
scripts/dtc pipeline the build uses; all 3620 expand. 503 of them hold
at least one GPIO chip select that can reach the quirk, 765 in total.
761 match both tests, 1 needs only the property scan, 3 need only the
nodename, and none match neither, so no in-tree board changes behaviour.
Patch 2 names the four exceptions individually.

The collision with NAND is in-tree but latent: 36 board trees, all
Atmel/Microchip at91, give their nand@3 controller a cs-gpios line and
escape only because the sole child of those nodes is a partitions
container with no reg for the quirk to match against.

drivers/gpio/gpiolib-of.o builds warning-free at W=1 with CONFIG_SPI=y
and with CONFIG_SPI=n.

Note on bisect
--------------

The two tests are complementary rather than layered, so patch 1 on its
own would stop applying the quirk to the three peripherals that carry no
"spi-" property (a panel, an mcp251xfd and one ecspi child, all named in
patch 2). Only the pair is behaviour-preserving. They are split because
the property scan and the nodename test are separate arguments and I
expect the second to attract more discussion. Happy to squash them into
one commit if you would rather not have that window.

Maciej Andrzejewski ICEYE (2):
  gpiolib: of: Only apply the SPI CS quirk to SPI peripherals
  gpiolib: of: Identify SPI controllers by nodename

 drivers/gpio/gpiolib-of.c | 40 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)


base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 0/2] gpiolib: of: Scope the legacy SPI CS quirk to SPI

[Maciej Andrzejewski ICEYE]

of_gpio_flags_quirks() decides whether to apply SPI chip-select polarity
semantics purely from the property name "cs-gpios". Nothing about that
name is SPI specific. nand-controller.yaml documents it for NAND
controllers too, and rawnand_dt_parse_gpio_cs() asks for those lines
with gpiod_count(dev, "cs"), which gpiolib expands to "cs-gpios". The
result is that a NAND chip node whose first reg cell happens to equal a
GPIO index gets flipped to active low, and since the NAND core requests
the descriptors GPIOD_OUT_HIGH and drivers assert with a logical 0, the
die is never selected.

We hit this on an Arasan controller with a GPIO CS1 and worked around it
downstream by putting spi-cs-high on the NAND chip node, which is
plainly the wrong property to need. These two patches narrow the quirk
instead, so that it only fires on an SPI bus.

Device tree has no bus type marker, so the series uses two tests, one
per patch, and applies the quirk if either matches:

  1. the matched child has a property namespaced with "spi-"
  2. the controller nodename has the "spi" prefix that
     spi-controller.yaml requires

Testing
-------

Both tests were scored against every board device tree in the kernel.
Each .dts under arch/*/boot/dts was expanded with the same cpp and
scripts/dtc pipeline the build uses; all 3620 expand. 503 of them hold
at least one GPIO chip select that can reach the quirk, 765 in total.
761 match both tests, 1 needs only the property scan, 3 need only the
nodename, and none match neither, so no in-tree board changes behaviour.
Patch 2 names the four exceptions individually.

The collision with NAND is in-tree but latent: 36 board trees, all
Atmel/Microchip at91, give their nand@3 controller a cs-gpios line and
escape only because the sole child of those nodes is a partitions
container with no reg for the quirk to match against.

drivers/gpio/gpiolib-of.o builds warning-free at W=1 with CONFIG_SPI=y
and with CONFIG_SPI=n.

Note on bisect
--------------

The two tests are complementary rather than layered, so patch 1 on its
own would stop applying the quirk to the three peripherals that carry no
"spi-" property (a panel, an mcp251xfd and one ecspi child, all named in
patch 2). Only the pair is behaviour-preserving. They are split because
the property scan and the nodename test are separate arguments and I
expect the second to attract more discussion. Happy to squash them into
one commit if you would rather not have that window.

Maciej Andrzejewski ICEYE (2):
  gpiolib: of: Only apply the SPI CS quirk to SPI peripherals
  gpiolib: of: Identify SPI controllers by nodename

 drivers/gpio/gpiolib-of.c | 40 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)


base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
-- 
2.50.1

[PATCH 1/2] gpiolib: of: Only apply the SPI CS quirk to SPI peripherals

[Maciej Andrzejewski ICEYE]

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 what
the peripheral advertises about itself. 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. Skip the quirk when the
matched child has no "spi-" prefixed property.

This is a convention rather than a guarantee: compatible and reg are the
only properties spi-controller.yaml makes mandatory for a peripheral, so
a minimal one may carry no "spi-" property at all. The next patch covers
those by looking at the controller instead.

Signed-off-by: Maciej Andrzejewski ICEYE <maciej.andrzejewski@m-works.net>
---
 drivers/gpio/gpiolib-of.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 940b566946ce..bd9a623f2ae1 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -340,6 +340,25 @@ 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 go by what the peripheral advertises about itself: properties
+ * of an SPI peripheral are namespaced with "spi-".
+ */
+static bool of_gpio_child_is_spi_peripheral(const struct device_node *child)
+{
+	struct property *pp;
+
+	for_each_property_of_node(child, pp)
+		if (str_has_prefix(pp->name, "spi-"))
+			return true;
+
+	return false;
+}
+
 static void of_gpio_flags_quirks(const struct device_node *np,
 				 const char *propname,
 				 enum of_gpio_flags *flags,
@@ -374,6 +393,11 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 			if (ret)
 				continue;
 			if (cs == index) {
+				bool active_high;
+
+				if (!of_gpio_child_is_spi_peripheral(child))
+					break;
+
 				/*
 				 * SPI children have active low chip selects
 				 * by default. This can be specified negatively
@@ -386,8 +410,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 				 * conflict and the "spi-cs-high" flag will
 				 * take precedence.
 				 */
-				bool active_high = of_property_read_bool(child,
-								"spi-cs-high");
+				active_high = of_property_read_bool(child,
+								    "spi-cs-high");
 				of_gpio_quirk_polarity(child, active_high,
 						       flags);
 				break;
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] gpiolib: of: Only apply the SPI CS quirk to SPI peripherals

[Maciej Andrzejewski ICEYE]

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 what
the peripheral advertises about itself. 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. Skip the quirk when the
matched child has no "spi-" prefixed property.

This is a convention rather than a guarantee: compatible and reg are the
only properties spi-controller.yaml makes mandatory for a peripheral, so
a minimal one may carry no "spi-" property at all. The next patch covers
those by looking at the controller instead.

Signed-off-by: Maciej Andrzejewski ICEYE <maciej.andrzejewski@m-works.net>
---
 drivers/gpio/gpiolib-of.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 940b566946ce..bd9a623f2ae1 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -340,6 +340,25 @@ 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 go by what the peripheral advertises about itself: properties
+ * of an SPI peripheral are namespaced with "spi-".
+ */
+static bool of_gpio_child_is_spi_peripheral(const struct device_node *child)
+{
+	struct property *pp;
+
+	for_each_property_of_node(child, pp)
+		if (str_has_prefix(pp->name, "spi-"))
+			return true;
+
+	return false;
+}
+
 static void of_gpio_flags_quirks(const struct device_node *np,
 				 const char *propname,
 				 enum of_gpio_flags *flags,
@@ -374,6 +393,11 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 			if (ret)
 				continue;
 			if (cs == index) {
+				bool active_high;
+
+				if (!of_gpio_child_is_spi_peripheral(child))
+					break;
+
 				/*
 				 * SPI children have active low chip selects
 				 * by default. This can be specified negatively
@@ -386,8 +410,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 				 * conflict and the "spi-cs-high" flag will
 				 * take precedence.
 				 */
-				bool active_high = of_property_read_bool(child,
-								"spi-cs-high");
+				active_high = of_property_read_bool(child,
+								    "spi-cs-high");
 				of_gpio_quirk_polarity(child, active_high,
 						       flags);
 				break;
-- 
2.50.1

[PATCH 2/2] gpiolib: of: Identify SPI controllers by nodename

[Maciej Andrzejewski ICEYE]

The "spi-" property scan added by the previous patch misses SPI
peripherals that declare nothing beyond compatible and reg, which are
the only two properties spi-controller.yaml makes mandatory for a child
node. Let the controller settle those cases: spi-controller.yaml
constrains the controller nodename to ^spi(@.*|-[0-9]+)?$, so the name
is the one bus marker every conforming SPI controller has to carry, and
of_node_name_prefix() is the existing helper for testing it.

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

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.

Signed-off-by: Maciej Andrzejewski ICEYE <maciej.andrzejewski@m-works.net>
---
 drivers/gpio/gpiolib-of.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bd9a623f2ae1..959b32b3bea0 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -359,6 +359,17 @@ static bool of_gpio_child_is_spi_peripheral(const struct device_node *child)
 	return false;
 }
 
+/*
+ * A minimal SPI peripheral may carry no "spi-" property at all, so also let
+ * the controller identify the bus. spi-controller.yaml constrains the
+ * controller nodename to ^spi(@.*|-[0-9]+)?$, which makes the name the one
+ * bus marker a controller is required to have.
+ */
+static bool of_gpio_node_is_spi_controller(const struct device_node *np)
+{
+	return of_node_name_prefix(np, "spi");
+}
+
 static void of_gpio_flags_quirks(const struct device_node *np,
 				 const char *propname,
 				 enum of_gpio_flags *flags,
@@ -395,7 +406,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 			if (cs == index) {
 				bool active_high;
 
-				if (!of_gpio_child_is_spi_peripheral(child))
+				if (!of_gpio_child_is_spi_peripheral(child) &&
+				    !of_gpio_node_is_spi_controller(np))
 					break;
 
 				/*
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] gpiolib: of: Identify SPI controllers by nodename

[Maciej Andrzejewski ICEYE]

The "spi-" property scan added by the previous patch misses SPI
peripherals that declare nothing beyond compatible and reg, which are
the only two properties spi-controller.yaml makes mandatory for a child
node. Let the controller settle those cases: spi-controller.yaml
constrains the controller nodename to ^spi(@.*|-[0-9]+)?$, so the name
is the one bus marker every conforming SPI controller has to carry, and
of_node_name_prefix() is the existing helper for testing it.

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

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.

Signed-off-by: Maciej Andrzejewski ICEYE <maciej.andrzejewski@m-works.net>
---
 drivers/gpio/gpiolib-of.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bd9a623f2ae1..959b32b3bea0 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -359,6 +359,17 @@ static bool of_gpio_child_is_spi_peripheral(const struct device_node *child)
 	return false;
 }
 
+/*
+ * A minimal SPI peripheral may carry no "spi-" property at all, so also let
+ * the controller identify the bus. spi-controller.yaml constrains the
+ * controller nodename to ^spi(@.*|-[0-9]+)?$, which makes the name the one
+ * bus marker a controller is required to have.
+ */
+static bool of_gpio_node_is_spi_controller(const struct device_node *np)
+{
+	return of_node_name_prefix(np, "spi");
+}
+
 static void of_gpio_flags_quirks(const struct device_node *np,
 				 const char *propname,
 				 enum of_gpio_flags *flags,
@@ -395,7 +406,8 @@ static void of_gpio_flags_quirks(const struct device_node *np,
 			if (cs == index) {
 				bool active_high;
 
-				if (!of_gpio_child_is_spi_peripheral(child))
+				if (!of_gpio_child_is_spi_peripheral(child) &&
+				    !of_gpio_node_is_spi_controller(np))
 					break;
 
 				/*
-- 
2.50.1

Re: [PATCH 1/2] gpiolib: of: Only apply the SPI CS quirk to SPI peripherals

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Functional regression breaking SPI chip select polarity for minimal SPI peripherals, breaking git bisect.
--

--- Patch [1]: [PATCH 1/2] gpiolib: of: Only apply the SPI CS quirk to SPI peripherals ---
commit 119ec452c0cd58efb0a9c22023e3cbf0565c7c18
Author: Maciej Andrzejewski ICEYE <maciej.andrzejewski@m-works.net>

gpiolib: of: Only apply the SPI CS quirk to SPI peripherals

This commit restricts the legacy SPI chip-select polarity quirk to nodes
that actually represent SPI peripherals, identifying them by checking for
properties with an "spi-" prefix. It aims to prevent the quirk from
improperly applying to other subsystems like NAND controllers that also use
the "cs-gpios" property.

>     This is a convention rather than a guarantee: compatible and reg are the
>     only properties spi-controller.yaml makes mandatory for a peripheral, so
>     a minimal one may carry no "spi-" property at all. The next patch covers
>     those by looking at the controller instead.

[Severity: High]
Does deferring the fix for minimal SPI peripherals to the next patch
introduce a functional regression at this specific commit?

Since every individual commit must be self-contained and fully functional to
preserve git bisect, should the logic covering the controller be combined
into this patch to avoid breaking boards with minimal SPI peripherals during
a bisection?

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 940b566946ce3..bd9a623f2ae16 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
[ ... ]
> @@ -374,6 +393,11 @@ static void of_gpio_flags_quirks(const struct device_node *np,
>  			if (ret)
>  				continue;
>  			if (cs == index) {
> +				bool active_high;
> +
> +				if (!of_gpio_child_is_spi_peripheral(child))
> +					break;

[Severity: High]
Will this break SPI chip select polarity for minimal SPI peripherals that
lack "spi-" prefixed properties?

If of_gpio_child_is_spi_peripheral() returns false for a node that only has
"compatible" and "reg", the early break skips the of_gpio_quirk_polarity()
fixup. Their chip selects would default to active-high, potentially leaving
the device permanently deselected and unusable.

> +
>  				/*
>  				 * SPI children have active low chip selects
>  				 * by default. This can be specified negatively

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810141629.81650-1-maciej.andrzejewski@m-works.net?part=1