Re: [PATCH 3/4] drm/panel: visionox-vtdr6130: Modularize panel config

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] Incomplete multi-mode support: `num_modes` is ignored in `visionox_vtdr6130_get_modes`.
- [Medium] Physical dimensions (`width_mm` and `height_mm`) are no longer populated on the duplicated `drm_display_mode`.
- [Medium] Casting the return value of `of_device_get_match_data()` discards `const`, leaving the descriptor and its function pointers in writable memory.
- [Low] Newly added fields `connector` and `bpc` are completely unused.
- [Low] Missing NULL check for the optional `init_sequence` function pointer.
- [Low] The panel descriptor provides a custom `init_sequence` but lacks a corresponding `off_sequence`.
--

--- Patch [3]: [PATCH 3/4] drm/panel: visionox-vtdr6130: Modularize panel config ---
commit 4c3c6a745daceb0c973dc23c581b4254bee31bfc
Author: Aaron Kling <webgeek1234@gmail.com>

drm/panel: visionox-vtdr6130: Modularize panel config

This commit modularizes the visionox-vtdr6130 panel driver by introducing
a descriptor structure to hold panel-specific configurations. This prepares
the driver to support multiple panels sharing the same display driver IC.

> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> @@ -19,11 +19,28 @@
>  
>  struct visionox_vtdr6130 {
>  	struct drm_panel panel;
> -	struct drm_dsc_config dsc;
> +	struct drm_connector *connector;

[Severity: Low]
Is this connector field used anywhere else in the driver? It appears to be
declared but never utilized.

>  	struct mipi_dsi_device *dsi;
>  	struct gpio_desc *reset_gpio;
>  	struct regulator_bulk_data *supplies;
>  	enum drm_panel_orientation orientation;
> +	struct visionox_vtdr6130_desc *desc;
> +};
> +
> +struct visionox_vtdr6130_desc {
> +	unsigned int width_mm;
> +	unsigned int height_mm;
> +
> +	unsigned int bpc;

[Severity: Low]
Similar to the connector field above, is this bpc field intended for a
future patch? It is defined and initialized later but doesn't seem to be
used in the current code.

> +	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 visionox_vtdr6130 *ctx);
> +
> +	struct drm_dsc_config dsc;
>  };

[ ... ]

> @@ -162,7 +179,7 @@ static int visionox_vtdr6130_prepare(struct drm_panel *panel)
>  
>  	visionox_vtdr6130_reset(ctx);
>  
> -	ret = visionox_vtdr6130_on(ctx);
> +	ret = ctx->desc->init_sequence(ctx);

[Severity: Low]
Could this cause a null pointer dereference if a future panel omits the
init_sequence callback?

Should there be a check before calling it, similar to:

	if (ctx->desc->init_sequence)
		ret = ctx->desc->init_sequence(ctx);

[Severity: Low]
This isn't a bug, but should the power-off sequence be abstracted as well?

Currently, visionox_vtdr6130_prepare() uses the abstracted init_sequence(),
but visionox_vtdr6130_unprepare() still relies on a hardcoded call to
visionox_vtdr6130_off().

[ ... ]

> -static const struct drm_display_mode visionox_vtdr6130_mode = {
> -	.clock = (1080 + 20 + 2 + 20) * (2400 + 20 + 2 + 18) * 144 / 1000,
[ ... ]
> -	.vtotal = 2400 + 20 + 2 + 18,
> -	.width_mm = 71,
> -	.height_mm = 157,
> +static const struct drm_display_mode visionox_vtdr6130_modes[] = {
> +	{
> +		.clock = (1080 + 20 + 2 + 20) * (2400 + 20 + 2 + 18) * 144 / 1000,
[ ... ]
> +		.vtotal = 2400 + 20 + 2 + 18,
> +	},
> +};

[Severity: Medium]
Because width_mm and height_mm were moved out of the drm_display_mode struct
definition and into the descriptor, will the duplicated mode end up with 0 for
its physical dimensions?

Userspace compositors might rely on these values inside the mode object to
calculate DPI.

> +static struct visionox_vtdr6130_desc visionox_vtdr6130_panel_desc = {

[Severity: Medium]
Should this descriptor be declared as const?

Since it contains a function pointer (init_sequence), keeping it in writable
memory provides a potential target for control-flow hijacking.

> +	.modes = visionox_vtdr6130_modes,
> +	.num_modes = ARRAY_SIZE(visionox_vtdr6130_modes),
[ ... ]
>  static int visionox_vtdr6130_get_modes(struct drm_panel *panel,
>  				       struct drm_connector *connector)
>  {
> +	struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel);
>  	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_duplicate(connector->dev, &visionox_vtdr6130_mode);
> +	mode = drm_mode_duplicate(connector->dev, ctx->desc->modes);

[Severity: Medium]
Does this logic fully support multiple modes?

It looks like it only duplicates the first mode in the ctx->desc->modes
array, completely ignoring the ctx->desc->num_modes field. If a future panel
adds multiple modes, only the first one would be exposed to userspace here.

[ ... ]

> @@ -284,23 +325,17 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
>  		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
>  				     "Failed to get reset-gpios\n");
>  
> +	ctx->desc = (struct visionox_vtdr6130_desc *)of_device_get_match_data(dev);

[Severity: Medium]
Does casting the return value of of_device_get_match_data() discard the
const qualifier?

This API returns a const void *, which matches the suggestion to make the
descriptor struct const above.

> +	if (!ctx->desc)
> +		return -ENODEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-rp6-panel-v1-0-111c1aeccf0f@gmail.com?part=3