[PATCH 0/7] Add timing override to sitronix,st7789v

[PATCH 0/7] Add timing override to sitronix,st7789v

[Gerald Loacker]

This patch set adds additional functionality to the sitronix,st7789v
driver.

Patches 1,3 and 4 propagate useful flags to the drm subsystem.
Patch 2 adds the orientation property.
Patch 5 parses the device tree for a panel-timing and makes it possible to
  override the default timing.
Patches 6 and 7 add the new properties to the dt-bindings.

Gerald Loacker (4):
  drm/panel: sitronix-st7789v: propagate h/v-sync polarity
  drm/panel: sitronix-st7789v: add bus_flags to connector
  drm/panel: sitronix-st7789v: parse device tree to override timing mode
  dt-bindings: display: add panel-timing property to sitronix,st7789v

Michael Riesch (3):
  drm/panel: sitronix-st7789v: propagate RGB666 format
  drm/panel: sitronix-st7789v: add panel orientation support
  dt-bindings: display: add rotation property to sitronix,st7789v

 .../display/panel/sitronix,st7789v.yaml       |  19 ++
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 204 +++++++++++++++---
 2 files changed, 191 insertions(+), 32 deletions(-)

-- 
2.37.2

[PATCH 1/7] drm/panel: sitronix-st7789v: propagate RGB666 format

[Gerald Loacker]

From: Michael Riesch <michael.riesch@wolfvision.net>

The ST7789V display is operated in RGB666 (18-bit) mode. Propagate
this format via the display info.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bbc4569cbcdc..9535437271d3 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -5,6 +5,7 @@
 
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
@@ -171,6 +172,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) {
@@ -188,6 +190,9 @@ 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.37.2

[PATCH 2/7] drm/panel: sitronix-st7789v: add panel orientation support

[Gerald Loacker]

From: Michael Riesch <michael.riesch@wolfvision.net>

Determine the orientation of the display based on the device tree and
propagate it.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 28 +++++++++++++++----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 9535437271d3..5d4542c12f44 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -117,6 +117,7 @@ struct st7789v {
 	struct spi_device *spi;
 	struct gpio_desc *reset;
 	struct regulator *power;
+	enum drm_panel_orientation orientation;
 };
 
 enum st7789v_prefix {
@@ -171,6 +172,7 @@ 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);
 	struct drm_display_mode *mode;
 	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
@@ -193,9 +195,22 @@ static int st7789v_get_modes(struct drm_panel *panel,
 	drm_display_info_set_bus_formats(&connector->display_info, &bus_format,
 					 1);
 
+	/*
+	 * TODO: Remove once all drm drivers call
+	 * drm_connector_set_orientation_from_panel()
+	 */
+	drm_connector_set_panel_orientation(connector, ctx->orientation);
+
 	return 1;
 }
 
+static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
+{
+	struct st7789v *ctx = panel_to_st7789v(p);
+
+	return ctx->orientation;
+}
+
 static int st7789v_prepare(struct drm_panel *panel)
 {
 	struct st7789v *ctx = panel_to_st7789v(panel);
@@ -351,11 +366,12 @@ static int st7789v_unprepare(struct drm_panel *panel)
 }
 
 static const struct drm_panel_funcs st7789v_drm_funcs = {
-	.disable	= st7789v_disable,
-	.enable		= st7789v_enable,
-	.get_modes	= st7789v_get_modes,
-	.prepare	= st7789v_prepare,
-	.unprepare	= st7789v_unprepare,
+	.disable = st7789v_disable,
+	.enable = st7789v_enable,
+	.get_modes = st7789v_get_modes,
+	.get_orientation = st7789v_get_orientation,
+	.prepare = st7789v_prepare,
+	.unprepare = st7789v_unprepare,
 };
 
 static int st7789v_probe(struct spi_device *spi)
@@ -387,6 +403,8 @@ static int st7789v_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	of_drm_get_panel_orientation(spi->dev.of_node, &ctx->orientation);
+
 	drm_panel_add(&ctx->panel);
 
 	return 0;
-- 
2.37.2

[PATCH 3/7] drm/panel: sitronix-st7789v: propagate h/v-sync polarity

[Gerald Loacker]

Adds h/v sync polarity, which is hardcoded in display prepare, to
drm_display_mode.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 5d4542c12f44..24636722c2ff 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -167,6 +167,7 @@ static const struct drm_display_mode default_mode = {
 	.vsync_start = 320 + 8,
 	.vsync_end = 320 + 8 + 4,
 	.vtotal = 320 + 8 + 4 + 4,
+	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
 };
 
 static int st7789v_get_modes(struct drm_panel *panel,
-- 
2.37.2

[PATCH 4/7] drm/panel: sitronix-st7789v: add bus_flags to connector

[Gerald Loacker]

Propagate pixel clock sampling edge and data enable polarity. Both are
hardcoded.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 24636722c2ff..1ca04585aff2 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -195,6 +195,8 @@ static int st7789v_get_modes(struct drm_panel *panel,
 
 	drm_display_info_set_bus_formats(&connector->display_info, &bus_format,
 					 1);
+	connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH |
+					    DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
 
 	/*
 	 * TODO: Remove once all drm drivers call
-- 
2.37.2

[PATCH 5/7] drm/panel: sitronix-st7789v: parse device tree to override timing mode

[Gerald Loacker]

Parse device tree for panel-timing and allow to override the typical
timing. This requires the timing to be defined as display_timing instead
of drm_display_mode.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c    | 174 +++++++++++++++---
 1 file changed, 144 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 1ca04585aff2..ebde8a70deee 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -10,6 +10,9 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
+#include <video/display_timing.h>
+#include <video/of_display_timing.h>
+#include <video/videomode.h>
 #include <video/mipi_display.h>
 
 #include <drm/drm_device.h>
@@ -28,6 +31,8 @@
 #define ST7789V_RGBCTRL_CMD		0xb1
 #define ST7789V_RGBCTRL_WO			BIT(7)
 #define ST7789V_RGBCTRL_RCM(n)			(((n) & 3) << 5)
+#define ST7789V_RGBCTRL_RCM_DE			2
+#define ST7789V_RGBCTRL_RCM_HV			3
 #define ST7789V_RGBCTRL_VSYNC_HIGH		BIT(3)
 #define ST7789V_RGBCTRL_HSYNC_HIGH		BIT(2)
 #define ST7789V_RGBCTRL_PCLK_HIGH		BIT(1)
@@ -117,6 +122,7 @@ struct st7789v {
 	struct spi_device *spi;
 	struct gpio_desc *reset;
 	struct regulator *power;
+	struct drm_display_mode override_mode;
 	enum drm_panel_orientation orientation;
 };
 
@@ -157,39 +163,84 @@ static int st7789v_write_data(struct st7789v *ctx, u8 cmd)
 	return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
 }
 
-static const struct drm_display_mode default_mode = {
-	.clock = 7000,
-	.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,
-	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+static const struct display_timing st7789v_timing = {
+	.pixelclock = { 1, 7000000, 8333333 },
+	.hactive = { 240, 240, 240 },
+	.hfront_porch = { 2, 38, 500 },
+	.hback_porch = { 4, 10, 21 },
+	.hsync_len = { 2, 10, 10 },
+	.vactive = { 320, 320, 320 },
+	.vfront_porch = { 8, 8, 500 },
+	.vback_porch = { 4, 4, 117 },
+	.vsync_len = { 4, 4, 10 },
+	.flags = DISPLAY_FLAGS_HSYNC_HIGH | DISPLAY_FLAGS_VSYNC_HIGH |
+		 DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE |
+		 DISPLAY_FLAGS_SYNC_POSEDGE,
 };
 
-static int st7789v_get_modes(struct drm_panel *panel,
-			     struct drm_connector *connector)
+struct panel_desc {
+	u32 bus_format;
+	u32 bus_flags;
+};
+
+static struct panel_desc st7789v_desc = {
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
+	.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
+static unsigned int st7789v_get_timings_modes(struct drm_panel *panel,
+					      struct drm_connector *connector)
 {
-	struct st7789v *ctx = panel_to_st7789v(panel);
 	struct drm_display_mode *mode;
-	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
-	mode = drm_mode_duplicate(connector->dev, &default_mode);
+	const struct display_timing *dt = &st7789v_timing;
+	struct videomode vm;
+
+	videomode_from_timing(dt, &vm);
+	mode = drm_mode_create(connector->dev);
 	if (!mode) {
-		dev_err(panel->dev, "failed to add mode %ux%ux@%u\n",
-			default_mode.hdisplay, default_mode.vdisplay,
-			drm_mode_vrefresh(&default_mode));
-		return -ENOMEM;
+		dev_err(panel->dev, "failed to add timing %ux%ux\n",
+			dt->hactive.typ, dt->vactive.typ);
+		return 0;
 	}
 
-	drm_mode_set_name(mode);
+	drm_display_mode_from_videomode(&vm, mode);
+
+	mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
 
-	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
 	drm_mode_probed_add(connector, mode);
 
+	return 1;
+}
+
+static int st7789v_get_modes(struct drm_panel *panel,
+			     struct drm_connector *connector)
+{
+	struct st7789v *ctx = panel_to_st7789v(panel);
+	struct drm_display_mode *mode;
+	bool has_override = ctx->override_mode.type;
+	u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+	unsigned int num;
+
+	if (has_override) {
+		mode = drm_mode_duplicate(connector->dev, &ctx->override_mode);
+		if (!mode) {
+			dev_err(panel->dev, "failed to add mode %ux%ux@%u\n",
+				ctx->override_mode.hdisplay,
+				ctx->override_mode.vdisplay,
+				drm_mode_vrefresh(&ctx->override_mode));
+			return 0;
+		}
+
+		drm_mode_set_name(mode);
+
+		mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+		drm_mode_probed_add(connector, mode);
+		num = 1;
+	} else {
+		num = st7789v_get_timings_modes(panel, connector);
+	}
+
 	connector->display_info.width_mm = 61;
 	connector->display_info.height_mm = 103;
 
@@ -204,7 +255,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
 	 */
 	drm_connector_set_panel_orientation(connector, ctx->orientation);
 
-	return 1;
+	return num;
 }
 
 static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
@@ -217,6 +268,8 @@ static enum drm_panel_orientation st7789v_get_orientation(struct drm_panel *p)
 static int st7789v_prepare(struct drm_panel *panel)
 {
 	struct st7789v *ctx = panel_to_st7789v(panel);
+	struct drm_display_mode mode = ctx->override_mode;
+	u16 hbp, vbp, rcm, hsync = 0, vsync = 0, pclk = 0;
 	int ret;
 
 	ret = regulator_enable(ctx->power);
@@ -327,15 +380,29 @@ static int st7789v_prepare(struct drm_panel *panel)
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_EPF(3) |
 					     ST7789V_RAMCTRL_MAGIC));
 
+	if ((st7789v_desc.bus_flags & DRM_BUS_FLAG_DE_HIGH) ||
+	    (st7789v_desc.bus_flags & DRM_BUS_FLAG_DE_LOW))
+		rcm = ST7789V_RGBCTRL_RCM_DE;
+	else
+		rcm = ST7789V_RGBCTRL_RCM_HV;
+
+	if (st7789v_desc.bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
+		pclk = ST7789V_RGBCTRL_PCLK_HIGH;
+
+	if (mode.flags & DRM_MODE_FLAG_PHSYNC)
+		hsync = ST7789V_RGBCTRL_HSYNC_HIGH;
+
+	if (mode.flags & DRM_MODE_FLAG_PVSYNC)
+		vsync = ST7789V_RGBCTRL_VSYNC_HIGH;
+
+	vbp = mode.vtotal - mode.vsync_start;
+	hbp = mode.htotal - mode.hsync_start;
 	ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RGBCTRL_CMD));
 	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
-					     ST7789V_RGBCTRL_RCM(2) |
-					     ST7789V_RGBCTRL_VSYNC_HIGH |
-					     ST7789V_RGBCTRL_HSYNC_HIGH |
-					     ST7789V_RGBCTRL_PCLK_HIGH));
-	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
-	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20)));
-
+					     ST7789V_RGBCTRL_RCM(rcm) |
+					     vsync | hsync | pclk));
+	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(vbp)));
+	ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(hbp)));
 	return 0;
 }
 
@@ -377,8 +444,50 @@ static const struct drm_panel_funcs st7789v_drm_funcs = {
 	.unprepare = st7789v_unprepare,
 };
 
+static void st7789v_set_videomode(struct device *dev, struct st7789v *ctx,
+				  const struct display_timing *dt)
+{
+	struct videomode vm;
+
+	videomode_from_timing(dt, &vm);
+	drm_display_mode_from_videomode(&vm, &ctx->override_mode);
+	ctx->override_mode.type |= DRM_MODE_TYPE_DRIVER |
+				   DRM_MODE_TYPE_PREFERRED;
+
+	drm_bus_flags_from_videomode(&vm, &st7789v_desc.bus_flags);
+}
+
+#define ST7789V_BOUNDS_CHECK(to_check, bounds, field) \
+	(to_check->field.typ >= bounds->field.min &&  \
+	 to_check->field.typ <= bounds->field.max)
+static void st7789v_parse_panel_timing_node(struct device *dev,
+					    struct st7789v *ctx,
+					    const struct display_timing *ot)
+{
+	const struct display_timing *dt;
+	bool ot_timing_valid = true;
+
+	dt = &st7789v_timing;
+
+	if (!ST7789V_BOUNDS_CHECK(ot, dt, hactive) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, hfront_porch) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, hback_porch) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, hsync_len) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, vactive) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, vfront_porch) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, vback_porch) ||
+	    !ST7789V_BOUNDS_CHECK(ot, dt, vsync_len))
+		ot_timing_valid = false;
+
+	if (ot_timing_valid)
+		dt = ot;
+
+	st7789v_set_videomode(dev, ctx, dt);
+}
+
 static int st7789v_probe(struct spi_device *spi)
 {
+	struct display_timing dt;
 	struct st7789v *ctx;
 	int ret;
 
@@ -408,6 +517,11 @@ static int st7789v_probe(struct spi_device *spi)
 
 	of_drm_get_panel_orientation(spi->dev.of_node, &ctx->orientation);
 
+	if (!of_get_display_timing(spi->dev.of_node, "panel-timing", &dt))
+		st7789v_parse_panel_timing_node(&spi->dev, ctx, &dt);
+	else
+		st7789v_set_videomode(&spi->dev, ctx, &st7789v_timing);
+
 	drm_panel_add(&ctx->panel);
 
 	return 0;
-- 
2.37.2

[PATCH 6/7] dt-bindings: display: add rotation property to sitronix, st7789v

[Gerald Loacker]

From: Michael Riesch <michael.riesch@wolfvision.net>

The sitronix-st7789v driver now considers the rotation property.
Add the property to the documentation.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 .../devicetree/bindings/display/panel/sitronix,st7789v.yaml     | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index d984b59daa4a..ed942cd3620f 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -22,6 +22,7 @@ properties:
   power-supply: true
   backlight: true
   port: true
+  rotation: true
 
   spi-cpha: true
   spi-cpol: true
@@ -48,6 +49,7 @@ examples:
             reset-gpios = <&pio 6 11 GPIO_ACTIVE_LOW>;
             backlight = <&pwm_bl>;
             power-supply = <&power>;
+            rotation = <180>;
             spi-max-frequency = <100000>;
             spi-cpol;
             spi-cpha;
-- 
2.37.2

[PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix, st7789v

[Gerald Loacker]

The sitronix-st7789v driver now considers the panel-timing property.
Add the property to the documentation.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 .../display/panel/sitronix,st7789v.yaml         | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index ed942cd3620f..8810f123dedf 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -21,6 +21,7 @@ properties:
   reset-gpios: true
   power-supply: true
   backlight: true
+  panel-timing: true
   port: true
   rotation: true
 
@@ -54,6 +55,22 @@ examples:
             spi-cpol;
             spi-cpha;
 
+            panel-timing {
+                clock-frequency = <7000000>;
+                hactive = <240>;
+                vactive = <320>;
+                hfront-porch = <38>;
+                hback-porch = <10>;
+                hsync-len = <10>;
+                vfront-porch = <8>;
+                vback-porch = <4>;
+                vsync-len = <4>;
+                hsync-active = <1>;
+                vsync-active = <1>;
+                de-active = <1>;
+                pixelclk-active = <1>;
+            };
+
             port {
                 panel_input: endpoint {
                     remote-endpoint = <&tcon0_out_panel>;
-- 
2.37.2

Re: [PATCH 6/7] dt-bindings: display: add rotation property to sitronix,st7789v

[Krzysztof Kozlowski]

On 14/03/2023 12:56, Gerald Loacker wrote:
> From: Michael Riesch <michael.riesch@wolfvision.net>
> 
> The sitronix-st7789v driver now considers the rotation property.
> Add the property to the documentation.
> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---

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

Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Krzysztof Kozlowski]

On 14/03/2023 12:56, Gerald Loacker wrote:
> The sitronix-st7789v driver now considers the panel-timing property.
> Add the property to the documentation.
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>


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

Best regards,
Krzysztof

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Rob Herring]

On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
> The sitronix-st7789v driver now considers the panel-timing property.

I read the patch for that and still don't know 'why'. Commit messages 
should answer why.

> Add the property to the documentation.

We generally don't put timings in DT for panels. Why is this one 
special?

> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  .../display/panel/sitronix,st7789v.yaml         | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> index ed942cd3620f..8810f123dedf 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
> @@ -21,6 +21,7 @@ properties:
>    reset-gpios: true
>    power-supply: true
>    backlight: true
> +  panel-timing: true
>    port: true
>    rotation: true
>  
> @@ -54,6 +55,22 @@ examples:
>              spi-cpol;
>              spi-cpha;
>  
> +            panel-timing {
> +                clock-frequency = <7000000>;
> +                hactive = <240>;
> +                vactive = <320>;
> +                hfront-porch = <38>;
> +                hback-porch = <10>;
> +                hsync-len = <10>;
> +                vfront-porch = <8>;
> +                vback-porch = <4>;
> +                vsync-len = <4>;
> +                hsync-active = <1>;
> +                vsync-active = <1>;
> +                de-active = <1>;
> +                pixelclk-active = <1>;
> +            };
> +
>              port {
>                  panel_input: endpoint {
>                      remote-endpoint = <&tcon0_out_panel>;
> -- 
> 2.37.2
> 

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Michael Riesch]

Hi Rob,

On 3/16/23 22:57, Rob Herring wrote:
> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
>> The sitronix-st7789v driver now considers the panel-timing property.
> 
> I read the patch for that and still don't know 'why'. Commit messages 
> should answer why.
> 
>> Add the property to the documentation.
> 
> We generally don't put timings in DT for panels. Why is this one 
> special?

For now, having the timings in the device tree allows for setting the
hsync/vsync/de polarity.

As a next step, we aim to implement the partial mode feature of this
panel. It is possible to use only a certain region of the panel, which
is helpful e.g., when a part of the panel is occluded and should not be
considered by DRM. We thought that this could be specified as timing in DT.

(The hactive and vactive properties serve as dimensions of this certain
region, of course. We still need to specify somehow the position of the
region. Maybe with additional properties hactive-start and vactive-start?)

What do you think about that?

Thanks and best regards,
Michael

> 
>>
>> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
>> ---
>>  .../display/panel/sitronix,st7789v.yaml         | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> index ed942cd3620f..8810f123dedf 100644
>> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> @@ -21,6 +21,7 @@ properties:
>>    reset-gpios: true
>>    power-supply: true
>>    backlight: true
>> +  panel-timing: true
>>    port: true
>>    rotation: true
>>  
>> @@ -54,6 +55,22 @@ examples:
>>              spi-cpol;
>>              spi-cpha;
>>  
>> +            panel-timing {
>> +                clock-frequency = <7000000>;
>> +                hactive = <240>;
>> +                vactive = <320>;
>> +                hfront-porch = <38>;
>> +                hback-porch = <10>;
>> +                hsync-len = <10>;
>> +                vfront-porch = <8>;
>> +                vback-porch = <4>;
>> +                vsync-len = <4>;
>> +                hsync-active = <1>;
>> +                vsync-active = <1>;
>> +                de-active = <1>;
>> +                pixelclk-active = <1>;
>> +            };
>> +
>>              port {
>>                  panel_input: endpoint {
>>                      remote-endpoint = <&tcon0_out_panel>;
>> -- 
>> 2.37.2
>>

Re: [PATCH 0/7] Add timing override to sitronix,st7789v

[Gerald Loacker]

Hi,

Besides dt-bindings, there has been no feedback on this series yet. How
to proceed?

For clarification: Besides adjusting panel parameters, we need the
panel-timing to add a partial display mode later.

Regards,
Gerald

Am 14.03.2023 um 12:56 schrieb Gerald Loacker:
> This patch set adds additional functionality to the sitronix,st7789v
> driver.
> 
> Patches 1,3 and 4 propagate useful flags to the drm subsystem.
> Patch 2 adds the orientation property.
> Patch 5 parses the device tree for a panel-timing and makes it possible to
>   override the default timing.
> Patches 6 and 7 add the new properties to the dt-bindings.
> 
> Gerald Loacker (4):
>   drm/panel: sitronix-st7789v: propagate h/v-sync polarity
>   drm/panel: sitronix-st7789v: add bus_flags to connector
>   drm/panel: sitronix-st7789v: parse device tree to override timing mode
>   dt-bindings: display: add panel-timing property to sitronix,st7789v
> 
> Michael Riesch (3):
>   drm/panel: sitronix-st7789v: propagate RGB666 format
>   drm/panel: sitronix-st7789v: add panel orientation support
>   dt-bindings: display: add rotation property to sitronix,st7789v
> 
>  .../display/panel/sitronix,st7789v.yaml       |  19 ++
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 204 +++++++++++++++---
>  2 files changed, 191 insertions(+), 32 deletions(-)
> 

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Maxime Ripard]

On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
> Hi Rob,
> 
> On 3/16/23 22:57, Rob Herring wrote:
> > On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
> >> The sitronix-st7789v driver now considers the panel-timing property.
> > 
> > I read the patch for that and still don't know 'why'. Commit messages 
> > should answer why.
> > 
> >> Add the property to the documentation.
> > 
> > We generally don't put timings in DT for panels. Why is this one 
> > special?
> 
> For now, having the timings in the device tree allows for setting the
> hsync/vsync/de polarity.
> 
> As a next step, we aim to implement the partial mode feature of this
> panel. It is possible to use only a certain region of the panel, which
> is helpful e.g., when a part of the panel is occluded and should not be
> considered by DRM. We thought that this could be specified as timing in DT.
>
> (The hactive and vactive properties serve as dimensions of this certain
> region, of course. We still need to specify somehow the position of the
> region. Maybe with additional properties hactive-start and vactive-start?)
> 
> What do you think about that?

I don't see why we would need the device tree to support that. What you
described is essentially what overscan is for HDMI/analog output, and we
already have everything to deal with overscan properly in KMS.

Maxime

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Michael Riesch]

Hi Maxime,

On 3/29/23 11:16, Maxime Ripard wrote:
> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
>> Hi Rob,
>>
>> On 3/16/23 22:57, Rob Herring wrote:
>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
>>>> The sitronix-st7789v driver now considers the panel-timing property.
>>>
>>> I read the patch for that and still don't know 'why'. Commit messages 
>>> should answer why.
>>>
>>>> Add the property to the documentation.
>>>
>>> We generally don't put timings in DT for panels. Why is this one 
>>> special?
>>
>> For now, having the timings in the device tree allows for setting the
>> hsync/vsync/de polarity.
>>
>> As a next step, we aim to implement the partial mode feature of this
>> panel. It is possible to use only a certain region of the panel, which
>> is helpful e.g., when a part of the panel is occluded and should not be
>> considered by DRM. We thought that this could be specified as timing in DT.
>>
>> (The hactive and vactive properties serve as dimensions of this certain
>> region, of course. We still need to specify somehow the position of the
>> region. Maybe with additional properties hactive-start and vactive-start?)
>>
>> What do you think about that?
> 
> I don't see why we would need the device tree to support that. What you
> described is essentially what overscan is for HDMI/analog output, and we
> already have everything to deal with overscan properly in KMS.

Thanks for your response, but I am afraid I don't quite follow.

How are we supposed to expose control over the hsync/vsync/data enable
polarity? I only know that the display controller and the panel need to
agree on a setting that works for both. What is the canonical way to do
this?

A different question is the partial mode, for which (IIUC) you suggest
the overscan feature. As I have never heard of this before, it would be
very nice if you could point me to some examples. Where would the
effective resolution be set in this case?

We thought that this should enter the device tree as in our case the
display is partially occluded due to hardware constraints. For the user
there is only one reasonable configuration.

Alternatively, we could follow a different approach and handle a
separate compatible in the panel driver. Would this be acceptable for
mainline inclusion?

Best regards,
Michael

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Maxime Ripard]

On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote:
> On 3/29/23 11:16, Maxime Ripard wrote:
> > On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
> >> Hi Rob,
> >>
> >> On 3/16/23 22:57, Rob Herring wrote:
> >>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
> >>>> The sitronix-st7789v driver now considers the panel-timing property.
> >>>
> >>> I read the patch for that and still don't know 'why'. Commit messages 
> >>> should answer why.
> >>>
> >>>> Add the property to the documentation.
> >>>
> >>> We generally don't put timings in DT for panels. Why is this one 
> >>> special?
> >>
> >> For now, having the timings in the device tree allows for setting the
> >> hsync/vsync/de polarity.
> >>
> >> As a next step, we aim to implement the partial mode feature of this
> >> panel. It is possible to use only a certain region of the panel, which
> >> is helpful e.g., when a part of the panel is occluded and should not be
> >> considered by DRM. We thought that this could be specified as timing in DT.
> >>
> >> (The hactive and vactive properties serve as dimensions of this certain
> >> region, of course. We still need to specify somehow the position of the
> >> region. Maybe with additional properties hactive-start and vactive-start?)
> >>
> >> What do you think about that?
> > 
> > I don't see why we would need the device tree to support that. What you
> > described is essentially what overscan is for HDMI/analog output, and we
> > already have everything to deal with overscan properly in KMS.
> 
> Thanks for your response, but I am afraid I don't quite follow.
> 
> How are we supposed to expose control over the hsync/vsync/data enable
> polarity? I only know that the display controller and the panel need to
> agree on a setting that works for both. What is the canonical way to do
> this?

So typically, it would come from the panel datasheet and would thus be
exposed by the panel driver. st7789v is not a panel itself but a (pretty
flexible) panel controller so it's not fixed and I don't think we have a
good answer to that (yet).

> A different question is the partial mode, for which (IIUC) you suggest
> the overscan feature. As I have never heard of this before, it would be
> very nice if you could point me to some examples. Where would the
> effective resolution be set in this case?

So, back when CRT were a thing the edges of the tube were masked by the
plastic case. HDMI inherited from that and that's why you still have
some UI on some devices (like consoles) to setup the active area of the
display.

The underlying issue is exactly what you describe: the active area is
larger than what the plastic case allows to see. I don't think anyone
ever had the usecase you have, but it would be the right solution to me
to solve essentially the same issue the same way we do on other output
types.

Maxime

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Michael Riesch]

Hi Maxime,

On 3/30/23 16:58, Maxime Ripard wrote:
> On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote:
>> On 3/29/23 11:16, Maxime Ripard wrote:
>>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
>>>> Hi Rob,
>>>>
>>>> On 3/16/23 22:57, Rob Herring wrote:
>>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
>>>>>> The sitronix-st7789v driver now considers the panel-timing property.
>>>>>
>>>>> I read the patch for that and still don't know 'why'. Commit messages 
>>>>> should answer why.
>>>>>
>>>>>> Add the property to the documentation.
>>>>>
>>>>> We generally don't put timings in DT for panels. Why is this one 
>>>>> special?
>>>>
>>>> For now, having the timings in the device tree allows for setting the
>>>> hsync/vsync/de polarity.
>>>>
>>>> As a next step, we aim to implement the partial mode feature of this
>>>> panel. It is possible to use only a certain region of the panel, which
>>>> is helpful e.g., when a part of the panel is occluded and should not be
>>>> considered by DRM. We thought that this could be specified as timing in DT.
>>>>
>>>> (The hactive and vactive properties serve as dimensions of this certain
>>>> region, of course. We still need to specify somehow the position of the
>>>> region. Maybe with additional properties hactive-start and vactive-start?)
>>>>
>>>> What do you think about that?
>>>
>>> I don't see why we would need the device tree to support that. What you
>>> described is essentially what overscan is for HDMI/analog output, and we
>>> already have everything to deal with overscan properly in KMS.
>>
>> Thanks for your response, but I am afraid I don't quite follow.
>>
>> How are we supposed to expose control over the hsync/vsync/data enable
>> polarity? I only know that the display controller and the panel need to
>> agree on a setting that works for both. What is the canonical way to do
>> this?
> 
> So typically, it would come from the panel datasheet and would thus be
> exposed by the panel driver. st7789v is not a panel itself but a (pretty
> flexible) panel controller so it's not fixed and I don't think we have a
> good answer to that (yet).

Then it seems to me that creating a panel driver (= st8879v panel
controller driver with a new compatible) would make sense. By
coincidence Sebastian Reichel has come up with this approach recently,
see
https://lore.kernel.org/dri-devel/20230317232355.1554980-1-sre@kernel.org/
We just need a way to resolve the conflicts between the two series.

Cc: Sebastian

>> A different question is the partial mode, for which (IIUC) you suggest
>> the overscan feature. As I have never heard of this before, it would be
>> very nice if you could point me to some examples. Where would the
>> effective resolution be set in this case?
> 
> So, back when CRT were a thing the edges of the tube were masked by the
> plastic case. HDMI inherited from that and that's why you still have
> some UI on some devices (like consoles) to setup the active area of the
> display.
> 
> The underlying issue is exactly what you describe: the active area is
> larger than what the plastic case allows to see. I don't think anyone
> ever had the usecase you have, but it would be the right solution to me
> to solve essentially the same issue the same way we do on other output
> types.

OK, we'll look into the overscan feature. But still the information
about the active area should come from the driver, right?

Thanks and best regards,
Michael

Re: [PATCH 0/7] Add timing override to sitronix,st7789v

[Michael Riesch]

Hi all,

On 3/14/23 12:56, Gerald Loacker wrote:
> This patch set adds additional functionality to the sitronix,st7789v
> driver.
> 
> Patches 1,3 and 4 propagate useful flags to the drm subsystem.
> Patch 2 adds the orientation property.

If there are no objections, patches 1-4 and 6 could be applied from our
point of view. Or should we spin a v2?

> Patch 5 parses the device tree for a panel-timing and makes it possible to
>   override the default timing.
> Patches 6 and 7 add the new properties to the dt-bindings.

Parsing the timing from the device tree (patches 5 and 7) can be
ignored, we'll come up with a different approach.

Best regards,
Michael

> 
> Gerald Loacker (4):
>   drm/panel: sitronix-st7789v: propagate h/v-sync polarity
>   drm/panel: sitronix-st7789v: add bus_flags to connector
>   drm/panel: sitronix-st7789v: parse device tree to override timing mode
>   dt-bindings: display: add panel-timing property to sitronix,st7789v
> 
> Michael Riesch (3):
>   drm/panel: sitronix-st7789v: propagate RGB666 format
>   drm/panel: sitronix-st7789v: add panel orientation support
>   dt-bindings: display: add rotation property to sitronix,st7789v
> 
>  .../display/panel/sitronix,st7789v.yaml       |  19 ++
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    | 204 +++++++++++++++---
>  2 files changed, 191 insertions(+), 32 deletions(-)
> 

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Maxime Ripard]

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

On Fri, Mar 31, 2023 at 11:36:43AM +0200, Michael Riesch wrote:
> On 3/30/23 16:58, Maxime Ripard wrote:
> > On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote:
> >> On 3/29/23 11:16, Maxime Ripard wrote:
> >>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
> >>>> Hi Rob,
> >>>>
> >>>> On 3/16/23 22:57, Rob Herring wrote:
> >>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
> >>>>>> The sitronix-st7789v driver now considers the panel-timing property.
> >>>>>
> >>>>> I read the patch for that and still don't know 'why'. Commit messages 
> >>>>> should answer why.
> >>>>>
> >>>>>> Add the property to the documentation.
> >>>>>
> >>>>> We generally don't put timings in DT for panels. Why is this one 
> >>>>> special?
> >>>>
> >>>> For now, having the timings in the device tree allows for setting the
> >>>> hsync/vsync/de polarity.
> >>>>
> >>>> As a next step, we aim to implement the partial mode feature of this
> >>>> panel. It is possible to use only a certain region of the panel, which
> >>>> is helpful e.g., when a part of the panel is occluded and should not be
> >>>> considered by DRM. We thought that this could be specified as timing in DT.
> >>>>
> >>>> (The hactive and vactive properties serve as dimensions of this certain
> >>>> region, of course. We still need to specify somehow the position of the
> >>>> region. Maybe with additional properties hactive-start and vactive-start?)
> >>>>
> >>>> What do you think about that?
> >>>
> >>> I don't see why we would need the device tree to support that. What you
> >>> described is essentially what overscan is for HDMI/analog output, and we
> >>> already have everything to deal with overscan properly in KMS.
> >>
> >> Thanks for your response, but I am afraid I don't quite follow.
> >>
> >> How are we supposed to expose control over the hsync/vsync/data enable
> >> polarity? I only know that the display controller and the panel need to
> >> agree on a setting that works for both. What is the canonical way to do
> >> this?
> > 
> > So typically, it would come from the panel datasheet and would thus be
> > exposed by the panel driver. st7789v is not a panel itself but a (pretty
> > flexible) panel controller so it's not fixed and I don't think we have a
> > good answer to that (yet).
> 
> Then it seems to me that creating a panel driver (= st8879v panel
> controller driver with a new compatible) would make sense.

I don't see why? The entire controller is the same except (maybe) for
some initialization data. Doing a new driver for it seems like taking
the easy way out?

> By coincidence Sebastian Reichel has come up with this approach
> recently, see
> https://lore.kernel.org/dri-devel/20230317232355.1554980-1-sre@kernel.org/
> We just need a way to resolve the conflicts between the two series.
> 
> Cc: Sebastian

That's not a new driver though? That approach looks sane to me.

> >> A different question is the partial mode, for which (IIUC) you suggest
> >> the overscan feature. As I have never heard of this before, it would be
> >> very nice if you could point me to some examples. Where would the
> >> effective resolution be set in this case?
> > 
> > So, back when CRT were a thing the edges of the tube were masked by the
> > plastic case. HDMI inherited from that and that's why you still have
> > some UI on some devices (like consoles) to setup the active area of the
> > display.
> > 
> > The underlying issue is exactly what you describe: the active area is
> > larger than what the plastic case allows to see. I don't think anyone
> > ever had the usecase you have, but it would be the right solution to me
> > to solve essentially the same issue the same way we do on other output
> > types.
> 
> OK, we'll look into the overscan feature. But still the information
> about the active area should come from the driver, right?

No, the userspace is in charge there.

Maxime

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

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Michael Riesch]

Hi Maxime,

On 4/4/23 18:04, Maxime Ripard wrote:
> On Fri, Mar 31, 2023 at 11:36:43AM +0200, Michael Riesch wrote:
>> On 3/30/23 16:58, Maxime Ripard wrote:
>>> On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote:
>>>> On 3/29/23 11:16, Maxime Ripard wrote:
>>>>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>> On 3/16/23 22:57, Rob Herring wrote:
>>>>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
>>>>>>>> The sitronix-st7789v driver now considers the panel-timing property.
>>>>>>>
>>>>>>> I read the patch for that and still don't know 'why'. Commit messages 
>>>>>>> should answer why.
>>>>>>>
>>>>>>>> Add the property to the documentation.
>>>>>>>
>>>>>>> We generally don't put timings in DT for panels. Why is this one 
>>>>>>> special?
>>>>>>
>>>>>> For now, having the timings in the device tree allows for setting the
>>>>>> hsync/vsync/de polarity.
>>>>>>
>>>>>> As a next step, we aim to implement the partial mode feature of this
>>>>>> panel. It is possible to use only a certain region of the panel, which
>>>>>> is helpful e.g., when a part of the panel is occluded and should not be
>>>>>> considered by DRM. We thought that this could be specified as timing in DT.
>>>>>>
>>>>>> (The hactive and vactive properties serve as dimensions of this certain
>>>>>> region, of course. We still need to specify somehow the position of the
>>>>>> region. Maybe with additional properties hactive-start and vactive-start?)
>>>>>>
>>>>>> What do you think about that?
>>>>>
>>>>> I don't see why we would need the device tree to support that. What you
>>>>> described is essentially what overscan is for HDMI/analog output, and we
>>>>> already have everything to deal with overscan properly in KMS.
>>>>
>>>> Thanks for your response, but I am afraid I don't quite follow.
>>>>
>>>> How are we supposed to expose control over the hsync/vsync/data enable
>>>> polarity? I only know that the display controller and the panel need to
>>>> agree on a setting that works for both. What is the canonical way to do
>>>> this?
>>>
>>> So typically, it would come from the panel datasheet and would thus be
>>> exposed by the panel driver. st7789v is not a panel itself but a (pretty
>>> flexible) panel controller so it's not fixed and I don't think we have a
>>> good answer to that (yet).
>>
>> Then it seems to me that creating a panel driver (= st8879v panel
>> controller driver with a new compatible) would make sense.
> 
> I don't see why? The entire controller is the same except (maybe) for
> some initialization data. Doing a new driver for it seems like taking
> the easy way out?
> 
>> By coincidence Sebastian Reichel has come up with this approach
>> recently, see
>> https://lore.kernel.org/dri-devel/20230317232355.1554980-1-sre@kernel.org/
>> We just need a way to resolve the conflicts between the two series.
>>
>> Cc: Sebastian
> 
> That's not a new driver though? That approach looks sane to me.

Sorry for the ambiguity. The plan is now to add a new compatible to the
st8879v panel controller driver.

>>>> A different question is the partial mode, for which (IIUC) you suggest
>>>> the overscan feature. As I have never heard of this before, it would be
>>>> very nice if you could point me to some examples. Where would the
>>>> effective resolution be set in this case?
>>>
>>> So, back when CRT were a thing the edges of the tube were masked by the
>>> plastic case. HDMI inherited from that and that's why you still have
>>> some UI on some devices (like consoles) to setup the active area of the
>>> display.
>>>
>>> The underlying issue is exactly what you describe: the active area is
>>> larger than what the plastic case allows to see. I don't think anyone
>>> ever had the usecase you have, but it would be the right solution to me
>>> to solve essentially the same issue the same way we do on other output
>>> types.
>>
>> OK, we'll look into the overscan feature. But still the information
>> about the active area should come from the driver, right?
> 
> No, the userspace is in charge there.

I'd prefer not to have the hardware description in user space. But we
can continue this discussing once our v2 is out.

Best regards,
Michael

Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Maxime Ripard]

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

On Tue, Apr 04, 2023 at 06:26:25PM +0200, Michael Riesch wrote:
> >>>> A different question is the partial mode, for which (IIUC) you suggest
> >>>> the overscan feature. As I have never heard of this before, it would be
> >>>> very nice if you could point me to some examples. Where would the
> >>>> effective resolution be set in this case?
> >>>
> >>> So, back when CRT were a thing the edges of the tube were masked by the
> >>> plastic case. HDMI inherited from that and that's why you still have
> >>> some UI on some devices (like consoles) to setup the active area of the
> >>> display.
> >>>
> >>> The underlying issue is exactly what you describe: the active area is
> >>> larger than what the plastic case allows to see. I don't think anyone
> >>> ever had the usecase you have, but it would be the right solution to me
> >>> to solve essentially the same issue the same way we do on other output
> >>> types.
> >>
> >> OK, we'll look into the overscan feature. But still the information
> >> about the active area should come from the driver, right?
> > 
> > No, the userspace is in charge there.
> 
> I'd prefer not to have the hardware description in user space. But we
> can continue this discussing once our v2 is out.

I'm not sure if it's worth doing something if you don't agree with it :)

At the end of the day, "the hardware" is a matter of semantics, and you
would argue that it's only the (user) visible part of the display, and I
would argue that it's the whole panel and we would both be somewhat
right.

The thing is: having the margins definition allows the userspace to be
aware that there's overscan to deal with, and for example setup the
scaler to properly display whatever you put in there.

If you just report a weird mode that doesn't match any kind of standard,
well, you could still do that, but you would need to tell the compositor
which mode you would need to scale down from.

In both case the userspace is involved. One is generic, the other isn't
and probably requires some extra development.

Maxime

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