public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels
@ 2026-05-14 17:24 Aaron Kling via B4 Relay
  2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-05-14 17:24 UTC (permalink / raw)
  To: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: dri-devel, devicetree, linux-kernel, Aaron Kling, Teguh Sobirin

This driver is based on the one by Teguh Sobirin [0].
    
Due to [1], the AYN vendor description patch has been folded into the
AYN QCS8550 dt series. Which means this series depends on said series
and it must be picked up before this.

[0] https://github.com/AYNTechnologies/linux/commit/4c5e76e974db7cca853619ca138eecd8f004622f
[1] https://lore.kernel.org/linux-arm-msm/c7fb3f89-6574-4761-9ef2-2fdf6d4801b5@kernel.org

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
Changes in v2:
- Fix lint warning in patch 1
- Add ayaneo,pocketds-panel-top compatible to patches 1 and 2, it uses the
  same init sequence as the odin 2 portal panel.
- Link to v1: https://lore.kernel.org/r/20260509-icna35xx-v1-0-688d3d4e10f9@gmail.com

---
Aaron Kling (1):
      dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings

Teguh Sobirin (1):
      drm/panel: Add panel driver for Chipone ICNA35XX based panels

 .../bindings/display/panel/chipone,icna35xx.yaml   |  79 +++
 drivers/gpu/drm/panel/Kconfig                      |  11 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-chipone-icna35xx.c     | 619 +++++++++++++++++++++
 4 files changed, 710 insertions(+)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260220-icna35xx-ec9afa13e1aa
prerequisite-change-id: 20260217-ayn-qcs8550-16c07b63de26:v8
prerequisite-patch-id: 2b010637c46f5ea0e19a2fa87cc5ccb19bfd9204
prerequisite-patch-id: d0c633dc17f0aba726a8e8d21fee52b944bf67ff
prerequisite-patch-id: 11f4208bd788df984cec7404088c7d071ec49dfc
prerequisite-patch-id: 76b61799dba520fc7fa3ff39b044e24103337af3
prerequisite-patch-id: 3844bef2eda3cf59031b1d131eb6ba9295629bb4
prerequisite-patch-id: 3331648bc6f4ac3bb156f0525aa1ede92bfc57f1

Best regards,
-- 
Aaron Kling <webgeek1234@gmail.com>



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

* [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 17:24 [PATCH v2 0/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
@ 2026-05-14 17:24 ` Aaron Kling via B4 Relay
  2026-05-14 18:06   ` Conor Dooley
  2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
  2026-05-14 17:24 ` [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
  2026-05-16  0:36 ` Claude Code Review Bot
  2 siblings, 2 replies; 15+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-05-14 17:24 UTC (permalink / raw)
  To: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: dri-devel, devicetree, linux-kernel, Aaron Kling

From: Aaron Kling <webgeek1234@gmail.com>

The Chipone ICNA35xx series of DDICs are high refresh, low power
MIPI-DSI drivers for OLED panels. The icna3512 is used by the Ayn Odin 2
Portal and the icna3520 is used by the Ayn Thor top panel and the Ayn
Odin 3.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 .../bindings/display/panel/chipone,icna35xx.yaml   | 79 ++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
new file mode 100644
index 00000000000000..6464d8d0cf86d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/chipone,icna35xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Chipone ICNA35xx display drivers
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+
+description:
+  The Chipone ICNA35xx are a series of DDICs connected
+  using a MIPI-DSI video interface.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - ayaneo,pocketds-panel-top
+              - ayntec,odin2portal-panel
+          - const: chipone,icna3512
+
+      - items:
+          - enum:
+              - ayntec,odin3-panel
+              - ayntec,thor-panel-top
+          - const: chipone,icna3520
+
+  reg:
+    maxItems: 1
+    description: DSI virtual channel
+
+  vdd-supply: true
+  vddio-supply: true
+  vci-supply: true
+  disp-supply: true
+  blvdd-supply: true
+
+  port: true
+  reset-gpios: true
+  rotation: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+  - vci-supply
+  - disp-supply
+  - blvdd-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "ayntec,odin2portal-panel", "chipone,icna3512";
+            reg = <0>;
+            vdd-supply = <&vreg_l11b_1p2>;
+            vddio-supply = <&vreg_l12b_1p8>;
+            vci-supply = <&vreg_l13b_3p0>;
+            disp-supply = <&vdd_disp_2v8>;
+            blvdd-supply = <&vdd_bl_5v0>;
+            reset-gpios = <&tlmm 133 GPIO_ACTIVE_LOW>;
+        };
+    };
+
+...

-- 
2.53.0



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

* [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 17:24 [PATCH v2 0/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
  2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
@ 2026-05-14 17:24 ` Aaron Kling via B4 Relay
  2026-05-14 18:05   ` Conor Dooley
  2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
  2026-05-16  0:36 ` Claude Code Review Bot
  2 siblings, 2 replies; 15+ messages in thread
From: Aaron Kling via B4 Relay @ 2026-05-14 17:24 UTC (permalink / raw)
  To: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: dri-devel, devicetree, linux-kernel, Aaron Kling, Teguh Sobirin

From: Teguh Sobirin <teguh@sobir.in>

This adds support for the ICNA3512 and ICNA3520 DDICs used in the AYN
Odin 2 Portal and for both the AYN Odin 3 and the AYN Thor top panel
respectively.

Signed-off-by: Teguh Sobirin <teguh@sobir.in>
Co-developed-by: Aaron Kling <webgeek1234@gmail.com>
Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 drivers/gpu/drm/panel/Kconfig                  |  11 +
 drivers/gpu/drm/panel/Makefile                 |   1 +
 drivers/gpu/drm/panel/panel-chipone-icna35xx.c | 619 +++++++++++++++++++++++++
 3 files changed, 631 insertions(+)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 7450b27622a233..1368b5a0b6c912 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -105,6 +105,17 @@ config DRM_PANEL_BOE_TV101WUM_LL2
 	  Say Y here if you want to support for BOE TV101WUM-LL2
 	  WUXGA PANEL DSI Video Mode panel
 
+config DRM_PANEL_CHIPONE_ICNA35XX
+	tristate "Chipone ICNA35XX panel driver"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_DISPLAY_HELPER
+	help
+	  Say Y here if you want to enable support for the panels built
+	  around the Chipone ICNA3512 and ICNA3520 display controllers,
+	  such as some Tianma panels used in AYN Odin2 Portal and Thor.
+
 config DRM_PANEL_CHIPWEALTH_CH13726A
 	tristate "CHIPWEALTH CH13726A-based DSI panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index c2c5cf81711633..d39a8f82fa8c06 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_BOE_TD4320) += panel-boe-td4320.o
 obj-$(CONFIG_DRM_PANEL_BOE_TH101MB31UIG002_28A) += panel-boe-th101mb31ig002-28a.o
 obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_LL2) += panel-boe-tv101wum-ll2.o
 obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_NL6) += panel-boe-tv101wum-nl6.o
+obj-$(CONFIG_DRM_PANEL_CHIPONE_ICNA35XX) += panel-chipone-icna35xx.o
 obj-$(CONFIG_DRM_PANEL_CHIPWEALTH_CH13726A) += panel-chipwealth-ch13726a.o
 obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o
 obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
new file mode 100644
index 00000000000000..958f205a7f4f93
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
@@ -0,0 +1,619 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Chipone ICNA35XX Driver IC panels driver
+ *
+ * Copyright (c) 2025 Teguh Sobirin <teguh@sobir.in>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/display/drm_dsc.h>
+#include <drm/display/drm_dsc_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+struct panel_info {
+	struct drm_panel panel;
+	struct drm_connector *connector;
+	struct mipi_dsi_device *dsi;
+	struct panel_desc *desc;
+	enum drm_panel_orientation orientation;
+
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data *supplies;
+};
+
+struct panel_desc {
+	unsigned int width_mm;
+	unsigned int height_mm;
+
+	unsigned int bpc;
+	unsigned int lanes;
+	unsigned long mode_flags;
+	enum mipi_dsi_pixel_format format;
+
+	const struct drm_display_mode *modes;
+	unsigned int num_modes;
+	int (*init_sequence)(struct panel_info *pinfo);
+
+	struct drm_dsc_config dsc;
+};
+
+static const struct regulator_bulk_data panel_supplies[] = {
+	{ .supply = "vdd" },
+	{ .supply = "vddio" },
+	{ .supply = "vci" },
+	{ .supply = "disp" },
+	{ .supply = "blvdd" },
+};
+
+static inline struct panel_info *to_panel_info(struct drm_panel *panel)
+{
+	return container_of(panel, struct panel_info, panel);
+}
+
+static int icna35xx_get_current_mode(struct panel_info *pinfo)
+{
+	struct drm_connector *connector = pinfo->connector;
+	struct drm_crtc_state *crtc_state;
+	int i;
+
+	/* Return the default (first) mode if no info available yet */
+	if (!connector->state || !connector->state->crtc)
+		return 0;
+
+	crtc_state = connector->state->crtc->state;
+
+	for (i = 0; i < pinfo->desc->num_modes; i++) {
+		if (drm_mode_match(&crtc_state->mode,
+				   &pinfo->desc->modes[i],
+				   DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_CLOCK))
+			return i;
+	}
+
+	return 0;
+}
+
+static int icna3512_init_sequence(struct panel_info *pinfo)
+{
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi };
+	struct drm_dsc_picture_parameter_set pps;
+
+	int cur_mode = icna35xx_get_current_mode(pinfo);
+	int cur_vrefresh = drm_mode_vrefresh(&pinfo->desc->modes[cur_mode]);
+
+	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9C, 0xA5, 0xA5);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0x5A, 0x5A);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x53, 0xE0);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x35, 0x00);
+
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+
+	mipi_dsi_msleep(&dsi_ctx, 120);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x0F);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xCE, 0x22);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	if (cur_vrefresh == 165) {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x20);
+	} else if (cur_vrefresh == 144) {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xE0, 0xA0, 0x10, 0xC8, 0x00, 0x02, 0x83,
+			0x00, 0x10, 0x14, 0x00, 0x00, 0xC3, 0x00, 0x10,
+			0x14, 0x00, 0x00, 0xE0, 0x00, 0x10, 0x14, 0x00,
+			0x00, 0xE0, 0xA0, 0x10, 0xC8, 0x22, 0x18, 0x18,
+			0x18, 0x18, 0x18);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x07);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB5,
+			0x04, 0x0A, 0x08, 0x0A, 0x04, 0x00, 0xC4);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xD9,
+			0x66, 0xE4, 0xE4, 0x66, 0xE4, 0xE4, 0x00, 0xC4,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xCE,
+			0x01, 0x01, 0x01, 0x01, 0x04, 0x07, 0xA4);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x30);
+	} else if (cur_vrefresh == 120) {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xE0, 0xA0, 0x10, 0xC8, 0x00, 0x02, 0x83,
+			0x00, 0x10, 0x14, 0x00, 0x00, 0xC3, 0x00, 0x10,
+			0x14, 0x00, 0x00, 0xE0, 0x10, 0x10, 0x9C, 0x00,
+			0x00, 0xE0, 0xA0, 0x10, 0xC8, 0x22, 0x18, 0x18,
+			0x18, 0x18, 0x18);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x07);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB5,
+			0x04, 0x0C, 0x08, 0x0C, 0x04, 0x00, 0xC4);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xD9,
+			0x88, 0x40, 0x40, 0x88, 0x40, 0x40, 0x00, 0xEB,
+			0x11, 0xFF);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xCE,
+			0x01, 0x01, 0x01, 0x01, 0x04, 0x09, 0x2C);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x30);
+	} else if (cur_vrefresh == 90) {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xE0, 0x40, 0x10, 0xA8, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x07);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB2,
+			0x04, 0x10, 0x08, 0x0C, 0x04, 0x00, 0xC4);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xD3,
+			0x55, 0x80, 0x80, 0x55, 0x80, 0xB0, 0x00, 0x9C,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xCB,
+			0x01, 0x01, 0x01, 0x01, 0x04, 0x06, 0x1C);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x00);
+	} else {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xE0, 0xA0, 0x10, 0xC8, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x07);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB2,
+			0x04, 0x18, 0x08, 0x0C, 0x02, 0x00, 0xC4);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xD3,
+			0x88, 0x4A, 0x4A, 0x88, 0x4A, 0x4A, 0x00, 0xEB,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xCB,
+			0x01, 0x01, 0x01, 0x01, 0x04, 0x2C);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x00);
+	}
+
+	drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc);
+	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+
+	mipi_dsi_msleep(&dsi_ctx, 20);
+
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+
+	return dsi_ctx.accum_err;
+}
+
+static int icna3520_init_sequence(struct panel_info *pinfo)
+{
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi };
+	struct drm_dsc_picture_parameter_set pps;
+
+	int cur_mode = icna35xx_get_current_mode(pinfo);
+	int cur_vrefresh = drm_mode_vrefresh(&pinfo->desc->modes[cur_mode]);
+
+	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9C, 0xA5, 0xA5);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0x5A, 0x5A);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x53, 0xE0);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x35, 0x00);
+
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+
+	mipi_dsi_msleep(&dsi_ctx, 120);
+
+	if (cur_vrefresh == 120) {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xD8, 0x00, 0x1C, 0x00, 0x4C);
+	} else {
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x10);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x00);
+		mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB3,
+			0x00, 0xDB, 0x00, 0x1C, 0x00, 0x1C, 0x00, 0x00,
+			0xDB, 0x00, 0x1C, 0x07, 0xD6, 0x00);
+	}
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB2, 0x00);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x0D);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB2, 0x27);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB6, 0x03);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xBB, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xB2, 0x24);
+
+	drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc);
+	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+
+	mipi_dsi_msleep(&dsi_ctx, 20);
+
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+
+	return dsi_ctx.accum_err;
+}
+
+static const struct drm_display_mode odin2portal_modes[] = {
+	{
+		/* 165Hz */
+		.clock = (1080 + 98 + 1 + 23) * (1920 + 20 + 1 + 15) * 165 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 98,
+		.hsync_end = 1080 + 98 + 1,
+		.htotal = 1080 + 98 + 1 + 23,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 20,
+		.vsync_end = 1920 + 20 + 1,
+		.vtotal = 1920 + 20 + 1 + 15,
+	},
+	{
+		/* 144Hz */
+		.clock = (1080 + 156 + 1 + 23) * (1920 + 20 + 1 + 15) * 144 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 156,
+		.hsync_end = 1080 + 156 + 1,
+		.htotal = 1080 + 156 + 1 + 23,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 20,
+		.vsync_end = 1920 + 20 + 1,
+		.vtotal = 1920 + 20 + 1 + 15,
+	},
+	{
+		/* 120Hz */
+		.clock = (1080 + 156 + 1 + 23) * (1920 + 412 + 1 + 15) * 120 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 156,
+		.hsync_end = 1080 + 156 + 1,
+		.htotal = 1080 + 156 + 1 + 23,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 412,
+		.vsync_end = 1920 + 412 + 1,
+		.vtotal = 1920 + 412 + 1 + 15,
+	},
+	{
+		/* 90Hz */
+		.clock = (1080 + 156 + 1 + 23) * (1920 + 1192 + 1 + 15) * 90 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 156,
+		.hsync_end = 1080 + 156 + 1,
+		.htotal = 1080 + 156 + 1 + 23,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 1192,
+		.vsync_end = 1920 + 1192 + 1,
+		.vtotal = 1920 + 1192 + 1 + 15,
+	},
+	{
+		/* 60Hz */
+		.clock = (1080 + 156 + 1 + 23) * (1920 + 2760 + 1 + 15) * 60 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 156,
+		.hsync_end = 1080 + 156 + 1,
+		.htotal = 1080 + 156 + 1 + 23,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 2760,
+		.vsync_end = 1920 + 2760 + 1,
+		.vtotal = 1920 + 2760 + 1 + 15,
+	}
+};
+
+static const struct drm_display_mode thor_top_modes[] = {
+	{
+		/* 120Hz */
+		.clock = (1080 + 24 + 1 + 24) * (1920 + 28 + 1 + 28) * 120 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 24,
+		.hsync_end = 1080 + 24 + 1,
+		.htotal = 1080 + 24 + 1 + 24,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 28,
+		.vsync_end = 1920 + 28 + 1,
+		.vtotal = 1920 + 28 + 1 + 28,
+	},
+	{
+		/* 60Hz */
+		.clock = (1080 + 24 + 1 + 24) * (1920 + 2006 + 1 + 28) * 60 / 1000,
+		.hdisplay = 1080,
+		.hsync_start = 1080 + 24,
+		.hsync_end = 1080 + 24 + 1,
+		.htotal = 1080 + 24 + 1 + 24,
+		.vdisplay = 1920,
+		.vsync_start = 1920 + 2006,
+		.vsync_end = 1920 + 2006 + 1,
+		.vtotal = 1920 + 2006 + 1 + 28,
+	}
+};
+
+static struct panel_desc odin2portal_desc = {
+	.modes = odin2portal_modes,
+	.num_modes = ARRAY_SIZE(odin2portal_modes),
+	.width_mm = 160,
+	.height_mm = 89,
+	.bpc = 8,
+	.lanes = 4,
+	.format = MIPI_DSI_FMT_RGB888,
+	.mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS |
+			MIPI_DSI_MODE_LPM,
+	.init_sequence = icna3512_init_sequence,
+	.dsc = {
+		.dsc_version_major = 0x1,
+		.dsc_version_minor = 0x1,
+		.slice_height = 20,
+		.slice_width = 540,
+		.slice_count = 2,
+		.bits_per_component = 8,
+		.bits_per_pixel = 8 << 4,
+		.block_pred_enable = true,
+	},
+};
+
+static struct panel_desc thor_top_desc = {
+	.modes = thor_top_modes,
+	.num_modes = ARRAY_SIZE(thor_top_modes),
+	.width_mm = 136,
+	.height_mm = 68,
+	.bpc = 8,
+	.lanes = 4,
+	.format = MIPI_DSI_FMT_RGB888,
+	.mode_flags =  MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS |
+			MIPI_DSI_MODE_LPM,
+	.init_sequence = icna3520_init_sequence,
+	.dsc = {
+		.dsc_version_major = 0x1,
+		.dsc_version_minor = 0x1,
+		.slice_height = 12,
+		.slice_width = 540,
+		.slice_count = 2,
+		.bits_per_component = 8,
+		.bits_per_pixel = 8 << 4,
+		.block_pred_enable = true,
+	},
+};
+
+static void icna35xx_reset(struct panel_info *pinfo)
+{
+	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
+	usleep_range(20000, 21000);
+	gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
+	usleep_range(20000, 21000);
+	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
+	usleep_range(20000, 21000);
+}
+
+static int icna35xx_prepare(struct drm_panel *panel)
+{
+	struct panel_info *pinfo = to_panel_info(panel);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(panel_supplies), pinfo->supplies);
+	if (ret < 0) {
+		dev_err(panel->dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	icna35xx_reset(pinfo);
+
+	ret = pinfo->desc->init_sequence(pinfo);
+	if (ret < 0) {
+		regulator_bulk_disable(ARRAY_SIZE(panel_supplies), pinfo->supplies);
+		dev_err(panel->dev, "failed to initialize panel: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int icna35xx_disable(struct drm_panel *panel)
+{
+	struct panel_info *pinfo = to_panel_info(panel);
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi };
+
+	pinfo->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 50);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 120);
+
+	return dsi_ctx.accum_err;
+}
+
+static int icna35xx_unprepare(struct drm_panel *panel)
+{
+	struct panel_info *pinfo = to_panel_info(panel);
+
+	gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(panel_supplies), pinfo->supplies);
+
+	return 0;
+}
+
+static void icna35xx_remove(struct mipi_dsi_device *dsi)
+{
+	struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = mipi_dsi_detach(pinfo->dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
+
+	drm_panel_remove(&pinfo->panel);
+}
+
+static int icna35xx_get_modes(struct drm_panel *panel,
+			       struct drm_connector *connector)
+{
+	struct panel_info *pinfo = to_panel_info(panel);
+	int i;
+
+	for (i = 0; i < pinfo->desc->num_modes; i++) {
+		const struct drm_display_mode *m = &pinfo->desc->modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(connector->dev, m);
+		if (!mode) {
+			dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
+			return -ENOMEM;
+		}
+
+		mode->type = DRM_MODE_TYPE_DRIVER;
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+		drm_mode_probed_add(connector, mode);
+	}
+
+	connector->display_info.width_mm = pinfo->desc->width_mm;
+	connector->display_info.height_mm = pinfo->desc->height_mm;
+	connector->display_info.bpc = pinfo->desc->bpc;
+	pinfo->connector = connector;
+
+	return pinfo->desc->num_modes;
+}
+
+static enum drm_panel_orientation icna35xx_get_orientation(struct drm_panel *panel)
+{
+	struct panel_info *pinfo = to_panel_info(panel);
+
+	return pinfo->orientation;
+}
+
+static const struct drm_panel_funcs icna35xx_panel_funcs = {
+	.disable = icna35xx_disable,
+	.prepare = icna35xx_prepare,
+	.unprepare = icna35xx_unprepare,
+	.get_modes = icna35xx_get_modes,
+	.get_orientation = icna35xx_get_orientation,
+};
+
+static int icna35xx_bl_update_status(struct backlight_device *bl)
+{
+	struct mipi_dsi_device *dsi = bl_get_data(bl);
+	u16 brightness = backlight_get_brightness(bl);
+	int ret;
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
+	if (ret < 0)
+		return ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	return 0;
+}
+
+static int icna35xx_bl_get_brightness(struct backlight_device *bl)
+{
+	struct mipi_dsi_device *dsi = bl_get_data(bl);
+	u16 brightness;
+	int ret;
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
+	if (ret < 0)
+		return ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	return brightness;
+}
+
+static const struct backlight_ops icna35xx_bl_ops = {
+	.update_status = icna35xx_bl_update_status,
+	.get_brightness = icna35xx_bl_get_brightness,
+};
+
+static struct backlight_device *icna35xx_create_backlight(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	const struct backlight_properties props = {
+		.type = BACKLIGHT_RAW,
+		.brightness = 4096,
+		.max_brightness = 4096,
+	};
+
+	return devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
+					      &icna35xx_bl_ops, &props);
+}
+
+static int icna35xx_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct panel_info *pinfo;
+	int ret;
+
+	pinfo = devm_drm_panel_alloc(dev, __typeof(*pinfo), panel,
+				     &icna35xx_panel_funcs,
+				     DRM_MODE_CONNECTOR_DSI);
+	if (IS_ERR(pinfo))
+		return PTR_ERR(pinfo);
+
+	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(panel_supplies),
+	panel_supplies, &pinfo->supplies);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+	pinfo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(pinfo->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(pinfo->reset_gpio), "failed to get reset gpio\n");
+
+	pinfo->desc = (struct panel_desc *)of_device_get_match_data(dev);
+	if (!pinfo->desc)
+		return -ENODEV;
+
+	pinfo->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, pinfo);
+
+	ret = of_drm_get_panel_orientation(dev->of_node, &pinfo->orientation);
+	if (ret < 0) {
+		dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, ret);
+		return ret;
+	}
+
+	pinfo->panel.prepare_prev_first = true;
+
+	pinfo->panel.backlight = icna35xx_create_backlight(dsi);
+	if (IS_ERR(pinfo->panel.backlight))
+		return dev_err_probe(dev, PTR_ERR(pinfo->panel.backlight),
+				     "Failed to create backlight\n");
+
+	drm_panel_add(&pinfo->panel);
+
+	pinfo->dsi->lanes = pinfo->desc->lanes;
+	pinfo->dsi->format = pinfo->desc->format;
+	pinfo->dsi->mode_flags = pinfo->desc->mode_flags;
+	pinfo->dsi->dsc = &pinfo->desc->dsc;
+
+	ret = mipi_dsi_attach(pinfo->dsi);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to attach to DSI host\n");
+		drm_panel_remove(&pinfo->panel);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id icna35xx_of_match[] = {
+	{ .compatible = "ayaneo,pocketds-panel-top", .data = &odin2portal_desc },
+	{ .compatible = "ayntec,odin2portal-panel", .data = &odin2portal_desc },
+	{ .compatible = "ayntec,odin3-panel", .data = &thor_top_desc },
+	{ .compatible = "ayntec,thor-panel-top", .data = &thor_top_desc },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, icna35xx_of_match);
+
+static struct mipi_dsi_driver icna35xx_driver = {
+	.probe = icna35xx_probe,
+	.remove = icna35xx_remove,
+	.driver = {
+		.name = "panel-chipone-icna35xx",
+		.of_match_table = icna35xx_of_match,
+	},
+};
+module_mipi_dsi_driver(icna35xx_driver);
+
+MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");
+MODULE_DESCRIPTION("DRM driver for Chipone ICNA35XX based MIPI DSI panels");
+MODULE_LICENSE("GPL");

-- 
2.53.0



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

* Re: [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 17:24 ` [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
@ 2026-05-14 18:05   ` Conor Dooley
  2026-05-14 18:24     ` Aaron Kling
  2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
  1 sibling, 1 reply; 15+ messages in thread
From: Conor Dooley @ 2026-05-14 18:05 UTC (permalink / raw)
  To: webgeek1234
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel, Teguh Sobirin

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

On Thu, May 14, 2026 at 12:24:50PM -0500, Aaron Kling via B4 Relay wrote:

> +static const struct of_device_id icna35xx_of_match[] = {
> +	{ .compatible = "ayaneo,pocketds-panel-top", .data = &odin2portal_desc },
> +	{ .compatible = "ayntec,odin2portal-panel", .data = &odin2portal_desc },
> +	{ .compatible = "ayntec,odin3-panel", .data = &thor_top_desc },
> +	{ .compatible = "ayntec,thor-panel-top", .data = &thor_top_desc },

This looks like you could be using the fallback compatibles instead of
matching on the panel's. Why aren't you?

> +	{ /* sentinel */ }

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

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

* Re: [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
@ 2026-05-14 18:06   ` Conor Dooley
  2026-05-14 18:20     ` Aaron Kling
  2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
  1 sibling, 1 reply; 15+ messages in thread
From: Conor Dooley @ 2026-05-14 18:06 UTC (permalink / raw)
  To: webgeek1234
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

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

On Thu, May 14, 2026 at 12:24:49PM -0500, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.com>
> 
> The Chipone ICNA35xx series of DDICs are high refresh, low power
> MIPI-DSI drivers for OLED panels. The icna3512 is used by the Ayn Odin 2
> Portal and the icna3520 is used by the Ayn Thor top panel and the Ayn
> Odin 3.
> 
> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>  .../bindings/display/panel/chipone,icna35xx.yaml   | 79 ++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> new file mode 100644
> index 00000000000000..6464d8d0cf86d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml

Filename matching a compatible please.

> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/chipone,icna35xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Chipone ICNA35xx display drivers
> +
> +maintainers:
> +  - Neil Armstrong <neil.armstrong@linaro.org>
> +
> +description:
> +  The Chipone ICNA35xx are a series of DDICs connected
> +  using a MIPI-DSI video interface.
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ayaneo,pocketds-panel-top
> +              - ayntec,odin2portal-panel
> +          - const: chipone,icna3512
> +
> +      - items:
> +          - enum:
> +              - ayntec,odin3-panel
> +              - ayntec,thor-panel-top
> +          - const: chipone,icna3520

Commit message should state why these ddics are not compatible with one
another.

Rest of this seems okay.

pw-bot: changes-requested

Thanks,
Conor.

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

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

* Re: [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 18:06   ` Conor Dooley
@ 2026-05-14 18:20     ` Aaron Kling
  2026-05-14 18:23       ` Conor Dooley
  0 siblings, 1 reply; 15+ messages in thread
From: Aaron Kling @ 2026-05-14 18:20 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

On Thu, May 14, 2026 at 1:07 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, May 14, 2026 at 12:24:49PM -0500, Aaron Kling via B4 Relay wrote:
> > From: Aaron Kling <webgeek1234@gmail.com>
> >
> > The Chipone ICNA35xx series of DDICs are high refresh, low power
> > MIPI-DSI drivers for OLED panels. The icna3512 is used by the Ayn Odin 2
> > Portal and the icna3520 is used by the Ayn Thor top panel and the Ayn
> > Odin 3.
> >
> > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > ---
> >  .../bindings/display/panel/chipone,icna35xx.yaml   | 79 ++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> > new file mode 100644
> > index 00000000000000..6464d8d0cf86d5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
>
> Filename matching a compatible please.

Does this require splitting / duplicating the files for each ddic, or
can one be arbitrarily picked and the other stay with it?

> > @@ -0,0 +1,79 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/panel/chipone,icna35xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Chipone ICNA35xx display drivers
> > +
> > +maintainers:
> > +  - Neil Armstrong <neil.armstrong@linaro.org>
> > +
> > +description:
> > +  The Chipone ICNA35xx are a series of DDICs connected
> > +  using a MIPI-DSI video interface.
> > +
> > +allOf:
> > +  - $ref: panel-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - ayaneo,pocketds-panel-top
> > +              - ayntec,odin2portal-panel
> > +          - const: chipone,icna3512
> > +
> > +      - items:
> > +          - enum:
> > +              - ayntec,odin3-panel
> > +              - ayntec,thor-panel-top
> > +          - const: chipone,icna3520
>
> Commit message should state why these ddics are not compatible with one
> another.

Ack. Short version: some of the mipi vendor commands don't match.

> Rest of this seems okay.
>
> pw-bot: changes-requested
>
> Thanks,
> Conor.

Aaron

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

* Re: [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 18:20     ` Aaron Kling
@ 2026-05-14 18:23       ` Conor Dooley
  2026-05-14 18:27         ` Aaron Kling
  0 siblings, 1 reply; 15+ messages in thread
From: Conor Dooley @ 2026-05-14 18:23 UTC (permalink / raw)
  To: Aaron Kling
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

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

On Thu, May 14, 2026 at 01:20:57PM -0500, Aaron Kling wrote:
> On Thu, May 14, 2026 at 1:07 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, May 14, 2026 at 12:24:49PM -0500, Aaron Kling via B4 Relay wrote:
> > > From: Aaron Kling <webgeek1234@gmail.com>
> > >
> > > The Chipone ICNA35xx series of DDICs are high refresh, low power
> > > MIPI-DSI drivers for OLED panels. The icna3512 is used by the Ayn Odin 2
> > > Portal and the icna3520 is used by the Ayn Thor top panel and the Ayn
> > > Odin 3.
> > >
> > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > ---
> > >  .../bindings/display/panel/chipone,icna35xx.yaml   | 79 ++++++++++++++++++++++
> > >  1 file changed, 79 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> > > new file mode 100644
> > > index 00000000000000..6464d8d0cf86d5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> >
> > Filename matching a compatible please.
> 
> Does this require splitting / duplicating the files for each ddic, or
> can one be arbitrarily picked and the other stay with it?

Pick an arbitrary one. Maybe which one is older?

> > > @@ -0,0 +1,79 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/panel/chipone,icna35xx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Chipone ICNA35xx display drivers
> > > +
> > > +maintainers:
> > > +  - Neil Armstrong <neil.armstrong@linaro.org>
> > > +
> > > +description:
> > > +  The Chipone ICNA35xx are a series of DDICs connected
> > > +  using a MIPI-DSI video interface.
> > > +
> > > +allOf:
> > > +  - $ref: panel-common.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - ayaneo,pocketds-panel-top
> > > +              - ayntec,odin2portal-panel
> > > +          - const: chipone,icna3512
> > > +
> > > +      - items:
> > > +          - enum:
> > > +              - ayntec,odin3-panel
> > > +              - ayntec,thor-panel-top
> > > +          - const: chipone,icna3520
> >
> > Commit message should state why these ddics are not compatible with one
> > another.
> 
> Ack. Short version: some of the mipi vendor commands don't match.
> 
> > Rest of this seems okay.
> >
> > pw-bot: changes-requested
> >
> > Thanks,
> > Conor.
> 
> Aaron

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

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

* Re: [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 18:05   ` Conor Dooley
@ 2026-05-14 18:24     ` Aaron Kling
  2026-05-14 18:53       ` Conor Dooley
  0 siblings, 1 reply; 15+ messages in thread
From: Aaron Kling @ 2026-05-14 18:24 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel, Teguh Sobirin

On Thu, May 14, 2026 at 1:05 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, May 14, 2026 at 12:24:50PM -0500, Aaron Kling via B4 Relay wrote:
>
> > +static const struct of_device_id icna35xx_of_match[] = {
> > +     { .compatible = "ayaneo,pocketds-panel-top", .data = &odin2portal_desc },
> > +     { .compatible = "ayntec,odin2portal-panel", .data = &odin2portal_desc },
> > +     { .compatible = "ayntec,odin3-panel", .data = &thor_top_desc },
> > +     { .compatible = "ayntec,thor-panel-top", .data = &thor_top_desc },
>
> This looks like you could be using the fallback compatibles instead of
> matching on the panel's. Why aren't you?

I'm uncertain if the panels are actually the same between these
devices or if the init sequences are just similar enough to share. I
am told that the thor top panel and the odin 3 panel are the same
model. But I don't know about the portal and the pocket ds, just know
that they both fire up and work as expected with the same init
sequence. I'm trying to follow the bindings idea that these might be
different physical panels that only share a ddic and thus driver
handling.

> > +     { /* sentinel */ }

Aaron

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

* Re: [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 18:23       ` Conor Dooley
@ 2026-05-14 18:27         ` Aaron Kling
  0 siblings, 0 replies; 15+ messages in thread
From: Aaron Kling @ 2026-05-14 18:27 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

On Thu, May 14, 2026 at 1:23 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, May 14, 2026 at 01:20:57PM -0500, Aaron Kling wrote:
> > On Thu, May 14, 2026 at 1:07 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Thu, May 14, 2026 at 12:24:49PM -0500, Aaron Kling via B4 Relay wrote:
> > > > From: Aaron Kling <webgeek1234@gmail.com>
> > > >
> > > > The Chipone ICNA35xx series of DDICs are high refresh, low power
> > > > MIPI-DSI drivers for OLED panels. The icna3512 is used by the Ayn Odin 2
> > > > Portal and the icna3520 is used by the Ayn Thor top panel and the Ayn
> > > > Odin 3.
> > > >
> > > > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > > ---
> > > >  .../bindings/display/panel/chipone,icna35xx.yaml   | 79 ++++++++++++++++++++++
> > > >  1 file changed, 79 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> > > > new file mode 100644
> > > > index 00000000000000..6464d8d0cf86d5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/display/panel/chipone,icna35xx.yaml
> > >
> > > Filename matching a compatible please.
> >
> > Does this require splitting / duplicating the files for each ddic, or
> > can one be arbitrarily picked and the other stay with it?
>
> Pick an arbitrary one. Maybe which one is older?

Alright. I'm going to assume, perhaps unsafely, that the lower number is older.
>
> > > > @@ -0,0 +1,79 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/display/panel/chipone,icna35xx.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Chipone ICNA35xx display drivers
> > > > +
> > > > +maintainers:
> > > > +  - Neil Armstrong <neil.armstrong@linaro.org>
> > > > +
> > > > +description:
> > > > +  The Chipone ICNA35xx are a series of DDICs connected
> > > > +  using a MIPI-DSI video interface.
> > > > +
> > > > +allOf:
> > > > +  - $ref: panel-common.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - enum:
> > > > +              - ayaneo,pocketds-panel-top
> > > > +              - ayntec,odin2portal-panel
> > > > +          - const: chipone,icna3512
> > > > +
> > > > +      - items:
> > > > +          - enum:
> > > > +              - ayntec,odin3-panel
> > > > +              - ayntec,thor-panel-top
> > > > +          - const: chipone,icna3520
> > >
> > > Commit message should state why these ddics are not compatible with one
> > > another.
> >
> > Ack. Short version: some of the mipi vendor commands don't match.
> >
> > > Rest of this seems okay.
> > >
> > > pw-bot: changes-requested
> > >
> > > Thanks,
> > > Conor.
> >
> > Aaron

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

* Re: [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 18:24     ` Aaron Kling
@ 2026-05-14 18:53       ` Conor Dooley
  0 siblings, 0 replies; 15+ messages in thread
From: Conor Dooley @ 2026-05-14 18:53 UTC (permalink / raw)
  To: Aaron Kling
  Cc: Neil Armstrong, Jessica Zhang, David Airlie, Simona Vetter,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel, Teguh Sobirin

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

On Thu, May 14, 2026 at 01:24:19PM -0500, Aaron Kling wrote:
> On Thu, May 14, 2026 at 1:05 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, May 14, 2026 at 12:24:50PM -0500, Aaron Kling via B4 Relay wrote:
> >
> > > +static const struct of_device_id icna35xx_of_match[] = {
> > > +     { .compatible = "ayaneo,pocketds-panel-top", .data = &odin2portal_desc },
> > > +     { .compatible = "ayntec,odin2portal-panel", .data = &odin2portal_desc },
> > > +     { .compatible = "ayntec,odin3-panel", .data = &thor_top_desc },
> > > +     { .compatible = "ayntec,thor-panel-top", .data = &thor_top_desc },
> >
> > This looks like you could be using the fallback compatibles instead of
> > matching on the panel's. Why aren't you?
> 
> I'm uncertain if the panels are actually the same between these
> devices or if the init sequences are just similar enough to share. I
> am told that the thor top panel and the odin 3 panel are the same
> model. But I don't know about the portal and the pocket ds, just know
> that they both fire up and work as expected with the same init
> sequence. I'm trying to follow the bindings idea that these might be
> different physical panels that only share a ddic and thus driver
> handling.

That's a reasonable rationale. Could you include this in your commit
message please?

> 
> > > +     { /* sentinel */ }
> 
> Aaron

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

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

* Claude review: drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 21:29 [PATCH v3 0/2] " Aaron Kling via B4 Relay
  2026-05-14 21:29 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
@ 2026-05-16  0:22 ` Claude Code Review Bot
  1 sibling, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  0:22 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/panel: Add panel driver for Chipone ICNA35XX based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 3
Reviewed: 2026-05-16T10:22:32.473513

---

This is a v3 two-patch series adding a DRM panel driver for Chipone ICNA3512/ICNA3520 DDICs used in AYN and Ayaneo handheld gaming devices. The overall structure is sound and follows modern kernel panel driver conventions (`devm_drm_panel_alloc`, `mipi_dsi_multi_context`, `devm_regulator_bulk_get_const`). The DT binding is clean and correctly structured.

The driver works but has several issues ranging from likely bugs (swapped physical dimensions) to style/modernization items. The `get_current_mode` pattern is established (matches `panel-novatek-nt35950`), the minimal DSC config matches `panel-visionox-r66451`, and the overall probe/remove flow is correct.

**Key concerns:**
1. The `width_mm`/`height_mm` values appear to be swapped for both panels (likely bug).
2. The driver could use `devm_mipi_dsi_attach()` to simplify/eliminate the `.remove` callback.
3. Unnecessary `#include <linux/of_graph.h>`.
4. Minor style nits (indentation, `__typeof` vs struct name directly).

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 21:29 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
@ 2026-05-16  0:22   ` Claude Code Review Bot
  0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  0:22 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This is the main driver patch. It follows current conventions well but has several issues:

**1. Likely bug: `width_mm` / `height_mm` are swapped (both descriptors)**

Both panels have a native resolution of 1080x1920 (portrait). The physical dimensions should have `width_mm < height_mm` to match:

```c
static struct panel_desc odin2portal_desc = {
	...
	.width_mm = 160,
	.height_mm = 89,
```

For 1080x1920 at ~7", computing from pixel density: width should be ~87-89mm and height ~155-160mm. These values are transposed. It should be:

```c
	.width_mm = 89,
	.height_mm = 160,
```

Same for `thor_top_desc`:
```c
	.width_mm = 136,
	.height_mm = 68,
```
Should likely be `.width_mm = 68, .height_mm = 136`. As-is, userspace DPI calculations will be incorrect.

**2. `panel_desc` structs should be `const`**

```c
static struct panel_desc odin2portal_desc = {
```

These are never modified after initialization and should be `static const struct panel_desc`. The `pinfo->desc` member should then become `const struct panel_desc *desc`. The one complication is `pinfo->dsi->dsc = &pinfo->desc->dsc` assigning to a non-const pointer, which can be addressed with a cast or by copying the DSC config into `panel_info` (as `panel-novatek-nt35950` does).

**3. Use `devm_mipi_dsi_attach()` to eliminate the remove callback**

There are now 18+ panel drivers in-tree using `devm_mipi_dsi_attach()`. Since the driver already uses `devm_drm_panel_alloc()`, switching to `devm_mipi_dsi_attach()` would allow removing the `icna35xx_remove()` function entirely:

```c
static void icna35xx_remove(struct mipi_dsi_device *dsi)
{
	struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
	int ret;

	ret = mipi_dsi_detach(pinfo->dsi);
	if (ret < 0)
		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);

	drm_panel_remove(&pinfo->panel);
}
```

This entire function can be deleted if `mipi_dsi_attach()` is replaced with `devm_mipi_dsi_attach()` in probe, and the `drm_panel_remove()` error path becomes `return dev_err_probe(...)` since devm handles cleanup.

**4. Unnecessary include**

```c
#include <linux/of_graph.h>
```

No `of_graph_*` functions are used in this driver. This include should be removed.

**5. `__typeof` usage is non-idiomatic**

```c
	pinfo = devm_drm_panel_alloc(dev, __typeof(*pinfo), panel,
```

All other in-tree callers pass the struct type directly:

```c
	pinfo = devm_drm_panel_alloc(dev, struct panel_info, panel,
```

**6. Indentation issue in `devm_regulator_bulk_get_const` call**

```c
	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(panel_supplies),
	panel_supplies, &pinfo->supplies);
```

The continuation line should be aligned with the opening parenthesis or at least indented further:

```c
	ret = devm_regulator_bulk_get_const(dev, ARRAY_SIZE(panel_supplies),
					    panel_supplies, &pinfo->supplies);
```

**7. `of_device_get_match_data()` cast**

```c
	pinfo->desc = (struct panel_desc *)of_device_get_match_data(dev);
```

The preferred modern API is `device_get_match_data(dev)` which works with both OF and ACPI and returns `const void *`. Using it would also reinforce the constness issue from point 2.

**8. `icna35xx_disable` never restores `MIPI_DSI_MODE_LPM`**

```c
static int icna35xx_disable(struct drm_panel *panel)
{
	...
	pinfo->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
	...
}
```

The LPM flag is cleared but never restored. In practice this is fine because `prepare()` calls `init_sequence()` which re-sets it via `pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM`, and the full mode_flags are reset from `pinfo->desc->mode_flags` in a new attach cycle. But it would be cleaner to restore it at the end of disable, matching the pattern in the backlight ops.

**9. Minor: `init_sequence` register naming could benefit from defines**

The init sequences use raw vendor register addresses (`0x9F`, `0x48`, `0xB3`, etc.) without any symbolic names. This is common for vendor-derived init sequences and not a blocker, but adding at least a comment for the page-select register (`0x9F`) would help future maintainability.

**10. DSC config completeness**

The DSC configs only populate basic fields (version, slice dimensions, bpc, bpp, block_pred_enable). This matches the pattern in `panel-visionox-r66451.c` and is acceptable for panels where the DDIC handles decompression internally, so this is fine.

**Summary:** The main actionable items are the swapped `width_mm`/`height_mm` (likely bug), switching to `devm_mipi_dsi_attach`, making descriptors `const`, removing the unused `of_graph.h` include, and fixing the indentation/style nits. The overall driver structure and approach are correct.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 17:24 [PATCH v2 0/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
  2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
  2026-05-14 17:24 ` [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
@ 2026-05-16  0:36 ` Claude Code Review Bot
  2 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  0:36 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/panel: Add panel driver for Chipone ICNA35XX based panels
Author: Aaron Kling via B4 Relay <devnull+webgeek1234.gmail.com@kernel.org>
Patches: 10
Reviewed: 2026-05-16T10:36:59.921908

---

This is a v2 series adding a DRM panel driver for Chipone ICNA35xx DDICs (ICNA3512 / ICNA3520) used in AYN handheld gaming devices. The series is 2 patches: DT bindings + driver. It depends on a prerequisite AYN QCS8550 DT series.

The driver uses modern APIs (`devm_drm_panel_alloc`, `devm_regulator_bulk_get_const`, `mipi_dsi_multi_context`) which is good. However, there are several issues: the `panel_desc` structs should be `const`, there's a stale `connector` pointer stashed in the panel's private struct (fragile anti-pattern), an unnecessary include, and a const-discarding cast. The DT binding looks mostly fine.

Overall the driver is in reasonable shape but needs a cleanup pass before merging.

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings
  2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
  2026-05-14 18:06   ` Conor Dooley
@ 2026-05-16  0:37   ` Claude Code Review Bot
  1 sibling, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  0:37 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch adds the YAML DT binding for `chipone,icna35xx.yaml`.

**Generally looks good.** The binding correctly uses the two-level compatible scheme (board-specific + chip-generic fallback). A few minor observations:

1. **Non-standard supply names lack descriptions.** `disp-supply` and `blvdd-supply` are not common names. While `vdd`, `vddio`, and `vci` are well-known, these two would benefit from brief descriptions explaining what they power:

```yaml
  disp-supply: true
  blvdd-supply: true
```

Consider adding something like:
```yaml
  disp-supply:
    description: Display bias voltage supply
  blvdd-supply:
    description: Backlight LED voltage supply
```

2. The example is clean and follows conventions. The `port` property is optional, which is fine.

No blocking issues.

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/panel: Add panel driver for Chipone ICNA35XX based panels
  2026-05-14 17:24 ` [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
  2026-05-14 18:05   ` Conor Dooley
@ 2026-05-16  0:37   ` Claude Code Review Bot
  1 sibling, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  0:37 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This is the main driver. Several issues:

**1. `connector` pointer stash is fragile (medium)**

The driver stores a `struct drm_connector *connector` in `panel_info`:

```c
struct panel_info {
	struct drm_panel panel;
	struct drm_connector *connector;
	...
};
```

It's set in `icna35xx_get_modes()`:
```c
	pinfo->connector = connector;
```

Then accessed in `icna35xx_get_current_mode()` (called from `prepare()` → `init_sequence()`):
```c
	struct drm_connector *connector = pinfo->connector;
	...
	if (!connector->state || !connector->state->crtc)
		return 0;

	crtc_state = connector->state->crtc->state;
```

Modern panel drivers do not store the connector pointer. This pattern is fragile — if `prepare()` were ever called before `get_modes()`, `pinfo->connector` would be NULL and the NULL check on `connector->state` would crash (it checks `connector->state`, not `connector` itself). The lifetime of the connector is also not guaranteed to match the panel.

While this anti-pattern exists in some older drivers, it should be avoided in new ones. If multi-mode init sequences are truly needed, consider passing mode information through `drm_panel`'s `enable()` path or documenting the assumption clearly.

**2. `panel_desc` structs should be `const` (minor/style)**

```c
static struct panel_desc odin2portal_desc = {
```
```c
static struct panel_desc thor_top_desc = {
```

These should be `static const struct panel_desc`. All other modern panel drivers in the tree (e.g., `panel-ilitek-ili9882t.c`, `panel-boe-himax8279d.c`) declare their descriptors as `const`. Correspondingly, `pinfo->desc` should be `const struct panel_desc *desc`.

**3. Casting away const from `of_device_get_match_data` (minor)**

```c
	pinfo->desc = (struct panel_desc *)of_device_get_match_data(dev);
```

`of_device_get_match_data()` returns `const void *`. The explicit cast drops const. If the `desc` pointer and structs are made `const` (per point 2), this cast becomes unnecessary and the line becomes:
```c
	pinfo->desc = of_device_get_match_data(dev);
```

**4. Unnecessary include (nit)**

```c
#include <linux/of_graph.h>
```

The driver doesn't use any `of_graph_*()` functions. This include should be removed.

**5. `icna35xx_disable` clears LPM before sending commands (question)**

```c
static int icna35xx_disable(struct drm_panel *panel)
{
	...
	pinfo->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
	...
```

Display off and sleep entry commands are typically sent in LP mode. Clearing `MIPI_DSI_MODE_LPM` to send them in HS mode is unusual. If this is required by the hardware, a brief comment would help future maintainers understand why. The same pattern appears in the backlight ops — is this intentional or an error?

**6. `icna3512_init_sequence` sets LPM on `mode_flags` directly (minor)**

```c
	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
```

The init sequence modifies `dsi->mode_flags` directly. This is fine for `prepare()`, but note that `MIPI_DSI_MODE_LPM` is already included in the `mode_flags` set in `probe()`:

```c
	.mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS |
			MIPI_DSI_MODE_LPM,
```

So the OR in init_sequence is redundant on the first call. After `disable()` clears it, `prepare()` would restore it. The interplay between `disable()` clearing LPM and `prepare()` restoring it works but is subtle — worth checking this is intentional.

**7. `icna35xx_reset` sequence (ok)**

```c
	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
	usleep_range(20000, 21000);
	gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
	usleep_range(20000, 21000);
	gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
	usleep_range(20000, 21000);
```

With `GPIOD_OUT_LOW` at probe and `GPIO_ACTIVE_LOW` in DT, this is a valid deassert→assert→deassert pulse sequence. Consistent with other panel drivers. No issue here.

**8. `drm_panel_remove` / `mipi_dsi_detach` in remove callback (ok)**

The `remove()` callback calls both `mipi_dsi_detach()` and `drm_panel_remove()`. This is the correct pattern when using `devm_drm_panel_alloc()` — devm handles the panel allocation lifetime, but `drm_panel_remove()` is still needed to unregister from the panel list.

**9. DSC config is incomplete (question)**

The DSC configurations only set a few fields:

```c
	.dsc = {
		.dsc_version_major = 0x1,
		.dsc_version_minor = 0x1,
		.slice_height = 20,
		.slice_width = 540,
		.slice_count = 2,
		.bits_per_component = 8,
		.bits_per_pixel = 8 << 4,
		.block_pred_enable = true,
	},
```

Many DSC fields like `line_buf_depth`, `rc_model_size`, `rc_range_parameters`, `initial_xmit_delay`, `initial_dec_delay`, `mux_word_size`, etc. are left at zero/default. Is the DSI host driver expected to compute these? If so, this should be verified against the DSI controller driver being used (likely `msm` for Qualcomm QCS8550). If the DSI host doesn't fill in defaults, the DSC encoding could produce garbage.

**Summary:** The main actionable items are making `panel_desc` const (items 2-3), removing the unused include (item 4), and ideally rethinking the connector stash pattern (item 1). Items 5-6 and 9 deserve clarification from the author.

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-05-16  0:37 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-14 17:24 [PATCH v2 0/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
2026-05-14 17:24 ` [PATCH v2 1/2] dt-bindings: display: panel: Add Chipone ICNA 35xx OLED driver bindings Aaron Kling via B4 Relay
2026-05-14 18:06   ` Conor Dooley
2026-05-14 18:20     ` Aaron Kling
2026-05-14 18:23       ` Conor Dooley
2026-05-14 18:27         ` Aaron Kling
2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
2026-05-14 17:24 ` [PATCH v2 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels Aaron Kling via B4 Relay
2026-05-14 18:05   ` Conor Dooley
2026-05-14 18:24     ` Aaron Kling
2026-05-14 18:53       ` Conor Dooley
2026-05-16  0:37   ` Claude review: " Claude Code Review Bot
2026-05-16  0:36 ` Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-14 21:29 [PATCH v3 0/2] " Aaron Kling via B4 Relay
2026-05-14 21:29 ` [PATCH v3 2/2] " Aaron Kling via B4 Relay
2026-05-16  0:22   ` Claude review: " Claude Code Review Bot
2026-05-16  0:22 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox