Message-ID: <20190626095844.GA6362@ulmo>
Date:   Wed, 26 Jun 2019 11:58:44 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Daniel Thompson <daniel.thompson@...aro.org>,
        Paul Cercueil <paul@...pouillou.net>,
        Lee Jones <lee.jones@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        od@...c.me, linux-pwm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered
 down

On Wed, Jun 26, 2019 at 10:58:27AM +0200, Uwe Kleine-König wrote:
> On Tue, Jun 25, 2019 at 11:38:39AM +0200, Thierry Reding wrote:
> > On Mon, Jun 24, 2019 at 12:28:44PM +0100, Daniel Thompson wrote:
> > > [...] 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.
> 
> I thought you can do:
> 
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_in_first_pincontroller>, <&pinctrl_in_another_controller>;
> 
> if two (or more) controllers are involved.

You're right. Both the bindings say that this can be done and the code
is also there to parse multiple states per pinctrl-* entry.

> > 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.
> 
> So the options are:
> 
>  a) put "active" and "inactive" pinctrls into the pwm-node, and nothing
>     related to the involved PWM pins in the consumer
> 
>  b) put the PWM pin config in the consumer's "default" pinctrl (and
>     maybe leave it out int "init" if you want smooth taking over).

You can't put it into the "default" state because that state is applied
before the consumer driver's ->probe().

> 
> (Or maybe use "enabled" and "disabled" in a) to match the pwm_states
> .enabled?)

Yeah, I think this is what we'll need to do in order to implement the
explicit behaviour that we need here.

> The advantages I see in b) over a) are:
> 
>  - "default" and "init" are a known pinctrl concept that most people
>    should have understood.

The problem is that they won't work in this case. The "init" state will
be applied before the consumer driver's ->probe() if it exists. If it
doesn't then "default" will be applied instead. Both cases are not
something that we want if we want to take over the existing
configuration.

>  - You have all pinctrl config for the backlight in a single place.

Depending on your point of view this could be considered a disadvantage.

>  - none of the involved driver must explicitly handle pinctrl stuff

Like I said, none of the automatic state handling is flexible enough for
this situation. Also, my understanding is that even if you use the
standard pinctrl state names ("default" and "idle") you still need to
explicitly select them at the right time. "default" will always be
applied before the consumer driver's ->probe(), but if you want to go to
the "idle" state you have to make that explicit. Now, there are helpers
to simplify this a bit, but you still need to implement suspend/resume
callbacks (or however you want to deal with it) that call these helpers.

In the case of PWM I think what we want is to select an "active" and
"idle" state on enable and disable, respectively. I suppose we could add
some infrastructure to help with this, such as perhaps scanning the
device tree for per-PWM pin control states at PWM chip registration time
and then adding helpers to select these states at the driver's
discretion. I don't think we can add generic code to do this because the
exact time when the pin control state needs to be applied may vary from
one PWM controller to another.

> You presume that b) being commonly done is a sign of "our device trees
> and kernel subsystems still maturing". But maybe it's only that the
> capabilities provided by pinctrl subsystem without extra effort is good
> enough?

Like I pointed out above, I don't think that's the case. But I don't
want to overcomplicate things, so if you can prove that it can be done
with the existing pinctrl helpers, I'd be happy to be proven wrong.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists