Re: [PATCH 3/3] drm/panel: himax-hx83121a: add backlight regulator support

[sashiko-bot@kernel.org]

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 <zhengxingda@iscas.ac.cn>

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/20260608162622.403713-1-zhengxingda@iscas.ac.cn?part=3