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@xxxxxxxxxx> 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.