| Message ID | e8d784f7fb06fd3a6a92bc730f919352c4ad4411.1554026842.git.hns@goldelico.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: fix issues with legacy spi-cs-high handling | expand |
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
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
* 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
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";
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(+)