Re: [PATCH v1 15/22] drm/panel: sony-acx565akm: Backlight update

From: daniel@ffwll.ch
Cc: Daniel Thompson <daniel.thompson@linaro.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH v1 15/22] drm/panel: sony-acx565akm: Backlight update
Date: Tue, 4 Aug 2020 19:09:59 +0200	[thread overview]
Message-ID: <20200804170959.GP6419@phenom.ffwll.local> (raw)
In-Reply-To: <20200802110636.1018743-16-sam@ravnborg.org>

On Sun, Aug 02, 2020 at 01:06:29PM +0200, Sam Ravnborg wrote:
> - Use backlight_get_brightness() helper
> - Use backlight_is_blank() helper
> - Use macro for initialization
> - Drop direct access to backlight properties
> - Use the devm_ variant for registering backlight device, and drop
>   all explicit unregistering of the backlight device.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c | 44 +++++++-------------
>  1 file changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> index 5c4b6f6e5c2d..3fc572d1de13 100644
> --- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> +++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
> @@ -298,13 +298,7 @@ static void acx565akm_set_brightness(struct acx565akm_panel *lcd, int level)
>  static int acx565akm_bl_update_status_locked(struct backlight_device *dev)
>  {
>  	struct acx565akm_panel *lcd = dev_get_drvdata(&dev->dev);
> -	int level;
> -
> -	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
> -	    dev->props.power == FB_BLANK_UNBLANK)
> -		level = dev->props.brightness;
> -	else
> -		level = 0;
> +	int level = backlight_get_brightness(dev);
>  
>  	acx565akm_set_brightness(lcd, level);
>  
> @@ -330,8 +324,7 @@ static int acx565akm_bl_get_intensity(struct backlight_device *dev)
>  
>  	mutex_lock(&lcd->mutex);
>  
> -	if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
> -	    dev->props.power == FB_BLANK_UNBLANK)
> +	if (backlight_is_blank(dev))
>  		intensity = acx565akm_get_actual_brightness(lcd);
>  	else
>  		intensity = 0;
> @@ -348,39 +341,34 @@ static const struct backlight_ops acx565akm_bl_ops = {
>  
>  static int acx565akm_backlight_init(struct acx565akm_panel *lcd)
>  {
> -	struct backlight_properties props = {
> -		.fb_blank = FB_BLANK_UNBLANK,
> -		.power = FB_BLANK_UNBLANK,
> -		.type = BACKLIGHT_RAW,
> -	};
>  	int ret;
> -
> -	lcd->backlight = backlight_device_register(lcd->name, &lcd->spi->dev,
> -						   lcd, &acx565akm_bl_ops,
> -						   &props);
> -	if (IS_ERR(lcd->backlight)) {
> -		ret = PTR_ERR(lcd->backlight);
> -		lcd->backlight = NULL;
> +	struct backlight_device *bd;
> +	DECLARE_BACKLIGHT_INIT_RAW(props, 0, 255);
> +
> +	bd = devm_backlight_device_register(&lcd->spi->dev, lcd->name,
> +					    &lcd->spi->dev, lcd,
> +					    &acx565akm_bl_ops, &props);

It's been in a bunch of earlier patches already, but devm_bl freaks me out
a bit with our long-term goal of storing a backlight pointer into
drm_connector->backlight.

Since drm_connector and the underlying backlight device have different
lifetimes that would mean we need to refcount somewhere, or protect
drm_connector->backlight with some lock. The lock might not work because
the drm connector property paths come from the other direction than the
backlight driver unload ... so probably needs to be refcounting.
-Daniel

> +	if (IS_ERR(bd)) {
> +		ret = PTR_ERR(bd);
>  		return ret;
>  	}
>  
> +	lcd->backlight = bd;
>  	if (lcd->has_cabc) {
> -		ret = sysfs_create_group(&lcd->backlight->dev.kobj,
> +		ret = sysfs_create_group(&bd->dev.kobj,
>  					 &acx565akm_cabc_attr_group);
>  		if (ret < 0) {
>  			dev_err(&lcd->spi->dev,
>  				"%s failed to create sysfs files\n", __func__);
> -			backlight_device_unregister(lcd->backlight);
>  			return ret;
>  		}
>  
>  		lcd->cabc_mode = acx565akm_get_hw_cabc_mode(lcd);
>  	}
>  
> -	lcd->backlight->props.max_brightness = 255;
> -	lcd->backlight->props.brightness = acx565akm_get_actual_brightness(lcd);
> -
> -	acx565akm_bl_update_status_locked(lcd->backlight);
> +	backlight_set_brightness(bd, acx565akm_get_actual_brightness(lcd));
> +	backlight_set_power_on(bd);
> +	backlight_update_status(bd);
>  
>  	return 0;
>  }
> @@ -390,8 +378,6 @@ static void acx565akm_backlight_cleanup(struct acx565akm_panel *lcd)
>  	if (lcd->has_cabc)
>  		sysfs_remove_group(&lcd->backlight->dev.kobj,
>  				   &acx565akm_cabc_attr_group);
> -
> -	backlight_device_unregister(lcd->backlight);
>  }
>  
>  /* -----------------------------------------------------------------------------
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel