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

[Maxime Ripard]

On Mon, Jun 19, 2023 at 12:19:58PM +0200, Miquel Raynal wrote:
> Hi Maxime,
> 
> maxime at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230619/ac32b570/attachment-0001.sig>