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 <[email protected]> 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/[email protected]?part=3
