[BUG/PATCH 0/4] gpiolib: fix issues with legacy spi-cs-high handling

[BUG/PATCH 0/4] gpiolib: fix issues with legacy spi-cs-high handling

[H. Nikolaus Schaller]

2018-07-25 07:55:28: 

H. Nikolaus Schaller (4):
  DTS: GTA04: SPI / gpiolib: work around a regression that unhides a
    broken legacy handler, which makes the gta04 display inoperable
  REVERT: DTS: GTA04: SPI / gpiolib: no no no, we do not want to "fix"
    our device tree file
  gpiolib: make legacy handling for spi-cs-high optional
  gpiolib: spi-cs-high is checking the wrong node (the controller and
    not the SPI slave device)

 drivers/gpio/Kconfig      | 8 ++++++++
 drivers/gpio/gpiolib-of.c | 8 +++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.19.1

[BUG/PATCH 1/4] DTS: GTA04: SPI / gpiolib: work around a regression that unhides a broken legacy handler, which makes the gta04 display inoperable

[H. Nikolaus Schaller]

This is an ugly workaround for a regression introduced into v5.1-rc1 by

<c1c04cea13dc234ce9a4504879ddd36ea524d880>

The GTA04 board uses a bitbanged SPI interface to control the LCD and it did work
right from the introduction of the cs-gpios 5 years ago by <c2e138bc8ed80ae291d689917e88d2d09bf1d478>
but unexpectedly stopped with v5.1-rc1.

Since we already use the gpio-flags for high level CS, we did not add spi-cs-high,
which is listed in the bindings documentation as optional and indicative and not
described as required to make it work. And it worked without this flag until now.

Now the newly introduced patch to gpiolib-of.c removes a bug which prevented an
ugly legacy handler for of SPI active high chip select to run. This now suddenly
requires spi-cs-high to be defined in addition to the gpio flags.

Trying to find a workaround in DTS shows that there seems to be another bug in the
legacy handler code in gpiolib-of.c because it checks for the spi-cs-high flag not
in the individual slave node (as one could expect from bindings documentation) but
the spi controller node.

Unfortunately it requires us to add legacy flags to a DTS to make the workaround
code functioning.

I would prefer that the workaround code and the spi-cs-high property is completely
removed from gpiolib-of.c and all DTS which still use legacy gpio flags are fixed.

fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of board candidates.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index 04f2b53d4d3d..1e134dfab428 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -114,6 +114,7 @@
 		gpio-miso = <&gpio1 18 GPIO_ACTIVE_HIGH>;
 		gpio-mosi = <&gpio1 20 GPIO_ACTIVE_HIGH>;
 		cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+		spi-cs-high;	/* make the legacy handler in gpiolib not intervene */
 		num-chipselects = <1>;
 
 		/* lcd panel */
@@ -123,6 +124,7 @@
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
+			spi-cs-high;	/* here is where it should be but this is ignored by a bug in the gpiolib handler */
 
 			backlight= <&backlight>;
 			label = "lcd";
-- 
2.19.1

[BUG/PATCH 2/4] REVERT: DTS: GTA04: SPI / gpiolib: no no no, we do not want to "fix" our device tree file

[H. Nikolaus Schaller]

just because some legacy boards seem to require changes in the gpio-lib
which make our so far correct DTS/DTB being wrongly interpreted.

A better solution seems to make the legacy handler optional so that DTS/DTB
developed without legacy modes are not harmed.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index 1e134dfab428..04f2b53d4d3d 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -114,7 +114,6 @@
 		gpio-miso = <&gpio1 18 GPIO_ACTIVE_HIGH>;
 		gpio-mosi = <&gpio1 20 GPIO_ACTIVE_HIGH>;
 		cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
-		spi-cs-high;	/* make the legacy handler in gpiolib not intervene */
 		num-chipselects = <1>;
 
 		/* lcd panel */
@@ -124,7 +123,6 @@
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
-			spi-cs-high;	/* here is where it should be but this is ignored by a bug in the gpiolib handler */
 
 			backlight= <&backlight>;
 			label = "lcd";
-- 
2.19.1

[BUG/PATCH 3/4] gpiolib: make legacy handling for spi-cs-high optional

[H. Nikolaus Schaller]

It is better to make legacy support a config option instead
of requiring non-legacy device tree files to be updated.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/Kconfig      | 8 ++++++++
 drivers/gpio/gpiolib-of.c | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..1a82619727c1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -39,6 +39,14 @@ config OF_GPIO
 	depends on OF
 	depends on HAS_IOMEM
 
+config SPI_CS_HIGH_LEGACY_HANDLING
+	def_bool n
+	depends on OF_GPIO
+	help
+	   This enables a legacy handler which decodes the boolean spi-cs-high
+	   property found in some DTS/DTB to correctly work with cs-gpios
+	   with GPIO_ACTIVE_HIGH.
+
 config GPIO_ACPI
 	def_bool y
 	depends on ACPI
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 8b9c3ab70f6e..a22d2233458b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -114,6 +114,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
 			of_node_full_name(np));
 	}
 
+#if defined(CONFIG_SPI_CS_HIGH_LEGACY_HANDLING)
 	/*
 	 * Legacy handling of SPI active high chip select. If we have a
 	 * property named "cs-gpios" we need to inspect the child node
@@ -158,6 +159,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
 			}
 		}
 	}
+#endif
 }
 
 /**
-- 
2.19.1

[BUG/PATCH 4/4] gpiolib: spi-cs-high is checking the wrong node (the controller and not the SPI slave device)

[H. Nikolaus Schaller]

Bindings say:

All slave nodes can contain the following optional properties:
- spi-cs-high     - Empty property indicating device requires chip select
		    active high.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpio/gpiolib-of.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index a22d2233458b..82e7a1f63b0a 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -143,16 +143,16 @@ static void of_gpio_flags_quirks(struct device_node *np,
 				 * conflict and the "spi-cs-high" flag will
 				 * take precedence.
 				 */
-				if (of_property_read_bool(np, "spi-cs-high")) {
+				if (of_property_read_bool(child, "spi-cs-high")) {
 					if (*flags & OF_GPIO_ACTIVE_LOW) {
 						pr_warn("%s GPIO handle specifies active low - ignored\n",
-							of_node_full_name(np));
+							of_node_full_name(child));
 						*flags &= ~OF_GPIO_ACTIVE_LOW;
 					}
 				} else {
 					if (!(*flags & OF_GPIO_ACTIVE_LOW))
 						pr_info("%s enforce active low on chipselect handle\n",
-							of_node_full_name(np));
+							of_node_full_name(child));
 					*flags |= OF_GPIO_ACTIVE_LOW;
 				}
 				break;
-- 
2.19.1

Re: [BUG/PATCH 1/4] DTS: GTA04: SPI / gpiolib: work around a regression that unhides a broken legacy handler, which makes the gta04 display inoperable

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller <hns@goldelico.com> [190331 10:08]:
> This is an ugly workaround for a regression introduced into v5.1-rc1 by
> 
> <c1c04cea13dc234ce9a4504879ddd36ea524d880>

You should standardized commit references like this instead:

c1c04cea13dc ("gpio: of: Fix logic inversion")

Then please also include a proper Fixes tag.

You can configure an alias for it to make things easier:

git log --abbrev=12 --pretty=format:"%h (\"%s\")"

> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -114,6 +114,7 @@
>  		gpio-miso = <&gpio1 18 GPIO_ACTIVE_HIGH>;
>  		gpio-mosi = <&gpio1 20 GPIO_ACTIVE_HIGH>;
>  		cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> +		spi-cs-high;	/* make the legacy handler in gpiolib not intervene */
>  		num-chipselects = <1>;
>  
>  		/* lcd panel */
> @@ -123,6 +124,7 @@
>  			spi-max-frequency = <100000>;
>  			spi-cpol;
>  			spi-cpha;
> +			spi-cs-high;	/* here is where it should be but this is ignored by a bug in the gpiolib handler */
>  
>  			backlight= <&backlight>;
>  			label = "lcd";

So what's the plan with this fix? Add this for v5.2-rc cycle,
then revert it later? I'd probably go directly for the proper
fix if possible.

Regards,

Tony

Re: [BUG/PATCH 1/4] DTS: GTA04: SPI / gpiolib: work around a regression that unhides a broken legacy handler, which makes the gta04 display inoperable

[H. Nikolaus Schaller]

Hi Tony,

> Am 01.04.2019 um 18:46 schrieb Tony Lindgren <tony@atomide.com>:
> 
> Hi,
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [190331 10:08]:
>> This is an ugly workaround for a regression introduced into v5.1-rc1 by
>> 
>> <c1c04cea13dc234ce9a4504879ddd36ea524d880>
> 
> You should standardized commit references like this instead:
> 
> c1c04cea13dc ("gpio: of: Fix logic inversion")
> 
> Then please also include a proper Fixes tag.
> 
> You can configure an alias for it to make things easier:
> 
> git log --abbrev=12 --pretty=format:"%h (\"%s\")"
> 
>> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
>> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
>> @@ -114,6 +114,7 @@
>> 		gpio-miso = <&gpio1 18 GPIO_ACTIVE_HIGH>;
>> 		gpio-mosi = <&gpio1 20 GPIO_ACTIVE_HIGH>;
>> 		cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
>> +		spi-cs-high;	/* make the legacy handler in gpiolib not intervene */
>> 		num-chipselects = <1>;
>> 
>> 		/* lcd panel */
>> @@ -123,6 +124,7 @@
>> 			spi-max-frequency = <100000>;
>> 			spi-cpol;
>> 			spi-cpha;
>> +			spi-cs-high;	/* here is where it should be but this is ignored by a bug in the gpiolib handler */
>> 
>> 			backlight= <&backlight>;
>> 			label = "lcd";
> 
> So what's the plan with this fix? Add this for v5.2-rc cycle,
> then revert it later? I'd probably go directly for the proper
> fix if possible.

Well, I learned just some minutes ago that a new patch for gpiolib
came through 5.1-rc3 which does fix this.

It seems to check for "cs-gpios" and then disable the legacy handling.

This means that we do not need to patch the DTS at all.

BR and thanks,
Nikolaus

Re: [BUG/PATCH 1/4] DTS: GTA04: SPI / gpiolib: work around a regression that unhides a broken legacy handler, which makes the gta04 display inoperable

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [190401 16:57]:
> > Am 01.04.2019 um 18:46 schrieb Tony Lindgren <tony@atomide.com>:
> > So what's the plan with this fix? Add this for v5.2-rc cycle,
> > then revert it later? I'd probably go directly for the proper
> > fix if possible.
> 
> Well, I learned just some minutes ago that a new patch for gpiolib
> came through 5.1-rc3 which does fix this.
> 
> It seems to check for "cs-gpios" and then disable the legacy handling.
> 
> This means that we do not need to patch the DTS at all.

OK good to hear, thanks for the update.

Regards,

Tony

Re: [BUG/PATCH 0/4] gpiolib: fix issues with legacy spi-cs-high handling

[H. Nikolaus Schaller]

Hi all,

> Am 31.03.2019 um 12:07 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 2018-07-25 07:55:28: 
> 
> H. Nikolaus Schaller (4):
>  DTS: GTA04: SPI / gpiolib: work around a regression that unhides a
>    broken legacy handler, which makes the gta04 display inoperable
>  REVERT: DTS: GTA04: SPI / gpiolib: no no no, we do not want to "fix"
>    our device tree file
>  gpiolib: make legacy handling for spi-cs-high optional
>  gpiolib: spi-cs-high is checking the wrong node (the controller and
>    not the SPI slave device)
> 
> drivers/gpio/Kconfig      | 8 ++++++++
> drivers/gpio/gpiolib-of.c | 8 +++++---
> 2 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.19.1
> 

I learned just some minutes ago that a new patch for gpiolib
came through 5.1-rc3 which does fix this in almost the same way
as I proposed.

Except that it seems to check for "cs-gpios" and then disable the
legacy handling.

So we do not need to add a config option for this legacy
handling (although I'd prefer to have it anyways to remove
some dead code).

Summary: this series can now be ignored.

BR and thanks,
Nikolaus

Re: [BUG/PATCH 0/4] gpiolib: fix issues with legacy spi-cs-high handling

[Linus Walleij]

On Mon, Apr 1, 2019 at 11:59 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> I learned just some minutes ago that a new patch for gpiolib
> came through 5.1-rc3 which does fix this in almost the same way
> as I proposed.

I am sorry for not being very attentive, I have been travelling.
I collected the most urgent fixes and this was fixed as part of
that quick patchup.

> Except that it seems to check for "cs-gpios" and then disable the
> legacy handling.

Yups.

> So we do not need to add a config option for this legacy
> handling (although I'd prefer to have it anyways to remove
> some dead code).
>
> Summary: this series can now be ignored.

OK please verify that we are doing things right now, the big
worry I have is some interrim DTBs that use the GPIO flag
GPIO_ACTIVE_HIGH, which is zero and therefore often the
default, and expect it to actually work, while the SPI bindings
by definition will ignore that and make the line active low.

I'm not very happy about this but it is a result of how we quickly
implemented some bindings in the past while the GPIO flags
were not yet standardized.

Yours,
Linus Walleij