[2/2] DTS: ARM: gta04: introduce legacy spi-cs-high to make display work again
diff mbox series

Message ID 8ae7cf816b22ef9cecee0d789fcf9e8a06495c39.1562597164.git.hns@goldelico.com
State Changes Requested
Headers show
Series
  • DTS: ARM: some minor updates and fixes for GTA04
Related show

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 25 lines checked"

Commit Message

H. Nikolaus Schaller July 8, 2019, 2:46 p.m. UTC
commit 6953c57ab172 "gpio: of: Handle SPI chipselect legacy bindings"

did introduce logic to centrally handle the legacy spi-cs-high property
in combination with cs-gpios. This assumes that the polarity
of the CS has to be inverted if spi-cs-high is missing, even
and especially if non-legacy GPIO_ACTIVE_HIGH is specified.

The DTS for the GTA04 was orginally introduced under the assumption
that there is no need for spi-cs-high if the gpio is defined with
proper polarity GPIO_ACTIVE_HIGH.

This was not a problem until gpiolib changed the interpretation of
GPIO_ACTIVE_HIGH and missing spi-cs-high.

The effect is that the missing spi-cs-high is now interpreted as CS being
low (despite GPIO_ACTIVE_HIGH) which turns off the SPI interface when the
panel is to be programmed by the panel driver.

Therefore, we have to add the redundant and legacy spi-cs-high property
to properly pass through the legacy handler.

Since this is nowhere documented in the bindings, we add some words of
WARNING.

Cc: stable@vger.kernel.org
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt | 6 ++++++
 arch/arm/boot/dts/omap3-gta04.dtsi                | 1 +
 2 files changed, 7 insertions(+)

Comments

Rob Herring July 24, 2019, 7:42 p.m. UTC | #1
On Mon, Jul 08, 2019 at 04:46:05PM +0200, H. Nikolaus Schaller wrote:
> commit 6953c57ab172 "gpio: of: Handle SPI chipselect legacy bindings"
> 
> did introduce logic to centrally handle the legacy spi-cs-high property
> in combination with cs-gpios. This assumes that the polarity
> of the CS has to be inverted if spi-cs-high is missing, even
> and especially if non-legacy GPIO_ACTIVE_HIGH is specified.
> 
> The DTS for the GTA04 was orginally introduced under the assumption
> that there is no need for spi-cs-high if the gpio is defined with
> proper polarity GPIO_ACTIVE_HIGH.

Given that spi-cs-high is called legacy, that would imply that DT's 
should not have to use spi-cs-high.

> This was not a problem until gpiolib changed the interpretation of
> GPIO_ACTIVE_HIGH and missing spi-cs-high.

Then we should fix gpiolib...

> The effect is that the missing spi-cs-high is now interpreted as CS being
> low (despite GPIO_ACTIVE_HIGH) which turns off the SPI interface when the
> panel is to be programmed by the panel driver.
> 
> Therefore, we have to add the redundant and legacy spi-cs-high property
> to properly pass through the legacy handler.
> 
> Since this is nowhere documented in the bindings, we add some words of
> WARNING.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt | 6 ++++++
>  arch/arm/boot/dts/omap3-gta04.dtsi                | 1 +
>  2 files changed, 7 insertions(+)
H. Nikolaus Schaller July 25, 2019, 6:23 a.m. UTC | #2
Hi Rob,

> Am 24.07.2019 um 21:42 schrieb Rob Herring <robh@kernel.org>:
> 
> On Mon, Jul 08, 2019 at 04:46:05PM +0200, H. Nikolaus Schaller wrote:
>> commit 6953c57ab172 "gpio: of: Handle SPI chipselect legacy bindings"
>> 
>> did introduce logic to centrally handle the legacy spi-cs-high property
>> in combination with cs-gpios. This assumes that the polarity
>> of the CS has to be inverted if spi-cs-high is missing, even
>> and especially if non-legacy GPIO_ACTIVE_HIGH is specified.
>> 
>> The DTS for the GTA04 was orginally introduced under the assumption
>> that there is no need for spi-cs-high if the gpio is defined with
>> proper polarity GPIO_ACTIVE_HIGH.
> 
> Given that spi-cs-high is called legacy, that would imply that DT's 
> should not have to use spi-cs-high.

Yes.

> 
>> This was not a problem until gpiolib changed the interpretation of
>> GPIO_ACTIVE_HIGH and missing spi-cs-high.
> 
> Then we should fix gpiolib...

I tried to convince Linus that this is the right way but he convinced
me that a fix that handles all cases does not exist.

There seem to be embedded devices with older DTB (potentially in ROM)
which provide a plain 0 value for a gpios definition. And either with
or without spi-cs-high.

Since "0" is the same as "GPIO_ACTIVE_HIGH", the absence of
spi-cs-high was and must be interpreted as active low for these
devices. This leads to the inversion logic in code.

AFAIR it boils down to the question if gpiolib and the bindings
should still support such legacy devices with out-of tree DTB,
but force in-tree DTS to add the legacy spi-cs-high property.

Or if we should fix the 2 or 3 cases of in-tree legacy cases
and potentially break out-of tree DTBs.

IMHO it is more general to keep the out-of-tree DTBs working
and "fix" what we can control (in-tree DTS).

> 
>> The effect is that the missing spi-cs-high is now interpreted as CS being
>> low (despite GPIO_ACTIVE_HIGH) which turns off the SPI interface when the
>> panel is to be programmed by the panel driver.
>> 
>> Therefore, we have to add the redundant and legacy spi-cs-high property
>> to properly pass through the legacy handler.
>> 
>> Since this is nowhere documented in the bindings, we add some words of
>> WARNING.
>> 
>> Cc: stable@vger.kernel.org
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> Documentation/devicetree/bindings/spi/spi-bus.txt | 6 ++++++
>> arch/arm/boot/dts/omap3-gta04.dtsi                | 1 +
>> 2 files changed, 7 insertions(+)

BR,
Nikolaus
Rob Herring July 25, 2019, 10:42 p.m. UTC | #3
On Thu, Jul 25, 2019 at 12:23 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> Hi Rob,
>
> > Am 24.07.2019 um 21:42 schrieb Rob Herring <robh@kernel.org>:
> >
> > On Mon, Jul 08, 2019 at 04:46:05PM +0200, H. Nikolaus Schaller wrote:
> >> commit 6953c57ab172 "gpio: of: Handle SPI chipselect legacy bindings"
> >>
> >> did introduce logic to centrally handle the legacy spi-cs-high property
> >> in combination with cs-gpios. This assumes that the polarity
> >> of the CS has to be inverted if spi-cs-high is missing, even
> >> and especially if non-legacy GPIO_ACTIVE_HIGH is specified.
> >>
> >> The DTS for the GTA04 was orginally introduced under the assumption
> >> that there is no need for spi-cs-high if the gpio is defined with
> >> proper polarity GPIO_ACTIVE_HIGH.
> >
> > Given that spi-cs-high is called legacy, that would imply that DT's
> > should not have to use spi-cs-high.
>
> Yes.
>
> >
> >> This was not a problem until gpiolib changed the interpretation of
> >> GPIO_ACTIVE_HIGH and missing spi-cs-high.
> >
> > Then we should fix gpiolib...
>
> I tried to convince Linus that this is the right way but he convinced
> me that a fix that handles all cases does not exist.
>
> There seem to be embedded devices with older DTB (potentially in ROM)
> which provide a plain 0 value for a gpios definition. And either with
> or without spi-cs-high.
>
> Since "0" is the same as "GPIO_ACTIVE_HIGH", the absence of
> spi-cs-high was and must be interpreted as active low for these
> devices. This leads to the inversion logic in code.
>
> AFAIR it boils down to the question if gpiolib and the bindings
> should still support such legacy devices with out-of tree DTB,
> but force in-tree DTS to add the legacy spi-cs-high property.
>
> Or if we should fix the 2 or 3 cases of in-tree legacy cases
> and potentially break out-of tree DTBs.

If it is small number of platforms, then the kernel could handle those
cases explicitly as needed.

> IMHO it is more general to keep the out-of-tree DTBs working
> and "fix" what we can control (in-tree DTS).

If we do this, then we need to not call spi-cs-high legacy because
we're stuck with it forever.

Rob
Linus Walleij Aug. 5, 2019, 10:29 a.m. UTC | #4
On Fri, Jul 26, 2019 at 12:43 AM Rob Herring <robh@kernel.org> wrote:
> On Thu, Jul 25, 2019 at 12:23 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> > I tried to convince Linus that this is the right way but he convinced
> > me that a fix that handles all cases does not exist.
> >
> > There seem to be embedded devices with older DTB (potentially in ROM)
> > which provide a plain 0 value for a gpios definition. And either with
> > or without spi-cs-high.
> >
> > Since "0" is the same as "GPIO_ACTIVE_HIGH", the absence of
> > spi-cs-high was and must be interpreted as active low for these
> > devices. This leads to the inversion logic in code.
> >
> > AFAIR it boils down to the question if gpiolib and the bindings
> > should still support such legacy devices with out-of tree DTB,
> > but force in-tree DTS to add the legacy spi-cs-high property.
> >
> > Or if we should fix the 2 or 3 cases of in-tree legacy cases
> > and potentially break out-of tree DTBs.
>
> If it is small number of platforms, then the kernel could handle those
> cases explicitly as needed.
>
> > IMHO it is more general to keep the out-of-tree DTBs working
> > and "fix" what we can control (in-tree DTS).
>
> If we do this, then we need to not call spi-cs-high legacy because
> we're stuck with it forever.

I agree. The background on it is here:
https://lkml.org/lkml/2019/4/2/4

Not using the negatively defined (i.e. if it is no there, the line is
by default active low) spi-cs-high would break
PowerPC, who were AFAICT using this to ship devices.

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 1f6e86f787ef..982aa590058b 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -47,6 +47,10 @@  cs1 : native
 cs2 : &gpio1 1 0
 cs3 : &gpio1 2 0
 
+WARNING: the polarity of cs-gpios may be inverted in some cases compared
+to what is specified in the third parameter. In that case the spi-cs-high
+property must be defined for slave nodes.
+
 
 SPI slave nodes must be children of the SPI controller node.
 
@@ -69,6 +73,8 @@  All slave nodes can contain the following optional properties:
 		    phase (CPHA) mode.
 - spi-cs-high     - Empty property indicating device requires chip select
 		    active high.
+                   WARNING: this is especially required even if the cs-gpios
+		    define the gpio as GPIO_ACTIVE_HIGH
 - spi-3wire       - Empty property indicating device requires 3-wire mode.
 - spi-lsb-first   - Empty property indicating device requires LSB first mode.
 - spi-tx-bus-width - The bus width (number of data wires) that is used for MOSI.
diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index 9a9a29fe88ec..47bab8e1040e 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -124,6 +124,7 @@ 
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
+			spi-cs-high;
 
 			backlight= <&backlight>;
 			label = "lcd";