[PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Ying Liu]

Bootloader may leave gpio direction as input and gpio value as logical low.
It hints that initial backlight power state should be FB_BLANK_POWERDOWN
since the gpio value is literally logical low.  So, let's drop output gpio
direction check and only check gpio value to set the initial power state.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/video/backlight/gpio_backlight.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index d3bea42407f1..d28c30b2a35d 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -87,8 +87,7 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		/* Not booted with device tree or no phandle link to the node */
 		bl->props.power = def_value ? FB_BLANK_UNBLANK
 					    : FB_BLANK_POWERDOWN;
-	else if (gpiod_get_direction(gbl->gpiod) == 0 &&
-		 gpiod_get_value_cansleep(gbl->gpiod) == 0)
+	else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
 		bl->props.power = FB_BLANK_POWERDOWN;
 	else
 		bl->props.power = FB_BLANK_UNBLANK;
-- 
2.37.1

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Daniel Thompson]

On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> Bootloader may leave gpio direction as input and gpio value as logical low.
> It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> since the gpio value is literally logical low.

To be honest this probably "hints" that the bootloader simply didn't
consider the backlight at all :-) . I'd rather the patch description
focus on what circumstances lead to the current code making a bad
decision. More like:

  If the GPIO pin is in the input state but the backlight is currently
  off due to default pull downs then ...

> So, let's drop output gpio
> direction check and only check gpio value to set the initial power state.

This check was specifically added by Bartosz so I'd be interested in his
opinion of this change (especially since he is now a GPIO maintainer)!

What motivates (or motivated) the need to check the direction rather
than just read that current logic level on the pin?


Daniel.
[I'm done but since Bartosz and Linus were not on copy of the original
thread I've left the rest of the patch below as a convenience ;-) ]


> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/video/backlight/gpio_backlight.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index d3bea42407f1..d28c30b2a35d 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -87,8 +87,7 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		/* Not booted with device tree or no phandle link to the node */
>  		bl->props.power = def_value ? FB_BLANK_UNBLANK
>  					    : FB_BLANK_POWERDOWN;
> -	else if (gpiod_get_direction(gbl->gpiod) == 0 &&
> -		 gpiod_get_value_cansleep(gbl->gpiod) == 0)
> +	else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
>  		bl->props.power = FB_BLANK_POWERDOWN;
>  	else
>  		bl->props.power = FB_BLANK_UNBLANK;
> --
> 2.37.1
>

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Bartosz Golaszewski]

On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > Bootloader may leave gpio direction as input and gpio value as logical low.
> > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > since the gpio value is literally logical low.
>
> To be honest this probably "hints" that the bootloader simply didn't
> consider the backlight at all :-) . I'd rather the patch description
> focus on what circumstances lead to the current code making a bad
> decision. More like:
>
>   If the GPIO pin is in the input state but the backlight is currently
>   off due to default pull downs then ...
>
> > So, let's drop output gpio
> > direction check and only check gpio value to set the initial power state.
>
> This check was specifically added by Bartosz so I'd be interested in his
> opinion of this change (especially since he is now a GPIO maintainer)!
>
> What motivates (or motivated) the need to check the direction rather
> than just read that current logic level on the pin?
>
>
> Daniel.
> [I'm done but since Bartosz and Linus were not on copy of the original
> thread I've left the rest of the patch below as a convenience ;-) ]
>

This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
set the direction of the GPIO").

Let me quote myself from it:

--
The GPIO backlight driver currently requests the line 'as is', without
actively setting its direction. This can lead to problems: if the line
is in input mode by default, we won't be able to drive it later when
updating the status and also reading its initial value doesn't make
sense for backlight setting.
--

I agree with Thomas that it's highly unlikely the bootloader "hints"
at any specific backlight settings. That being said, the change itself
looks correct to me. The other branch of that if will always unblank
the backlight if the GPIO is in input mode which may not be desirable.
I don't see any obvious problem with this change, just make sure the
commit message makes more sense.

Bartosz

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Daniel Thompson]

On Thu, Jul 20, 2023 at 02:56:32PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > > Bootloader may leave gpio direction as input and gpio value as logical low.
> > > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > > since the gpio value is literally logical low.
> >
> > To be honest this probably "hints" that the bootloader simply didn't
> > consider the backlight at all :-) . I'd rather the patch description
> > focus on what circumstances lead to the current code making a bad
> > decision. More like:
> >
> >   If the GPIO pin is in the input state but the backlight is currently
> >   off due to default pull downs then ...
> >
> > > So, let's drop output gpio
> > > direction check and only check gpio value to set the initial power state.
> >
> > This check was specifically added by Bartosz so I'd be interested in his
> > opinion of this change (especially since he is now a GPIO maintainer)!
> >
> > What motivates (or motivated) the need to check the direction rather
> > than just read that current logic level on the pin?
> >
> >
> > Daniel.
> > [I'm done but since Bartosz and Linus were not on copy of the original
> > thread I've left the rest of the patch below as a convenience ;-) ]
> >
>
> This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
> set the direction of the GPIO").
>
> Let me quote myself from it:
> --
> The GPIO backlight driver currently requests the line 'as is', without
> actively setting its direction. This can lead to problems: if the line
> is in input mode by default, we won't be able to drive it later when
> updating the status and also reading its initial value doesn't make
> sense for backlight setting.
> --

You are perhaps quoting the wrong bit here ;-). The currently proposed
patch leaves the code to put the pin into output mode unmodified. However
there was an extra line at the bottom of your commit message:
--
Also: check the current direction and only read the value if it's output.
--

This was the bit I wanted to check on, since the proposed patch
literally reverses this!

However...


> I agree with Thomas that it's highly unlikely the bootloader "hints"
> at any specific backlight settings. That being said, the change itself
> looks correct to me. The other branch of that if will always unblank
> the backlight if the GPIO is in input mode which may not be desirable.

... if you're happy the proposed change is OK then I'm happy too!
I came to the same conclusion after reviewing the GPIO code this morning,
however I copied you in because I was worried I might have overlooked
something.


> I don't see any obvious problem with this change, just make sure the
> commit message makes more sense.

Agreed.


Daniel.

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Bartosz Golaszewski]

On Thu, Jul 20, 2023 at 3:10 PM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jul 20, 2023 at 02:56:32PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > > > Bootloader may leave gpio direction as input and gpio value as logical low.
> > > > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > > > since the gpio value is literally logical low.
> > >
> > > To be honest this probably "hints" that the bootloader simply didn't
> > > consider the backlight at all :-) . I'd rather the patch description
> > > focus on what circumstances lead to the current code making a bad
> > > decision. More like:
> > >
> > >   If the GPIO pin is in the input state but the backlight is currently
> > >   off due to default pull downs then ...
> > >
> > > > So, let's drop output gpio
> > > > direction check and only check gpio value to set the initial power state.
> > >
> > > This check was specifically added by Bartosz so I'd be interested in his
> > > opinion of this change (especially since he is now a GPIO maintainer)!
> > >
> > > What motivates (or motivated) the need to check the direction rather
> > > than just read that current logic level on the pin?
> > >
> > >
> > > Daniel.
> > > [I'm done but since Bartosz and Linus were not on copy of the original
> > > thread I've left the rest of the patch below as a convenience ;-) ]
> > >
> >
> > This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
> > set the direction of the GPIO").
> >
> > Let me quote myself from it:
> > --
> > The GPIO backlight driver currently requests the line 'as is', without
> > actively setting its direction. This can lead to problems: if the line
> > is in input mode by default, we won't be able to drive it later when
> > updating the status and also reading its initial value doesn't make
> > sense for backlight setting.
> > --
>
> You are perhaps quoting the wrong bit here ;-). The currently proposed
> patch leaves the code to put the pin into output mode unmodified. However
> there was an extra line at the bottom of your commit message:
> --
> Also: check the current direction and only read the value if it's output.
> --

Yeah I'm no longer sure why I did this. The commit doesn't look harmful though.

Bart

>
> This was the bit I wanted to check on, since the proposed patch
> literally reverses this!
>
> However...
>
>
> > I agree with Thomas that it's highly unlikely the bootloader "hints"
> > at any specific backlight settings. That being said, the change itself
> > looks correct to me. The other branch of that if will always unblank
> > the backlight if the GPIO is in input mode which may not be desirable.
>
> ... if you're happy the proposed change is OK then I'm happy too!
> I came to the same conclusion after reviewing the GPIO code this morning,
> however I copied you in because I was worried I might have overlooked
> something.
>
>
> > I don't see any obvious problem with this change, just make sure the
> > commit message makes more sense.
>
> Agreed.
>
>
> Daniel.

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 2:27 PM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > Bootloader may leave gpio direction as input and gpio value as logical low.
> > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > since the gpio value is literally logical low.
>
> To be honest this probably "hints" that the bootloader simply didn't
> consider the backlight at all :-) . I'd rather the patch description
> focus on what circumstances lead to the current code making a bad
> decision. More like:
>
>   If the GPIO pin is in the input state but the backlight is currently
>   off due to default pull downs then ...
>
> > So, let's drop output gpio
> > direction check and only check gpio value to set the initial power state.
>
> This check was specifically added by Bartosz so I'd be interested in his
> opinion of this change (especially since he is now a GPIO maintainer)!
>
> What motivates (or motivated) the need to check the direction rather
> than just read that current logic level on the pin?

...

> > -     else if (gpiod_get_direction(gbl->gpiod) == 0 &&
> > -              gpiod_get_value_cansleep(gbl->gpiod) == 0)
> > +     else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> >               bl->props.power = FB_BLANK_POWERDOWN;

The code before this patch needs a bit of elaboration. There is no
prohibition on reading value for the pin that is in any direction.
I.o.w. if the direction here is a problem it should have been
configured beforehand.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Ying Liu]

Hi Daniel,

On Thursday, July 20, 2023 7:28 PM Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > Bootloader may leave gpio direction as input and gpio value as logical low.
> > It hints that initial backlight power state should be
> FB_BLANK_POWERDOWN
> > since the gpio value is literally logical low.
> 
> To be honest this probably "hints" that the bootloader simply didn't
> consider the backlight at all :-) . I'd rather the patch description
> focus on what circumstances lead to the current code making a bad
> decision. More like:
> 
>   If the GPIO pin is in the input state but the backlight is currently
>   off due to default pull downs then ...

How about this patch description?

---------------------------------8<-------------------------------------------
Without this patch, if gpio pin is in input state but backlight is currently
off due to default pull downs, then initial power state is set to
FB_BLANK_UNBLANK in DT boot mode with phandle link and the backlight is
effectively turned on in gpio_backlight_probe(), which is undesirable
according to patch description of commit ec665b756e6f ("backlight:
gpio-backlight: Correct initial power state handling").

Quote:
---
If in DT boot we have phandle link then leave the GPIO in a state which the
bootloader left it and let the user of the backlight to configure it further.
---

So, let's drop output gpio direction check and only check gpio value to set
the initial power state.
---------------------------------8<-------------------------------------------

Regards,
Liu Ying

Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 8:17 AM Ying Liu <victor.liu@nxp.com> wrote:
> On Thursday, July 20, 2023 7:28 PM Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:
> > > Bootloader may leave gpio direction as input and gpio value as logical low.
> > > It hints that initial backlight power state should be
> > FB_BLANK_POWERDOWN
> > > since the gpio value is literally logical low.
> >
> > To be honest this probably "hints" that the bootloader simply didn't
> > consider the backlight at all :-) . I'd rather the patch description
> > focus on what circumstances lead to the current code making a bad
> > decision. More like:
> >
> >   If the GPIO pin is in the input state but the backlight is currently
> >   off due to default pull downs then ...
>
> How about this patch description?
>
> ---------------------------------8<-------------------------------------------
> Without this patch, if gpio pin is in input state but backlight is currently

s/Without this patch, if/If/

> off due to default pull downs, then initial power state is set to
> FB_BLANK_UNBLANK in DT boot mode with phandle link and the backlight is
> effectively turned on in gpio_backlight_probe(), which is undesirable
> according to patch description of commit ec665b756e6f ("backlight:
> gpio-backlight: Correct initial power state handling").
>
> Quote:
> ---
> If in DT boot we have phandle link then leave the GPIO in a state which the
> bootloader left it and let the user of the backlight to configure it further.
> ---
>
> So, let's drop output gpio direction check and only check gpio value to set
> the initial power state.
> ---------------------------------8<-------------------------------------------

-- 
With Best Regards,
Andy Shevchenko