[PATCH] drm/panel-sony-acx424akp: Modernize backlight handling

[PATCH] drm/panel-sony-acx424akp: Modernize backlight handling

[Linus Walleij]

This converts the internal backlight in the Sony ACX424AKP
driver to do it the canonical way:

- Assign the panel->backlight during probe.
- Let the panel framework handle the backlight.
- Make the backlight .set_brightness() turn the backlight
  off completely if blank.
- Fix some dev_err_probe() use cases along the way.

Tested on the U8500 HREF520 reference design.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/panel/panel-sony-acx424akp.c | 84 +++++++-------------
 1 file changed, 28 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sony-acx424akp.c b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
index 95659a4d15e9..163f0e0cee1c 100644
--- a/drivers/gpu/drm/panel/panel-sony-acx424akp.c
+++ b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
@@ -40,7 +40,6 @@
 struct acx424akp {
 	struct drm_panel panel;
 	struct device *dev;
-	struct backlight_device *bl;
 	struct regulator *supply;
 	struct gpio_desc *reset_gpio;
 	bool video_mode;
@@ -102,6 +101,20 @@ static int acx424akp_set_brightness(struct backlight_device *bl)
 	u8 par;
 	int ret;
 
+
+	if (backlight_is_blank(bl)) {
+		/* Disable backlight */
+		par = 0x00;
+		ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+					 &par, 1);
+		if (ret) {
+			dev_err(acx->dev, "failed to disable display backlight (%d)\n", ret);
+			return ret;
+		}
+		return 0;
+	}
+
+
 	/* Calculate the PWM duty cycle in n/256's */
 	pwm_ratio = max(((duty_ns * 256) / period_ns) - 1, 1);
 	pwm_div = max(1,
@@ -172,6 +185,12 @@ static const struct backlight_ops acx424akp_bl_ops = {
 	.update_status = acx424akp_set_brightness,
 };
 
+static const struct backlight_properties acx424akp_bl_props = {
+	.type = BACKLIGHT_PLATFORM,
+	.brightness = 512,
+	.max_brightness = 1023,
+};
+
 static int acx424akp_read_id(struct acx424akp *acx)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
@@ -310,8 +329,6 @@ static int acx424akp_prepare(struct drm_panel *panel)
 		}
 	}
 
-	acx->bl->props.power = FB_BLANK_NORMAL;
-
 	return 0;
 
 err_power_off:
@@ -323,18 +340,8 @@ static int acx424akp_unprepare(struct drm_panel *panel)
 {
 	struct acx424akp *acx = panel_to_acx424akp(panel);
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
-	u8 par;
 	int ret;
 
-	/* Disable backlight */
-	par = 0x00;
-	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
-				 &par, 1);
-	if (ret) {
-		dev_err(acx->dev, "failed to disable display backlight (%d)\n", ret);
-		return ret;
-	}
-
 	ret = mipi_dsi_dcs_set_display_off(dsi);
 	if (ret) {
 		dev_err(acx->dev, "failed to turn display off (%d)\n", ret);
@@ -350,36 +357,10 @@ static int acx424akp_unprepare(struct drm_panel *panel)
 	msleep(85);
 
 	acx424akp_power_off(acx);
-	acx->bl->props.power = FB_BLANK_POWERDOWN;
-
-	return 0;
-}
-
-static int acx424akp_enable(struct drm_panel *panel)
-{
-	struct acx424akp *acx = panel_to_acx424akp(panel);
-
-	/*
-	 * The backlight is on as long as the display is on
-	 * so no use to call backlight_enable() here.
-	 */
-	acx->bl->props.power = FB_BLANK_UNBLANK;
 
 	return 0;
 }
 
-static int acx424akp_disable(struct drm_panel *panel)
-{
-	struct acx424akp *acx = panel_to_acx424akp(panel);
-
-	/*
-	 * The backlight is on as long as the display is on
-	 * so no use to call backlight_disable() here.
-	 */
-	acx->bl->props.power = FB_BLANK_NORMAL;
-
-	return 0;
-}
 
 static int acx424akp_get_modes(struct drm_panel *panel,
 			       struct drm_connector *connector)
@@ -409,10 +390,8 @@ static int acx424akp_get_modes(struct drm_panel *panel,
 }
 
 static const struct drm_panel_funcs acx424akp_drm_funcs = {
-	.disable = acx424akp_disable,
 	.unprepare = acx424akp_unprepare,
 	.prepare = acx424akp_prepare,
-	.enable = acx424akp_enable,
 	.get_modes = acx424akp_get_modes,
 };
 
@@ -458,25 +437,18 @@ static int acx424akp_probe(struct mipi_dsi_device *dsi)
 	/* This asserts RESET by default */
 	acx->reset_gpio = devm_gpiod_get_optional(dev, "reset",
 						  GPIOD_OUT_HIGH);
-	if (IS_ERR(acx->reset_gpio)) {
-		ret = PTR_ERR(acx->reset_gpio);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "failed to request GPIO (%d)\n", ret);
-		return ret;
-	}
+	if (IS_ERR(acx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(acx->reset_gpio),
+				     "failed to request GPIO\n");
 
 	drm_panel_init(&acx->panel, dev, &acx424akp_drm_funcs,
 		       DRM_MODE_CONNECTOR_DSI);
 
-	acx->bl = devm_backlight_device_register(dev, "acx424akp", dev, acx,
-						 &acx424akp_bl_ops, NULL);
-	if (IS_ERR(acx->bl)) {
-		dev_err(dev, "failed to register backlight device\n");
-		return PTR_ERR(acx->bl);
-	}
-	acx->bl->props.max_brightness = 1023;
-	acx->bl->props.brightness = 512;
-	acx->bl->props.power = FB_BLANK_POWERDOWN;
+	acx->panel.backlight = devm_backlight_device_register(dev, "acx424akp", dev, acx,
+					&acx424akp_bl_ops, &acx424akp_bl_props);
+	if (IS_ERR(acx->panel.backlight))
+		return dev_err_probe(dev, PTR_ERR(acx->panel.backlight),
+				     "failed to register backlight device\n");
 
 	drm_panel_add(&acx->panel);
 
-- 
2.31.1

Re: [PATCH] drm/panel-sony-acx424akp: Modernize backlight handling

[Sam Ravnborg]

Hi Linus,

On Thu, Jul 15, 2021 at 11:28:08AM +0200, Linus Walleij wrote:
> This converts the internal backlight in the Sony ACX424AKP
> driver to do it the canonical way:
> 
> - Assign the panel->backlight during probe.
> - Let the panel framework handle the backlight.
> - Make the backlight .set_brightness() turn the backlight
>   off completely if blank.
> - Fix some dev_err_probe() use cases along the way.

Very nice cleanup - thanks.
One issue below, with that fixed:

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

I assume you will apply the patch yourself.

	Sam

> 
> Tested on the U8500 HREF520 reference design.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/panel/panel-sony-acx424akp.c | 84 +++++++-------------
>  1 file changed, 28 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx424akp.c b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
> index 95659a4d15e9..163f0e0cee1c 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx424akp.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
> @@ -40,7 +40,6 @@
>  struct acx424akp {
>  	struct drm_panel panel;
>  	struct device *dev;
> -	struct backlight_device *bl;
>  	struct regulator *supply;
>  	struct gpio_desc *reset_gpio;
>  	bool video_mode;
> @@ -102,6 +101,20 @@ static int acx424akp_set_brightness(struct backlight_device *bl)
>  	u8 par;
>  	int ret;
>  
> +
> +	if (backlight_is_blank(bl)) {
> +		/* Disable backlight */
> +		par = 0x00;
> +		ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +					 &par, 1);
> +		if (ret) {
> +			dev_err(acx->dev, "failed to disable display backlight (%d)\n", ret);
> +			return ret;
> +		}
> +		return 0;
> +	}
> +
> +
>  	/* Calculate the PWM duty cycle in n/256's */
>  	pwm_ratio = max(((duty_ns * 256) / period_ns) - 1, 1);
>  	pwm_div = max(1,
> @@ -172,6 +185,12 @@ static const struct backlight_ops acx424akp_bl_ops = {
>  	.update_status = acx424akp_set_brightness,
>  };
>  
> +static const struct backlight_properties acx424akp_bl_props = {
> +	.type = BACKLIGHT_PLATFORM,
Other dsi panels uses BACKLIGHT_RAW here, which I think is more
correct. So unless there is good arguments for use of PLATFORM, please
change this to RAW.

It only influences how backlight is reported via sysfs, and there is no
functional change AFAICT.


	Sam