[PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

Instead of checking the state of various backlight_properties fields
against the memorised state in atmel_lcdfb_info.bl_power,
atmel_bl_update_status() should retrieve the desired state using
backlight_get_brightness (which takes into account the power state,
blanking etc.). This means the explicit checks using props.fb_blank
and props.power can be dropped.

Then brightness can only be negative if the backlight is on but
props.brightness is negative, so the test before reading the
brightness value from the hardware can be simplified to
(brightness < 0).

As a result, bl_power in struct atmel_lcdfb_info is no longer
necessary, so remove that while we're at it. Since we only ever care
about reading the current state in backlight_properties, drop the
updates at the end of the function.

Signed-off-by: Stephen Kitt <steve@sk2.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/video/fbdev/atmel_lcdfb.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index 1fc8de4ecbeb..06159a4da293 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -49,7 +49,6 @@ struct atmel_lcdfb_info {
 	struct clk		*lcdc_clk;
 
 	struct backlight_device	*backlight;
-	u8			bl_power;
 	u8			saved_lcdcon;
 
 	u32			pseudo_palette[16];
@@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
 static int atmel_bl_update_status(struct backlight_device *bl)
 {
 	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
-	int			power = sinfo->bl_power;
-	int			brightness = bl->props.brightness;
+	int			brightness = backlight_get_brightness(bl);
 
-	/* REVISIT there may be a meaningful difference between
-	 * fb_blank and power ... there seem to be some cases
-	 * this doesn't handle correctly.
-	 */
-	if (bl->props.fb_blank != sinfo->bl_power)
-		power = bl->props.fb_blank;
-	else if (bl->props.power != sinfo->bl_power)
-		power = bl->props.power;
-
-	if (brightness < 0 && power == FB_BLANK_UNBLANK)
+	if (brightness < 0)
 		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
-	else if (power != FB_BLANK_UNBLANK)
-		brightness = 0;
 
 	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
 	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)
@@ -133,8 +120,6 @@ static int atmel_bl_update_status(struct backlight_device *bl)
 	else
 		lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, contrast_ctr);
 
-	bl->props.fb_blank = bl->props.power = sinfo->bl_power = power;
-
 	return 0;
 }
 
@@ -155,8 +140,6 @@ static void init_backlight(struct atmel_lcdfb_info *sinfo)
 	struct backlight_properties props;
 	struct backlight_device	*bl;
 
-	sinfo->bl_power = FB_BLANK_UNBLANK;
-
 	if (sinfo->backlight)
 		return;
 

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.30.2

[PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

Instead of checking the state of various backlight_properties fields
against the memorised state in atmel_lcdfb_info.bl_power,
atmel_bl_update_status() should retrieve the desired state using
backlight_get_brightness (which takes into account the power state,
blanking etc.). This means the explicit checks using props.fb_blank
and props.power can be dropped.

Then brightness can only be negative if the backlight is on but
props.brightness is negative, so the test before reading the
brightness value from the hardware can be simplified to
(brightness < 0).

As a result, bl_power in struct atmel_lcdfb_info is no longer
necessary, so remove that while we're at it. Since we only ever care
about reading the current state in backlight_properties, drop the
updates at the end of the function.

Signed-off-by: Stephen Kitt <steve@sk2.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/video/fbdev/atmel_lcdfb.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index 1fc8de4ecbeb..06159a4da293 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -49,7 +49,6 @@ struct atmel_lcdfb_info {
 	struct clk		*lcdc_clk;
 
 	struct backlight_device	*backlight;
-	u8			bl_power;
 	u8			saved_lcdcon;
 
 	u32			pseudo_palette[16];
@@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
 static int atmel_bl_update_status(struct backlight_device *bl)
 {
 	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
-	int			power = sinfo->bl_power;
-	int			brightness = bl->props.brightness;
+	int			brightness = backlight_get_brightness(bl);
 
-	/* REVISIT there may be a meaningful difference between
-	 * fb_blank and power ... there seem to be some cases
-	 * this doesn't handle correctly.
-	 */
-	if (bl->props.fb_blank != sinfo->bl_power)
-		power = bl->props.fb_blank;
-	else if (bl->props.power != sinfo->bl_power)
-		power = bl->props.power;
-
-	if (brightness < 0 && power == FB_BLANK_UNBLANK)
+	if (brightness < 0)
 		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
-	else if (power != FB_BLANK_UNBLANK)
-		brightness = 0;
 
 	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
 	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)
@@ -133,8 +120,6 @@ static int atmel_bl_update_status(struct backlight_device *bl)
 	else
 		lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, contrast_ctr);
 
-	bl->props.fb_blank = bl->props.power = sinfo->bl_power = power;
-
 	return 0;
 }
 
@@ -155,8 +140,6 @@ static void init_backlight(struct atmel_lcdfb_info *sinfo)
 	struct backlight_properties props;
 	struct backlight_device	*bl;
 
-	sinfo->bl_power = FB_BLANK_UNBLANK;
-
 	if (sinfo->backlight)
 		return;
 

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.30.2

[PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

Instead of checking the state of various backlight_properties fields
against the memorised state in atmel_lcdfb_info.bl_power,
atmel_bl_update_status() should retrieve the desired state using
backlight_get_brightness (which takes into account the power state,
blanking etc.). This means the explicit checks using props.fb_blank
and props.power can be dropped.

Then brightness can only be negative if the backlight is on but
props.brightness is negative, so the test before reading the
brightness value from the hardware can be simplified to
(brightness < 0).

As a result, bl_power in struct atmel_lcdfb_info is no longer
necessary, so remove that while we're at it. Since we only ever care
about reading the current state in backlight_properties, drop the
updates at the end of the function.

Signed-off-by: Stephen Kitt <steve@sk2.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Helge Deller <deller@gmx.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/video/fbdev/atmel_lcdfb.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index 1fc8de4ecbeb..06159a4da293 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -49,7 +49,6 @@ struct atmel_lcdfb_info {
 	struct clk		*lcdc_clk;
 
 	struct backlight_device	*backlight;
-	u8			bl_power;
 	u8			saved_lcdcon;
 
 	u32			pseudo_palette[16];
@@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
 static int atmel_bl_update_status(struct backlight_device *bl)
 {
 	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
-	int			power = sinfo->bl_power;
-	int			brightness = bl->props.brightness;
+	int			brightness = backlight_get_brightness(bl);
 
-	/* REVISIT there may be a meaningful difference between
-	 * fb_blank and power ... there seem to be some cases
-	 * this doesn't handle correctly.
-	 */
-	if (bl->props.fb_blank != sinfo->bl_power)
-		power = bl->props.fb_blank;
-	else if (bl->props.power != sinfo->bl_power)
-		power = bl->props.power;
-
-	if (brightness < 0 && power == FB_BLANK_UNBLANK)
+	if (brightness < 0)
 		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
-	else if (power != FB_BLANK_UNBLANK)
-		brightness = 0;
 
 	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
 	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)
@@ -133,8 +120,6 @@ static int atmel_bl_update_status(struct backlight_device *bl)
 	else
 		lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_CTR, contrast_ctr);
 
-	bl->props.fb_blank = bl->props.power = sinfo->bl_power = power;
-
 	return 0;
 }
 
@@ -155,8 +140,6 @@ static void init_backlight(struct atmel_lcdfb_info *sinfo)
 	struct backlight_properties props;
 	struct backlight_device	*bl;
 
-	sinfo->bl_power = FB_BLANK_UNBLANK;
-
 	if (sinfo->backlight)
 		return;
 

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> Instead of checking the state of various backlight_properties fields
> against the memorised state in atmel_lcdfb_info.bl_power,
> atmel_bl_update_status() should retrieve the desired state using
> backlight_get_brightness (which takes into account the power state,
> blanking etc.). This means the explicit checks using props.fb_blank
> and props.power can be dropped.
> 
> Then brightness can only be negative if the backlight is on but
> props.brightness is negative, so the test before reading the
> brightness value from the hardware can be simplified to
> (brightness < 0).

props.brightness should always be in the interval 0..max_brightness.

This is enforced by the main backlight code (and APIs to set the
brightness use unsigned values). Thus props.brightness could only be
negative is the driver explicitly sets a negative value as some kind of
placeholder (which this driver does not do).

I don't think there is any need to keep this logic.


Daniel.


> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index 1fc8de4ecbeb..06159a4da293 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
>  static int atmel_bl_update_status(struct backlight_device *bl)
>  {
>  	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
> -	int			power = sinfo->bl_power;
> -	int			brightness = bl->props.brightness;
> +	int			brightness = backlight_get_brightness(bl);
>  
> -	/* REVISIT there may be a meaningful difference between
> -	 * fb_blank and power ... there seem to be some cases
> -	 * this doesn't handle correctly.
> -	 */
> -	if (bl->props.fb_blank != sinfo->bl_power)
> -		power = bl->props.fb_blank;
> -	else if (bl->props.power != sinfo->bl_power)
> -		power = bl->props.power;
> -
> -	if (brightness < 0 && power == FB_BLANK_UNBLANK)
> +	if (brightness < 0)
>  		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> -	else if (power != FB_BLANK_UNBLANK)
> -		brightness = 0;
>  
>  	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
>  	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> Instead of checking the state of various backlight_properties fields
> against the memorised state in atmel_lcdfb_info.bl_power,
> atmel_bl_update_status() should retrieve the desired state using
> backlight_get_brightness (which takes into account the power state,
> blanking etc.). This means the explicit checks using props.fb_blank
> and props.power can be dropped.
> 
> Then brightness can only be negative if the backlight is on but
> props.brightness is negative, so the test before reading the
> brightness value from the hardware can be simplified to
> (brightness < 0).

props.brightness should always be in the interval 0..max_brightness.

This is enforced by the main backlight code (and APIs to set the
brightness use unsigned values). Thus props.brightness could only be
negative is the driver explicitly sets a negative value as some kind of
placeholder (which this driver does not do).

I don't think there is any need to keep this logic.


Daniel.


> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index 1fc8de4ecbeb..06159a4da293 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
>  static int atmel_bl_update_status(struct backlight_device *bl)
>  {
>  	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
> -	int			power = sinfo->bl_power;
> -	int			brightness = bl->props.brightness;
> +	int			brightness = backlight_get_brightness(bl);
>  
> -	/* REVISIT there may be a meaningful difference between
> -	 * fb_blank and power ... there seem to be some cases
> -	 * this doesn't handle correctly.
> -	 */
> -	if (bl->props.fb_blank != sinfo->bl_power)
> -		power = bl->props.fb_blank;
> -	else if (bl->props.power != sinfo->bl_power)
> -		power = bl->props.power;
> -
> -	if (brightness < 0 && power == FB_BLANK_UNBLANK)
> +	if (brightness < 0)
>  		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> -	else if (power != FB_BLANK_UNBLANK)
> -		brightness = 0;
>  
>  	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
>  	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> Instead of checking the state of various backlight_properties fields
> against the memorised state in atmel_lcdfb_info.bl_power,
> atmel_bl_update_status() should retrieve the desired state using
> backlight_get_brightness (which takes into account the power state,
> blanking etc.). This means the explicit checks using props.fb_blank
> and props.power can be dropped.
> 
> Then brightness can only be negative if the backlight is on but
> props.brightness is negative, so the test before reading the
> brightness value from the hardware can be simplified to
> (brightness < 0).

props.brightness should always be in the interval 0..max_brightness.

This is enforced by the main backlight code (and APIs to set the
brightness use unsigned values). Thus props.brightness could only be
negative is the driver explicitly sets a negative value as some kind of
placeholder (which this driver does not do).

I don't think there is any need to keep this logic.


Daniel.


> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index 1fc8de4ecbeb..06159a4da293 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -109,22 +108,10 @@ static u32 contrast_ctr = ATMEL_LCDC_PS_DIV8
>  static int atmel_bl_update_status(struct backlight_device *bl)
>  {
>  	struct atmel_lcdfb_info *sinfo = bl_get_data(bl);
> -	int			power = sinfo->bl_power;
> -	int			brightness = bl->props.brightness;
> +	int			brightness = backlight_get_brightness(bl);
>  
> -	/* REVISIT there may be a meaningful difference between
> -	 * fb_blank and power ... there seem to be some cases
> -	 * this doesn't handle correctly.
> -	 */
> -	if (bl->props.fb_blank != sinfo->bl_power)
> -		power = bl->props.fb_blank;
> -	else if (bl->props.power != sinfo->bl_power)
> -		power = bl->props.power;
> -
> -	if (brightness < 0 && power == FB_BLANK_UNBLANK)
> +	if (brightness < 0)
>  		brightness = lcdc_readl(sinfo, ATMEL_LCDC_CONTRAST_VAL);
> -	else if (power != FB_BLANK_UNBLANK)
> -		brightness = 0;
>  
>  	lcdc_writel(sinfo, ATMEL_LCDC_CONTRAST_VAL, brightness);
>  	if (contrast_ctr & ATMEL_LCDC_POL_POSITIVE)

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Sam Ravnborg]

Hi Stephen,

thanks for taking care of all these backlight simplifications - this
really helps to make the code simpler and more readable.

On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> > Instead of checking the state of various backlight_properties fields
> > against the memorised state in atmel_lcdfb_info.bl_power,
> > atmel_bl_update_status() should retrieve the desired state using
> > backlight_get_brightness (which takes into account the power state,
> > blanking etc.). This means the explicit checks using props.fb_blank
> > and props.power can be dropped.
> > 
> > Then brightness can only be negative if the backlight is on but
> > props.brightness is negative, so the test before reading the
> > brightness value from the hardware can be simplified to
> > (brightness < 0).
> 
> props.brightness should always be in the interval 0..max_brightness.
> 
> This is enforced by the main backlight code (and APIs to set the
> brightness use unsigned values). Thus props.brightness could only be
> negative is the driver explicitly sets a negative value as some kind of
> placeholder (which this driver does not do).
> 
> I don't think there is any need to keep this logic.

Daniel is right - please drop the "if (brightness < 0)" logic.
I have looked a bit on the datasheet in my attempt to do a drm version
of this driver - something that I am yet to succeed and the backlight
core avoid any negative values.

	Sam

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Sam Ravnborg]

Hi Stephen,

thanks for taking care of all these backlight simplifications - this
really helps to make the code simpler and more readable.

On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> > Instead of checking the state of various backlight_properties fields
> > against the memorised state in atmel_lcdfb_info.bl_power,
> > atmel_bl_update_status() should retrieve the desired state using
> > backlight_get_brightness (which takes into account the power state,
> > blanking etc.). This means the explicit checks using props.fb_blank
> > and props.power can be dropped.
> > 
> > Then brightness can only be negative if the backlight is on but
> > props.brightness is negative, so the test before reading the
> > brightness value from the hardware can be simplified to
> > (brightness < 0).
> 
> props.brightness should always be in the interval 0..max_brightness.
> 
> This is enforced by the main backlight code (and APIs to set the
> brightness use unsigned values). Thus props.brightness could only be
> negative is the driver explicitly sets a negative value as some kind of
> placeholder (which this driver does not do).
> 
> I don't think there is any need to keep this logic.

Daniel is right - please drop the "if (brightness < 0)" logic.
I have looked a bit on the datasheet in my attempt to do a drm version
of this driver - something that I am yet to succeed and the backlight
core avoid any negative values.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Sam Ravnborg]

Hi Stephen,

thanks for taking care of all these backlight simplifications - this
really helps to make the code simpler and more readable.

On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:
> > Instead of checking the state of various backlight_properties fields
> > against the memorised state in atmel_lcdfb_info.bl_power,
> > atmel_bl_update_status() should retrieve the desired state using
> > backlight_get_brightness (which takes into account the power state,
> > blanking etc.). This means the explicit checks using props.fb_blank
> > and props.power can be dropped.
> > 
> > Then brightness can only be negative if the backlight is on but
> > props.brightness is negative, so the test before reading the
> > brightness value from the hardware can be simplified to
> > (brightness < 0).
> 
> props.brightness should always be in the interval 0..max_brightness.
> 
> This is enforced by the main backlight code (and APIs to set the
> brightness use unsigned values). Thus props.brightness could only be
> negative is the driver explicitly sets a negative value as some kind of
> placeholder (which this driver does not do).
> 
> I don't think there is any need to keep this logic.

Daniel is right - please drop the "if (brightness < 0)" logic.
I have looked a bit on the datasheet in my attempt to do a drm version
of this driver - something that I am yet to succeed and the backlight
core avoid any negative values.

	Sam

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

Hi Sam, Daniel,

On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> thanks for taking care of all these backlight simplifications - this
> really helps to make the code simpler and more readable.

You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
started digging...

> On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> > On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:  
> > > Instead of checking the state of various backlight_properties fields
> > > against the memorised state in atmel_lcdfb_info.bl_power,
> > > atmel_bl_update_status() should retrieve the desired state using
> > > backlight_get_brightness (which takes into account the power state,
> > > blanking etc.). This means the explicit checks using props.fb_blank
> > > and props.power can be dropped.
> > > 
> > > Then brightness can only be negative if the backlight is on but
> > > props.brightness is negative, so the test before reading the
> > > brightness value from the hardware can be simplified to
> > > (brightness < 0).  
> > 
> > props.brightness should always be in the interval 0..max_brightness.
> > 
> > This is enforced by the main backlight code (and APIs to set the
> > brightness use unsigned values). Thus props.brightness could only be
> > negative is the driver explicitly sets a negative value as some kind of
> > placeholder (which this driver does not do).
> > 
> > I don't think there is any need to keep this logic.  
> 
> Daniel is right - please drop the "if (brightness < 0)" logic.
> I have looked a bit on the datasheet in my attempt to do a drm version
> of this driver - something that I am yet to succeed and the backlight
> core avoid any negative values.

Thanks for the reviews!

I’ve prepared a v2 without the (brightness < 0) logic, I’m about to submit
it.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

Hi Sam, Daniel,

On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> thanks for taking care of all these backlight simplifications - this
> really helps to make the code simpler and more readable.

You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
started digging...

> On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> > On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:  
> > > Instead of checking the state of various backlight_properties fields
> > > against the memorised state in atmel_lcdfb_info.bl_power,
> > > atmel_bl_update_status() should retrieve the desired state using
> > > backlight_get_brightness (which takes into account the power state,
> > > blanking etc.). This means the explicit checks using props.fb_blank
> > > and props.power can be dropped.
> > > 
> > > Then brightness can only be negative if the backlight is on but
> > > props.brightness is negative, so the test before reading the
> > > brightness value from the hardware can be simplified to
> > > (brightness < 0).  
> > 
> > props.brightness should always be in the interval 0..max_brightness.
> > 
> > This is enforced by the main backlight code (and APIs to set the
> > brightness use unsigned values). Thus props.brightness could only be
> > negative is the driver explicitly sets a negative value as some kind of
> > placeholder (which this driver does not do).
> > 
> > I don't think there is any need to keep this logic.  
> 
> Daniel is right - please drop the "if (brightness < 0)" logic.
> I have looked a bit on the datasheet in my attempt to do a drm version
> of this driver - something that I am yet to succeed and the backlight
> core avoid any negative values.

Thanks for the reviews!

I’ve prepared a v2 without the (brightness < 0) logic, I’m about to submit
it.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1.1: Type: text/plain, Size: 1918 bytes --]

Hi Sam, Daniel,

On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> thanks for taking care of all these backlight simplifications - this
> really helps to make the code simpler and more readable.

You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
started digging...

> On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote:
> > On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote:  
> > > Instead of checking the state of various backlight_properties fields
> > > against the memorised state in atmel_lcdfb_info.bl_power,
> > > atmel_bl_update_status() should retrieve the desired state using
> > > backlight_get_brightness (which takes into account the power state,
> > > blanking etc.). This means the explicit checks using props.fb_blank
> > > and props.power can be dropped.
> > > 
> > > Then brightness can only be negative if the backlight is on but
> > > props.brightness is negative, so the test before reading the
> > > brightness value from the hardware can be simplified to
> > > (brightness < 0).  
> > 
> > props.brightness should always be in the interval 0..max_brightness.
> > 
> > This is enforced by the main backlight code (and APIs to set the
> > brightness use unsigned values). Thus props.brightness could only be
> > negative is the driver explicitly sets a negative value as some kind of
> > placeholder (which this driver does not do).
> > 
> > I don't think there is any need to keep this logic.  
> 
> Daniel is right - please drop the "if (brightness < 0)" logic.
> I have looked a bit on the datasheet in my attempt to do a drm version
> of this driver - something that I am yet to succeed and the backlight
> core avoid any negative values.

Thanks for the reviews!

I’ve prepared a v2 without the (brightness < 0) logic, I’m about to submit
it.

Regards,

Stephen

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> Hi Sam, Daniel,
>
> On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> > thanks for taking care of all these backlight simplifications - this
> > really helps to make the code simpler and more readable.
>
> You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
> started digging...

I saw Sam's comment and kinda wished I'd thought to say that... definitely
good to see these things being tidied up.


Daniel.

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> Hi Sam, Daniel,
>
> On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> > thanks for taking care of all these backlight simplifications - this
> > really helps to make the code simpler and more readable.
>
> You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
> started digging...

I saw Sam's comment and kinda wished I'd thought to say that... definitely
good to see these things being tidied up.


Daniel.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Daniel Thompson]

On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> Hi Sam, Daniel,
>
> On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> > thanks for taking care of all these backlight simplifications - this
> > really helps to make the code simpler and more readable.
>
> You’re welcome! I noticed fb_blank was deprecated and near enough unused, and
> started digging...

I saw Sam's comment and kinda wished I'd thought to say that... definitely
good to see these things being tidied up.


Daniel.

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Fri, 10 Jun 2022 10:49:55 +0100, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> > Hi Sam, Daniel,
> >
> > On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> >  
> > > thanks for taking care of all these backlight simplifications - this
> > > really helps to make the code simpler and more readable.  
> >
> > You’re welcome! I noticed fb_blank was deprecated and near enough unused,
> > and started digging...  
> 
> I saw Sam's comment and kinda wished I'd thought to say that... definitely
> good to see these things being tidied up.

Thanks! I saw the nice wrapper functions in backlight.h and couldn’t resist.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Fri, 10 Jun 2022 10:49:55 +0100, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> > Hi Sam, Daniel,
> >
> > On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> >  
> > > thanks for taking care of all these backlight simplifications - this
> > > really helps to make the code simpler and more readable.  
> >
> > You’re welcome! I noticed fb_blank was deprecated and near enough unused,
> > and started digging...  
> 
> I saw Sam's comment and kinda wished I'd thought to say that... definitely
> good to see these things being tidied up.

Thanks! I saw the nice wrapper functions in backlight.h and couldn’t resist.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates

[Stephen Kitt]

[-- Attachment #1.1: Type: text/plain, Size: 757 bytes --]

On Fri, 10 Jun 2022 10:49:55 +0100, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On Thu, Jun 09, 2022 at 07:45:11PM +0200, Stephen Kitt wrote:
> > Hi Sam, Daniel,
> >
> > On Thu, 9 Jun 2022 19:30:57 +0200, Sam Ravnborg <sam@ravnborg.org> wrote:
> >  
> > > thanks for taking care of all these backlight simplifications - this
> > > really helps to make the code simpler and more readable.  
> >
> > You’re welcome! I noticed fb_blank was deprecated and near enough unused,
> > and started digging...  
> 
> I saw Sam's comment and kinda wished I'd thought to say that... definitely
> good to see these things being tidied up.

Thanks! I saw the nice wrapper functions in backlight.h and couldn’t resist.

Regards,

Stephen

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel