On Mon, Jun 19, 2023 at 12:19:58PM +0200, Miquel Raynal wrote:
> Hi Maxime,
>
> maxime@xxxxxxxxxx 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@xxxxxxxxxx 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@xxxxxxxxxxx>
> > > > > ---
> > > > > .../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@xxxxxxxxxxx/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
Attachment:
signature.asc
Description: PGP signature
