| Message ID | 20230720061105.154821-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | backlight: gpio_backlight: Drop output gpio direction check for initial power state | expand |
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 >
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
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.
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.
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.
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
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<-------------------------------------------
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;
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(-)