[PATCH] drm/panel: chipone-icna35xx: Fix picture parameter set

[PATCH] drm/panel: chipone-icna35xx: Fix picture parameter set

[Aaron Kling via B4 Relay]

From: Aaron Kling <webgeek1234@gmail.com>

These ddic's do not handle the standard mipi dsi command for pps.
Instead, there are per chip vendor commands to write the packed struct
directly to manufacturer registers.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
This patch results in no change at runtime because the ddic defaults to
the config being set by the existing mode. The issue was found while
doing testing running the panel in a 10-bit mode. But since it is a real
problem, better to fix it early.
---
 drivers/gpu/drm/panel/panel-chipone-icna35xx.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
index 2cc16494a4cef..b2422ffc1d1c5 100644
--- a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
+++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
@@ -69,6 +69,7 @@ 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;
+	u8 *pps_ptr = (u8 *)&pps;
 
 	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
 
@@ -91,7 +92,14 @@ static int icna3512_init_sequence(struct panel_info *pinfo)
 	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x20);
 
 	drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc);
-	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC6, 0x11, 0x88);
+
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC7, pps_ptr, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC8, pps_ptr + 32, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC9, pps_ptr + 64, 24);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
 
 	mipi_dsi_msleep(&dsi_ctx, 20);
 
@@ -104,6 +112,7 @@ 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;
+	u8 *pps_ptr = (u8 *)&pps;
 
 	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
 
@@ -133,7 +142,14 @@ static int icna3520_init_sequence(struct panel_info *pinfo)
 	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_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC0, 0x1A, 0x71);
+
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC1, pps_ptr, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC2, pps_ptr + 32, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC3, pps_ptr + 64, 24);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
 
 	mipi_dsi_msleep(&dsi_ctx, 20);
 

---
base-commit: 0d33d21e47d9dc66f91e44da3fc9220c74d93df7
change-id: 20260727-icna35xx-pps-bdd353e188cc

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

[PATCH] drm/panel: chipone-icna35xx: Fix picture parameter set

[Aaron Kling]

These ddic's do not handle the standard mipi dsi command for pps.
Instead, there are per chip vendor commands to write the packed struct
directly to manufacturer registers.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
This patch results in no change at runtime because the ddic defaults to
the config being set by the existing mode. The issue was found while
doing testing running the panel in a 10-bit mode. But since it is a real
problem, better to fix it early.
---
 drivers/gpu/drm/panel/panel-chipone-icna35xx.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
index 2cc16494a4cef..b2422ffc1d1c5 100644
--- a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
+++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
@@ -69,6 +69,7 @@ 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;
+	u8 *pps_ptr = (u8 *)&pps;
 
 	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
 
@@ -91,7 +92,14 @@ static int icna3512_init_sequence(struct panel_info *pinfo)
 	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x20);
 
 	drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc);
-	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC6, 0x11, 0x88);
+
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC7, pps_ptr, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC8, pps_ptr + 32, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC9, pps_ptr + 64, 24);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
 
 	mipi_dsi_msleep(&dsi_ctx, 20);
 
@@ -104,6 +112,7 @@ 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;
+	u8 *pps_ptr = (u8 *)&pps;
 
 	pinfo->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
 
@@ -133,7 +142,14 @@ static int icna3520_init_sequence(struct panel_info *pinfo)
 	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_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC0, 0x1A, 0x71);
+
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC1, pps_ptr, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC2, pps_ptr + 32, 32);
+	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC3, pps_ptr + 64, 24);
+
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
 
 	mipi_dsi_msleep(&dsi_ctx, 20);
 

---
base-commit: 0d33d21e47d9dc66f91e44da3fc9220c74d93df7
change-id: 20260727-icna35xx-pps-bdd353e188cc

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

Re: [PATCH] drm/panel: chipone-icna35xx: Fix picture parameter set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Direct calls to `mipi_dsi_dcs_write()` bypass the `mipi_dsi_multi_context` error accumulation, ignoring both previous errors and their own failures.
--

--- Patch [1]: [PATCH] drm/panel: chipone-icna35xx: Fix picture parameter set ---
commit 5c76e85884612ecbbcfd07cbe14b3a2d283f6c53
Author: Aaron Kling <webgeek1234@gmail.com>

drm/panel: chipone-icna35xx: Fix picture parameter set
    
This commit updates the chipone-icna35xx panel driver to send picture
parameter set (PPS) data using vendor-specific DCS writes rather than the
standard MIPI DSI command. This resolves an issue with configuring 10-bit
modes on these panels.

> diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> index 2cc16494a4cef..b2422ffc1d1c5 100644
> --- a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> +++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c

[ ... ]

> @@ -91,7 +92,14 @@ static int icna3512_init_sequence(struct panel_info *pinfo)
>  	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x48, 0x20);
>  
>  	drm_dsc_pps_payload_pack(&pps, &pinfo->desc->dsc);
> -	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC6, 0x11, 0x88);
> +
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC7, pps_ptr, 32);
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC8, pps_ptr + 32, 32);
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC9, pps_ptr + 64, 24);

[Severity: Medium]
Do these direct mipi_dsi_dcs_write() calls bypass the multi-context error
handling?

Since icna3512_init_sequence() is using mipi_dsi_multi_context, the
_multi() variants will check and update dsi_ctx.accum_err. Calling
mipi_dsi_dcs_write() directly ignores any previous errors and discards
its own return values, which might lead to silent failures if the PPS data
transmission fails.

> +
> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
>  
>  	mipi_dsi_msleep(&dsi_ctx, 20);

[ ... ]

> @@ -133,7 +142,14 @@ static int icna3520_init_sequence(struct panel_info *pinfo)
>  	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_generic_write_seq_multi(&dsi_ctx, 0x9F, 0x01);
> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xC0, 0x1A, 0x71);
> +
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC1, pps_ptr, 32);
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC2, pps_ptr + 32, 32);
> +	mipi_dsi_dcs_write(dsi_ctx.dsi, 0xC3, pps_ptr + 64, 24);

[Severity: Medium]
Does this sequence in icna3520_init_sequence() also bypass the error
accumulation?

Similar to the above, we might need a multi-aware wrapper or macro here
to ensure errors are caught and execution stops appropriately if a previous
command failed.

> +
> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xFD, 0xA5, 0xA5);
>  
>  	mipi_dsi_msleep(&dsi_ctx, 20);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-icna35xx-pps-v1-1-691d79d13ee4@gmail.com?part=1