The backlight power state handling is supremely confusing. We have:
- props.power, using FB_BLANK_* defines
- props.fb_blank, using the same, but deprecated int favour of
props.state
- props.state, using the BL_CORE_* defines
- and finally a bunch of backlight drivers treat brightness == 0 as
off. But of course not all of them.
This is way too much confusion to fix in a simple patch, but at least
prevent more hilarity from spreading by removing the unused BL_CORE_*
defines. I have no idea why exactly anyone would need that.
Wrt the ideal state, we really just want a boolean state. The 4 power
saving states that the fbdev subsystem uses are overkill in todays hw
(this was only relevant for VGA and similar analog circuits like
TV-out), the new drm atomic modeset api simplified even the uapi to a
simple bool. And there was never a valid technical reason to have the
intermediate fbdev power states for backlights (those really only can
be either off or on).
Cleanup motivated by Meghana's questions about all this.
Cc: Lee Jones <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Meghana Madhyastha <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
include/linux/backlight.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 2baab6f3861d..1db67662bfcb 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -84,9 +84,6 @@ struct backlight_properties {
#define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
#define BL_CORE_FBBLANK (1 << 1) /* backlight is under an fb blank event */
-#define BL_CORE_DRIVER4 (1 << 28) /* reserved for driver specific use */
-#define BL_CORE_DRIVER3 (1 << 29) /* reserved for driver specific use */
-#define BL_CORE_DRIVER2 (1 << 30) /* reserved for driver specific use */
#define BL_CORE_DRIVER1 (1 << 31) /* reserved for driver specific use */
};
--
2.17.0
Nothing in the entire tree ever sets this, which means this is dead
code. Remove it.
Cc: Lee Jones <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
drivers/video/backlight/generic_bl.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/video/backlight/generic_bl.c b/drivers/video/backlight/generic_bl.c
index 67dfb939a514..4dea91acea13 100644
--- a/drivers/video/backlight/generic_bl.c
+++ b/drivers/video/backlight/generic_bl.c
@@ -21,9 +21,6 @@ static int genericbl_intensity;
static struct backlight_device *generic_backlight_device;
static struct generic_bl_info *bl_machinfo;
-/* Flag to signal when the battery is low */
-#define GENERICBL_BATTLOW BL_CORE_DRIVER1
-
static int genericbl_send_intensity(struct backlight_device *bd)
{
int intensity = bd->props.brightness;
@@ -34,8 +31,6 @@ static int genericbl_send_intensity(struct backlight_device *bd)
intensity = 0;
if (bd->props.state & BL_CORE_SUSPENDED)
intensity = 0;
- if (bd->props.state & GENERICBL_BATTLOW)
- intensity &= bl_machinfo->limit_mask;
bl_machinfo->set_bl_intensity(intensity);
--
2.17.0
For the same reasons we've added dri-devel for all fbdev patches: Most
of the actively developed drivers using this infrastructure are in
drivers/gpu/. It just makes sense to cross-post patches and keep
aligned. And total activity in the backlight subsystem is miniscule
compared to drm overall.
Cc: Lee Jones <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fa72c117bcd5..0f902399a348 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2589,6 +2589,7 @@ BACKLIGHT CLASS/SUBSYSTEM
M: Lee Jones <[email protected]>
M: Daniel Thompson <[email protected]>
M: Jingoo Han <[email protected]>
+L: [email protected]
T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
S: Maintained
F: drivers/video/backlight/
--
2.17.0
Now that the 3 drivers using this are cleaned up we can also remove
this final bit of confusion of leaking driver internals into the
backlight power interface.
The backlight power interface itself is still a massive mess.
Cc: Lee Jones <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
include/linux/backlight.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 1db67662bfcb..7fbf0539e14a 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -84,7 +84,6 @@ struct backlight_properties {
#define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
#define BL_CORE_FBBLANK (1 << 1) /* backlight is under an fb blank event */
-#define BL_CORE_DRIVER1 (1 << 31) /* reserved for driver specific use */
};
--
2.17.0
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 <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
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(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ dev_err(&pdev->dev, "failed to allocate driver private data\n");
+ return -ENOMEM;
+ }
+
memset(&props, 0, sizeof(props));
props.max_brightness = MAX_USER_VALUE;
props.type = BACKLIGHT_RAW;
bl = devm_backlight_device_register(&pdev->dev, pdev->name, &pdev->dev,
- NULL, &pandora_backlight_ops, &props);
+ priv, &pandora_backlight_ops, &props);
if (IS_ERR(bl)) {
dev_err(&pdev->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
Leaking driver internal tracking into the already massively confusing
backlight power tracking is really confusing.
Luckily we have already a drvdata structure, so fixing this is really
easy.
Cc: Lee Jones <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Jingoo Han <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
---
drivers/staging/fbtft/fbtft-core.c | 4 ++--
drivers/staging/fbtft/fbtft.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 0e36b66ae5f7..731e47149af8 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -246,7 +246,7 @@ static int fbtft_request_gpios_dt(struct fbtft_par *par)
static int fbtft_backlight_update_status(struct backlight_device *bd)
{
struct fbtft_par *par = bl_get_data(bd);
- bool polarity = !!(bd->props.state & BL_CORE_DRIVER1);
+ bool polarity = par->polarity;
fbtft_par_dbg(DEBUG_BACKLIGHT, par,
"%s: polarity=%d, power=%d, fb_blank=%d\n",
@@ -296,7 +296,7 @@ void fbtft_register_backlight(struct fbtft_par *par)
/* Assume backlight is off, get polarity from current state of pin */
bl_props.power = FB_BLANK_POWERDOWN;
if (!gpio_get_value(par->gpio.led[0]))
- bl_props.state |= BL_CORE_DRIVER1;
+ par->polarity = true;
bd = backlight_device_register(dev_driver_string(par->info->device),
par->info->device, par,
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index e19e64e0d094..c7cb4a7896f4 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -229,6 +229,7 @@ struct fbtft_par {
ktime_t update_time;
bool bgr;
void *extra;
+ bool polarity;
};
#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))
--
2.17.0
On Wednesday, April 25, 2018 1:43 PM, Daniel Vetter wrote:
>
> The backlight power state handling is supremely confusing. We have:
> - props.power, using FB_BLANK_* defines
> - props.fb_blank, using the same, but deprecated int favour of
> props.state
> - props.state, using the BL_CORE_* defines
> - and finally a bunch of backlight drivers treat brightness == 0 as
> off. But of course not all of them.
>
> This is way too much confusion to fix in a simple patch, but at least
> prevent more hilarity from spreading by removing the unused BL_CORE_*
> defines. I have no idea why exactly anyone would need that.
>
> Wrt the ideal state, we really just want a boolean state. The 4 power
> saving states that the fbdev subsystem uses are overkill in todays hw
> (this was only relevant for VGA and similar analog circuits like
> TV-out), the new drm atomic modeset api simplified even the uapi to a
> simple bool. And there was never a valid technical reason to have the
> intermediate fbdev power states for backlights (those really only can
> be either off or on).
>
> Cleanup motivated by Meghana's questions about all this.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Meghana Madhyastha <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Acked-by: Jingoo Han <[email protected]>
I really love this patch!
Good job!
Thank you.
Best regards,
Jingoo Han
> ---
> include/linux/backlight.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 2baab6f3861d..1db67662bfcb 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -84,9 +84,6 @@ struct backlight_properties {
>
> #define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
> #define BL_CORE_FBBLANK (1 << 1) /* backlight is
under an fb
> blank event */
> -#define BL_CORE_DRIVER4 (1 << 28) /* reserved for
driver
> specific use */
> -#define BL_CORE_DRIVER3 (1 << 29) /* reserved for
driver
> specific use */
> -#define BL_CORE_DRIVER2 (1 << 30) /* reserved for
driver
> specific use */
> #define BL_CORE_DRIVER1 (1 << 31) /* reserved for
driver
> specific use */
>
> };
> --
> 2.17.0
On Wednesday, April 25, 2018 1:43 PM, Daniel Vetter wrote:
>
> For the same reasons we've added dri-devel for all fbdev patches: Most
> of the actively developed drivers using this infrastructure are in
> drivers/gpu/. It just makes sense to cross-post patches and keep
> aligned. And total activity in the backlight subsystem is miniscule
> compared to drm overall.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Acked-by: Jingoo Han <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Best regards,
Jingoo Han
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa72c117bcd5..0f902399a348 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2589,6 +2589,7 @@ BACKLIGHT CLASS/SUBSYSTEM
> M: Lee Jones <[email protected]>
> M: Daniel Thompson <[email protected]>
> M: Jingoo Han <[email protected]>
> +L: [email protected]
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
> S: Maintained
> F: drivers/video/backlight/
> --
> 2.17.0
Greg, Thomas,
On Wed, 25 Apr 2018, Daniel Vetter wrote:
> Leaking driver internal tracking into the already massively confusing
> backlight power tracking is really confusing.
>
> Luckily we have already a drvdata structure, so fixing this is really
> easy.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> ---
> drivers/staging/fbtft/fbtft-core.c | 4 ++--
> drivers/staging/fbtft/fbtft.h | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
Do you want a pull-request for this patch or can I just take it?
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index 0e36b66ae5f7..731e47149af8 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -246,7 +246,7 @@ static int fbtft_request_gpios_dt(struct fbtft_par *par)
> static int fbtft_backlight_update_status(struct backlight_device *bd)
> {
> struct fbtft_par *par = bl_get_data(bd);
> - bool polarity = !!(bd->props.state & BL_CORE_DRIVER1);
> + bool polarity = par->polarity;
>
> fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> "%s: polarity=%d, power=%d, fb_blank=%d\n",
> @@ -296,7 +296,7 @@ void fbtft_register_backlight(struct fbtft_par *par)
> /* Assume backlight is off, get polarity from current state of pin */
> bl_props.power = FB_BLANK_POWERDOWN;
> if (!gpio_get_value(par->gpio.led[0]))
> - bl_props.state |= BL_CORE_DRIVER1;
> + par->polarity = true;
>
> bd = backlight_device_register(dev_driver_string(par->info->device),
> par->info->device, par,
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index e19e64e0d094..c7cb4a7896f4 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -229,6 +229,7 @@ struct fbtft_par {
> ktime_t update_time;
> bool bgr;
> void *extra;
> + bool polarity;
> };
>
> #define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> wrote:
> The backlight power state handling is supremely confusing. We have:
> - props.power, using FB_BLANK_* defines
> - props.fb_blank, using the same, but deprecated int favour of
> props.state
> - props.state, using the BL_CORE_* defines
> - and finally a bunch of backlight drivers treat brightness == 0 as
> off. But of course not all of them.
>
> This is way too much confusion to fix in a simple patch, but at least
> prevent more hilarity from spreading by removing the unused BL_CORE_*
> defines. I have no idea why exactly anyone would need that.
>
> Wrt the ideal state, we really just want a boolean state. The 4 power
> saving states that the fbdev subsystem uses are overkill in todays hw
> (this was only relevant for VGA and similar analog circuits like
> TV-out), the new drm atomic modeset api simplified even the uapi to a
> simple bool. And there was never a valid technical reason to have the
> intermediate fbdev power states for backlights (those really only can
> be either off or on).
>
> Cleanup motivated by Meghana's questions about all this.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Meghana Madhyastha <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Reviewed-by: Jani Nikula <[email protected]>
> ---
> include/linux/backlight.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 2baab6f3861d..1db67662bfcb 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -84,9 +84,6 @@ struct backlight_properties {
>
> #define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
> #define BL_CORE_FBBLANK (1 << 1) /* backlight is under an fb blank event */
> -#define BL_CORE_DRIVER4 (1 << 28) /* reserved for driver specific use */
> -#define BL_CORE_DRIVER3 (1 << 29) /* reserved for driver specific use */
> -#define BL_CORE_DRIVER2 (1 << 30) /* reserved for driver specific use */
> #define BL_CORE_DRIVER1 (1 << 31) /* reserved for driver specific use */
>
> };
--
Jani Nikula, Intel Open Source Technology Center
On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> 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 <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Reviewed-by: Jani Nikula <[email protected]>
> ---
> 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(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(&pdev->dev, "failed to allocate driver private data\n");
> + return -ENOMEM;
> + }
> +
> memset(&props, 0, sizeof(props));
> props.max_brightness = MAX_USER_VALUE;
> props.type = BACKLIGHT_RAW;
> bl = devm_backlight_device_register(&pdev->dev, pdev->name, &pdev->dev,
> - NULL, &pandora_backlight_ops, &props);
> + priv, &pandora_backlight_ops, &props);
> if (IS_ERR(bl)) {
> dev_err(&pdev->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
On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Reviewed-by: Jani Nikula <[email protected]>
> ---
> drivers/video/backlight/generic_bl.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/video/backlight/generic_bl.c b/drivers/video/backlight/generic_bl.c
> index 67dfb939a514..4dea91acea13 100644
> --- a/drivers/video/backlight/generic_bl.c
> +++ b/drivers/video/backlight/generic_bl.c
> @@ -21,9 +21,6 @@ static int genericbl_intensity;
> static struct backlight_device *generic_backlight_device;
> static struct generic_bl_info *bl_machinfo;
>
> -/* Flag to signal when the battery is low */
> -#define GENERICBL_BATTLOW BL_CORE_DRIVER1
> -
> static int genericbl_send_intensity(struct backlight_device *bd)
> {
> int intensity = bd->props.brightness;
> @@ -34,8 +31,6 @@ static int genericbl_send_intensity(struct backlight_device *bd)
> intensity = 0;
> if (bd->props.state & BL_CORE_SUSPENDED)
> intensity = 0;
> - if (bd->props.state & GENERICBL_BATTLOW)
> - intensity &= bl_machinfo->limit_mask;
>
> bl_machinfo->set_bl_intensity(intensity);
--
Jani Nikula, Intel Open Source Technology Center
On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> wrote:
> Leaking driver internal tracking into the already massively confusing
> backlight power tracking is really confusing.
>
> Luckily we have already a drvdata structure, so fixing this is really
> easy.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
Reviewed-by: Jani Nikula <[email protected]>
> ---
> drivers/staging/fbtft/fbtft-core.c | 4 ++--
> drivers/staging/fbtft/fbtft.h | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index 0e36b66ae5f7..731e47149af8 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -246,7 +246,7 @@ static int fbtft_request_gpios_dt(struct fbtft_par *par)
> static int fbtft_backlight_update_status(struct backlight_device *bd)
> {
> struct fbtft_par *par = bl_get_data(bd);
> - bool polarity = !!(bd->props.state & BL_CORE_DRIVER1);
> + bool polarity = par->polarity;
>
> fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> "%s: polarity=%d, power=%d, fb_blank=%d\n",
> @@ -296,7 +296,7 @@ void fbtft_register_backlight(struct fbtft_par *par)
> /* Assume backlight is off, get polarity from current state of pin */
> bl_props.power = FB_BLANK_POWERDOWN;
> if (!gpio_get_value(par->gpio.led[0]))
> - bl_props.state |= BL_CORE_DRIVER1;
> + par->polarity = true;
>
> bd = backlight_device_register(dev_driver_string(par->info->device),
> par->info->device, par,
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index e19e64e0d094..c7cb4a7896f4 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -229,6 +229,7 @@ struct fbtft_par {
> ktime_t update_time;
> bool bgr;
> void *extra;
> + bool polarity;
> };
>
> #define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))
--
Jani Nikula, Intel Open Source Technology Center
On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> wrote:
> Now that the 3 drivers using this are cleaned up we can also remove
> this final bit of confusion of leaking driver internals into the
> backlight power interface.
>
> The backlight power interface itself is still a massive mess.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Jingoo Han <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> ---
> include/linux/backlight.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 1db67662bfcb..7fbf0539e14a 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -84,7 +84,6 @@ struct backlight_properties {
>
> #define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
> #define BL_CORE_FBBLANK (1 << 1) /* backlight is under an fb blank event */
> -#define BL_CORE_DRIVER1 (1 << 31) /* reserved for driver specific use */
Please also remove the
/* Upper 4 bits are reserved for driver internal use */
comment a few lines up to not give anyone ideas.
With that,
Reviewed-by: Jani Nikula <[email protected]>
>
> };
--
Jani Nikula, Intel Open Source Technology Center
On Mon, Apr 30, 2018 at 10:54:15AM +0100, Lee Jones wrote:
> Greg, Thomas,
>
> On Wed, 25 Apr 2018, Daniel Vetter wrote:
> > Leaking driver internal tracking into the already massively confusing
> > backlight power tracking is really confusing.
> >
> > Luckily we have already a drvdata structure, so fixing this is really
> > easy.
> >
> > Cc: Lee Jones <[email protected]>
> > Cc: Daniel Thompson <[email protected]>
> > Cc: Jingoo Han <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>
> > Acked-by: Daniel Thompson <[email protected]>
> > Signed-off-by: Daniel Vetter <[email protected]>
> > ---
> > drivers/staging/fbtft/fbtft-core.c | 4 ++--
> > drivers/staging/fbtft/fbtft.h | 1 +
> > 2 files changed, 3 insertions(+), 2 deletions(-)
>
> Do you want a pull-request for this patch or can I just take it?
Please take it if you need to take the whole series.
Acked-by: Greg Kroah-Hartman <[email protected]>
On Mon, 30 Apr 2018, Jani Nikula wrote:
> On Wed, 25 Apr 2018, Daniel Vetter <[email protected]> wrote:
> > Now that the 3 drivers using this are cleaned up we can also remove
> > this final bit of confusion of leaking driver internals into the
> > backlight power interface.
> >
> > The backlight power interface itself is still a massive mess.
> >
> > Cc: Lee Jones <[email protected]>
> > Cc: Daniel Thompson <[email protected]>
> > Cc: Jingoo Han <[email protected]>
> > Acked-by: Daniel Thompson <[email protected]>
> > Signed-off-by: Daniel Vetter <[email protected]>
> > ---
> > include/linux/backlight.h | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> > index 1db67662bfcb..7fbf0539e14a 100644
> > --- a/include/linux/backlight.h
> > +++ b/include/linux/backlight.h
> > @@ -84,7 +84,6 @@ struct backlight_properties {
> >
> > #define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
> > #define BL_CORE_FBBLANK (1 << 1) /* backlight is under an fb blank event */
> > -#define BL_CORE_DRIVER1 (1 << 31) /* reserved for driver specific use */
>
> Please also remove the
>
> /* Upper 4 bits are reserved for driver internal use */
>
> comment a few lines up to not give anyone ideas.
>
> With that,
>
> Reviewed-by: Jani Nikula <[email protected]>
I'm merging these patches (with Jani's Acks) now.
Please send this request as a subsequent patch.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 25 Apr 2018, Daniel Vetter wrote:
> The backlight power state handling is supremely confusing. We have:
> - props.power, using FB_BLANK_* defines
> - props.fb_blank, using the same, but deprecated int favour of
> props.state
> - props.state, using the BL_CORE_* defines
> - and finally a bunch of backlight drivers treat brightness == 0 as
> off. But of course not all of them.
>
> This is way too much confusion to fix in a simple patch, but at least
> prevent more hilarity from spreading by removing the unused BL_CORE_*
> defines. I have no idea why exactly anyone would need that.
>
> Wrt the ideal state, we really just want a boolean state. The 4 power
> saving states that the fbdev subsystem uses are overkill in todays hw
> (this was only relevant for VGA and similar analog circuits like
> TV-out), the new drm atomic modeset api simplified even the uapi to a
> simple bool. And there was never a valid technical reason to have the
> intermediate fbdev power states for backlights (those really only can
> be either off or on).
>
> Cleanup motivated by Meghana's questions about all this.
All applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog