Re: [PATCH v3 2/4] dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sending DSI commands in `unprepare` after asserting the reset GPIO will cause DSI transmission timeouts.
- [High] The `MIPI_DSI_MODE_LPM` flag toggle for the PPS transfer is wrapped around the wrong function, rendering it ineffective.
- [Medium] The `compatible` property definition enforces an incorrect generic-first string order and mismatches the device tree example, causing validation to fail.
--

commit faf055f392f23751b6b923b1d2e80556b912da18
Author: Paul Sajna <sajattack@postmarketos.org>

dt-bindings: display: panel: Add documentation for lg,sw49410-lh609qh1

This commit documents the bindings for the LG LH609QH1 MIPI DSI panel. It
also introduces the DRM panel driver for the SW49410 controller found in the
LG G7 ThinQ smartphone.

> diff --git a/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
> new file mode 100644
> index 0000000000000..4c4bf4d89353f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/lg,sw49410.yaml
> @@ -0,0 +1,79 @@
[ ... ]
> +properties:
> +  compatible:
> +    items:
> +      - const: lg,sw49410
> +      - const: lg,sw49410-lh609qh1

[Severity: Medium]
Does the compatible array enforce a generic-to-specific order?

Device tree standard typically dictates specific-to-generic ordering.
Additionally, since the device tree example further down only provides a
single string:

	compatible = "lg,sw49410-lh609qh1";

Will this single-string example fail validation against the two-item schema
constraint defined here?

> +
> +  reg:
[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-lg-sw49410.c b/drivers/gpu/drm/panel/panel-lg-sw49410.c
> new file mode 100644
> index 0000000000000..02d1b85c3affb
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg-sw49410.c
> @@ -0,0 +1,528 @@
[ ... ]
> +static int sw49410_panel_program(struct sw49410_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +	struct drm_dsc_picture_parameter_set pps;
[ ... ]
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +
> +	ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);

[Severity: High]
Are the MIPI_DSI_MODE_LPM flags being toggled around the right function?

This code wraps the local memory packing function drm_dsc_pps_payload_pack()
instead of the actual transmission function
mipi_dsi_picture_parameter_set_multi(). 

Does this mean the PPS command is still incorrectly sent in low-power mode?

> +
> +	mipi_dsi_compression_mode_ext_multi(&dsi_ctx, true, MIPI_DSI_COMPRESSION_DSC, 1);
[ ... ]
> +static int sw49410_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct sw49410_panel *ctx = to_sw49410_panel(panel);
> +
> +	gpiod_set_value(ctx->reset_gpio, 1);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);

[Severity: High]
Could asserting the reset GPIO before sending DSI commands lead to
transmission timeouts?

Because the panel is already put into reset by gpiod_set_value(..., 1),
it seems like it won't be able to acknowledge the display_off and
enter_sleep_mode commands sent immediately after.

> +	mipi_dsi_msleep(&dsi_ctx, 100);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-judyln-panel-v3-0-07f4134441bd@postmarketos.org?part=2