Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Probe fails unconditionally if the optional backlight regulator is not present in the device tree, breaking backward compatibility. - [High] Skipping the DCS brightness command when brightness is 0 prevents the backlight from turning off on devices without an external regulator. - [Medium] The backlight supply regulator's enable count is leaked if the driver is unbound while the backlight is active. --
commit 700de7e190150a663957123f411692a25d437849 Author: Icenowy Zheng <[email protected]> drm/panel: himax-hx83121a: add backlight regulator support This patch adds support for an external backlight regulator to the himax-hx83121a panel driver. It turns off the regulator when brightness is 0 and enables it when the backlight is operating. > diff --git a/drivers/gpu/drm/panel/panel-himax-hx83121a.c > b/drivers/gpu/drm/panel/panel-himax-hx83121a.c > index 1a7e0125bced8..e31e2fba0a787 100644 > --- a/drivers/gpu/drm/panel/panel-himax-hx83121a.c > +++ b/drivers/gpu/drm/panel/panel-himax-hx83121a.c [ ... ] > @@ -195,7 +197,27 @@ static int himax_bl_update_status(struct > backlight_device *bl) > { > struct himax *ctx = bl_get_data(bl); > u16 brightness = backlight_get_brightness(bl); > + int ret = 0; > + > + if (!brightness) { > + if (ctx->backlight_enabled) > + ret = regulator_disable(ctx->bl_supply); > + if (ret) > + return ret; > + > + ctx->backlight_enabled = false; > + > + return 0; > + } [Severity: High] Does this early return prevent the display from turning off the backlight via DCS? For panels that do not have an external bl-supply regulator and rely entirely on DCS commands, disabling the dummy/absent regulator is a no-op, but the early return bypasses the mipi_dsi_dcs_set_display_brightness_large() call. Will this leave the backlight powered on at its previous level when it should be turned off? > + > /* TODO: brightness to raw map table */ > + if (!ctx->backlight_enabled) > + ret = regulator_enable(ctx->bl_supply); [Severity: Medium] Could this leak the regulator enable count if the driver is unbound while the backlight is active? If the panel driver is unbound or the module is removed while brightness is greater than 0, the devres infrastructure will unregister the backlight device without triggering an update_status to 0. Since there is no explicit disable in a remove callback or devm_add_action teardown hook, will the subsequent regulator_put() hit a warning for a non-zero enable count? > + if (ret) > + return ret; > + > + ctx->backlight_enabled = true; > + > return mipi_dsi_dcs_set_display_brightness_large(to_primary_dsi(ctx), > brightness); > } [ ... ] > @@ -647,6 +669,10 @@ static int himax_probe(struct mipi_dsi_device *dsi) > ctx->panel.prepare_prev_first = true; > > if (desc->has_dcs_backlight) { > + ctx->bl_supply = devm_regulator_get_optional(dev, "bl"); > + if (IS_ERR(ctx->bl_supply)) > + return dev_err_probe(dev, PTR_ERR(ctx->bl_supply), > + "Failed to get backlight > supply\n"); [Severity: High] Will this break probe backward compatibility for existing device trees? The devm_regulator_get_optional() function returns ERR_PTR(-ENODEV) when the requested regulator is absent from the device tree. Because this bl-supply property was just introduced and is optional, existing devices do not have it. Should this code explicitly check for and handle -ENODEV (for example, by setting the pointer to NULL) instead of failing the probe unconditionally? > ctx->backlight = himax_create_backlight(ctx); > if (IS_ERR(ctx->backlight)) > return dev_err_probe(dev, PTR_ERR(ctx->backlight), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
