On Thu, Jul 20, 2023 at 02:56:32PM +0200, Bartosz Golaszewski wrote:

On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
[off-list ref] wrote:

quoted

On Thu, Jul 20, 2023 at 06:06:27AM +0000, Ying Liu wrote:

quoted

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

quoted

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.