dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2 RESEND] Add driver for Novatek NT35596S panel
@ 2023-07-19 15:20 Arnaud Ferraris
  2023-07-19 15:20 ` [PATCH v5 1/2 RESEND] dt-bindings: display: panel: Add Novatek NT35596S panel bindings Arnaud Ferraris
  2023-07-19 15:20 ` [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver Arnaud Ferraris
  0 siblings, 2 replies; 6+ messages in thread
From: Arnaud Ferraris @ 2023-07-19 15:20 UTC (permalink / raw)
  To: Sumit Semwal, Neil Armstrong, Sam Ravnborg, David Airlie,
	Daniel Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	dri-devel, devicetree, linux-kernel
  Cc: Arnaud Ferraris, Molly Sophia

These patches add support for Novatek NT35596S based JDI FHD panels,
found in Xiaomi Mi Mix2S mobile phones.

Notes:
- I'm taking over this series as the original submitter is no longer
  able to work on/test those patches.
- Re-sending as I messed the1st attempt by not including the mailing
  lists; sorry to those impacted by the noise

Changes in v5:
- Move changelogs out of commit messages.
- Wrap comment/text lines around 80 chars.

Changes in v4:
- Correct numeric order of the items in binding.

Changes in v3:
- Embed the support into existing driver (panel-novatek-nt36672a), as
  these two IC are similar with different initialization commands.

Changes in v2:
- Correct items order in Makefile and improve failure handling.

Molly Sophia (2):
  dt-bindings: display: panel: Add Novatek NT35596S panel bindings
  drm: panel: Add novatek nt35596s panel driver

 .../display/panel/novatek,nt36672a.yaml       |  21 +-
 drivers/gpu/drm/panel/Kconfig                 |   7 +-
 .../gpu/drm/panel/panel-novatek-nt36672a.c    | 251 ++++++++++++++++--
 3 files changed, 251 insertions(+), 28 deletions(-)

-- 
2.40.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v5 1/2 RESEND] dt-bindings: display: panel: Add Novatek NT35596S panel bindings
  2023-07-19 15:20 [PATCH v5 0/2 RESEND] Add driver for Novatek NT35596S panel Arnaud Ferraris
@ 2023-07-19 15:20 ` Arnaud Ferraris
  2023-07-19 15:20 ` [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver Arnaud Ferraris
  1 sibling, 0 replies; 6+ messages in thread
From: Arnaud Ferraris @ 2023-07-19 15:20 UTC (permalink / raw)
  To: Sumit Semwal, Neil Armstrong, Sam Ravnborg, David Airlie,
	Daniel Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	dri-devel, devicetree, linux-kernel
  Cc: Arnaud Ferraris, Krzysztof Kozlowski, Molly Sophia

From: Molly Sophia <mollysophia379@gmail.com>

Add documentation for "novatek,nt35596s" panel.

Signed-off-by: Molly Sophia <mollysophia379@gmail.com>
Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Changes in v5:
- Move changelog out of commit message.
- Wrap header comment block lines around 80 chars.

Changes in v4:
- No change.

Changes in v3:
- Embed the support into existing driver (panel-novatek-nt36672a), as
  these two IC are similar with different initialization commands.

 .../display/panel/novatek,nt36672a.yaml       | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml b/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
index ae821f465e1c..53b9e771af56 100644
--- a/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
+++ b/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
@@ -20,14 +20,21 @@ allOf:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - tianma,fhd-video
-      - const: novatek,nt36672a
+    oneOf:
+      - items:
+          - enum:
+              - jdi,fhd-nt35596s
+          - const: novatek,nt35596s
+
+      - items:
+          - enum:
+              - tianma,fhd-video
+          - const: novatek,nt36672a
+
     description: This indicates the panel manufacturer of the panel that is
-      in turn using the NT36672A panel driver. This compatible string
-      determines how the NT36672A panel driver is configured for the indicated
-      panel. The novatek,nt36672a compatible shall always be provided as a fallback.
+      in turn using the NT36672A or the NT35596S panel driver. This compatible
+      string determines how the panel driver is configured for the indicated
+      panel.
 
   reset-gpios:
     maxItems: 1
-- 
2.40.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver
  2023-07-19 15:20 [PATCH v5 0/2 RESEND] Add driver for Novatek NT35596S panel Arnaud Ferraris
  2023-07-19 15:20 ` [PATCH v5 1/2 RESEND] dt-bindings: display: panel: Add Novatek NT35596S panel bindings Arnaud Ferraris
@ 2023-07-19 15:20 ` Arnaud Ferraris
  2023-07-23 16:45   ` Linus Walleij
  1 sibling, 1 reply; 6+ messages in thread
From: Arnaud Ferraris @ 2023-07-19 15:20 UTC (permalink / raw)
  To: Neil Armstrong, Sam Ravnborg, David Airlie, Daniel Vetter,
	Sumit Semwal, dri-devel, linux-kernel
  Cc: Arnaud Ferraris, Molly Sophia

From: Molly Sophia <mollysophia379@gmail.com>

Novatek NT35596s is a generic DSI IC that drives command and video mode
panels. Add the driver for it. Currently add support for the LCD panel
from JDI connected with this IC, as found on Xiaomi Mi Mix2s phones.

Signed-off-by: Molly Sophia <mollysophia379@gmail.com>
Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
---
Changes in v5:
- Move changelog out of commit message.
- Wrap description lines around 80 chars.

Changes in v4:
- Correct numeric order of the items.

Changes in v3:
- Embed the documentation into existing one (novatek,nt36672a).

 drivers/gpu/drm/panel/Kconfig                 |   7 +-
 .../gpu/drm/panel/panel-novatek-nt36672a.c    | 251 ++++++++++++++++--
 2 files changed, 237 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 1a0fd0754692..28ac4d980472 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -399,14 +399,15 @@ config DRM_PANEL_NOVATEK_NT36523
 	  Boe panels used in Xiaomi Mi Pad 5 and 5 Pro tablets.
 
 config DRM_PANEL_NOVATEK_NT36672A
-	tristate "Novatek NT36672A DSI panel"
+	tristate "Novatek NT36672A/NT35596S DSI panel"
 	depends on OF
 	depends on DRM_MIPI_DSI
 	depends on BACKLIGHT_CLASS_DEVICE
 	help
 	  Say Y here if you want to enable support for the panels built
-	  around the Novatek NT36672A display controller, such as some
-	  Tianma panels used in a few Xiaomi Poco F1 mobile phones.
+	  around the Novatek NT36672A or NT35596S display controller, such
+	  as some Tianma panels used in a few Xiaomi Poco F1 mobile phones
+	  or the JDI panels used in Xiaomi Mi Mix2S mobile phones.
 
 config DRM_PANEL_NOVATEK_NT39016
 	tristate "Novatek NT39016 RGB/SPI panel"
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
index 73bcffa1e0c1..68600000618b 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
@@ -3,13 +3,16 @@
  * Copyright (C) 2020 Linaro Ltd
  * Author: Sumit Semwal <sumit.semwal@linaro.org>
  *
- * This driver is for the DSI interface to panels using the NT36672A display driver IC
- * from Novatek.
- * Currently supported are the Tianma FHD+ panels found in some Xiaomi phones, including
- * some variants of the Poco F1 phone.
+ * Copyright (C) 2022 Molly Sophia <mollysophia379@gmail.com>
  *
- * Panels using the Novatek NT37762A IC should add appropriate configuration per-panel and
- * use this driver.
+ * This driver is for the DSI interface to panels using the NT36672A/NT35596S
+ * display driver IC from Novatek.
+ * Currently supported are the Tianma FHD+ panels found in some Xiaomi phones,
+ * including some variants of the Poco F1 phone, and the JDI FHD+ panels found
+ * in Xiaomi Mi Mix2S phones.
+ *
+ * Panels using the Novatek NT37762A or NT35596S IC should add appropriate
+ * configuration per-panel and use this driver.
  */
 
 #include <linux/delay.h>
@@ -123,12 +126,14 @@ static int nt36672a_panel_unprepare(struct drm_panel *panel)
 	if (!pinfo->prepared)
 		return 0;
 
-	/* send off cmds */
-	ret = nt36672a_send_cmds(panel, pinfo->desc->off_cmds,
-				 pinfo->desc->num_off_cmds);
+	if (pinfo->desc->num_off_cmds != 0) {
+		/* send off cmds if present */
+		ret = nt36672a_send_cmds(panel, pinfo->desc->off_cmds,
+					pinfo->desc->num_off_cmds);
 
-	if (ret < 0)
-		dev_err(panel->dev, "failed to send DCS off cmds: %d\n", ret);
+		if (ret < 0)
+			dev_err(panel->dev, "failed to send DCS off cmds: %d\n", ret);
+	}
 
 	ret = mipi_dsi_dcs_set_display_off(pinfo->link);
 	if (ret < 0)
@@ -211,13 +216,15 @@ static int nt36672a_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	/* Send rest of the init cmds */
-	err = nt36672a_send_cmds(panel, pinfo->desc->on_cmds_2,
-				 pinfo->desc->num_on_cmds_2);
+	if (pinfo->desc->num_on_cmds_2 != 0) {
+		/* Send rest of the init cmds if present */
+		err = nt36672a_send_cmds(panel, pinfo->desc->on_cmds_2,
+					pinfo->desc->num_on_cmds_2);
 
-	if (err < 0) {
-		dev_err(panel->dev, "failed to send DCS Init 2nd Code: %d\n", err);
-		goto poweroff;
+		if (err < 0) {
+			dev_err(panel->dev, "failed to send DCS Init 2nd Code: %d\n", err);
+			goto poweroff;
+		}
 	}
 
 	msleep(120);
@@ -601,6 +608,212 @@ static const struct nt36672a_panel_desc tianma_fhd_video_panel_desc = {
 	.num_off_cmds = ARRAY_SIZE(tianma_fhd_video_off_cmds),
 };
 
+static const struct nt36672a_panel_cmd jdi_nt35596s_video_on_cmds[] = {
+	{ .data = { 0xff, 0x24 } },
+	{ .data = { 0x9d, 0x34 } },
+	{ .data = { 0xfb, 0x01 } },
+	{ .data = { 0xc4, 0x25 } },
+	{ .data = { 0xd1, 0x08 } },
+	{ .data = { 0xd2, 0x84 } },
+	{ .data = { 0xff, 0x26 } },
+	{ .data = { 0xfb, 0x01 } },
+	{ .data = { 0x03, 0x1c } },
+	{ .data = { 0x3b, 0x08 } },
+	{ .data = { 0x6b, 0x08 } },
+	{ .data = { 0x97, 0x08 } },
+	{ .data = { 0xc5, 0x08 } },
+	{ .data = { 0xfb, 0x01 } },
+	{ .data = { 0xff, 0x23 } },
+	{ .data = { 0xfb, 0x01 } },
+	{ .data = { 0x01, 0x84 } },
+	{ .data = { 0x05, 0x2d } },
+	{ .data = { 0x06, 0x00 } },
+	{ .data = { 0x33, 0x07 } },
+	{ .data = { 0x21, 0xee } },
+	{ .data = { 0x22, 0xed } },
+	{ .data = { 0x23, 0xea } },
+	{ .data = { 0x24, 0xe8 } },
+	{ .data = { 0x25, 0xe5 } },
+	{ .data = { 0x26, 0xe2 } },
+	{ .data = { 0x27, 0xde } },
+	{ .data = { 0x28, 0xbb } },
+	{ .data = { 0x29, 0x87 } },
+	{ .data = { 0x2a, 0x77 } },
+	{ .data = { 0x32, 0x0c } },
+	{ .data = { 0x13, 0x3f } },
+	{ .data = { 0x14, 0x34 } },
+	{ .data = { 0x15, 0x2a } },
+	{ .data = { 0x16, 0x25 } },
+	{ .data = { 0x17, 0x9d } },
+	{ .data = { 0x18, 0x9a } },
+	{ .data = { 0x19, 0x97 } },
+	{ .data = { 0x1a, 0x94 } },
+	{ .data = { 0x1b, 0x91 } },
+	{ .data = { 0x1c, 0x8e } },
+	{ .data = { 0x1d, 0x8b } },
+	{ .data = { 0x1e, 0x89 } },
+	{ .data = { 0x1f, 0x86 } },
+	{ .data = { 0x20, 0x83 } },
+	{ .data = { 0xff, 0x22 } },
+	{ .data = { 0x00, 0x0a } },
+	{ .data = { 0x01, 0x43 } },
+	{ .data = { 0x02, 0x5b } },
+	{ .data = { 0x03, 0x6a } },
+	{ .data = { 0x04, 0x7a } },
+	{ .data = { 0x05, 0x82 } },
+	{ .data = { 0x06, 0x85 } },
+	{ .data = { 0x07, 0x80 } },
+	{ .data = { 0x08, 0x7c } },
+	{ .data = { 0x09, 0x7c } },
+	{ .data = { 0x0a, 0x74 } },
+	{ .data = { 0x0b, 0x71 } },
+	{ .data = { 0x0c, 0x6e } },
+	{ .data = { 0x0d, 0x68 } },
+	{ .data = { 0x0e, 0x65 } },
+	{ .data = { 0x0f, 0x5c } },
+	{ .data = { 0x10, 0x32 } },
+	{ .data = { 0x11, 0x18 } },
+	{ .data = { 0x12, 0x00 } },
+	{ .data = { 0x13, 0x00 } },
+	{ .data = { 0x1a, 0x00 } },
+	{ .data = { 0x1b, 0x00 } },
+	{ .data = { 0x1c, 0x00 } },
+	{ .data = { 0x1d, 0x00 } },
+	{ .data = { 0x1e, 0x00 } },
+	{ .data = { 0x1f, 0x00 } },
+	{ .data = { 0x20, 0x00 } },
+	{ .data = { 0x21, 0x00 } },
+	{ .data = { 0x22, 0x00 } },
+	{ .data = { 0x23, 0x00 } },
+	{ .data = { 0x24, 0x00 } },
+	{ .data = { 0x25, 0x00 } },
+	{ .data = { 0x26, 0x00 } },
+	{ .data = { 0x27, 0x00 } },
+	{ .data = { 0x28, 0x00 } },
+	{ .data = { 0x29, 0x00 } },
+	{ .data = { 0x2a, 0x00 } },
+	{ .data = { 0x2b, 0x00 } },
+	{ .data = { 0x2f, 0x00 } },
+	{ .data = { 0x30, 0x00 } },
+	{ .data = { 0x31, 0x00 } },
+	{ .data = { 0x32, 0x0c } },
+	{ .data = { 0x33, 0x0c } },
+	{ .data = { 0x34, 0x0c } },
+	{ .data = { 0x35, 0x0b } },
+	{ .data = { 0x36, 0x09 } },
+	{ .data = { 0x37, 0x09 } },
+	{ .data = { 0x38, 0x08 } },
+	{ .data = { 0x39, 0x05 } },
+	{ .data = { 0x3a, 0x03 } },
+	{ .data = { 0x3b, 0x00 } },
+	{ .data = { 0x3f, 0x00 } },
+	{ .data = { 0x40, 0x00 } },
+	{ .data = { 0x41, 0x00 } },
+	{ .data = { 0x42, 0x00 } },
+	{ .data = { 0x43, 0x00 } },
+	{ .data = { 0x44, 0x00 } },
+	{ .data = { 0x45, 0x00 } },
+	{ .data = { 0x46, 0x00 } },
+	{ .data = { 0x47, 0x00 } },
+	{ .data = { 0x48, 0x00 } },
+	{ .data = { 0x49, 0x03 } },
+	{ .data = { 0x4a, 0x06 } },
+	{ .data = { 0x4b, 0x07 } },
+	{ .data = { 0x4c, 0x07 } },
+	{ .data = { 0x53, 0x01 } },
+	{ .data = { 0x54, 0x01 } },
+	{ .data = { 0x55, 0x89 } },
+	{ .data = { 0x56, 0x00 } },
+	{ .data = { 0x58, 0x00 } },
+	{ .data = { 0x68, 0x00 } },
+	{ .data = { 0x84, 0xff } },
+	{ .data = { 0x85, 0xff } },
+	{ .data = { 0x86, 0x03 } },
+	{ .data = { 0x87, 0x00 } },
+	{ .data = { 0x88, 0x00 } },
+	{ .data = { 0xa2, 0x20 } },
+	{ .data = { 0xa9, 0x01 } },
+	{ .data = { 0xaa, 0x12 } },
+	{ .data = { 0xab, 0x13 } },
+	{ .data = { 0xac, 0x0a } },
+	{ .data = { 0xad, 0x74 } },
+	{ .data = { 0xaf, 0x33 } },
+	{ .data = { 0xb0, 0x03 } },
+	{ .data = { 0xb1, 0x14 } },
+	{ .data = { 0xb2, 0x42 } },
+	{ .data = { 0xb3, 0x40 } },
+	{ .data = { 0xb4, 0xa5 } },
+	{ .data = { 0xb6, 0x44 } },
+	{ .data = { 0xb7, 0x04 } },
+	{ .data = { 0xb8, 0x14 } },
+	{ .data = { 0xb9, 0x42 } },
+	{ .data = { 0xba, 0x40 } },
+	{ .data = { 0xbb, 0xa5 } },
+	{ .data = { 0xbd, 0x44 } },
+	{ .data = { 0xbe, 0x04 } },
+	{ .data = { 0xbf, 0x00 } },
+	{ .data = { 0xc0, 0x75 } },
+	{ .data = { 0xc1, 0x6a } },
+	{ .data = { 0xc2, 0xa5 } },
+	{ .data = { 0xc4, 0x22 } },
+	{ .data = { 0xc5, 0x02 } },
+	{ .data = { 0xc6, 0x00 } },
+	{ .data = { 0xc7, 0x95 } },
+	{ .data = { 0xc8, 0x8a } },
+	{ .data = { 0xc9, 0xa5 } },
+	{ .data = { 0xcb, 0x22 } },
+	{ .data = { 0xcc, 0x02 } },
+	{ .data = { 0xcd, 0x00 } },
+	{ .data = { 0xce, 0xb5 } },
+	{ .data = { 0xcf, 0xaa } },
+	{ .data = { 0xd0, 0xa5 } },
+	{ .data = { 0xd2, 0x22 } },
+	{ .data = { 0xd3, 0x02 } },
+	{ .data = { 0xfb, 0x01 } },
+	{ .data = { 0xff, 0x10 } },
+	{ .data = { 0x26, 0x02 } },
+	{ .data = { 0x35, 0x00 } },
+	{ .data = { 0x51, 0xff } },
+	{ .data = { 0x53, 0x24 } },
+	{ .data = { 0x55, 0x00 } },
+	{ .data = { 0xb0, 0x00 } },
+};
+
+static const struct drm_display_mode jdi_nt35596s_video_panel_mode = {
+	.clock = (1080 + 16 + 28 + 40) * (2160 + 7 + 4 + 24) * 60 / 1000,
+
+	.hdisplay = 1080,
+	.hsync_start = 1080 + 16,
+	.hsync_end = 1080 + 16 + 28,
+	.htotal = 1080 + 16 + 28 + 40,
+
+	.vdisplay = 2160,
+	.vsync_start = 2160 + 7,
+	.vsync_end = 2160 + 7 + 4,
+	.vtotal = 2160 + 7 + 4 + 24,
+
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static const struct nt36672a_panel_desc jdi_nt35596s_video_panel_desc = {
+	.display_mode = &jdi_nt35596s_video_panel_mode,
+
+	.width_mm = 68,
+	.height_mm = 136,
+
+	.mode_flags = MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_VIDEO |
+		      MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_CLOCK_NON_CONTINUOUS |
+		      MIPI_DSI_MODE_VIDEO_BURST,
+	.format = MIPI_DSI_FMT_RGB888,
+	.lanes = 4,
+	.on_cmds_1 = jdi_nt35596s_video_on_cmds,
+	.num_on_cmds_1 = ARRAY_SIZE(jdi_nt35596s_video_on_cmds),
+	.on_cmds_2 = NULL,
+	.num_on_cmds_2 = 0,
+	.off_cmds = NULL,
+	.num_off_cmds = 0,
+};
+
 static int nt36672a_panel_add(struct nt36672a_panel *pinfo)
 {
 	struct device *dev = &pinfo->link->dev;
@@ -699,6 +912,7 @@ static void nt36672a_panel_shutdown(struct mipi_dsi_device *dsi)
 
 static const struct of_device_id tianma_fhd_video_of_match[] = {
 	{ .compatible = "tianma,fhd-video", .data = &tianma_fhd_video_panel_desc },
+	{ .compatible = "jdi,fhd-nt35596s", .data = &jdi_nt35596s_video_panel_desc },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, tianma_fhd_video_of_match);
@@ -715,5 +929,6 @@ static struct mipi_dsi_driver nt36672a_panel_driver = {
 module_mipi_dsi_driver(nt36672a_panel_driver);
 
 MODULE_AUTHOR("Sumit Semwal <sumit.semwal@linaro.org>");
-MODULE_DESCRIPTION("NOVATEK NT36672A based MIPI-DSI LCD panel driver");
+MODULE_AUTHOR("Molly Sophia <mollysophia379@gmail.com>");
+MODULE_DESCRIPTION("NOVATEK NT36672A/NT35596S based MIPI-DSI LCD panel driver");
 MODULE_LICENSE("GPL");
-- 
2.40.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver
  2023-07-19 15:20 ` [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver Arnaud Ferraris
@ 2023-07-23 16:45   ` Linus Walleij
  2023-07-25 14:15     ` Arnaud Ferraris
  0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2023-07-23 16:45 UTC (permalink / raw)
  To: Arnaud Ferraris
  Cc: Neil Armstrong, Sam Ravnborg, linux-kernel, dri-devel,
	Molly Sophia, Sumit Semwal

Hi Arnaud & Molly,

overall the driver looks very good!

On Wed, Jul 19, 2023 at 5:20 PM Arnaud Ferraris
<arnaud.ferraris@collabora.com> wrote:

> From: Molly Sophia <mollysophia379@gmail.com>
>
> Novatek NT35596s is a generic DSI IC that drives command and video mode
> panels. Add the driver for it. Currently add support for the LCD panel
> from JDI connected with this IC, as found on Xiaomi Mi Mix2s phones.
>
> Signed-off-by: Molly Sophia <mollysophia379@gmail.com>
> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
(...)

>  .../gpu/drm/panel/panel-novatek-nt36672a.c    | 251 ++++++++++++++++--

So are you sure the nt35596s panel driver is so similar to nt36672a that
they should share the same driver?

With all the magic number sequences I'm not so sure, do they really share
any of the magic numbers?

If not, consider creating a new driver, and then base it on the
nt35510 instead.

> +static const struct nt36672a_panel_cmd jdi_nt35596s_video_on_cmds[] = {
> +       { .data = { 0xff, 0x24 } },
> +       { .data = { 0x9d, 0x34 } },
(...)

These are never nice. Do you have a datasheet so you can provide
defines for the magic hex values?

The construction with these .data seqence array
should be replaces with some open coded sequences
I feel, bit this pattern is already in the driver so I guess it's OK.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver
  2023-07-23 16:45   ` Linus Walleij
@ 2023-07-25 14:15     ` Arnaud Ferraris
  2023-07-25 18:22       ` Linus Walleij
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaud Ferraris @ 2023-07-25 14:15 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Neil Armstrong, Sam Ravnborg, linux-kernel, dri-devel,
	Molly Sophia, Sumit Semwal

Hi Linus,

Le 23/07/2023 à 18:45, Linus Walleij a écrit :
> Hi Arnaud & Molly,
>
> overall the driver looks very good!
>
> On Wed, Jul 19, 2023 at 5:20 PM Arnaud Ferraris
> <arnaud.ferraris@collabora.com>  wrote:
>
>> From: Molly Sophia<mollysophia379@gmail.com>
>>
>> Novatek NT35596s is a generic DSI IC that drives command and video mode
>> panels. Add the driver for it. Currently add support for the LCD panel
>> from JDI connected with this IC, as found on Xiaomi Mi Mix2s phones.
>>
>> Signed-off-by: Molly Sophia<mollysophia379@gmail.com>
>> Signed-off-by: Arnaud Ferraris<arnaud.ferraris@collabora.com>
> (...)
>
>>   .../gpu/drm/panel/panel-novatek-nt36672a.c    | 251 ++++++++++++++++--
> So are you sure the nt35596s panel driver is so similar to nt36672a that
> they should share the same driver?
> With all the magic number sequences I'm not so sure, do they really share
> any of the magic numbers?

Their magic number sequences are quite different, however all the other 
bits (power/reset sequences and timings) fit in, which isn't the case of 
the other novatek panel drivers. Moreover, they have similar (although 
not identical) resolutions.

> If not, consider creating a new driver, and then base it on the
> nt35510 instead.

I would gladly make this a new driver, but I'd rather base it on the 
nt36672a for the reasons mentioned above. I do fear, however, that this 
would lead to unnecessary code duplication, as 90% of the driver (magic 
number sequences excluded) would be identical to the nt36672a (and to be 
fully honest, I don't think I have the needed knowledge to make it 
"better").

>> +static const struct nt36672a_panel_cmd jdi_nt35596s_video_on_cmds[] = {
>> +       { .data = { 0xff, 0x24 } },
>> +       { .data = { 0x9d, 0x34 } },
> (...)
>
> These are never nice. Do you have a datasheet so you can provide
> defines for the magic hex values?

I found an old (2012) datasheet for the NT35596 (without the final "S") 
which is marked "Draft", so I'm really unsure this will be enough to 
make sense of those numbers.

> The construction with these .data seqence array
> should be replaces with some open coded sequences
> I feel, bit this pattern is already in the driver so I guess it's OK.
> Yours,
> Linus Walleij

Cheers,
Arnaud

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver
  2023-07-25 14:15     ` Arnaud Ferraris
@ 2023-07-25 18:22       ` Linus Walleij
  0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2023-07-25 18:22 UTC (permalink / raw)
  To: Arnaud Ferraris
  Cc: Neil Armstrong, Sam Ravnborg, linux-kernel, dri-devel,
	Molly Sophia, Sumit Semwal

On Tue, Jul 25, 2023 at 4:16 PM Arnaud Ferraris
<arnaud.ferraris@collabora.com> wrote:

> Their magic number sequences are quite different, however all the other
> bits (power/reset sequences and timings) fit in, which isn't the case of
> the other novatek panel drivers. Moreover, they have similar (although
> not identical) resolutions.

Hm OK I guess.

> I found an old (2012) datasheet for the NT35596 (without the final "S")
> which is marked "Draft", so I'm really unsure this will be enough to
> make sense of those numbers.

I would try to add the most obvious cases. (Such as "command FF", I
am pretty sure it is clear what that does.)

As I pointed out in some other review, long term we want to support
things such as gamma correction for panels, and then this kind of
information will help greatly, even small pieces.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-07-25 18:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-19 15:20 [PATCH v5 0/2 RESEND] Add driver for Novatek NT35596S panel Arnaud Ferraris
2023-07-19 15:20 ` [PATCH v5 1/2 RESEND] dt-bindings: display: panel: Add Novatek NT35596S panel bindings Arnaud Ferraris
2023-07-19 15:20 ` [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver Arnaud Ferraris
2023-07-23 16:45   ` Linus Walleij
2023-07-25 14:15     ` Arnaud Ferraris
2023-07-25 18:22       ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).