Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The reset GPIO is requested with `GPIOD_OUT_LOW`, which deasserts it and can backpower the unpowered panel. - [High] The backlight callback modifies `dsi->mode_flags` without locking and permanently corrupts the state if the DSI transfer fails. - [Low] The macro `MIPI_DSI_MODE_DSC_ALL_SLICES_IN_PKT` is undefined in the kernel, causing a build failure. --
commit 3b5fdc2d207db00cc147a18d14281a4361ece19b Author: Alexander Koskovich <[email protected]> drm/panel: Add driver for Raydium RM69220 DDIC Add a panel driver for Raydium RM69220 DDIC based displays such as the BOE BF068MWM-TD0 found in the Nothing Phone (3a). > diff --git a/drivers/gpu/drm/panel/panel-raydium-rm69220.c > b/drivers/gpu/drm/panel/panel-raydium-rm69220.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-raydium-rm69220.c [ ... ] > +static int raydium_rm69220_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; > +} [Severity: High] Does this error path permanently corrupt the DSI mode flags? If mipi_dsi_dcs_set_display_brightness_large() fails, the function returns early without restoring MIPI_DSI_MODE_LPM. Userspace can trigger this easily by writing to the backlight sysfs node while the display is powered off, which will cause a transfer timeout. All future DSI initialization commands will then incorrectly be sent in high-speed mode, preventing the panel from turning back on until the system is rebooted. [ ... ] > +static int raydium_rm69220_probe(struct mipi_dsi_device *dsi) > +{ > + struct device *dev = &dsi->dev; > + struct raydium_rm69220 *ctx; > + int ret; [ ... ] > + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio), > + "Failed to get reset-gpios\n"); [Severity: High] Is GPIOD_OUT_LOW the correct initialization here? Because the reset pin is typically active-low, a logical 0 translates to driving the physical pin HIGH. This happens at probe time before the regulators are enabled, which can drive voltage into the panel's reset pin while its power supplies are off. This forward-biases the panel's ESD protection diodes, leaking current into the power plane and potentially causing hardware damage. Should this be initialized to GPIOD_OUT_HIGH to hold it safely in reset (0V) until power is applied? > + > + ctx->dsi = dsi; > + mipi_dsi_set_drvdata(dsi, ctx); > + > + dsi->lanes = 4; > + dsi->format = MIPI_DSI_FMT_RGB101010; > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | > + MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM | > + MIPI_DSI_MODE_DSC_ALL_SLICES_IN_PKT; [Severity: Low] Will this compile correctly? The macro MIPI_DSI_MODE_DSC_ALL_SLICES_IN_PKT appears to be undefined in the upstream kernel, possibly a leftover from a vendor tree. Compiling this driver will likely result in an undeclared identifier error. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
