Re: [PATCH v1 06/22] drm/panel: asus-z00t-tm5p5-n35596: Backlight update

From: daniel@ffwll.ch
Cc: Daniel Thompson <daniel.thompson@linaro.org>,
	Jingoo Han <jingoohan1@gmail.com>,
	Konrad Dybcio <konradybcio@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH v1 06/22] drm/panel: asus-z00t-tm5p5-n35596: Backlight update
Date: Tue, 4 Aug 2020 18:59:18 +0200	[thread overview]
Message-ID: <20200804165918.GN6419@phenom.ffwll.local> (raw)
In-Reply-To: <20200802110636.1018743-7-sam@ravnborg.org>

On Sun, Aug 02, 2020 at 01:06:20PM +0200, Sam Ravnborg wrote:
> Update backlight to use macro for initialization and the
> backlight_get_brightness() operation to simply the update operation.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Konrad Dybcio <konradybcio@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  .../gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c  | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> index 39e0f0373f3c..c090fc6f1ed5 100644
> --- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> +++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> @@ -216,14 +216,9 @@ static const struct drm_panel_funcs tm5p5_nt35596_panel_funcs = {
>  static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
>  {
>  	struct mipi_dsi_device *dsi = bl_get_data(bl);
> -	u16 brightness = bl->props.brightness;
> +	int brightness = backlight_get_brightness(bl);
>  	int ret;
>  
> -	if (bl->props.power != FB_BLANK_UNBLANK ||
> -	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> -	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> -		brightness = 0;
> -
>  	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>  
>  	ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> @@ -238,7 +233,7 @@ static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
>  static int tm5p5_nt35596_bl_get_brightness(struct backlight_device *bl)
>  {
>  	struct mipi_dsi_device *dsi = bl_get_data(bl);
> -	u16 brightness = bl->props.brightness;
> +	u16 brightness = backlight_get_brightness(bl);

I'm not sure why we do this, but your patch here changes behaviour in a
way that has bitten us in the past: This now reports a brightness of 0
when the backlight is off. On some backlights (especially firmware ones) 0
means "lowest value", not actually off, so that's one confusion. The other
problem is then that userspace tends to use this as the backlight value to
restore on next boot (or after resume, or after vt switch, resulting in a
very dark or black screen).

Therefore I think in these cases we actually need the direct
bl->props.brightness value.

I think an even cleaner way to solve this would be to change the
get_brightness code in actual_brightness_show to handle negative error
codes from ->get_brightness and use that to fall back to
bd->props.brightness, then we could remove this code here.

That reminds me, probably not a good idea to store a negative value in
backlight_force_update() if this goes wrong into bl->props.brightness.
-Daniel

>  	int ret;
>  
>  	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> @@ -261,11 +256,7 @@ static struct backlight_device *
>  tm5p5_nt35596_create_backlight(struct mipi_dsi_device *dsi)
>  {
>  	struct device *dev = &dsi->dev;
> -	const struct backlight_properties props = {
> -		.type = BACKLIGHT_RAW,
> -		.brightness = 255,
> -		.max_brightness = 255,
> -	};
> +	DECLARE_BACKLIGHT_INIT_RAW(props, 255, 255);
>  
>  	return devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
>  					      &tm5p5_nt35596_bl_ops, &props);
> -- 
> 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