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

[PATCH 0/7] 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. If deemed
irrelevant, this patch may be discarded.

Thanks,
Miquèl

Miquel Raynal (7):
  drm/panel: sitronix-st7789v: Prevent core spi warnings
  drm/panel: sitronix-st7789v: Use 9 bits per spi word by default
  drm/panel: sitronix-st7789v: Specify the expected bus format
  drm/panel: sitronix-st7789v: Use platform data
  dt-bindings: display: st7789v: Add the edt,et028013dma panel
    compatible
  drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support
  drm/panel: sitronix-st7789v: Check display ID

 .../display/panel/sitronix,st7789v.yaml       |   7 +-
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 157 +++++++++++++++++-
 2 files changed, 156 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Miquel Raynal]

The spi core warns us about using an of_device_id table without a
spi_device_id table aside for module utilities in orter to not rely on
OF modaliases. Just add this table using the device name without the
vendor prefix (as it is usually done).

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

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bbc4569cbcdc..c67b9adb157f 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -400,9 +400,16 @@ static const struct of_device_id st7789v_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
 
+static const struct spi_device_id st7789v_ids[] = {
+	{ "st7789v", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, st7789v_ids);
+
 static struct spi_driver st7789v_driver = {
 	.probe = st7789v_probe,
 	.remove = st7789v_remove,
+	.id_table = st7789v_ids,
 	.driver = {
 		.name = "st7789v",
 		.of_match_table = st7789v_of_match,
-- 
2.34.1

[PATCH 2/7] 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>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index c67b9adb157f..e9ca7ebb458a 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -138,7 +138,6 @@ static int st7789v_spi_write(struct st7789v *ctx, enum st7789v_prefix prefix,
 	spi_message_init(&msg);
 
 	xfer.tx_buf = &txbuf;
-	xfer.bits_per_word = 9;
 	xfer.len = sizeof(txbuf);
 
 	spi_message_add_tail(&xfer, &msg);
@@ -365,6 +364,13 @@ 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) {
+		dev_err(&spi->dev, "spi_setup failed: %d\n", ret);
+		return ret;
+	}
+
 	drm_panel_init(&ctx->panel, &spi->dev, &st7789v_drm_funcs,
 		       DRM_MODE_CONNECTOR_DPI);
 
-- 
2.34.1

[PATCH 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format

[Miquel Raynal]

The LCD controller supports RGB444, RGB565 and RGB888. The value that is
written in the COLMOD register indicates using RGB888, so let's clearly
specify the in-use bus format.

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

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index e9ca7ebb458a..0abb45bea18d 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/media-bus-format.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
@@ -170,6 +171,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
 			     struct drm_connector *connector)
 {
 	struct drm_display_mode *mode;
+	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
 	mode = drm_mode_duplicate(connector->dev, &default_mode);
 	if (!mode) {
@@ -186,6 +188,8 @@ static int st7789v_get_modes(struct drm_panel *panel,
 
 	connector->display_info.width_mm = 61;
 	connector->display_info.height_mm = 103;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
 
 	return 1;
 }
-- 
2.34.1

[PATCH 4/7] drm/panel: sitronix-st7789v: Use platform data

[Miquel Raynal]

The Sitronix ST7789V LCD controller is actually packaged in a number of
different panels with slightly different properties. Before introducing
the support for another pannel using this same LCD controller, let's
move all the panel-specific information into a dedicated structure that
is available as platform data.

There is no functional change.

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

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 0abb45bea18d..212bccc31804 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -112,11 +112,19 @@
 			return val;		\
 	} while (0)
 
+struct st7789v_panel_info {
+	const struct drm_display_mode *display_mode;
+	u16 width_mm;
+	u16 height_mm;
+	u32 bus_format;
+};
+
 struct st7789v {
 	struct drm_panel panel;
 	struct spi_device *spi;
 	struct gpio_desc *reset;
 	struct regulator *power;
+	const struct st7789v_panel_info *panel_info;
 };
 
 enum st7789v_prefix {
@@ -170,10 +178,11 @@ static const struct drm_display_mode default_mode = {
 static int st7789v_get_modes(struct drm_panel *panel,
 			     struct drm_connector *connector)
 {
+	struct st7789v *ctx = panel_to_st7789v(panel);
+	const struct st7789v_panel_info *panel_info = ctx->panel_info;
 	struct drm_display_mode *mode;
-	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
-	mode = drm_mode_duplicate(connector->dev, &default_mode);
+	mode = drm_mode_duplicate(connector->dev, panel_info->display_mode);
 	if (!mode) {
 		dev_err(panel->dev, "failed to add mode %ux%ux@%u\n",
 			default_mode.hdisplay, default_mode.vdisplay,
@@ -186,10 +195,10 @@ static int st7789v_get_modes(struct drm_panel *panel,
 	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
 	drm_mode_probed_add(connector, mode);
 
-	connector->display_info.width_mm = 61;
-	connector->display_info.height_mm = 103;
+	connector->display_info.width_mm = panel_info->width_mm;
+	connector->display_info.height_mm = panel_info->height_mm;
 	drm_display_info_set_bus_formats(&connector->display_info,
-					 &bus_format, 1);
+					 &panel_info->bus_format, 1);
 
 	return 1;
 }
@@ -365,6 +374,8 @@ static int st7789v_probe(struct spi_device *spi)
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->panel_info = device_get_match_data(&spi->dev);
+
 	spi_set_drvdata(spi, ctx);
 	ctx->spi = spi;
 
@@ -404,8 +415,15 @@ static void st7789v_remove(struct spi_device *spi)
 	drm_panel_remove(&ctx->panel);
 }
 
+static const struct st7789v_panel_info st7789v_info = {
+	.display_mode = &default_mode,
+	.width_mm = 64,
+	.height_mm = 103,
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct of_device_id st7789v_of_match[] = {
-	{ .compatible = "sitronix,st7789v" },
+	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
-- 
2.34.1

[PATCH 5/7] dt-bindings: display: st7789v: Add the edt, et028013dma panel compatible

[Miquel Raynal]

The ST7789V LCD controller is also embedded in the ET028013DMA panel. In
fact, "sitronix,st7789v" might not be totally relevant alone as most of
the time -if not all- the LCD controller will always be packaged into a
display with its own physical properties.

Let's keep "sitronix,st7789v" valid alone for backward compatibility,
but we should definitely provide two compatibles to fully describe such
panel, so let's expect to have both when describing a panel such as the
EDT ET028013DMA.

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

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index d984b59daa4a..d4c8af9a973d 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -15,7 +15,12 @@ allOf:
 
 properties:
   compatible:
-    const: sitronix,st7789v
+    oneOf:
+      - items:
+          - enum:
+              - edt,et028013dma
+          - const: sitronix,st7789v
+      - const: sitronix,st7789v
 
   reg: true
   reset-gpios: true
-- 
2.34.1

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

[Miquel Raynal]

This panel from Emerging Display Technologies Corporation features an
ST7789V2 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    | 34 +++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 212bccc31804..7de192a3a8aa 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -30,7 +30,8 @@
 #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_PCLK_RISING		0
 #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
 #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
 
@@ -117,6 +118,7 @@ struct st7789v_panel_info {
 	u16 width_mm;
 	u16 height_mm;
 	u32 bus_format;
+	u32 bus_flags;
 };
 
 struct st7789v {
@@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
 	.vtotal = 320 + 8 + 4 + 4,
 };
 
+static const struct drm_display_mode slow_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,
+};
+
 static int st7789v_get_modes(struct drm_panel *panel,
 			     struct drm_connector *connector)
 {
@@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
 
 	connector->display_info.width_mm = panel_info->width_mm;
 	connector->display_info.height_mm = panel_info->height_mm;
+	connector->display_info.bus_flags = panel_info->bus_flags;
 	drm_display_info_set_bus_formats(&connector->display_info,
 					 &panel_info->bus_format, 1);
 
@@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
 static int st7789v_prepare(struct drm_panel *panel)
 {
 	struct st7789v *ctx = panel_to_st7789v(panel);
+	const struct st7789v_panel_info *panel_info = ctx->panel_info;
+	u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
 	int ret;
 
+	if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
+		pck = ST7789V_RGBCTRL_PCLK_RISING;
+
 	ret = regulator_enable(ctx->power);
 	if (ret)
 		return ret;
@@ -321,7 +341,7 @@ static int st7789v_prepare(struct drm_panel *panel)
 					     ST7789V_RGBCTRL_RCM(2) |
 					     ST7789V_RGBCTRL_VSYNC_HIGH |
 					     ST7789V_RGBCTRL_HSYNC_HIGH |
-					     ST7789V_RGBCTRL_PCLK_HIGH));
+					     pck));
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20)));
 
@@ -422,14 +442,24 @@ static const struct st7789v_panel_info st7789v_info = {
 	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
 };
 
+static const struct st7789v_panel_info et028013dma_info = {
+	.display_mode = &slow_mode,
+	.width_mm = 43,
+	.height_mm = 58,
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+};
+
 static const struct of_device_id st7789v_of_match[] = {
 	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
+	{ .compatible = "edt,et028013dma", .data = &et028013dma_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
 
 static const struct spi_device_id st7789v_ids[] = {
 	{ "st7789v", },
+	{ "et028013dma", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, st7789v_ids);
-- 
2.34.1

[PATCH 7/7] 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 peace for
fastening 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.

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

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 7de192a3a8aa..fb21d52a7a63 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -113,6 +113,9 @@
 			return val;		\
 	} while (0)
 
+#define ST7789V_IDS { 0x85, 0x85, 0x52 }
+#define ST7789V_IDS_SIZE 3
+
 struct st7789v_panel_info {
 	const struct drm_display_mode *display_mode;
 	u16 width_mm;
@@ -165,6 +168,77 @@ 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];
+
+	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,
@@ -237,6 +311,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 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Sam Ravnborg]

On Fri, Jun 09, 2023 at 04:59:45PM +0200, Miquel Raynal wrote:
> The spi core warns us about using an of_device_id table without a
> spi_device_id table aside for module utilities in orter to not rely on
> OF modaliases. Just add this table using the device name without the
> vendor prefix (as it is usually done).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index bbc4569cbcdc..c67b9adb157f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -400,9 +400,16 @@ static const struct of_device_id st7789v_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, st7789v_of_match);
>  
> +static const struct spi_device_id st7789v_ids[] = {
> +	{ "st7789v", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> +
>  static struct spi_driver st7789v_driver = {
>  	.probe = st7789v_probe,
>  	.remove = st7789v_remove,
> +	.id_table = st7789v_ids,
>  	.driver = {
>  		.name = "st7789v",
>  		.of_match_table = st7789v_of_match,
> -- 
> 2.34.1

Re: [PATCH 2/7] drm/panel: sitronix-st7789v: Use 9 bits per spi word by default

[Sam Ravnborg]

On Fri, Jun 09, 2023 at 04:59:46PM +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>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index c67b9adb157f..e9ca7ebb458a 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -138,7 +138,6 @@ static int st7789v_spi_write(struct st7789v *ctx, enum st7789v_prefix prefix,
>  	spi_message_init(&msg);
>  
>  	xfer.tx_buf = &txbuf;
> -	xfer.bits_per_word = 9;
>  	xfer.len = sizeof(txbuf);
>  
>  	spi_message_add_tail(&xfer, &msg);
> @@ -365,6 +364,13 @@ 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) {
> +		dev_err(&spi->dev, "spi_setup failed: %d\n", ret);
> +		return ret;
> +	}

Use dev_err_probe().
With this change:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> +
>  	drm_panel_init(&ctx->panel, &spi->dev, &st7789v_drm_funcs,
>  		       DRM_MODE_CONNECTOR_DPI);
>  
> -- 
> 2.34.1

Re: [PATCH 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format

[Sam Ravnborg]

On Fri, Jun 09, 2023 at 04:59:47PM +0200, Miquel Raynal wrote:
> The LCD controller supports RGB444, RGB565 and RGB888. The value that is
> written in the COLMOD register indicates using RGB888, so let's clearly
> specify the in-use bus format.

Confused.
MEDIA_BUS_FMT_RGB666_1X18 assumes 6 bits per color.
But RGB888 is 8 bits per color.

Something that I have forgotten, or is this inconsistent?

	Sam

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index e9ca7ebb458a..0abb45bea18d 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/media-bus-format.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  
> @@ -170,6 +171,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  			     struct drm_connector *connector)
>  {
>  	struct drm_display_mode *mode;
> +	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>  
>  	mode = drm_mode_duplicate(connector->dev, &default_mode);
>  	if (!mode) {
> @@ -186,6 +188,8 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  
>  	connector->display_info.width_mm = 61;
>  	connector->display_info.height_mm = 103;
> +	drm_display_info_set_bus_formats(&connector->display_info,
> +					 &bus_format, 1);
>  
>  	return 1;
>  }
> -- 
> 2.34.1

Re: [PATCH 4/7] drm/panel: sitronix-st7789v: Use platform data

[Sam Ravnborg]

On Fri, Jun 09, 2023 at 04:59:48PM +0200, Miquel Raynal wrote:
> The Sitronix ST7789V LCD controller is actually packaged in a number of
> different panels with slightly different properties. Before introducing
> the support for another pannel using this same LCD controller, let's
> move all the panel-specific information into a dedicated structure that
> is available as platform data.
> 
> There is no functional change.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

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

[Sam Ravnborg]

On Fri, Jun 09, 2023 at 04:59:50PM +0200, Miquel Raynal wrote:
> This panel from Emerging Display Technologies Corporation features an
> ST7789V2 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.

The patch includes an additional change where the bus_flags are assigned
to the connector. At least tell why, or maybe split it out.
Other panels could learn to do the same - and I assume you did it
because it was required in the atmel_hlcd driver.

	Sam

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 212bccc31804..7de192a3a8aa 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -30,7 +30,8 @@
>  #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_PCLK_RISING		0
>  #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
>  #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
>  
> @@ -117,6 +118,7 @@ struct st7789v_panel_info {
>  	u16 width_mm;
>  	u16 height_mm;
>  	u32 bus_format;
> +	u32 bus_flags;
>  };
>  
>  struct st7789v {
> @@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
>  	.vtotal = 320 + 8 + 4 + 4,
>  };
>  
> +static const struct drm_display_mode slow_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,
> +};
> +
>  static int st7789v_get_modes(struct drm_panel *panel,
>  			     struct drm_connector *connector)
>  {
> @@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  
>  	connector->display_info.width_mm = panel_info->width_mm;
>  	connector->display_info.height_mm = panel_info->height_mm;
> +	connector->display_info.bus_flags = panel_info->bus_flags;
>  	drm_display_info_set_bus_formats(&connector->display_info,
>  					 &panel_info->bus_format, 1);
>  
> @@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  static int st7789v_prepare(struct drm_panel *panel)
>  {
>  	struct st7789v *ctx = panel_to_st7789v(panel);
> +	const struct st7789v_panel_info *panel_info = ctx->panel_info;
> +	u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
>  	int ret;
>  
> +	if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
> +		pck = ST7789V_RGBCTRL_PCLK_RISING;
> +
>  	ret = regulator_enable(ctx->power);
>  	if (ret)
>  		return ret;
> @@ -321,7 +341,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>  					     ST7789V_RGBCTRL_RCM(2) |
>  					     ST7789V_RGBCTRL_VSYNC_HIGH |
>  					     ST7789V_RGBCTRL_HSYNC_HIGH |
> -					     ST7789V_RGBCTRL_PCLK_HIGH));
> +					     pck));
>  	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>  	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20)));
>  
> @@ -422,14 +442,24 @@ static const struct st7789v_panel_info st7789v_info = {
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct st7789v_panel_info et028013dma_info = {
> +	.display_mode = &slow_mode,
> +	.width_mm = 43,
> +	.height_mm = 58,
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> +};
> +
>  static const struct of_device_id st7789v_of_match[] = {
>  	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
> +	{ .compatible = "edt,et028013dma", .data = &et028013dma_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, st7789v_of_match);
>  
>  static const struct spi_device_id st7789v_ids[] = {
>  	{ "st7789v", },
> +	{ "et028013dma", },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(spi, st7789v_ids);
> -- 
> 2.34.1

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

[Sam Ravnborg]

Hi Miquel,

On Fri, Jun 09, 2023 at 04:59:51PM +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 peace for
> fastening 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.

The read helper seems like a log of general boiler plate code.
I briefly looked into the use of regmap for the spi communication,
but it did not look like it supported the bit9 stuff.

I did not stare enough to add a reviewd by, but the approach is fine
and it is good to detech issues early.

Acked-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 7de192a3a8aa..fb21d52a7a63 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -113,6 +113,9 @@
>  			return val;		\
>  	} while (0)
>  
> +#define ST7789V_IDS { 0x85, 0x85, 0x52 }
> +#define ST7789V_IDS_SIZE 3
> +
>  struct st7789v_panel_info {
>  	const struct drm_display_mode *display_mode;
>  	u16 width_mm;
> @@ -165,6 +168,77 @@ 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];
> +
> +	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,
> @@ -237,6 +311,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 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format

[Maxime Ripard]

Hi,

On Fri, Jun 09, 2023 at 04:59:47PM +0200, Miquel Raynal wrote:
> The LCD controller supports RGB444, RGB565 and RGB888. The value that is
> written in the COLMOD register indicates using RGB888, so let's clearly
> specify the in-use bus format.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index e9ca7ebb458a..0abb45bea18d 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/media-bus-format.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  
> @@ -170,6 +171,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  			     struct drm_connector *connector)
>  {
>  	struct drm_display_mode *mode;
> +	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;

I'm not sure it should be set in stone in the driver.

This is a property of the panel/controller, but also one of the RGB
controller upstream that will and how the board has been wired.

It's not unusual for boards with a limited number of GPIOs to take a
RGB888 panel and connect only as a RGB565 with the lowest bits to the
ground for example.

So I think this should come from the device tree, ideally from the media
graph endpoint.

Maxime

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

[Maxime Ripard]

On Fri, Jun 09, 2023 at 04:59:50PM +0200, Miquel Raynal wrote:
> This panel from Emerging Display Technologies Corporation features an
> ST7789V2 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    | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 212bccc31804..7de192a3a8aa 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -30,7 +30,8 @@
>  #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_PCLK_RISING		0
>  #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
>  #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
>  
> @@ -117,6 +118,7 @@ struct st7789v_panel_info {
>  	u16 width_mm;
>  	u16 height_mm;
>  	u32 bus_format;
> +	u32 bus_flags;
>  };
>  
>  struct st7789v {
> @@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
>  	.vtotal = 320 + 8 + 4 + 4,
>  };
>  
> +static const struct drm_display_mode slow_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,
> +};

Why is it supposed to be slow (and compared to what)? It looks like a
fairly normal mode to me? Or is it because it's refreshed at 30Hz?

Either way, the ST7789V is a panel controller and can be connected to a
wide range of panels depending on the model, so it would be better to
just use the model name there.

>  static int st7789v_get_modes(struct drm_panel *panel,
>  			     struct drm_connector *connector)
>  {
> @@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  
>  	connector->display_info.width_mm = panel_info->width_mm;
>  	connector->display_info.height_mm = panel_info->height_mm;
> +	connector->display_info.bus_flags = panel_info->bus_flags;
>  	drm_display_info_set_bus_formats(&connector->display_info,
>  					 &panel_info->bus_format, 1);
>  
> @@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
>  static int st7789v_prepare(struct drm_panel *panel)
>  {
>  	struct st7789v *ctx = panel_to_st7789v(panel);
> +	const struct st7789v_panel_info *panel_info = ctx->panel_info;
> +	u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
>  	int ret;
>  
> +	if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
> +		pck = ST7789V_RGBCTRL_PCLK_RISING;
> +

I guess it could be in a separate commit

Maxime

>  	ret = regulator_enable(ctx->power);
>  	if (ret)
>  		return ret;
> @@ -321,7 +341,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>  					     ST7789V_RGBCTRL_RCM(2) |
>  					     ST7789V_RGBCTRL_VSYNC_HIGH |
>  					     ST7789V_RGBCTRL_HSYNC_HIGH |
> -					     ST7789V_RGBCTRL_PCLK_HIGH));
> +					     pck));
>  	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>  	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20)));
>  
> @@ -422,14 +442,24 @@ static const struct st7789v_panel_info st7789v_info = {
>  	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct st7789v_panel_info et028013dma_info = {
> +	.display_mode = &slow_mode,
> +	.width_mm = 43,
> +	.height_mm = 58,
> +	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> +};
> +
>  static const struct of_device_id st7789v_of_match[] = {
>  	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
> +	{ .compatible = "edt,et028013dma", .data = &et028013dma_info },

We should sort them by alphabetical order

Maxime

Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Michael Riesch]

Hi Miquel,

On 6/9/23 16:59, Miquel Raynal wrote:
> The spi core warns us about using an of_device_id table without a

s/spi/SPI ?

> spi_device_id table aside for module utilities in orter to not rely on

s/in orter to/in order to ?

> OF modaliases. Just add this table using the device name without the
> vendor prefix (as it is usually done).
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index bbc4569cbcdc..c67b9adb157f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -400,9 +400,16 @@ static const struct of_device_id
st7789v_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, st7789v_of_match);
>
> +static const struct spi_device_id st7789v_ids[] = {
> +	{ "st7789v", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> +
>  static struct spi_driver st7789v_driver = {
>  	.probe = st7789v_probe,
>  	.remove = st7789v_remove,
> +	.id_table = st7789v_ids,
>  	.driver = {
>  		.name = "st7789v",
>  		.of_match_table = st7789v_of_match,

May I point to you Sebastian Reichel's series that features a partial
overlap with your work? [0]

For instance, the patch at hand is comparable to [1].

Cc: Sebastian to keep him in the loop.

Best regards,
Michael

[0]
https://lore.kernel.org/dri-devel/20230422205012.2464933-1-sre@kernel.org/
[1]
https://lore.kernel.org/dri-devel/20230422205012.2464933-4-sre@kernel.org/

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

[Michael Riesch]

Hi Miquel,

On 6/9/23 16:59, 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 peace for
> fastening bring-ups.

I am afraid I don't get the last sentence. s/peace/piece? s/for
fastening/to speed up ? Only guessing here.

Best regards,
Michael

> 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.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> index 7de192a3a8aa..fb21d52a7a63 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> @@ -113,6 +113,9 @@
>  			return val;		\
>  	} while (0)
>  
> +#define ST7789V_IDS { 0x85, 0x85, 0x52 }
> +#define ST7789V_IDS_SIZE 3
> +
>  struct st7789v_panel_info {
>  	const struct drm_display_mode *display_mode;
>  	u16 width_mm;
> @@ -165,6 +168,77 @@ 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];
> +
> +	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,
> @@ -237,6 +311,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 */

Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Miquel Raynal]

Hi Michael,

michael.riesch@wolfvision.net wrote on Tue, 13 Jun 2023 08:15:26 +0200:

> Hi Miquel,
> 
> On 6/9/23 16:59, Miquel Raynal wrote:
> > The spi core warns us about using an of_device_id table without a  
> 
> s/spi/SPI ?

Actually both are accepted in the kernel, IIRC it depends whether you
refer to the name of the bus or the Linux subsystem. Same for picking
"a" vs "an" before "spi/SPI". An attempt to use a unique formatting was
actually canceled because both are used equivalently and I believe even
the spi maintainer and the spi-nor maintainer kind of disagreed on the
default :)

> > spi_device_id table aside for module utilities in orter to not rely on  
> 
> s/in orter to/in order to ?

Yes, sorry for this typo as well as the two others you rightly pointed
out in another patch. I'll fix them.

> > OF modaliases. Just add this table using the device name without the
> > vendor prefix (as it is usually done).
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c  
> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > index bbc4569cbcdc..c67b9adb157f 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > @@ -400,9 +400,16 @@ static const struct of_device_id  
> st7789v_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, st7789v_of_match);
> >
> > +static const struct spi_device_id st7789v_ids[] = {
> > +	{ "st7789v", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> > +
> >  static struct spi_driver st7789v_driver = {
> >  	.probe = st7789v_probe,
> >  	.remove = st7789v_remove,
> > +	.id_table = st7789v_ids,
> >  	.driver = {
> >  		.name = "st7789v",
> >  		.of_match_table = st7789v_of_match,  
> 
> May I point to you Sebastian Reichel's series that features a partial
> overlap with your work? [0]

Woow. That driver has been untouched for years and now two
contributions at the same time. Sebastian, what is the current state of
your series? Shall I base my work on top of yours? Or is it still too
premature and we shall instead try to merge both and contribute a new
version of the series bringing support for the two panels?

> For instance, the patch at hand is comparable to [1].
> 
> Cc: Sebastian to keep him in the loop.

Thanks a lot for pointing this out.

> Best regards,
> Michael
> 
> [0]
> https://lore.kernel.org/dri-devel/20230422205012.2464933-1-sre@kernel.org/
> [1]
> https://lore.kernel.org/dri-devel/20230422205012.2464933-4-sre@kernel.org/


Thanks,
Miquèl

Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Sebastian Reichel]

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

Hi,

On Tue, Jun 13, 2023 at 08:56:30AM +0200, Miquel Raynal wrote:
> Hi Michael,
> 
> michael.riesch@wolfvision.net wrote on Tue, 13 Jun 2023 08:15:26 +0200:
> 
> > Hi Miquel,
> > 
> > On 6/9/23 16:59, Miquel Raynal wrote:
> > > The spi core warns us about using an of_device_id table without a  
> > 
> > s/spi/SPI ?
> 
> Actually both are accepted in the kernel, IIRC it depends whether you
> refer to the name of the bus or the Linux subsystem. Same for picking
> "a" vs "an" before "spi/SPI". An attempt to use a unique formatting was
> actually canceled because both are used equivalently and I believe even
> the spi maintainer and the spi-nor maintainer kind of disagreed on the
> default :)
> 
> > > spi_device_id table aside for module utilities in orter to not rely on  
> > 
> > s/in orter to/in order to ?
> 
> Yes, sorry for this typo as well as the two others you rightly pointed
> out in another patch. I'll fix them.
> 
> > > OF modaliases. Just add this table using the device name without the
> > > vendor prefix (as it is usually done).
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c  
> > b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > index bbc4569cbcdc..c67b9adb157f 100644
> > > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > @@ -400,9 +400,16 @@ static const struct of_device_id  
> > st7789v_of_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(of, st7789v_of_match);
> > >
> > > +static const struct spi_device_id st7789v_ids[] = {
> > > +	{ "st7789v", },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> > > +
> > >  static struct spi_driver st7789v_driver = {
> > >  	.probe = st7789v_probe,
> > >  	.remove = st7789v_remove,
> > > +	.id_table = st7789v_ids,
> > >  	.driver = {
> > >  		.name = "st7789v",
> > >  		.of_match_table = st7789v_of_match,  
> > 
> > May I point to you Sebastian Reichel's series that features a partial
> > overlap with your work? [0]
> 
> Woow. That driver has been untouched for years and now two
> contributions at the same time.

Three actually. Michael also submitted a series :)

> Sebastian, what is the current state of your series?

The DT changes got Ack'd by Rob and I have the R-B from Michael
(minus a minor comment to make the panel struct 'static const').
It's mainly waiting for a review from Sam.

I was a bit distracted by a boot regression on the devices and
some other projects. The boot regression got solved, so I can
prepare a new version if that makes things easier.

> Shall I base my work on top of yours? Or is it still too
> premature and we shall instead try to merge both and contribute a new
> version of the series bringing support for the two panels?

I suppose whatever is easier for Sam to review.

-- Sebastian

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

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

[Sebastian Reichel]

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

Hi,

On Sat, Jun 10, 2023 at 10:45:25PM +0200, Sam Ravnborg wrote:
> Hi Miquel,
> 
> On Fri, Jun 09, 2023 at 04:59:51PM +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 peace for
> > fastening 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.
> 
> The read helper seems like a log of general boiler plate code.
> I briefly looked into the use of regmap for the spi communication,
> but it did not look like it supported the bit9 stuff.
> 
> I did not stare enough to add a reviewd by, but the approach is fine
> and it is good to detech issues early.

The st7789v datasheet describes a setup where SPI is connected
unidirectional (i.e. without MISO). In that case the ID check
will fail.

-- Sebastian

> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > index 7de192a3a8aa..fb21d52a7a63 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > @@ -113,6 +113,9 @@
> >  			return val;		\
> >  	} while (0)
> >  
> > +#define ST7789V_IDS { 0x85, 0x85, 0x52 }
> > +#define ST7789V_IDS_SIZE 3
> > +
> >  struct st7789v_panel_info {
> >  	const struct drm_display_mode *display_mode;
> >  	u16 width_mm;
> > @@ -165,6 +168,77 @@ 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];
> > +
> > +	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,
> > @@ -237,6 +311,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

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

Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Sam Ravnborg]

On Thu, Jun 15, 2023 at 01:22:17AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jun 13, 2023 at 08:56:30AM +0200, Miquel Raynal wrote:
> > Hi Michael,
> > 
> > michael.riesch@wolfvision.net wrote on Tue, 13 Jun 2023 08:15:26 +0200:
> > 
> > > Hi Miquel,
> > > 
> > > On 6/9/23 16:59, Miquel Raynal wrote:
> > > > The spi core warns us about using an of_device_id table without a  
> > > 
> > > s/spi/SPI ?
> > 
> > Actually both are accepted in the kernel, IIRC it depends whether you
> > refer to the name of the bus or the Linux subsystem. Same for picking
> > "a" vs "an" before "spi/SPI". An attempt to use a unique formatting was
> > actually canceled because both are used equivalently and I believe even
> > the spi maintainer and the spi-nor maintainer kind of disagreed on the
> > default :)
> > 
> > > > spi_device_id table aside for module utilities in orter to not rely on  
> > > 
> > > s/in orter to/in order to ?
> > 
> > Yes, sorry for this typo as well as the two others you rightly pointed
> > out in another patch. I'll fix them.
> > 
> > > > OF modaliases. Just add this table using the device name without the
> > > > vendor prefix (as it is usually done).
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c  
> > > b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > > index bbc4569cbcdc..c67b9adb157f 100644
> > > > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > > > @@ -400,9 +400,16 @@ static const struct of_device_id  
> > > st7789v_of_match[] = {
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, st7789v_of_match);
> > > >
> > > > +static const struct spi_device_id st7789v_ids[] = {
> > > > +	{ "st7789v", },
> > > > +	{ /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(spi, st7789v_ids);
> > > > +
> > > >  static struct spi_driver st7789v_driver = {
> > > >  	.probe = st7789v_probe,
> > > >  	.remove = st7789v_remove,
> > > > +	.id_table = st7789v_ids,
> > > >  	.driver = {
> > > >  		.name = "st7789v",
> > > >  		.of_match_table = st7789v_of_match,  
> > > 
> > > May I point to you Sebastian Reichel's series that features a partial
> > > overlap with your work? [0]
> > 
> > Woow. That driver has been untouched for years and now two
> > contributions at the same time.
> 
> Three actually. Michael also submitted a series :)
> 
> > Sebastian, what is the current state of your series?
> 
> The DT changes got Ack'd by Rob and I have the R-B from Michael
> (minus a minor comment to make the panel struct 'static const').
> It's mainly waiting for a review from Sam.
> 
> I was a bit distracted by a boot regression on the devices and
> some other projects. The boot regression got solved, so I can
> prepare a new version if that makes things easier.
> 
> > Shall I base my work on top of yours? Or is it still too
> > premature and we shall instead try to merge both and contribute a new
> > version of the series bringing support for the two panels?
> 
> I suppose whatever is easier for Sam to review.

Hi Sebastian.

Too much panel stuff going on, so I miss the most and I am happy to see
other people do a lot of good work here.
Can i get a pointer to lore or so, then I will try to take a look.

	Sam

Re: [PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

[Sebastian Reichel]

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

Hi Sam,

On Thu, Jun 15, 2023 at 07:43:46AM +0200, Sam Ravnborg wrote:
> On Thu, Jun 15, 2023 at 01:22:17AM +0200, Sebastian Reichel wrote:
> > > > May I point to you Sebastian Reichel's series that features a partial
> > > > overlap with your work? [0]
> > > 
> > > Woow. That driver has been untouched for years and now two
> > > contributions at the same time.
> > 
> > Three actually. Michael also submitted a series :)
> > 
> > > Sebastian, what is the current state of your series?
> > 
> > The DT changes got Ack'd by Rob and I have the R-B from Michael
> > (minus a minor comment to make the panel struct 'static const').
> > It's mainly waiting for a review from Sam.
> > 
> > I was a bit distracted by a boot regression on the devices and
> > some other projects. The boot regression got solved, so I can
> > prepare a new version if that makes things easier.
> > 
> > > Shall I base my work on top of yours? Or is it still too
> > > premature and we shall instead try to merge both and contribute a new
> > > version of the series bringing support for the two panels?
> > 
> > I suppose whatever is easier for Sam to review.
> 
> Hi Sebastian.
> 
> Too much panel stuff going on, so I miss the most and I am happy
> to see other people do a lot of good work here. Can i get a
> pointer to lore or so, then I will try to take a look.

Sure, 

Michael Riesch already referenced it earlier in this thread:

[0] https://lore.kernel.org/dri-devel/20230422205012.2464933-1-sre@kernel.org/

Thanks for taking a look,

-- Sebastian

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

Re: [PATCH 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format

[Miquel Raynal]

Hi Sam,

sam@ravnborg.org wrote on Sat, 10 Jun 2023 22:12:46 +0200:

> On Fri, Jun 09, 2023 at 04:59:47PM +0200, Miquel Raynal wrote:
> > The LCD controller supports RGB444, RGB565 and RGB888. The value that is
> > written in the COLMOD register indicates using RGB888, so let's clearly
> > specify the in-use bus format.  
> 
> Confused.
> MEDIA_BUS_FMT_RGB666_1X18 assumes 6 bits per color.
> But RGB888 is 8 bits per color.
> 
> Something that I have forgotten, or is this inconsistent?

That is a typo indeed, I meant RBG666.

Thanks, Miquèl

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

[Miquel Raynal]

Hi Maxime,

mripard@kernel.org wrote on Mon, 12 Jun 2023 10:51:09 +0200:

> On Fri, Jun 09, 2023 at 04:59:50PM +0200, Miquel Raynal wrote:
> > This panel from Emerging Display Technologies Corporation features an
> > ST7789V2 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    | 34 +++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > index 212bccc31804..7de192a3a8aa 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > @@ -30,7 +30,8 @@
> >  #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_PCLK_RISING		0
> >  #define ST7789V_RGBCTRL_VBP(n)			((n) & 0x7f)
> >  #define ST7789V_RGBCTRL_HBP(n)			((n) & 0x1f)
> >  
> > @@ -117,6 +118,7 @@ struct st7789v_panel_info {
> >  	u16 width_mm;
> >  	u16 height_mm;
> >  	u32 bus_format;
> > +	u32 bus_flags;
> >  };
> >  
> >  struct st7789v {
> > @@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
> >  	.vtotal = 320 + 8 + 4 + 4,
> >  };
> >  
> > +static const struct drm_display_mode slow_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,
> > +};  
> 
> Why is it supposed to be slow (and compared to what)? It looks like a
> fairly normal mode to me? Or is it because it's refreshed at 30Hz?

The initial support was using 60Hz and for a reason that I do not
understand (scope capture look right), the panel I use is highly
unstable at whatever frequency above 30Hz, so for me it was "slow" :-)
But of course I'll use the panel name to qualify the mode.

> Either way, the ST7789V is a panel controller and can be connected to a
> wide range of panels depending on the model, so it would be better to
> just use the model name there.

Sure.

> >  static int st7789v_get_modes(struct drm_panel *panel,
> >  			     struct drm_connector *connector)
> >  {
> > @@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
> >  
> >  	connector->display_info.width_mm = panel_info->width_mm;
> >  	connector->display_info.height_mm = panel_info->height_mm;
> > +	connector->display_info.bus_flags = panel_info->bus_flags;
> >  	drm_display_info_set_bus_formats(&connector->display_info,
> >  					 &panel_info->bus_format, 1);
> >  
> > @@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
> >  static int st7789v_prepare(struct drm_panel *panel)
> >  {
> >  	struct st7789v *ctx = panel_to_st7789v(panel);
> > +	const struct st7789v_panel_info *panel_info = ctx->panel_info;
> > +	u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
> >  	int ret;
> >  
> > +	if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
> > +		pck = ST7789V_RGBCTRL_PCLK_RISING;
> > +  
> 
> I guess it could be in a separate commit

I will rebase on top of Sebastian's series who already addressed that
point (as well as many others).

[...]

> >  
> > +static const struct st7789v_panel_info et028013dma_info = {
> > +	.display_mode = &slow_mode,
> > +	.width_mm = 43,
> > +	.height_mm = 58,
> > +	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> > +	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> > +};
> > +
> >  static const struct of_device_id st7789v_of_match[] = {
> >  	{ .compatible = "sitronix,st7789v", .data = &st7789v_info },
> > +	{ .compatible = "edt,et028013dma", .data = &et028013dma_info },  
> 
> We should sort them by alphabetical order
> 
> Maxime

Ok!

Thanks,
Miquèl

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

[Miquel Raynal]

Hi Sebastian,

sre@kernel.org wrote on Thu, 15 Jun 2023 01:27:24 +0200:

> Hi,
> 
> On Sat, Jun 10, 2023 at 10:45:25PM +0200, Sam Ravnborg wrote:
> > Hi Miquel,
> > 
> > On Fri, Jun 09, 2023 at 04:59:51PM +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 peace for
> > > fastening 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.  
> > 
> > The read helper seems like a log of general boiler plate code.
> > I briefly looked into the use of regmap for the spi communication,
> > but it did not look like it supported the bit9 stuff.
> > 
> > I did not stare enough to add a reviewd by, but the approach is fine
> > and it is good to detech issues early.  
> 
> The st7789v datasheet describes a setup where SPI is connected
> unidirectional (i.e. without MISO). In that case the ID check
> will fail.

Right. I'll add a (spi->mode & SPI_NO_RX) check, as the default is to
have both lines, if there is no MISO line, I'd expect it to be
described in the DT, otherwise the description is broken.

Thanks,
Miquèl

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

[Sebastian Reichel]

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

Hi,

On Fri, Jun 16, 2023 at 12:13:45PM +0200, Miquel Raynal wrote:
> sre@kernel.org wrote on Thu, 15 Jun 2023 01:27:24 +0200:
> > On Sat, Jun 10, 2023 at 10:45:25PM +0200, Sam Ravnborg wrote:
> > > On Fri, Jun 09, 2023 at 04:59:51PM +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 peace for
> > > > fastening 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.  
> > > 
> > > The read helper seems like a log of general boiler plate code.
> > > I briefly looked into the use of regmap for the spi communication,
> > > but it did not look like it supported the bit9 stuff.
> > > 
> > > I did not stare enough to add a reviewd by, but the approach is fine
> > > and it is good to detech issues early.  
> > 
> > The st7789v datasheet describes a setup where SPI is connected
> > unidirectional (i.e. without MISO). In that case the ID check
> > will fail.
> 
> Right. I'll add a (spi->mode & SPI_NO_RX) check, as the default is to
> have both lines, if there is no MISO line, I'd expect it to be
> described in the DT, otherwise the description is broken.

Checking for SPI_NO_RX sounds good to me.

-- Sebastian

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