Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered down

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> On Fri, Jun 21, 2019 at 03:56:08PM +0200, Thierry Reding wrote:
> > On Fri, Jun 21, 2019 at 01:41:45PM +0100, Daniel Thompson wrote:
> > > On 22/05/2019 17:34, Paul Cercueil wrote:
> > > > When the driver probes, the PWM pin is automatically configured to its
> > > > default state, which should be the "pwm" function.
> > > 
> > > At which point in the probe... and by who?
> > 
> > The driver core will select the "default" state of a device right before
> > calling the driver's probe, see:
> > 
> > 	drivers/base/pinctrl.c: pinctrl_bind_pins()
> > 
> > which is called from:
> > 
> > 	drivers/base/dd.c: really_probe()
> > 
> 
> Thanks. I assumed it would be something like that... although given
> pwm-backlight is essentially a wrapper driver round a PWM I wondered why
> the pinctrl was on the backlight node (rather than the PWM node).

I agree with this. We're defining the pin control state for the PWM pin,
so in my opinion it should be the PWM driver that controls it.

One reason why I think this is important is if we ever end up with a
device that requires pins from two different controllers to be
configured at runtime, then how would we model that? Since pin control
states cannot be aggregated, so you'd have to have multiple "default"
states, each for the pins that they control.

On the other hand if we associate the pin control states with each of
the resources that need those states, then when those resources are
controlled, they will automatically know how to deal with the states.
The top-level device (i.e. backlight) doesn't need to concern itself
with those details.

> Looking at the DTs in the upstream kernel it looks like ~20% of the
> backlight drivers have pinctrl on the backlight node. Others presumable
> have none or have it on the PWM node (and it looks like support for
> sleeping the pins is *very* rare amoung the PWM drivers).

I suspect that that's mostly a sign of our device trees and kernel
subsystems still maturing. For example, I think it's fairly rare for a
device to seamlessly take over the display configuration from the
bootloader. Most of the time you'll just see things go black (that's
actually one of the better cases) when the kernel takes over and then
the backlight will come up again at some point.

Taking over the bootloader's display configuration is pretty hard and
there are numerous pieces to the puzzle (need to make sure clocks and
power supplies are not automatically disabled after the initcalls,
display drivers need to know how to read out hardware, claim whatever
memory region the bootloader was using for a bootsplash, backlight is
supposed to remain enabled if the bootloader turned it on, ...).

I don't think the fact that PWM drivers don't support this implies that
hardware doesn't support it. I think we've just never needed it before
because we get away with it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]