LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/6] backlight: Nuke unused backlight.props.state states
@ 2018-04-25 17:42 Daniel Vetter
  2018-04-25 17:42 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
                   ` (7 more replies)
  0 siblings, 8 replies; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han,
	Meghana Madhyastha, 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
@ 2018-04-25 17:42 ` Daniel Vetter
  2018-04-30 10:21   ` Jani Nikula
  2018-04-25 17:42 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han, 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>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
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.17.0

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
  2018-04-25 17:42 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
@ 2018-04-25 17:42 ` Daniel Vetter
  2018-04-30 10:22   ` Jani Nikula
  2018-04-25 17:42 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han, 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>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
  2018-04-25 17:42 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
  2018-04-25 17:42 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
@ 2018-04-25 17:42 ` Daniel Vetter
  2018-04-30  9:54   ` Lee Jones
  2018-04-30 10:22   ` Jani Nikula
  2018-04-25 17:42 ` [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1 Daniel Vetter
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han,
	Thomas Petazzoni, 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>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
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 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
                   ` (2 preceding siblings ...)
  2018-04-25 17:42 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
@ 2018-04-25 17:42 ` Daniel Vetter
  2018-04-30 10:24   ` Jani Nikula
  2018-04-25 17:42 ` [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches Daniel Vetter
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han, 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>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
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 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
                   ` (3 preceding siblings ...)
  2018-04-25 17:42 ` [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1 Daniel Vetter
@ 2018-04-25 17:42 ` Daniel Vetter
  2018-04-25 19:43   ` Jingoo Han
  2018-04-25 19:42 ` [PATCH 1/6] backlight: Nuke unused backlight.props.state states Jingoo Han
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-04-25 17:42 UTC (permalink / raw)
  To: DRI Development, LKML
  Cc: Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han, 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>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Acked-by: 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 fa72c117bcd5..0f902399a348 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2589,6 +2589,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.17.0

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
                   ` (4 preceding siblings ...)
  2018-04-25 17:42 ` [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches Daniel Vetter
@ 2018-04-25 19:42 ` Jingoo Han
  2018-04-30 10:21 ` Jani Nikula
  2018-04-30 12:27 ` Lee Jones
  7 siblings, 0 replies; 24+ messages in thread
From: Jingoo Han @ 2018-04-25 19:42 UTC (permalink / raw)
  To: 'Daniel Vetter', 'DRI Development', 'LKML'
  Cc: 'Lee Jones', 'Daniel Thompson',
	'Meghana Madhyastha', 'Daniel Vetter'

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches
  2018-04-25 17:42 ` [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches Daniel Vetter
@ 2018-04-25 19:43   ` Jingoo Han
  0 siblings, 0 replies; 24+ messages in thread
From: Jingoo Han @ 2018-04-25 19:43 UTC (permalink / raw)
  To: 'Daniel Vetter', 'DRI Development', 'LKML'
  Cc: 'Lee Jones', 'Daniel Thompson', 'Daniel Vetter'

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

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 fa72c117bcd5..0f902399a348 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2589,6 +2589,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.17.0

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1
  2018-04-25 17:42 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
@ 2018-04-30  9:54   ` Lee Jones
  2018-04-30 10:59     ` Greg KH
  2018-04-30 10:22   ` Jani Nikula
  1 sibling, 1 reply; 24+ messages in thread
From: Lee Jones @ 2018-04-30  9:54 UTC (permalink / raw)
  To: Daniel Vetter, gregkh, thomas.petazzoni
  Cc: DRI Development, LKML, Daniel Thompson, Jingoo Han,
	Thomas Petazzoni, Daniel Vetter

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 <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>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> 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(-)

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
                   ` (5 preceding siblings ...)
  2018-04-25 19:42 ` [PATCH 1/6] backlight: Nuke unused backlight.props.state states Jingoo Han
@ 2018-04-30 10:21 ` Jani Nikula
  2018-04-30 12:27 ` Lee Jones
  7 siblings, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2018-04-30 10:21 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Daniel Thompson, Daniel Vetter, Jingoo Han, Meghana Madhyastha,
	Daniel Vetter, Lee Jones

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-04-25 17:42 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
@ 2018-04-30 10:21   ` Jani Nikula
  0 siblings, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2018-04-30 10:21 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Daniel Vetter, Daniel Vetter, Daniel Thompson, Lee Jones, Jingoo Han

On Wed, 25 Apr 2018, 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>
> 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>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1
  2018-04-25 17:42 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
@ 2018-04-30 10:22   ` Jani Nikula
  0 siblings, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2018-04-30 10:22 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Daniel Vetter, Daniel Vetter, Daniel Thompson, Lee Jones, Jingoo Han

On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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>
> 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>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1
  2018-04-25 17:42 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
  2018-04-30  9:54   ` Lee Jones
@ 2018-04-30 10:22   ` Jani Nikula
  1 sibling, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2018-04-30 10:22 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Thomas Petazzoni, Daniel Thompson, Daniel Vetter, Jingoo Han,
	Daniel Vetter, Lee Jones

On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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>
> 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>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1
  2018-04-25 17:42 ` [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1 Daniel Vetter
@ 2018-04-30 10:24   ` Jani Nikula
  2018-04-30 12:14     ` Lee Jones
  0 siblings, 1 reply; 24+ messages in thread
From: Jani Nikula @ 2018-04-30 10:24 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development, LKML
  Cc: Daniel Vetter, Daniel Vetter, Daniel Thompson, Lee Jones, Jingoo Han

On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> 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 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 <jani.nikula@intel.com>

>  
>  };

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/6] staging/fbtft: Stop using BL_CORE_DRIVER1
  2018-04-30  9:54   ` Lee Jones
@ 2018-04-30 10:59     ` Greg KH
  0 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2018-04-30 10:59 UTC (permalink / raw)
  To: Lee Jones
  Cc: Daniel Vetter, thomas.petazzoni, DRI Development, LKML,
	Daniel Thompson, Jingoo Han, Daniel Vetter

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 <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>
> > Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> > 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(-)
> 
> 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 <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1
  2018-04-30 10:24   ` Jani Nikula
@ 2018-04-30 12:14     ` Lee Jones
  0 siblings, 0 replies; 24+ messages in thread
From: Lee Jones @ 2018-04-30 12:14 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Daniel Vetter, DRI Development, LKML, Daniel Vetter,
	Daniel Thompson, Jingoo Han

On Mon, 30 Apr 2018, Jani Nikula wrote:

> On Wed, 25 Apr 2018, Daniel Vetter <daniel.vetter@ffwll.ch> 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>
> > Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> > 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 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 <jani.nikula@intel.com>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/6] backlight: Nuke unused backlight.props.state states
  2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
                   ` (6 preceding siblings ...)
  2018-04-30 10:21 ` Jani Nikula
@ 2018-04-30 12:27 ` Lee Jones
  7 siblings, 0 replies; 24+ messages in thread
From: Lee Jones @ 2018-04-30 12:27 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, LKML, Daniel Thompson, Jingoo Han,
	Meghana Madhyastha, Daniel Vetter

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 16:37     ` Daniel Thompson
@ 2018-01-18 13:08       ` Emil Velikov
  0 siblings, 0 replies; 24+ messages in thread
From: Emil Velikov @ 2018-01-18 13:08 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Daniel Vetter, DRI Development, LKML, Jingoo Han, Daniel Vetter,
	Lee Jones

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 17:13     ` Daniel Vetter
@ 2018-01-18  9:20       ` Daniel Thompson
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Thompson @ 2018-01-18  9:20 UTC (permalink / raw)
  To: DRI Development, LKML, Lee Jones, Jingoo Han, Daniel Vetter



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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 16:44   ` Daniel Thompson
@ 2018-01-17 17:13     ` Daniel Vetter
  2018-01-18  9:20       ` Daniel Thompson
  0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-01-17 17:13 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Daniel Vetter, DRI Development, LKML, Lee Jones, Jingoo Han,
	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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 14:01 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
  2018-01-17 14:36   ` Emil Velikov
@ 2018-01-17 16:44   ` Daniel Thompson
  2018-01-17 17:13     ` Daniel Vetter
  1 sibling, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2018-01-17 16:44 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development; +Cc: LKML, Lee Jones, Jingoo Han, Daniel Vetter

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 14:36   ` Emil Velikov
@ 2018-01-17 16:37     ` Daniel Thompson
  2018-01-18 13:08       ` Emil Velikov
  0 siblings, 1 reply; 24+ messages in thread
From: Daniel Thompson @ 2018-01-17 16:37 UTC (permalink / raw)
  To: Emil Velikov, Daniel Vetter
  Cc: DRI Development, LKML, Jingoo Han, Daniel Vetter, Lee Jones



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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 14:01 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
@ 2018-01-17 14:36   ` Emil Velikov
  2018-01-17 16:37     ` Daniel Thompson
  2018-01-17 16:44   ` Daniel Thompson
  1 sibling, 1 reply; 24+ messages in thread
From: Emil Velikov @ 2018-01-17 14:36 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: DRI Development, Daniel Thompson, LKML, Jingoo Han,
	Daniel Vetter, Lee Jones

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state
  2018-01-17 14:01 Daniel Vetter
@ 2018-01-17 14:01 ` Daniel Vetter
  2018-01-17 14:36   ` Emil Velikov
  2018-01-17 16:44   ` Daniel Thompson
  0 siblings, 2 replies; 24+ messages in thread
From: Daniel Vetter @ 2018-01-17 14:01 UTC (permalink / raw)
  To: DRI Development
  Cc: LKML, Daniel Vetter, Lee Jones, Daniel Thompson, Jingoo Han,
	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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2018-04-30 12:27 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25 17:42 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
2018-04-25 17:42 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
2018-04-30 10:21   ` Jani Nikula
2018-04-25 17:42 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
2018-04-30 10:22   ` Jani Nikula
2018-04-25 17:42 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
2018-04-30  9:54   ` Lee Jones
2018-04-30 10:59     ` Greg KH
2018-04-30 10:22   ` Jani Nikula
2018-04-25 17:42 ` [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1 Daniel Vetter
2018-04-30 10:24   ` Jani Nikula
2018-04-30 12:14     ` Lee Jones
2018-04-25 17:42 ` [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches Daniel Vetter
2018-04-25 19:43   ` Jingoo Han
2018-04-25 19:42 ` [PATCH 1/6] backlight: Nuke unused backlight.props.state states Jingoo Han
2018-04-30 10:21 ` Jani Nikula
2018-04-30 12:27 ` Lee Jones
  -- strict thread matches above, loose matches on Subject: below --
2018-01-17 14:01 Daniel Vetter
2018-01-17 14:01 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
2018-01-17 14:36   ` Emil Velikov
2018-01-17 16:37     ` Daniel Thompson
2018-01-18 13:08       ` Emil Velikov
2018-01-17 16:44   ` Daniel Thompson
2018-01-17 17:13     ` Daniel Vetter
2018-01-18  9:20       ` Daniel Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).