[PATCH v2 0/6] drm/panel: sitronix-st7789v: Support ET028013DMA panel

[PATCH v2 0/6] drm/panel: sitronix-st7789v: Support ET028013DMA panel

[Miquel Raynal]

Hello,

The aim of this series is to add support for the EDT ET028013DMA
panel. This panel features a Sitronix ST7789V2 LCD controller, which is
already supported mainline (or very close to the ST7789V for which
Maxime added support years ago).

The EDT panel is slightly different on the geometry and appears not to
support refresh rates higher than 30fps (above, glitches are visible,
despite the incoming signals being rather clean). While I was working on
this panel, I found quite inconvenient to not be able to read anything
back as it is a great tool for debugging purposes. So the last patch
actually adds a read helper and uses it to perform a sanity check at
probe time by verifying the Sitronix controller IDs.

This series applies on top of Sebastian's series which was also bringing
a number of good improvements to this driver. As Sebastian started and
contributed his work before me, I think his series is close to be merged
so I adapted my changes on top of it.

Link: https://lore.kernel.org/dri-devel/20230422205012.2464933-1-sre@kernel.org/

Thanks,
Miquèl

Changes in v2:
* Rebased on top of Sebastian's series and adapted all my changes to the
  existing infrastructure he has already added.
* Collected tags.
* Prevented the ID check to fail if there is no MISO line.
* Used dev_err_probe() when relevant.
* Sorted the IDs in the tables.
* Renamed the panel mode.
* Fixed typos.

Miquel Raynal (6):
  dt-bindings: display: st7789v: Add the edt,et028013dma panel
    compatible
  dt-bindings: display: st7789v: bound the number of Rx data lines
  drm/panel: sitronix-st7789v: Use 9 bits per spi word by default
  drm/panel: sitronix-st7789v: Clarify a definition
  drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support
  drm/panel: sitronix-st7789v: Check display ID

 .../display/panel/sitronix,st7789v.yaml       |   5 +
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 116 +++++++++++++++++-
 2 files changed, 118 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] dt-bindings: display: st7789v: Add the edt,et028013dma panel compatible

[Miquel Raynal]

The ST7789V LCD controller is also embedded in the ET028013DMA
panel. Add a compatible string to describe this other panel.

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

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index 7c5e4313db1d..0ccf0487fd8e 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -16,6 +16,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - edt,et028013dma
       - inanbo,t28cp45tn89-v17
       - sitronix,st7789v
 
-- 
2.34.1

Re: [PATCH v2 1/6] dt-bindings: display: st7789v: Add the edt,et028013dma panel compatible

[Krzysztof Kozlowski]

On 16/06/2023 18:32, Miquel Raynal wrote:
> The ST7789V LCD controller is also embedded in the ET028013DMA
> panel. Add a compatible string to describe this other panel.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/display/panel/sitronix,st7789v.yaml      | 1 +
>  1 file changed, 1 insertion(+)


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

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: display: st7789v: Add the edt,et028013dma panel compatible

[Maxime Ripard]

On Fri, 16 Jun 2023 18:32:50 +0200, Miquel Raynal wrote:
> The ST7789V LCD controller is also embedded in the ET028013DMA
> panel. Add a compatible string to describe this other panel.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

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

[Miquel Raynal]

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
+
 required:
   - compatible
   - reg
-- 
2.34.1

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

[Krzysztof Kozlowski]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Miquel Raynal]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Miquel Raynal]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4139 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 3/6] drm/panel: sitronix-st7789v: Use 9 bits per spi word by default

[Miquel Raynal]

The Sitronix controller expects 9-bit words, provide this as default at
probe time rather than specifying this in each and every access.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 172c6c1fc090..605b9f6d0f14 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -142,7 +142,6 @@ static int st7789v_spi_write(struct st7789v *ctx, enum st7789v_prefix prefix,
 	u16 txbuf = ((prefix & 1) << 8) | data;
 
 	xfer.tx_buf = &txbuf;
-	xfer.bits_per_word = 9;
 	xfer.len = sizeof(txbuf);
 
 	return spi_sync_transfer(ctx->spi, &xfer, 1);
@@ -436,6 +435,11 @@ static int st7789v_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, ctx);
 	ctx->spi = spi;
 
+	spi->bits_per_word = 9;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret, "Failed to setup spi\n");
+
 	ctx->info = device_get_match_data(&spi->dev);
 
 	drm_panel_init(&ctx->panel, dev, &st7789v_drm_funcs,
-- 
2.34.1

Re: [PATCH v2 3/6] drm/panel: sitronix-st7789v: Use 9 bits per spi word by default

[Maxime Ripard]

On Fri, 16 Jun 2023 18:32:52 +0200, Miquel Raynal wrote:
> The Sitronix controller expects 9-bit words, provide this as default at
> probe time rather than specifying this in each and every access.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v2 4/6] drm/panel: sitronix-st7789v: Clarify a definition

[Miquel Raynal]

The Sitronix datasheet explains BIT(1) of the RGBCTRL register as the
DOTCLK/PCLK edge used to sample the data lines:

    “0” The data is input on the positive edge of DOTCLK
    “1” The data is input on the negative edge of DOTCLK

IOW, this bit implies a falling edge and not a high state. Correct the
definition to ease the comparison with the datasheet.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 605b9f6d0f14..d7c5b3ad1baa 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -27,7 +27,7 @@
 #define ST7789V_RGBCTRL_RCM(n)			(((n) & 3) << 5)
 #define ST7789V_RGBCTRL_VSYNC_HIGH		BIT(3)
 #define ST7789V_RGBCTRL_HSYNC_HIGH		BIT(2)
-#define ST7789V_RGBCTRL_PCLK_HIGH		BIT(1)
+#define ST7789V_RGBCTRL_PCLK_FALLING		BIT(1)
 #define ST7789V_RGBCTRL_DE_LOW			BIT(0)
 #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
 #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
@@ -259,7 +259,7 @@ static int st7789v_prepare(struct drm_panel *panel)
 	if (ctx->info->mode->flags & DRM_MODE_FLAG_PHSYNC)
 		polarity |= ST7789V_RGBCTRL_HSYNC_HIGH;
 	if (ctx->info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
-		polarity |= ST7789V_RGBCTRL_PCLK_HIGH;
+		polarity |= ST7789V_RGBCTRL_PCLK_FALLING;
 	if (ctx->info->bus_flags & DRM_BUS_FLAG_DE_LOW)
 		polarity |= ST7789V_RGBCTRL_DE_LOW;
 
-- 
2.34.1

Re: [PATCH v2 4/6] drm/panel: sitronix-st7789v: Clarify a definition

[Maxime Ripard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

On Fri, 16 Jun 2023 18:32:53 +0200, Miquel Raynal wrote:
> The Sitronix datasheet explains BIT(1) of the RGBCTRL register as the
> DOTCLK/PCLK edge used to sample the data lines:
> 
>     “0” The data is input on the positive edge of DOTCLK
>     “1” The data is input on the negative edge of DOTCLK
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v2 5/6] drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support

[Miquel Raynal]

This panel from Emerging Display Technologies Corporation features an
ST7789V2 LCD controller panel inside which is almost identical to what
the Sitronix panel driver supports.

In practice, the module physical size is specific, and experiments show
that the display will malfunction if any of the following situation
occurs:
* Pixel clock is above 3MHz
* Pixel clock is not inverted
I could not properly identify the reasons behind these failures, scope
captures show valid input signals.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index d7c5b3ad1baa..8649966ceae8 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -187,6 +187,21 @@ static const struct drm_display_mode t28cp45tn89_mode = {
 	.flags = DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC,
 };
 
+static const struct drm_display_mode et028013dma_mode = {
+	.clock = 3000,
+	.hdisplay = 240,
+	.hsync_start = 240 + 38,
+	.hsync_end = 240 + 38 + 10,
+	.htotal = 240 + 38 + 10 + 10,
+	.vdisplay = 320,
+	.vsync_start = 320 + 8,
+	.vsync_end = 320 + 8 + 4,
+	.vtotal = 320 + 8 + 4 + 4,
+	.width_mm = 43,
+	.height_mm = 58,
+	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+};
+
 struct st7789_panel_info default_panel = {
 	.mode = &default_mode,
 	.invert_mode = true,
@@ -203,6 +218,14 @@ struct st7789_panel_info t28cp45tn89_panel = {
 		     DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
 };
 
+struct st7789_panel_info et028013dma_panel = {
+	.mode = &et028013dma_mode,
+	.invert_mode = true,
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH |
+		     DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+};
+
 static int st7789v_get_modes(struct drm_panel *panel,
 			     struct drm_connector *connector)
 {
@@ -474,6 +497,7 @@ static void st7789v_remove(struct spi_device *spi)
 static const struct spi_device_id st7789v_spi_id[] = {
 	{ "st7789v", (unsigned long) &default_panel },
 	{ "t28cp45tn89-v17", (unsigned long) &t28cp45tn89_panel },
+	{ "et028013dma", (unsigned long) &et028013dma_panel },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
@@ -481,6 +505,7 @@ MODULE_DEVICE_TABLE(spi, st7789v_spi_id);
 static const struct of_device_id st7789v_of_match[] = {
 	{ .compatible = "sitronix,st7789v", .data = &default_panel },
 	{ .compatible = "inanbo,t28cp45tn89-v17", .data = &t28cp45tn89_panel },
+	{ .compatible = "edt,et028013dma", .data = &et028013dma_panel },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
-- 
2.34.1

Re: [PATCH v2 5/6] drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support

[Maxime Ripard]

On Fri, 16 Jun 2023 18:32:54 +0200, Miquel Raynal wrote:
> This panel from Emerging Display Technologies Corporation features an
> ST7789V2 LCD controller panel inside which is almost identical to what
> the Sitronix panel driver supports.
> 
> In practice, the module physical size is specific, and experiments show
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v2 6/6] drm/panel: sitronix-st7789v: Check display ID

[Miquel Raynal]

A very basic debugging rule when a device is connected for the first
time is to access a read-only register which contains known data in
order to ensure the communication protocol is properly working. This
driver lacked any read helper which is often a critical piece for
speed-up bring-ups.

Add a read helper and use it to verify the communication with the panel
is working as soon as possible in order to fail early if this is not the
case.

As this panel may work with no MISO line, the check is discarded in this
case.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 8649966ceae8..36fb3f2453f1 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -110,6 +110,9 @@
 			return val;		\
 	} while (0)
 
+#define ST7789V_IDS { 0x85, 0x85, 0x52 }
+#define ST7789V_IDS_SIZE 3
+
 struct st7789_panel_info {
 	const struct drm_display_mode *mode;
 	u32 bus_format;
@@ -157,6 +160,80 @@ static int st7789v_write_data(struct st7789v *ctx, u8 cmd)
 	return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
 }
 
+static int st7789v_read_data(struct st7789v *ctx, u8 cmd, u8 *buf,
+			     unsigned int len)
+{
+	struct spi_transfer xfer[2] = { };
+	struct spi_message msg;
+	u16 txbuf = ((ST7789V_COMMAND & 1) << 8) | cmd;
+	u16 rxbuf[4] = {};
+	u8 bit9 = 0;
+	int ret, i;
+
+	switch (len) {
+	case 1:
+	case 3:
+	case 4:
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	spi_message_init(&msg);
+
+	xfer[0].tx_buf = &txbuf;
+	xfer[0].len = sizeof(txbuf);
+	spi_message_add_tail(&xfer[0], &msg);
+
+	xfer[1].rx_buf = rxbuf;
+	xfer[1].len = len * 2;
+	spi_message_add_tail(&xfer[1], &msg);
+
+	ret = spi_sync(ctx->spi, &msg);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = rxbuf[i] >> i | (bit9 << (9 - i));
+		if (i)
+			bit9 = rxbuf[i] & GENMASK(i - 1, 0);
+	}
+
+	return 0;
+}
+
+static int st7789v_check_id(struct drm_panel *panel)
+{
+	const u8 st7789v_ids[ST7789V_IDS_SIZE] = ST7789V_IDS;
+	struct st7789v *ctx = panel_to_st7789v(panel);
+	bool invalid_ids = false;
+	int ret, i;
+	u8 ids[3];
+
+	if (ctx->spi->mode & SPI_NO_RX)
+		return 0;
+
+	ret = st7789v_read_data(ctx, MIPI_DCS_GET_DISPLAY_ID, ids, ST7789V_IDS_SIZE);
+	if (ret) {
+		dev_err(panel->dev, "Failed to read IDs\n");
+		return ret;
+	}
+
+	for (i = 0; i < ST7789V_IDS_SIZE; i++) {
+		if (ids[i] != st7789v_ids[i]) {
+			invalid_ids = true;
+			break;
+		}
+	}
+
+	if (invalid_ids) {
+		dev_err(panel->dev, "Unrecognized panel IDs");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static const struct drm_display_mode default_mode = {
 	.clock = 7000,
 	.hdisplay = 240,
@@ -295,6 +372,10 @@ static int st7789v_prepare(struct drm_panel *panel)
 	gpiod_set_value(ctx->reset, 0);
 	msleep(120);
 
+	ret = st7789v_check_id(panel);
+	if (ret)
+		return ret;
+
 	ST7789V_TEST(ret, st7789v_write_command(ctx, MIPI_DCS_EXIT_SLEEP_MODE));
 
 	/* We need to wait 120ms after a sleep out command */
-- 
2.34.1

Re: [PATCH v2 6/6] drm/panel: sitronix-st7789v: Check display ID

[Maxime Ripard]

On Fri, 16 Jun 2023 18:32:55 +0200, Miquel Raynal wrote:
> A very basic debugging rule when a device is connected for the first
> time is to access a read-only register which contains known data in
> order to ensure the communication protocol is properly working. This
> driver lacked any read helper which is often a critical piece for
> speed-up bring-ups.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime