diff mbox series

[v2,2/6] dt-bindings: display: st7789v: bound the number of Rx data lines

Message ID 20230616163255.2804163-3-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show
Series drm/panel: sitronix-st7789v: Support ET028013DMA panel | expand

Commit Message

Miquel Raynal June 16, 2023, 4:32 p.m. UTC
The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
data line at all. The operating system needs to know whether it can read
registers from the device or not. Let's detail this specific design
possibility by bounding the spi-rx-bus-width property.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Kozlowski June 17, 2023, 8:53 a.m. UTC | #1
On 16/06/2023 18:32, Miquel Raynal wrote:
> The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> data line at all. The operating system needs to know whether it can read
> registers from the device or not. Let's detail this specific design
> possibility by bounding the spi-rx-bus-width property.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> index 0ccf0487fd8e..a25df7e1df88 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> @@ -29,6 +29,10 @@ properties:
>    spi-cpha: true
>    spi-cpol: true
>  
> +  spi-rx-bus-width:
> +    minimum: 0
> +    maximum: 1

0 is already minimum, but I understand you want to emphasize lack of RX.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Maxime Ripard June 18, 2023, 2:37 p.m. UTC | #2
Hi,

On Fri, Jun 16, 2023 at 06:32:51PM +0200, Miquel Raynal wrote:
> The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> data line at all. The operating system needs to know whether it can read
> registers from the device or not. Let's detail this specific design
> possibility by bounding the spi-rx-bus-width property.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> index 0ccf0487fd8e..a25df7e1df88 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> @@ -29,6 +29,10 @@ properties:
>    spi-cpha: true
>    spi-cpol: true
>  
> +  spi-rx-bus-width:
> +    minimum: 0
> +    maximum: 1
> +

It's not clear to me what the default would be?

Maxime
Miquel Raynal June 18, 2023, 5:37 p.m. UTC | #3
Hello Maxime,

maxime@cerno.tech wrote on Sun, 18 Jun 2023 16:37:58 +0200:

> Hi,
> 
> On Fri, Jun 16, 2023 at 06:32:51PM +0200, Miquel Raynal wrote:
> > The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> > data line at all. The operating system needs to know whether it can read
> > registers from the device or not. Let's detail this specific design
> > possibility by bounding the spi-rx-bus-width property.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > index 0ccf0487fd8e..a25df7e1df88 100644
> > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > @@ -29,6 +29,10 @@ properties:
> >    spi-cpha: true
> >    spi-cpol: true
> >  
> > +  spi-rx-bus-width:
> > +    minimum: 0
> > +    maximum: 1
> > +  
> 
> It's not clear to me what the default would be?

This binding references spi-peripheral-props.yaml which sets the
default to 1, I believe it is sane to keep it that way?

Thanks,
Miquèl
Maxime Ripard June 19, 2023, 9:39 a.m. UTC | #4
On Sun, Jun 18, 2023 at 07:37:32PM +0200, Miquel Raynal wrote:
> Hello Maxime,
> 
> maxime@cerno.tech wrote on Sun, 18 Jun 2023 16:37:58 +0200:
> 
> > Hi,
> > 
> > On Fri, Jun 16, 2023 at 06:32:51PM +0200, Miquel Raynal wrote:
> > > The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> > > data line at all. The operating system needs to know whether it can read
> > > registers from the device or not. Let's detail this specific design
> > > possibility by bounding the spi-rx-bus-width property.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > index 0ccf0487fd8e..a25df7e1df88 100644
> > > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > @@ -29,6 +29,10 @@ properties:
> > >    spi-cpha: true
> > >    spi-cpol: true
> > >  
> > > +  spi-rx-bus-width:
> > > +    minimum: 0
> > > +    maximum: 1
> > > +  
> > 
> > It's not clear to me what the default would be?
> 
> This binding references spi-peripheral-props.yaml which sets the
> default to 1, I believe it is sane to keep it that way?

I'm not sure.

The driver didn't need RX before, and we didn't have any property that
was expressing whether we had MISO in the device tree.

That means we had both devices with and without MISO expressed in the
same way, the driver handling both (by ignoring MISO entirely).

With this patch, you now introduce a property that specifies whether
MISO is connected or not, and defaults to MISO being there. And a later
patch will use MISO if it's available.

This means that, while it's working fine for devices that had MISO
connected, devices that didn't are assumed to have it, and the driver
makes use of it.

Maxime
Miquel Raynal June 19, 2023, 10:19 a.m. UTC | #5
Hi Maxime,

maxime@cerno.tech wrote on Mon, 19 Jun 2023 11:39:56 +0200:

> On Sun, Jun 18, 2023 at 07:37:32PM +0200, Miquel Raynal wrote:
> > Hello Maxime,
> > 
> > maxime@cerno.tech wrote on Sun, 18 Jun 2023 16:37:58 +0200:
> >   
> > > Hi,
> > > 
> > > On Fri, Jun 16, 2023 at 06:32:51PM +0200, Miquel Raynal wrote:  
> > > > The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> > > > data line at all. The operating system needs to know whether it can read
> > > > registers from the device or not. Let's detail this specific design
> > > > possibility by bounding the spi-rx-bus-width property.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > index 0ccf0487fd8e..a25df7e1df88 100644
> > > > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > @@ -29,6 +29,10 @@ properties:
> > > >    spi-cpha: true
> > > >    spi-cpol: true
> > > >  
> > > > +  spi-rx-bus-width:
> > > > +    minimum: 0
> > > > +    maximum: 1
> > > > +    
> > > 
> > > It's not clear to me what the default would be?  
> > 
> > This binding references spi-peripheral-props.yaml which sets the
> > default to 1, I believe it is sane to keep it that way?  
> 
> I'm not sure.
> 
> The driver didn't need RX before, and we didn't have any property that
> was expressing whether we had MISO in the device tree.
> 
> That means we had both devices with and without MISO expressed in the
> same way, the driver handling both (by ignoring MISO entirely).
> 
> With this patch, you now introduce a property that specifies whether
> MISO is connected or not, and defaults to MISO being there. And a later
> patch will use MISO if it's available.
> 
> This means that, while it's working fine for devices that had MISO
> connected, devices that didn't are assumed to have it, and the driver
> makes use of it.

Ah yes, I get your concern. I thought you wanted to talk about the fact
that it was not constrained in the yaml description. Your concern is
about breaking existing devices.

Your concern is real, designs not wiring the MISO line which do not
describe this in the device tree will no longer succeed to probe with
the current implementation. Technically speaking, they're broken since
2021:

c476d430bfc0 ("dt-bindings: display: Add SPI peripheral schema to SPI based displays")

We actually discussed this with Sebastian, right there:
https://lore.kernel.org/all/20230609145951.853533-6-miquel.raynal@bootlin.com/T/#m9286cdb4d617c5efc29052b552e981ecfa2628e4

And the conclusion was that we decided not to care about the broken
descriptions (because, let's agree on this, not wiring the MISO line is
not a standard spi design). But I don't have a strong opinion TBH, so
if you think it's best to prevent these probes from failing (note that
I already added a debug line explicitly saying why the probe would
fail, "easy" to identify) I'm fine turning the check as a warning and
ignoring the error to avoid failing.

Thanks,
Miquèl
Maxime Ripard June 19, 2023, 12:31 p.m. UTC | #6
On Mon, Jun 19, 2023 at 12:19:58PM +0200, Miquel Raynal wrote:
> Hi Maxime,
> 
> maxime@cerno.tech wrote on Mon, 19 Jun 2023 11:39:56 +0200:
> 
> > On Sun, Jun 18, 2023 at 07:37:32PM +0200, Miquel Raynal wrote:
> > > Hello Maxime,
> > > 
> > > maxime@cerno.tech wrote on Sun, 18 Jun 2023 16:37:58 +0200:
> > >   
> > > > Hi,
> > > > 
> > > > On Fri, Jun 16, 2023 at 06:32:51PM +0200, Miquel Raynal wrote:  
> > > > > The ST7789V LCD controller supports regular SPI wiring, as well as no Rx
> > > > > data line at all. The operating system needs to know whether it can read
> > > > > registers from the device or not. Let's detail this specific design
> > > > > possibility by bounding the spi-rx-bus-width property.
> > > > > 
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml   | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > > index 0ccf0487fd8e..a25df7e1df88 100644
> > > > > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> > > > > @@ -29,6 +29,10 @@ properties:
> > > > >    spi-cpha: true
> > > > >    spi-cpol: true
> > > > >  
> > > > > +  spi-rx-bus-width:
> > > > > +    minimum: 0
> > > > > +    maximum: 1
> > > > > +    
> > > > 
> > > > It's not clear to me what the default would be?  
> > > 
> > > This binding references spi-peripheral-props.yaml which sets the
> > > default to 1, I believe it is sane to keep it that way?  
> > 
> > I'm not sure.
> > 
> > The driver didn't need RX before, and we didn't have any property that
> > was expressing whether we had MISO in the device tree.
> > 
> > That means we had both devices with and without MISO expressed in the
> > same way, the driver handling both (by ignoring MISO entirely).
> > 
> > With this patch, you now introduce a property that specifies whether
> > MISO is connected or not, and defaults to MISO being there. And a later
> > patch will use MISO if it's available.
> > 
> > This means that, while it's working fine for devices that had MISO
> > connected, devices that didn't are assumed to have it, and the driver
> > makes use of it.
> 
> Ah yes, I get your concern. I thought you wanted to talk about the fact
> that it was not constrained in the yaml description. Your concern is
> about breaking existing devices.
> 
> Your concern is real, designs not wiring the MISO line which do not
> describe this in the device tree will no longer succeed to probe with
> the current implementation. Technically speaking, they're broken since
> 2021:
> 
> c476d430bfc0 ("dt-bindings: display: Add SPI peripheral schema to SPI based displays")

I mean, I guess, but an old DT was still booting fine. Validation didn't
pass but that's a bit irrelevant for devices that shipped already.

> We actually discussed this with Sebastian, right there:
> https://lore.kernel.org/all/20230609145951.853533-6-miquel.raynal@bootlin.com/T/#m9286cdb4d617c5efc29052b552e981ecfa2628e4
> 
> And the conclusion was that we decided not to care about the broken
> descriptions (because, let's agree on this, not wiring the MISO line is
> not a standard spi design).

It might not be, but it's there. Just like the 9 bits per word :)

All the current in-tree users seem to only mux (at least, I haven't
checked the design) MOSI, and the devices I worked on with this driver
also didn't iirc.

> But I don't have a strong opinion TBH, so if you think it's best to
> prevent these probes from failing (note that I already added a debug
> line explicitly saying why the probe would fail, "easy" to identify)
> I'm fine turning the check as a warning and ignoring the error to
> avoid failing.

Updating the DT isn't always an option, so yeah, please make it backward
compatible.

Maxime
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index 0ccf0487fd8e..a25df7e1df88 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -29,6 +29,10 @@  properties:
   spi-cpha: true
   spi-cpol: true
 
+  spi-rx-bus-width:
+    minimum: 0
+    maximum: 1
+
 required:
   - compatible
   - reg