[PATCH 1/6] backlight: Nuke unused backlight.props.state states

[PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/backlight.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index af7003548593..9776edb0ff06 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.15.1

[PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Vetter]

Nothing in the entire tree ever sets this, which means this is dead
code. Remove it.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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.15.1

[PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Vetter]

Nothing in the entire tree ever sets this, which means this is dead
code. Remove it.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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(&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");
+		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 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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 6d0363deba61..448929cc7ba1 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -255,7 +255,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",
@@ -305,7 +305,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 488ab788138e..54de7cdfdff7 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -240,6 +240,7 @@ struct fbtft_par {
 	ktime_t update_time;
 	bool bgr;
 	void *extra;
+	bool polarity;
 };
 
 #define NUMARGS(...)  (sizeof((int[]){__VA_ARGS__})/sizeof(int))
-- 
2.15.1

[PATCH 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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 6d0363deba61..448929cc7ba1 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -255,7 +255,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",
@@ -305,7 +305,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 488ab788138e..54de7cdfdff7 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -240,6 +240,7 @@ struct fbtft_par {
 	ktime_t update_time;
 	bool bgr;
 	void *extra;
+	bool polarity;
 };
 
 #define NUMARGS(...)  (sizeof((int[]){__VA_ARGS__})/sizeof(int))
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/backlight.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 9776edb0ff06..b6f7c99d1c8e 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.15.1

[PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/linux/backlight.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 9776edb0ff06..b6f7c99d1c8e 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.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7691e1025708..6534517f53b6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2546,6 +2546,7 @@ BACKLIGHT CLASS/SUBSYSTEM
 M:	Lee Jones <lee.jones@linaro.org>
 M:	Daniel Thompson <daniel.thompson@linaro.org>
 M:	Jingoo Han <jingoohan1@gmail.com>
+L:	dri-devel@lists.freedesktop.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
 S:	Maintained
 F:	drivers/video/backlight/
-- 
2.15.1

[PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7691e1025708..6534517f53b6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2546,6 +2546,7 @@ BACKLIGHT CLASS/SUBSYSTEM
 M:	Lee Jones <lee.jones@linaro.org>
 M:	Daniel Thompson <daniel.thompson@linaro.org>
 M:	Jingoo Han <jingoohan1@gmail.com>
+L:	dri-devel@lists.freedesktop.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
 S:	Maintained
 F:	drivers/video/backlight/
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 14:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/video/backlight/generic_bl.c | 5 -----

Fly-by comment, while waiting for coffee to kick-in.
I think this patch should be after pandora/others have stopped abusing
BL_CORE_DRIVER1.

-Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 14:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/video/backlight/generic_bl.c | 5 -----

Fly-by comment, while waiting for coffee to kick-in.
I think this patch should be after pandora/others have stopped abusing
BL_CORE_DRIVER1.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Daniel Thompson]

On 17/01/18 14:01, 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

Daniel: Ack is info for Lee J, not to imply you should take it through
         one of your trees.


Daniel.

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Thompson]

On 17/01/18 14:36, Emil Velikov wrote:
> On 17 January 2018 at 14:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> Nothing in the entire tree ever sets this, which means this is dead
>> code. Remove it.
>>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>   drivers/video/backlight/generic_bl.c | 5 -----
> 
> Fly-by comment, while waiting for coffee to kick-in.
> I think this patch should be after pandora/others have stopped abusing
> BL_CORE_DRIVER1.

You sure?

I can't see why the use or disuse of BL_CORE_DRIVER1 in this driver 
should influence another independent driver.


Daniel.

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Not sure whether to ack this one or not.

There is nothing wrong with the change but having taken a closer look 
the driver seems like it exists mostly to allow mach-XXX code to plug in 
function pointers and we don't do that sort of thing any more.

I think the entire driver is dead code!


Daniel.


> ---
>   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);
>   
> 

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter wrote:
> Nothing in the entire tree ever sets this, which means this is dead
> code. Remove it.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Not sure whether to ack this one or not.

There is nothing wrong with the change but having taken a closer look 
the driver seems like it exists mostly to allow mach-XXX code to plug in 
function pointers and we don't do that sort of thing any more.

I think the entire driver is dead code!


Daniel.


> ---
>   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);
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   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(&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");
> +		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 4/6] staging/fbtft: 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.
> 
> Luckily we have already a drvdata structure, so fixing this is really
> easy.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   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 6d0363deba61..448929cc7ba1 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -255,7 +255,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",
> @@ -305,7 +305,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 488ab788138e..54de7cdfdff7 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -240,6 +240,7 @@ struct fbtft_par {
>   	ktime_t update_time;
>   	bool bgr;
>   	void *extra;
> +	bool polarity;
>   };
>   
>   #define NUMARGS(...)  (sizeof((int[]){__VA_ARGS__})/sizeof(int))
> 

Re: [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   include/linux/backlight.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 9776edb0ff06..b6f7c99d1c8e 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 */
>   
>   };
>   
> 

Re: [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1

[Daniel Thompson]

On 17/01/18 14:01, Daniel Vetter 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   include/linux/backlight.h | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 9776edb0ff06..b6f7c99d1c8e 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 */
>   
>   };
>   
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches

[Daniel Thompson]

On 17/01/18 14:01, 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7691e1025708..6534517f53b6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2546,6 +2546,7 @@ BACKLIGHT CLASS/SUBSYSTEM
>   M:	Lee Jones <lee.jones@linaro.org>
>   M:	Daniel Thompson <daniel.thompson@linaro.org>
>   M:	Jingoo Han <jingoohan1@gmail.com>
> +L:	dri-devel@lists.freedesktop.org
>   T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
>   S:	Maintained
>   F:	drivers/video/backlight/
> 

Re: [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches

[Jingoo Han]

On Wednesday, January 17, 2018 11:53 AM, Daniel Thompson wrote:
> On 17/01/18 14:01, 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 <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

Best regards,
Jingoo Han

> 
> 
> > ---
> >   MAINTAINERS | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7691e1025708..6534517f53b6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2546,6 +2546,7 @@ BACKLIGHT CLASS/SUBSYSTEM
> >   M:	Lee Jones <lee.jones@linaro.org>
> >   M:	Daniel Thompson <daniel.thompson@linaro.org>
> >   M:	Jingoo Han <jingoohan1@gmail.com>
> > +L:	dri-devel@lists.freedesktop.org
> >   T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
> >   S:	Maintained
> >   F:	drivers/video/backlight/
> >

Re: [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches

[Jingoo Han]

On Wednesday, January 17, 2018 11:53 AM, Daniel Thompson wrote:
> On 17/01/18 14:01, 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 <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

Best regards,
Jingoo Han

> 
> 
> > ---
> >   MAINTAINERS | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7691e1025708..6534517f53b6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2546,6 +2546,7 @@ BACKLIGHT CLASS/SUBSYSTEM
> >   M:	Lee Jones <lee.jones@linaro.org>
> >   M:	Daniel Thompson <daniel.thompson@linaro.org>
> >   M:	Jingoo Han <jingoohan1@gmail.com>
> > +L:	dri-devel@lists.freedesktop.org
> >   T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
> >   S:	Maintained
> >   F:	drivers/video/backlight/
> >

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Vetter]

On Wed, Jan 17, 2018 at 04:44:00PM +0000, Daniel Thompson wrote:
> On 17/01/18 14:01, Daniel Vetter wrote:
> > Nothing in the entire tree ever sets this, which means this is dead
> > code. Remove it.
> > 
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Not sure whether to ack this one or not.
> 
> There is nothing wrong with the change but having taken a closer look the
> driver seems like it exists mostly to allow mach-XXX code to plug in
> function pointers and we don't do that sort of thing any more.
> 
> I think the entire driver is dead code!

Well I can also supply a patch to outright nuke the code, but figuring out
whether that's the right thing to do is definitely way above may pay grade
:-)

I only really stitched these together after a long discussion with Meghana
about why backlight seems to have 3+ different ways to enable/disable a
backlight. Just trying to help a bit with getting the
backlight_enable/disable stuff going, so that long-term, at least for
newer drivers, we have one blessed way to do that.

btw that kind of display pm simplification matches what we've done when
implementing atomic modesetting about 3 years ago: We've smashed all the
various power states drm (and fbdev/fbcon) knew about into a simple "is it
on?" boolean. Todays digital hw doesn't really know anything in-between.
Ofc there's tons of components to switch on/off to get the entire display
pipe up, and they might want different autosuspend delays to optimize the
overall system, but that's orthogonal (well, driver internal
implementation detail) really.

Cheers, Daniel

> 
> 
> Daniel.
> 
> 
> > --- 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);
> > 

-- 
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 +0000, 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 <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >   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(&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");
> > +		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 1/6] backlight: Nuke unused backlight.props.state states

[Lee Jones]

On Wed, 17 Jan 2018, Daniel Thompson wrote:

> On 17/01/18 14:01, 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 <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> 
> Daniel: Ack is info for Lee J, not to imply you should take it through
>         one of your trees.

Right.  I'd rather take the set as a complete unit once all Acks are
acquired though.  Hence my silence thus far.

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

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Daniel Thompson]

On 17/01/18 17:13, Daniel Vetter wrote:
> On Wed, Jan 17, 2018 at 04:44:00PM +0000, Daniel Thompson wrote:
>> On 17/01/18 14:01, Daniel Vetter wrote:
>>> Nothing in the entire tree ever sets this, which means this is dead
>>> code. Remove it.
>>>
>>> Cc: Lee Jones <lee.jones@linaro.org>
>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>
>> Not sure whether to ack this one or not.
>>
>> There is nothing wrong with the change but having taken a closer look the
>> driver seems like it exists mostly to allow mach-XXX code to plug in
>> function pointers and we don't do that sort of thing any more.
>>
>> I think the entire driver is dead code!
> 
> Well I can also supply a patch to outright nuke the code, but figuring out
> whether that's the right thing to do is definitely way above may pay grade
> :-)

I don't believe that for a second. ;-)

However after a bit more thought I think its best to take this patch as 
is and we can remove generic-bl after. Whilst I'm confident this code is 
not used, nuking it after your cleanups would result in a simpler revert 
if I were wrong.

So...

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


> I only really stitched these together after a long discussion with Meghana
> about why backlight seems to have 3+ different ways to enable/disable a
> backlight. Just trying to help a bit with getting the
> backlight_enable/disable stuff going, so that long-term, at least for
> newer drivers, we have one blessed way to do that.
> 
> btw that kind of display pm simplification matches what we've done when
> implementing atomic modesetting about 3 years ago: We've smashed all the
> various power states drm (and fbdev/fbcon) knew about into a simple "is it
> on?" boolean. Todays digital hw doesn't really know anything in-between.
> Ofc there's tons of components to switch on/off to get the entire display
> pipe up, and they might want different autosuspend delays to optimize the
> overall system, but that's orthogonal (well, driver internal
> implementation detail) really.
> 
> Cheers, Daniel
> 
>>
>>
>> Daniel.
>>
>>
>>> --- 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);
>>>
> 

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 +0000, 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 <lee.jones@linaro.org>
>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>    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.thompson@linaro.org>


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(&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");
>>> +		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 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 16:37, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
>
> On 17/01/18 14:36, Emil Velikov wrote:
>>
>> On 17 January 2018 at 14:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>
>>> Nothing in the entire tree ever sets this, which means this is dead
>>> code. Remove it.
>>>
>>> Cc: Lee Jones <lee.jones@linaro.org>
>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>   drivers/video/backlight/generic_bl.c | 5 -----
>>
>>
>> Fly-by comment, while waiting for coffee to kick-in.
>> I think this patch should be after pandora/others have stopped abusing
>> BL_CORE_DRIVER1.
>
>
> You sure?
>
> I can't see why the use or disuse of BL_CORE_DRIVER1 in this driver should
> influence another independent driver.
>
Right my bad. Got mislead by the driver name.

-Emil

Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state

[Emil Velikov]

On 17 January 2018 at 16:37, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
>
> On 17/01/18 14:36, Emil Velikov wrote:
>>
>> On 17 January 2018 at 14:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>
>>> Nothing in the entire tree ever sets this, which means this is dead
>>> code. Remove it.
>>>
>>> Cc: Lee Jones <lee.jones@linaro.org>
>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>   drivers/video/backlight/generic_bl.c | 5 -----
>>
>>
>> Fly-by comment, while waiting for coffee to kick-in.
>> I think this patch should be after pandora/others have stopped abusing
>> BL_CORE_DRIVER1.
>
>
> You sure?
>
> I can't see why the use or disuse of BL_CORE_DRIVER1 in this driver should
> influence another independent driver.
>
Right my bad. Got mislead by the driver name.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Lee Jones]

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

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Lee Jones]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Jani Nikula]

On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  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

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Jani Nikula]

On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  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

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Jingoo Han]

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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

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

Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Jingoo Han]

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 <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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

[PATCH 1/6] backlight: Nuke unused backlight.props.state states

[Daniel Vetter]

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 <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Meghana Madhyastha <meghana.madhyastha@gmail.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel