Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Jani Nikula]

On Wed, 25 Apr 2018, Daniel Vetter  wrote:
> Leaking driver internal tracking into the already massively confusing
> backlight power tracking is really confusing.
>
> Stop that by allocating a tiny driver private data structure instead.
>
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Acked-by: Daniel Thompson 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Jani Nikula 

> ---
> v2:
> - Consistently treating PANDORA_WAS_OFF as a non-bitfield
> - Drop the kfree that I left behind after switching to devm_kmalloc
> ---
>  drivers/video/backlight/pandora_bl.c | 25 ++---
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/pandora_bl.c 
> b/drivers/video/backlight/pandora_bl.c
> index a186bc677c7d..9618766e3866 100644
> --- a/drivers/video/backlight/pandora_bl.c
> +++ b/drivers/video/backlight/pandora_bl.c
> @@ -35,11 +35,15 @@
>  #define MAX_VALUE 63
>  #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
>  
> -#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
> +struct pandora_private {
> + unsigned old_state;
> +#define PANDORABL_WAS_OFF 1
> +};
>  
>  static int pandora_backlight_update_status(struct backlight_device *bl)
>  {
>   int brightness = bl->props.brightness;
> + struct pandora_private *priv = bl_get_data(bl);
>   u8 r;
>  
>   if (bl->props.power != FB_BLANK_UNBLANK)
> @@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>   brightness = MAX_USER_VALUE;
>  
>   if (brightness == 0) {
> - if (bl->props.state & PANDORABL_WAS_OFF)
> + if (priv->old_state == PANDORABL_WAS_OFF)
>   goto done;
>  
>   /* first disable PWM0 output, then clock */
> @@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>   goto done;
>   }
>  
> - if (bl->props.state & PANDORABL_WAS_OFF) {
> + if (priv->old_state == PANDORABL_WAS_OFF) {
>   /*
>* set PWM duty cycle to max. TPS61161 seems to use this
>* to calibrate it's PWM sensitivity when it starts.
> @@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>  
>  done:
>   if (brightness != 0)
> - bl->props.state &= ~PANDORABL_WAS_OFF;
> + priv->old_state = 0;
>   else
> - bl->props.state |= PANDORABL_WAS_OFF;
> + priv->old_state = PANDORABL_WAS_OFF;
>  
>   return 0;
>  }
> @@ -109,13 +113,20 @@ static int pandora_backlight_probe(struct 
> platform_device *pdev)
>  {
>   struct backlight_properties props;
>   struct backlight_device *bl;
> + struct pandora_private *priv;
>   u8 r;
>  
> + priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(>dev, "failed to allocate driver private data\n");
> + return -ENOMEM;
> + }
> +
>   memset(, 0, sizeof(props));
>   props.max_brightness = MAX_USER_VALUE;
>   props.type = BACKLIGHT_RAW;
>   bl = devm_backlight_device_register(>dev, pdev->name, >dev,
> - NULL, _backlight_ops, );
> + priv, _backlight_ops, );
>   if (IS_ERR(bl)) {
>   dev_err(>dev, "failed to register backlight\n");
>   return PTR_ERR(bl);
> @@ -126,7 +137,7 @@ static int pandora_backlight_probe(struct platform_device 
> *pdev)
>   /* 64 cycle period, ON position 0 */
>   twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
>  
> - bl->props.state |= PANDORABL_WAS_OFF;
> + priv->old_state = PANDORABL_WAS_OFF;
>   bl->props.brightness = MAX_USER_VALUE;
>   backlight_update_status(bl);

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Jani Nikula]

On Wed, 25 Apr 2018, Daniel Vetter  wrote:
> Leaking driver internal tracking into the already massively confusing
> backlight power tracking is really confusing.
>
> Stop that by allocating a tiny driver private data structure instead.
>
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Acked-by: Daniel Thompson 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Jani Nikula 

> ---
> v2:
> - Consistently treating PANDORA_WAS_OFF as a non-bitfield
> - Drop the kfree that I left behind after switching to devm_kmalloc
> ---
>  drivers/video/backlight/pandora_bl.c | 25 ++---
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/pandora_bl.c 
> b/drivers/video/backlight/pandora_bl.c
> index a186bc677c7d..9618766e3866 100644
> --- a/drivers/video/backlight/pandora_bl.c
> +++ b/drivers/video/backlight/pandora_bl.c
> @@ -35,11 +35,15 @@
>  #define MAX_VALUE 63
>  #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
>  
> -#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
> +struct pandora_private {
> + unsigned old_state;
> +#define PANDORABL_WAS_OFF 1
> +};
>  
>  static int pandora_backlight_update_status(struct backlight_device *bl)
>  {
>   int brightness = bl->props.brightness;
> + struct pandora_private *priv = bl_get_data(bl);
>   u8 r;
>  
>   if (bl->props.power != FB_BLANK_UNBLANK)
> @@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>   brightness = MAX_USER_VALUE;
>  
>   if (brightness == 0) {
> - if (bl->props.state & PANDORABL_WAS_OFF)
> + if (priv->old_state == PANDORABL_WAS_OFF)
>   goto done;
>  
>   /* first disable PWM0 output, then clock */
> @@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>   goto done;
>   }
>  
> - if (bl->props.state & PANDORABL_WAS_OFF) {
> + if (priv->old_state == PANDORABL_WAS_OFF) {
>   /*
>* set PWM duty cycle to max. TPS61161 seems to use this
>* to calibrate it's PWM sensitivity when it starts.
> @@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
> backlight_device *bl)
>  
>  done:
>   if (brightness != 0)
> - bl->props.state &= ~PANDORABL_WAS_OFF;
> + priv->old_state = 0;
>   else
> - bl->props.state |= PANDORABL_WAS_OFF;
> + priv->old_state = PANDORABL_WAS_OFF;
>  
>   return 0;
>  }
> @@ -109,13 +113,20 @@ static int pandora_backlight_probe(struct 
> platform_device *pdev)
>  {
>   struct backlight_properties props;
>   struct backlight_device *bl;
> + struct pandora_private *priv;
>   u8 r;
>  
> + priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(>dev, "failed to allocate driver private data\n");
> + return -ENOMEM;
> + }
> +
>   memset(, 0, sizeof(props));
>   props.max_brightness = MAX_USER_VALUE;
>   props.type = BACKLIGHT_RAW;
>   bl = devm_backlight_device_register(>dev, pdev->name, >dev,
> - NULL, _backlight_ops, );
> + priv, _backlight_ops, );
>   if (IS_ERR(bl)) {
>   dev_err(>dev, "failed to register backlight\n");
>   return PTR_ERR(bl);
> @@ -126,7 +137,7 @@ static int pandora_backlight_probe(struct platform_device 
> *pdev)
>   /* 64 cycle period, ON position 0 */
>   twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
>  
> - bl->props.state |= PANDORABL_WAS_OFF;
> + priv->old_state = PANDORABL_WAS_OFF;
>   bl->props.brightness = MAX_USER_VALUE;
>   backlight_update_status(bl);

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Acked-by: Daniel Thompson 
Signed-off-by: Daniel Vetter 
---
v2:
- Consistently treating PANDORA_WAS_OFF as a non-bitfield
- Drop the kfree that I left behind after switching to devm_kmalloc
---
 drivers/video/backlight/pandora_bl.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..9618766e3866 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
 #define MAX_VALUE 63
 #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
 
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1
+};
 
 static int pandora_backlight_update_status(struct backlight_device *bl)
 {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
 
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
 
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state == PANDORABL_WAS_OFF)
goto done;
 
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
 
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state == PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
 
 done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state = 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
 
return 0;
 }
@@ -109,13 +113,20 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
 {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
 
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
return PTR_ERR(bl);
@@ -126,7 +137,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
 
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
 
-- 
2.17.0

[PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Acked-by: Daniel Thompson 
Signed-off-by: Daniel Vetter 
---
v2:
- Consistently treating PANDORA_WAS_OFF as a non-bitfield
- Drop the kfree that I left behind after switching to devm_kmalloc
---
 drivers/video/backlight/pandora_bl.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..9618766e3866 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
 #define MAX_VALUE 63
 #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
 
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1
+};
 
 static int pandora_backlight_update_status(struct backlight_device *bl)
 {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
 
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
 
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state == PANDORABL_WAS_OFF)
goto done;
 
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
 
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state == PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
 
 done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state = 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
 
return 0;
 }
@@ -109,13 +113,20 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
 {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
 
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
return PTR_ERR(bl);
@@ -126,7 +137,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
 
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
 
-- 
2.17.0

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 21:38, Daniel Vetter wrote:

Thanks a lot for your comments.

On Wed, Jan 17, 2018 at 04:47:41PM +, Daniel Thompson wrote:

On 17/01/18 14:01, Daniel Vetter wrote:

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
   drivers/video/backlight/pandora_bl.c | 26 +++---
   1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
   #define MAX_VALUE 63
   #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1


Nit, but we using old_state like a bitfield so, BIT(0)?



+};
   static int pandora_backlight_update_status(struct backlight_device *bl)
   {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
   done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;


Well, we were using it like a bitfield until this bit...


I had a simple boolean first (because that's all we neeed), but that made
the code less readable. Should I s/1/true/ in the #define? The entire C99
bool tends to be a bit a bikeshed sometimes :-)


I'd prefer a simple boolean or just storing old_brightness directly in 
priv... it is only ever consumed as a boolean anyway. I'd have to say 
I'm not keen on #define true alongside the existing bitmask checks.


However... I'm happy to leave that with you. Providing the spurious 
kfree() is removed then please feel free to repost with:


Acked-by: Daniel Thompson 


Daniel.











return 0;
   }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
   {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);


Why can't we rely on devres for cleanup?


Argh, I had kmalloc first and then changed to devm_kmalloc. The kfree here
needs to go indeed.

Cheers, Daniel





return PTR_ERR(bl);
}
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 21:38, Daniel Vetter wrote:

Thanks a lot for your comments.

On Wed, Jan 17, 2018 at 04:47:41PM +, Daniel Thompson wrote:

On 17/01/18 14:01, Daniel Vetter wrote:

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
   drivers/video/backlight/pandora_bl.c | 26 +++---
   1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
   #define MAX_VALUE 63
   #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1


Nit, but we using old_state like a bitfield so, BIT(0)?



+};
   static int pandora_backlight_update_status(struct backlight_device *bl)
   {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
   done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;


Well, we were using it like a bitfield until this bit...


I had a simple boolean first (because that's all we neeed), but that made
the code less readable. Should I s/1/true/ in the #define? The entire C99
bool tends to be a bit a bikeshed sometimes :-)


I'd prefer a simple boolean or just storing old_brightness directly in 
priv... it is only ever consumed as a boolean anyway. I'd have to say 
I'm not keen on #define true alongside the existing bitmask checks.


However... I'm happy to leave that with you. Providing the spurious 
kfree() is removed then please feel free to repost with:


Acked-by: Daniel Thompson 


Daniel.











return 0;
   }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
   {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);


Why can't we rely on devres for cleanup?


Argh, I had kmalloc first and then changed to devm_kmalloc. The kfree here
needs to go indeed.

Cheers, Daniel





return PTR_ERR(bl);
}
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Thanks a lot for your comments.

On Wed, Jan 17, 2018 at 04:47:41PM +, Daniel Thompson wrote:
> On 17/01/18 14:01, Daniel Vetter wrote:
> > Leaking driver internal tracking into the already massively confusing
> > backlight power tracking is really confusing.
> > 
> > Stop that by allocating a tiny driver private data structure instead.
> > 
> > Cc: Lee Jones 
> > Cc: Daniel Thompson 
> > Cc: Jingoo Han 
> > Signed-off-by: Daniel Vetter 
> > ---
> >   drivers/video/backlight/pandora_bl.c | 26 +++---
> >   1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pandora_bl.c 
> > b/drivers/video/backlight/pandora_bl.c
> > index a186bc677c7d..6bd159946a47 100644
> > --- a/drivers/video/backlight/pandora_bl.c
> > +++ b/drivers/video/backlight/pandora_bl.c
> > @@ -35,11 +35,15 @@
> >   #define MAX_VALUE 63
> >   #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
> > -#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
> > +struct pandora_private {
> > +   unsigned old_state;
> > +#define PANDORABL_WAS_OFF 1
> 
> Nit, but we using old_state like a bitfield so, BIT(0)?
> 
> 
> > +};
> >   static int pandora_backlight_update_status(struct backlight_device *bl)
> >   {
> > int brightness = bl->props.brightness;
> > +   struct pandora_private *priv = bl_get_data(bl);
> > u8 r;
> > if (bl->props.power != FB_BLANK_UNBLANK)
> > @@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> > brightness = MAX_USER_VALUE;
> > if (brightness == 0) {
> > -   if (bl->props.state & PANDORABL_WAS_OFF)
> > +   if (priv->old_state & PANDORABL_WAS_OFF)
> > goto done;
> > /* first disable PWM0 output, then clock */
> > @@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> > goto done;
> > }
> > -   if (bl->props.state & PANDORABL_WAS_OFF) {
> > +   if (priv->old_state & PANDORABL_WAS_OFF) {
> > /*
> >  * set PWM duty cycle to max. TPS61161 seems to use this
> >  * to calibrate it's PWM sensitivity when it starts.
> > @@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> >   done:
> > if (brightness != 0)
> > -   bl->props.state &= ~PANDORABL_WAS_OFF;
> > +   priv->old_state 0;
> > else
> > -   bl->props.state |= PANDORABL_WAS_OFF;
> > +   priv->old_state = PANDORABL_WAS_OFF;
> 
> Well, we were using it like a bitfield until this bit...

I had a simple boolean first (because that's all we neeed), but that made
the code less readable. Should I s/1/true/ in the #define? The entire C99
bool tends to be a bit a bikeshed sometimes :-)

> 
> 
> > return 0;
> >   }
> > @@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct 
> > platform_device *pdev)
> >   {
> > struct backlight_properties props;
> > struct backlight_device *bl;
> > +   struct pandora_private *priv;
> > u8 r;
> > +   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv) {
> > +   dev_err(>dev, "failed to allocate driver private data\n");
> > +   return -ENOMEM;
> > +   }
> > +
> > memset(, 0, sizeof(props));
> > props.max_brightness = MAX_USER_VALUE;
> > props.type = BACKLIGHT_RAW;
> > bl = devm_backlight_device_register(>dev, pdev->name, >dev,
> > -   NULL, _backlight_ops, );
> > +   priv, _backlight_ops, );
> > if (IS_ERR(bl)) {
> > dev_err(>dev, "failed to register backlight\n");
> > +   kfree(priv);
> 
> Why can't we rely on devres for cleanup?

Argh, I had kmalloc first and then changed to devm_kmalloc. The kfree here
needs to go indeed.

Cheers, Daniel

> 
> 
> > return PTR_ERR(bl);
> > }
> > @@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct 
> > platform_device *pdev)
> > /* 64 cycle period, ON position 0 */
> > twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
> > -   bl->props.state |= PANDORABL_WAS_OFF;
> > +   priv->old_state = PANDORABL_WAS_OFF;
> > bl->props.brightness = MAX_USER_VALUE;
> > backlight_update_status(bl);
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Thanks a lot for your comments.

On Wed, Jan 17, 2018 at 04:47:41PM +, Daniel Thompson wrote:
> On 17/01/18 14:01, Daniel Vetter wrote:
> > Leaking driver internal tracking into the already massively confusing
> > backlight power tracking is really confusing.
> > 
> > Stop that by allocating a tiny driver private data structure instead.
> > 
> > Cc: Lee Jones 
> > Cc: Daniel Thompson 
> > Cc: Jingoo Han 
> > Signed-off-by: Daniel Vetter 
> > ---
> >   drivers/video/backlight/pandora_bl.c | 26 +++---
> >   1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pandora_bl.c 
> > b/drivers/video/backlight/pandora_bl.c
> > index a186bc677c7d..6bd159946a47 100644
> > --- a/drivers/video/backlight/pandora_bl.c
> > +++ b/drivers/video/backlight/pandora_bl.c
> > @@ -35,11 +35,15 @@
> >   #define MAX_VALUE 63
> >   #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
> > -#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
> > +struct pandora_private {
> > +   unsigned old_state;
> > +#define PANDORABL_WAS_OFF 1
> 
> Nit, but we using old_state like a bitfield so, BIT(0)?
> 
> 
> > +};
> >   static int pandora_backlight_update_status(struct backlight_device *bl)
> >   {
> > int brightness = bl->props.brightness;
> > +   struct pandora_private *priv = bl_get_data(bl);
> > u8 r;
> > if (bl->props.power != FB_BLANK_UNBLANK)
> > @@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> > brightness = MAX_USER_VALUE;
> > if (brightness == 0) {
> > -   if (bl->props.state & PANDORABL_WAS_OFF)
> > +   if (priv->old_state & PANDORABL_WAS_OFF)
> > goto done;
> > /* first disable PWM0 output, then clock */
> > @@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> > goto done;
> > }
> > -   if (bl->props.state & PANDORABL_WAS_OFF) {
> > +   if (priv->old_state & PANDORABL_WAS_OFF) {
> > /*
> >  * set PWM duty cycle to max. TPS61161 seems to use this
> >  * to calibrate it's PWM sensitivity when it starts.
> > @@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
> > backlight_device *bl)
> >   done:
> > if (brightness != 0)
> > -   bl->props.state &= ~PANDORABL_WAS_OFF;
> > +   priv->old_state 0;
> > else
> > -   bl->props.state |= PANDORABL_WAS_OFF;
> > +   priv->old_state = PANDORABL_WAS_OFF;
> 
> Well, we were using it like a bitfield until this bit...

I had a simple boolean first (because that's all we neeed), but that made
the code less readable. Should I s/1/true/ in the #define? The entire C99
bool tends to be a bit a bikeshed sometimes :-)

> 
> 
> > return 0;
> >   }
> > @@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct 
> > platform_device *pdev)
> >   {
> > struct backlight_properties props;
> > struct backlight_device *bl;
> > +   struct pandora_private *priv;
> > u8 r;
> > +   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv) {
> > +   dev_err(>dev, "failed to allocate driver private data\n");
> > +   return -ENOMEM;
> > +   }
> > +
> > memset(, 0, sizeof(props));
> > props.max_brightness = MAX_USER_VALUE;
> > props.type = BACKLIGHT_RAW;
> > bl = devm_backlight_device_register(>dev, pdev->name, >dev,
> > -   NULL, _backlight_ops, );
> > +   priv, _backlight_ops, );
> > if (IS_ERR(bl)) {
> > dev_err(>dev, "failed to register backlight\n");
> > +   kfree(priv);
> 
> Why can't we rely on devres for cleanup?

Argh, I had kmalloc first and then changed to devm_kmalloc. The kfree here
needs to go indeed.

Cheers, Daniel

> 
> 
> > return PTR_ERR(bl);
> > }
> > @@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct 
> > platform_device *pdev)
> > /* 64 cycle period, ON position 0 */
> > twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
> > -   bl->props.state |= PANDORABL_WAS_OFF;
> > +   priv->old_state = PANDORABL_WAS_OFF;
> > bl->props.brightness = MAX_USER_VALUE;
> > backlight_update_status(bl);
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter wrote:

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
  drivers/video/backlight/pandora_bl.c | 26 +++---
  1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
  #define MAX_VALUE 63
  #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
  
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1

+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1


Nit, but we using old_state like a bitfield so, BIT(0)?



+};
  
  static int pandora_backlight_update_status(struct backlight_device *bl)

  {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
  
  	if (bl->props.power != FB_BLANK_UNBLANK)

@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
  
  	if (brightness == 0) {

-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
  
  		/* first disable PWM0 output, then clock */

@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
  
-	if (bl->props.state & PANDORABL_WAS_OFF) {

+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
  
  done:

if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;


Well, we were using it like a bitfield until this bit...


  
  	return 0;

  }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
  {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
  
+	priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);

+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);


Why can't we rely on devres for cleanup?



return PTR_ERR(bl);
}
  
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device *pdev)

/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
  
-	bl->props.state |= PANDORABL_WAS_OFF;

+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
  

Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter wrote:

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
  drivers/video/backlight/pandora_bl.c | 26 +++---
  1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
  #define MAX_VALUE 63
  #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
  
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1

+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1


Nit, but we using old_state like a bitfield so, BIT(0)?



+};
  
  static int pandora_backlight_update_status(struct backlight_device *bl)

  {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
  
  	if (bl->props.power != FB_BLANK_UNBLANK)

@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
  
  	if (brightness == 0) {

-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
  
  		/* first disable PWM0 output, then clock */

@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
  
-	if (bl->props.state & PANDORABL_WAS_OFF) {

+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
  
  done:

if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;


Well, we were using it like a bitfield until this bit...


  
  	return 0;

  }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
  {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
  
+	priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);

+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);


Why can't we rely on devres for cleanup?



return PTR_ERR(bl);
}
  
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device *pdev)

/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
  
-	bl->props.state |= PANDORABL_WAS_OFF;

+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
  

[PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
 drivers/video/backlight/pandora_bl.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
 #define MAX_VALUE 63
 #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
 
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1
+};
 
 static int pandora_backlight_update_status(struct backlight_device *bl)
 {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
 
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
 
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
 
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
 
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
 
 done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
 
return 0;
 }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
 {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
 
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);
return PTR_ERR(bl);
}
 
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
 
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
 
-- 
2.15.1

[PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.

Stop that by allocating a tiny driver private data structure instead.

Cc: Lee Jones 
Cc: Daniel Thompson 
Cc: Jingoo Han 
Signed-off-by: Daniel Vetter 
---
 drivers/video/backlight/pandora_bl.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/video/backlight/pandora_bl.c 
b/drivers/video/backlight/pandora_bl.c
index a186bc677c7d..6bd159946a47 100644
--- a/drivers/video/backlight/pandora_bl.c
+++ b/drivers/video/backlight/pandora_bl.c
@@ -35,11 +35,15 @@
 #define MAX_VALUE 63
 #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
 
-#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
+struct pandora_private {
+   unsigned old_state;
+#define PANDORABL_WAS_OFF 1
+};
 
 static int pandora_backlight_update_status(struct backlight_device *bl)
 {
int brightness = bl->props.brightness;
+   struct pandora_private *priv = bl_get_data(bl);
u8 r;
 
if (bl->props.power != FB_BLANK_UNBLANK)
@@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
brightness = MAX_USER_VALUE;
 
if (brightness == 0) {
-   if (bl->props.state & PANDORABL_WAS_OFF)
+   if (priv->old_state & PANDORABL_WAS_OFF)
goto done;
 
/* first disable PWM0 output, then clock */
@@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
goto done;
}
 
-   if (bl->props.state & PANDORABL_WAS_OFF) {
+   if (priv->old_state & PANDORABL_WAS_OFF) {
/*
 * set PWM duty cycle to max. TPS61161 seems to use this
 * to calibrate it's PWM sensitivity when it starts.
@@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct 
backlight_device *bl)
 
 done:
if (brightness != 0)
-   bl->props.state &= ~PANDORABL_WAS_OFF;
+   priv->old_state 0;
else
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
 
return 0;
 }
@@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
 {
struct backlight_properties props;
struct backlight_device *bl;
+   struct pandora_private *priv;
u8 r;
 
+   priv = devm_kmalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   dev_err(>dev, "failed to allocate driver private data\n");
+   return -ENOMEM;
+   }
+
memset(, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(>dev, pdev->name, >dev,
-   NULL, _backlight_ops, );
+   priv, _backlight_ops, );
if (IS_ERR(bl)) {
dev_err(>dev, "failed to register backlight\n");
+   kfree(priv);
return PTR_ERR(bl);
}
 
@@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device 
*pdev)
/* 64 cycle period, ON position 0 */
twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
 
-   bl->props.state |= PANDORABL_WAS_OFF;
+   priv->old_state = PANDORABL_WAS_OFF;
bl->props.brightness = MAX_USER_VALUE;
backlight_update_status(bl);
 
-- 
2.15.1