Subject: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

From: Sam Ravnborg <[email protected]>

Avoiding direct access to backlight_properties.props.

Access to the deprecated props.fb_blank replaced by backlight_is_blank().
Access to props.power is dropped - it was only used for debug.

Signed-off-by: Sam Ravnborg <[email protected]>
Cc: Stephen Kitt <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Daniel Thompson <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
---
drivers/staging/fbtft/fb_ssd1351.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c
index b8d55aa8c5c7..995fbd2f3dc6 100644
--- a/drivers/staging/fbtft/fb_ssd1351.c
+++ b/drivers/staging/fbtft/fb_ssd1351.c
@@ -190,15 +190,12 @@ static struct fbtft_display display = {
static int update_onboard_backlight(struct backlight_device *bd)
{
struct fbtft_par *par = bl_get_data(bd);
- bool on;
+ bool blank = backlight_is_blank(bd);

- fbtft_par_dbg(DEBUG_BACKLIGHT, par,
- "%s: power=%d, fb_blank=%d\n",
- __func__, bd->props.power, bd->props.fb_blank);
+ fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: blank=%d\n", __func__, blank);

- on = !backlight_is_blank(bd);
/* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */
- write_reg(par, 0xB5, on ? 0x03 : 0x02);
+ write_reg(par, 0xB5, !blank ? 0x03 : 0x02);

return 0;
}

--
2.34.1


2023-01-08 20:25:01

by Stephen Kitt

[permalink] [raw]
Subject: Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

On Sat, 07 Jan 2023 19:26:23 +0100, Sam Ravnborg via B4 Submission Endpoint
<[email protected]> wrote:

> From: Sam Ravnborg <[email protected]>
>
> Avoiding direct access to backlight_properties.props.
>
> Access to the deprecated props.fb_blank replaced by backlight_is_blank().
> Access to props.power is dropped - it was only used for debug.
>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Cc: Stephen Kitt <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: [email protected]
> ---
> drivers/staging/fbtft/fb_ssd1351.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> b/drivers/staging/fbtft/fb_ssd1351.c index b8d55aa8c5c7..995fbd2f3dc6 100644
> --- a/drivers/staging/fbtft/fb_ssd1351.c
> +++ b/drivers/staging/fbtft/fb_ssd1351.c
> @@ -190,15 +190,12 @@ static struct fbtft_display display = {
> static int update_onboard_backlight(struct backlight_device *bd)
> {
> struct fbtft_par *par = bl_get_data(bd);
> - bool on;
> + bool blank = backlight_is_blank(bd);
>
> - fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> - "%s: power=%d, fb_blank=%d\n",
> - __func__, bd->props.power, bd->props.fb_blank);
> + fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: blank=%d\n", __func__,
> blank);
> - on = !backlight_is_blank(bd);
> /* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */
> - write_reg(par, 0xB5, on ? 0x03 : 0x02);
> + write_reg(par, 0xB5, !blank ? 0x03 : 0x02);
>
> return 0;
> }
>
> --
> 2.34.1

For debugging purposes here, would there be any point in logging props.state?
As in

fbtft_par_dbg(DEBUG_BACKLIGHT, par,
- "%s: power=%d, fb_blank=%d\n",
- __func__, bd->props.power, bd->props.fb_blank);
+ "%s: power=%d, state=%u\n",
+ __func__, bd->props.power, bd->props.state);

In any case,

Reviewed-by: Stephen Kitt <[email protected]>

Regards,

Stephen

Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2023-01-08 20:37:11

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

Hi Stephen,

On Sun, Jan 08, 2023 at 08:28:17PM +0100, Stephen Kitt wrote:
> On Sat, 07 Jan 2023 19:26:23 +0100, Sam Ravnborg via B4 Submission Endpoint
> <[email protected]> wrote:
>
> > From: Sam Ravnborg <[email protected]>
> >
> > Avoiding direct access to backlight_properties.props.
> >
> > Access to the deprecated props.fb_blank replaced by backlight_is_blank().
> > Access to props.power is dropped - it was only used for debug.
> >
> > Signed-off-by: Sam Ravnborg <[email protected]>
> > Cc: Stephen Kitt <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Daniel Thompson <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/staging/fbtft/fb_ssd1351.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> > b/drivers/staging/fbtft/fb_ssd1351.c index b8d55aa8c5c7..995fbd2f3dc6 100644
> > --- a/drivers/staging/fbtft/fb_ssd1351.c
> > +++ b/drivers/staging/fbtft/fb_ssd1351.c
> > @@ -190,15 +190,12 @@ static struct fbtft_display display = {
> > static int update_onboard_backlight(struct backlight_device *bd)
> > {
> > struct fbtft_par *par = bl_get_data(bd);
> > - bool on;
> > + bool blank = backlight_is_blank(bd);
> >
> > - fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> > - "%s: power=%d, fb_blank=%d\n",
> > - __func__, bd->props.power, bd->props.fb_blank);
> > + fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: blank=%d\n", __func__,
> > blank);
> > - on = !backlight_is_blank(bd);
> > /* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */
> > - write_reg(par, 0xB5, on ? 0x03 : 0x02);
> > + write_reg(par, 0xB5, !blank ? 0x03 : 0x02);
> >
> > return 0;
> > }
> >
> > --
> > 2.34.1
>
> For debugging purposes here, would there be any point in logging props.state?
> As in
>
> fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> - "%s: power=%d, fb_blank=%d\n",
> - __func__, bd->props.power, bd->props.fb_blank);
> + "%s: power=%d, state=%u\n",
> + __func__, bd->props.power, bd->props.state);

Thanks for the suggestion - and the reviews!

I was tempted to just remove the debugging.
If we require debugging, then this could be added in the backlight core,
thus everyone would benefit from it.

The solution above avoid any direct use of backlight_properties
which I consider a layer violation outside the backlight core.
(We cannot avoid it today with the current interface - but we can
minimize it).

Sam

2023-01-09 09:27:01

by Stephen Kitt

[permalink] [raw]
Subject: Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

Hi Sam,

On Sun, 8 Jan 2023 21:29:43 +0100, Sam Ravnborg <[email protected]> wrote:
> On Sun, Jan 08, 2023 at 08:28:17PM +0100, Stephen Kitt wrote:
> > On Sat, 07 Jan 2023 19:26:23 +0100, Sam Ravnborg via B4 Submission
> > Endpoint <[email protected]> wrote:
> >
> > > From: Sam Ravnborg <[email protected]>
> > >
> > > Avoiding direct access to backlight_properties.props.
> > >
> > > Access to the deprecated props.fb_blank replaced by
> > > backlight_is_blank(). Access to props.power is dropped - it was only
> > > used for debug.
> > >
> > > Signed-off-by: Sam Ravnborg <[email protected]>
> > > Cc: Stephen Kitt <[email protected]>
> > > Cc: Greg Kroah-Hartman <[email protected]>
> > > Cc: Daniel Thompson <[email protected]>
> > > Cc: Andy Shevchenko <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > drivers/staging/fbtft/fb_ssd1351.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> > > b/drivers/staging/fbtft/fb_ssd1351.c index b8d55aa8c5c7..995fbd2f3dc6
> > > 100644 --- a/drivers/staging/fbtft/fb_ssd1351.c
> > > +++ b/drivers/staging/fbtft/fb_ssd1351.c
> > > @@ -190,15 +190,12 @@ static struct fbtft_display display = {
> > > static int update_onboard_backlight(struct backlight_device *bd)
> > > {
> > > struct fbtft_par *par = bl_get_data(bd);
> > > - bool on;
> > > + bool blank = backlight_is_blank(bd);
> > >
> > > - fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> > > - "%s: power=%d, fb_blank=%d\n",
> > > - __func__, bd->props.power, bd->props.fb_blank);
> > > + fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: blank=%d\n", __func__,
> > > blank);
> > > - on = !backlight_is_blank(bd);
> > > /* Onboard backlight connected to GPIO0 on SSD1351, GPIO1
> > > unused */
> > > - write_reg(par, 0xB5, on ? 0x03 : 0x02);
> > > + write_reg(par, 0xB5, !blank ? 0x03 : 0x02);
> > >
> > > return 0;
> > > }
> > >
> > > --
> > > 2.34.1
> >
> > For debugging purposes here, would there be any point in logging
> > props.state? As in
> >
> > fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> > - "%s: power=%d, fb_blank=%d\n",
> > - __func__, bd->props.power, bd->props.fb_blank);
> > + "%s: power=%d, state=%u\n",
> > + __func__, bd->props.power, bd->props.state);
>
> Thanks for the suggestion - and the reviews!
>
> I was tempted to just remove the debugging.
> If we require debugging, then this could be added in the backlight core,
> thus everyone would benefit from it.
>
> The solution above avoid any direct use of backlight_properties
> which I consider a layer violation outside the backlight core.
> (We cannot avoid it today with the current interface - but we can
> minimize it).

Ah yes, ideally backlight_properties should be viewed as an opaque structure,
that makes sense.

Regards,

Stephen

Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2023-01-09 11:38:15

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

On Sat, Jan 07, 2023 at 07:26:23PM +0100, Sam Ravnborg via B4 Submission Endpoint wrote:
> From: Sam Ravnborg <[email protected]>
>
> Avoiding direct access to backlight_properties.props.
>
> Access to the deprecated props.fb_blank replaced by backlight_is_blank().
> Access to props.power is dropped - it was only used for debug.

> Signed-off-by: Sam Ravnborg <[email protected]>
> Cc: Stephen Kitt <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Daniel Thompson <[email protected]>
> Cc: Andy Shevchenko <[email protected]>

> Cc: [email protected]

Not sure why you have this (at least) explicitly mentioned as get_maintainer.pl
can generate it and git send-email can utilize the script output...

...

> - write_reg(par, 0xB5, on ? 0x03 : 0x02);
> + write_reg(par, 0xB5, !blank ? 0x03 : 0x02);

Why not positive conditional?

write_reg(par, 0xB5, blank ? 0x02 : 0x03);

--
With Best Regards,
Andy Shevchenko


2023-01-09 12:10:18

by Daniel Thompson

[permalink] [raw]
Subject: Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()

On Sun, Jan 08, 2023 at 09:29:43PM +0100, Sam Ravnborg wrote:
> Hi Stephen,
>
> On Sun, Jan 08, 2023 at 08:28:17PM +0100, Stephen Kitt wrote:
> > On Sat, 07 Jan 2023 19:26:23 +0100, Sam Ravnborg via B4 Submission Endpoint
> > <[email protected]> wrote:
> >
> > > From: Sam Ravnborg <[email protected]>
> > >
> > > Avoiding direct access to backlight_properties.props.
> > >
> > > Access to the deprecated props.fb_blank replaced by backlight_is_blank().
> > > Access to props.power is dropped - it was only used for debug.
> > >
> > > Signed-off-by: Sam Ravnborg <[email protected]>
> > > Cc: Stephen Kitt <[email protected]>
> > > Cc: Greg Kroah-Hartman <[email protected]>
> > > Cc: Daniel Thompson <[email protected]>
> > > Cc: Andy Shevchenko <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > drivers/staging/fbtft/fb_ssd1351.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> > > b/drivers/staging/fbtft/fb_ssd1351.c index b8d55aa8c5c7..995fbd2f3dc6 100644
> > > --- a/drivers/staging/fbtft/fb_ssd1351.c
> > > +++ b/drivers/staging/fbtft/fb_ssd1351.c
> > > @@ -190,15 +190,12 @@ static struct fbtft_display display = {
> > > static int update_onboard_backlight(struct backlight_device *bd)
> > > {
> > > struct fbtft_par *par = bl_get_data(bd);
> > > - bool on;
> > > + bool blank = backlight_is_blank(bd);
> > >
> > > - fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> > > - "%s: power=%d, fb_blank=%d\n",
> > > - __func__, bd->props.power, bd->props.fb_blank);
> > > + fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: blank=%d\n", __func__,
> > > blank);
> > > - on = !backlight_is_blank(bd);
> > > /* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */
> > > - write_reg(par, 0xB5, on ? 0x03 : 0x02);
> > > + write_reg(par, 0xB5, !blank ? 0x03 : 0x02);
> > >
> > > return 0;
> > > }
> > >
> > > --
> > > 2.34.1
> >
> > For debugging purposes here, would there be any point in logging props.state?
> > As in
> >
> > fbtft_par_dbg(DEBUG_BACKLIGHT, par,
> > - "%s: power=%d, fb_blank=%d\n",
> > - __func__, bd->props.power, bd->props.fb_blank);
> > + "%s: power=%d, state=%u\n",
> > + __func__, bd->props.power, bd->props.state);
>
> Thanks for the suggestion - and the reviews!
>
> I was tempted to just remove the debugging.
> If we require debugging, then this could be added in the backlight core,
> thus everyone would benefit from it.

I had the same instinct to remove the debug print here (esp. ones with
__func__ in them) but I think that's probably a much more widely scoped
clean up for fbtft ;-).

On that basis:
Reviewed-by: Daniel Thompson <[email protected]>


Daniel.