[PATCH] backlight/lp855x: Don't clear level on suspend/blank

[PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Sean Paul]

Don't clear the backlight level when we're going into suspend or
blanking. Instead, just temporarily set the level to 0 so we retain
the value when we resume.

Reported-by: Benson Leung <bleung@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 08ae72f..74daf7c 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 static int lp855x_bl_update_status(struct backlight_device *bl)
 {
 	struct lp855x *lp = bl_get_data(bl);
+	int brightness = bl->props.brightness;
 
 	if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
-		bl->props.brightness = 0;
+		brightness = 0;
 
-	if (lp->mode = PWM_BASED) {
-		int br = bl->props.brightness;
-		int max_br = bl->props.max_brightness;
-
-		lp855x_pwm_ctrl(lp, br, max_br);
-
-	} else if (lp->mode = REGISTER_BASED) {
-		u8 val = bl->props.brightness;
-
-		lp855x_write_byte(lp, lp->cfg->reg_brightness, val);
-	}
+	if (lp->mode = PWM_BASED)
+		lp855x_pwm_ctrl(lp, brightness, bl->props.max_brightness);
+	else if (lp->mode = REGISTER_BASED)
+		lp855x_write_byte(lp, lp->cfg->reg_brightness, (u8)brightness);
 
 	return 0;
 }
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Kim, Milo]

Hi Sean,

On 5/12/2015 5:32 AM, Sean Paul wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.

Could you describe what the problem is? I wrote same code in other 
backlight drivers. So maybe I need to look into them as well.

> ---
>   drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 08ae72f..74daf7c 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>   static int lp855x_bl_update_status(struct backlight_device *bl)
>   {
>   	struct lp855x *lp = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
>
>   	if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> -		bl->props.brightness = 0;
> +		brightness = 0;

After that, this driver can't show exact brightness value when the 
application tries to read 'brightness' through the sysfs.
The backlight is off but read value of 'brightness' is non-zero.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/video/backlight/backlight.c?id=refs/tags/v4.1-rc3#n159

Best regards,
Milo

Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Kim, Milo]

Hi Sean,

On 5/14/2015 4:33 AM, Sean Paul wrote:
>
>
> On Mon, May 11, 2015 at 3:48 PM, Kim, Milo <milo.kim@ti.com
> <mailto:milo.kim@ti.com>> wrote:
>
>     Hi Sean,
>
>     On 5/12/2015 5:32 AM, Sean Paul wrote:
>
>         Don't clear the backlight level when we're going into suspend or
>         blanking. Instead, just temporarily set the level to 0 so we retain
>         the value when we resume.
>
>
>     Could you describe what the problem is? I wrote same code in other
>     backlight drivers. So maybe I need to look into them as well.
>
>
> Hi Milo,
> Well, I'm not sure what the expected behavior is, but our userspace
> expects the driver to maintain its brightness across suspend/resume. The
> way things are coded now, the driver will clear the brightness value on
> suspend such that when we resume, the backlight is restored to 0.

Got it. Your patch makes sense. Thanks for catching this.

Best regards,
Milo

Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Kim, Milo]

On 5/12/2015 5:32 AM, Sean Paul wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Acked-by: Milo Kim <milo.kim@ti.com>

Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Benson Leung]

On Mon, May 11, 2015 at 1:32 PM, Sean Paul <seanpaul@chromium.org> wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>


-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank

[Lee Jones]

On Mon, 11 May 2015, Sean Paul wrote:

> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
> 
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)

Applied with Acks, thanks.

> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 08ae72f..74daf7c 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  static int lp855x_bl_update_status(struct backlight_device *bl)
>  {
>  	struct lp855x *lp = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
>  
>  	if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> -		bl->props.brightness = 0;
> +		brightness = 0;
>  
> -	if (lp->mode = PWM_BASED) {
> -		int br = bl->props.brightness;
> -		int max_br = bl->props.max_brightness;
> -
> -		lp855x_pwm_ctrl(lp, br, max_br);
> -
> -	} else if (lp->mode = REGISTER_BASED) {
> -		u8 val = bl->props.brightness;
> -
> -		lp855x_write_byte(lp, lp->cfg->reg_brightness, val);
> -	}
> +	if (lp->mode = PWM_BASED)
> +		lp855x_pwm_ctrl(lp, brightness, bl->props.max_brightness);
> +	else if (lp->mode = REGISTER_BASED)
> +		lp855x_write_byte(lp, lp->cfg->reg_brightness, (u8)brightness);
>  
>  	return 0;
>  }

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog