[RFC] pwm: Add the concept of an "active" pinctrl

Message ID 1411514246-6163-1-git-send-email-dianders@chromium.org
State New
Headers show

Commit Message

Doug Anderson Sept. 23, 2014, 11:17 p.m.
There are cases where you really don't want a PWM's pinctrl to take
effect until you know that the PWM is driving at the proper rate.  A
good example of this is a PWM-controlled regulator, where the boot
state of the pin (maybe a pulled down input) could cause a vastly
different voltage to take effect than having the pin setup as a PWM
but not having the PWM enabled yet.

Let's add a feature where you can define an "active" pinctrl state.
This state (if it exists) will be applied after the PWM has actually
been enabled.  If the PWM is ever disabled we'll go back to "default"
state.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
I'm sure Linus W or Thierry or Mark will tell me this is an idiotic
idea (or totally the wrong way to do it), but I figured I'd float it
out there and maybe someone can tell me something better to do.  ;)

 drivers/pwm/core.c  | 34 +++++++++++++++++++++++++++++++---
 include/linux/pwm.h |  8 ++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

Comments

Linus Walleij Sept. 25, 2014, 7:31 a.m. | #1
On Wed, Sep 24, 2014 at 1:17 AM, Doug Anderson <dianders@chromium.org> wrote:

> There are cases where you really don't want a PWM's pinctrl to take
> effect until you know that the PWM is driving at the proper rate.  A
> good example of this is a PWM-controlled regulator, where the boot
> state of the pin (maybe a pulled down input) could cause a vastly
> different voltage to take effect than having the pin setup as a PWM
> but not having the PWM enabled yet.
>
> Let's add a feature where you can define an "active" pinctrl state.
> This state (if it exists) will be applied after the PWM has actually
> been enabled.  If the PWM is ever disabled we'll go back to "default"
> state.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> I'm sure Linus W or Thierry or Mark will tell me this is an idiotic
> idea (or totally the wrong way to do it), but I figured I'd float it
> out there and maybe someone can tell me something better to do.  ;)

Hey it's not like this is obvious or anything....

The device core will handle three states for you: "default", "sleep", "idle".

What I worry about in this patch is that you redefine the meaning
of "default" from what it used to be into "sleep", basically, that
you want the device to come up in a relaxed mode, whereas
the "default" state is usually defined as "ready to go", i.e. what
you refer to as "active".

If you choose this approach, it is better to not define the "default"
state *at all*, just define "active" and "sleep" and/or "idle".
(The difference between the two latter is basically the same
semantic as suspend vs runtime suspend.)

We have some helpers in the device core to help getting
things right.

Inspecting the helper code in drivers/base/pinctrl.c you will find
that if the "default" state is defined it will be retrieved and
activated on boot, so you don't have to do this yourself. Then
if "sleep" and "idle" exist, these will be grabbed and made
accessible using special helper functions.

I would actually prefer, if it is not causing any trouble, to have
the same semantics as the core expects, then have the driver
do something like this in probe():

probe():
   pinctrl_pm_select_sleep_state(dev);

This means the device will be pushed to "default" before
it's probed, then set to sleep quickly after that. It requires that
CONFIG_PM is enabled for the platform, too. (As you can see
in drivers/base/pinctrl.c)

Then when you want it back into "active", i.e. "default" mode
use the pinctrl_pm_select_default_state(dev);

I realize that this means that you will end up with a transient
state where the pins are in "default" mode before probe
commence. If this is a big problem, yeah, then you have to
go for custom states. But then the "default" state should not
be defined at all, instead call them something unique like
"active" and "relaxed" or so, so you don't overrun states already
used by the pinctrl core. Then all confusion is gone.

The "default" state was added with the assumption that most
drivers want their pins to be in "ready to go" mode by default.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson Sept. 25, 2014, 8:26 p.m. | #2
Linus,

On Thu, Sep 25, 2014 at 12:31 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Wed, Sep 24, 2014 at 1:17 AM, Doug Anderson <dianders@chromium.org> wrote:
>
>> There are cases where you really don't want a PWM's pinctrl to take
>> effect until you know that the PWM is driving at the proper rate.  A
>> good example of this is a PWM-controlled regulator, where the boot
>> state of the pin (maybe a pulled down input) could cause a vastly
>> different voltage to take effect than having the pin setup as a PWM
>> but not having the PWM enabled yet.
>>
>> Let's add a feature where you can define an "active" pinctrl state.
>> This state (if it exists) will be applied after the PWM has actually
>> been enabled.  If the PWM is ever disabled we'll go back to "default"
>> state.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>> I'm sure Linus W or Thierry or Mark will tell me this is an idiotic
>> idea (or totally the wrong way to do it), but I figured I'd float it
>> out there and maybe someone can tell me something better to do.  ;)
>
> Hey it's not like this is obvious or anything....

;)


> The device core will handle three states for you: "default", "sleep", "idle".

Except it will handle only one state unless "CONFIG_PM" is defined,
right?  In other words "sleep" and "idle" totally don't exist in the
"!CONFIG_PM" case.

Maybe I shouldn't worry about the "!CONFIG_PM" case, but in my use
case this extra state was needed for correctness (so we didn't glitch
the voltage), so it seemed unwise to rely on the "sleep" and "idle"
states.  Normally it should be considered "safe" (though high power)
to disable CONFIG_PM, right?  ...of course I could say that anyone
using this particular feature should just put some hard dependency on
"PM" in their Kconfig and call it done...  ...or I I could make "idle"
present always.


> What I worry about in this patch is that you redefine the meaning
> of "default" from what it used to be into "sleep", basically, that
> you want the device to come up in a relaxed mode, whereas
> the "default" state is usually defined as "ready to go", i.e. what
> you refer to as "active".

Yeah, I didn't like that either.  That's mostly why I put the RFC and
"I'm gonna get yelled at" comment.  ;)

Hrm, I wonder if another reasonable option is to add a boolean
property somewhere saying that "pinctrl starts idle" would fix things
for me.


> If you choose this approach, it is better to not define the "default"
> state *at all*, just define "active" and "sleep" and/or "idle".
> (The difference between the two latter is basically the same
> semantic as suspend vs runtime suspend.)
>
> We have some helpers in the device core to help getting
> things right.
>
> Inspecting the helper code in drivers/base/pinctrl.c you will find
> that if the "default" state is defined it will be retrieved and
> activated on boot, so you don't have to do this yourself. Then
> if "sleep" and "idle" exist, these will be grabbed and made
> accessible using special helper functions.

How I deal with this would depend on whether I require CONFIG_PM.
What do you think?


> I would actually prefer, if it is not causing any trouble, to have
> the same semantics as the core expects, then have the driver
> do something like this in probe():
>
> probe():
>    pinctrl_pm_select_sleep_state(dev);
>
> This means the device will be pushed to "default" before
> it's probed, then set to sleep quickly after that. It requires that
> CONFIG_PM is enabled for the platform, too. (As you can see
> in drivers/base/pinctrl.c)
>
> Then when you want it back into "active", i.e. "default" mode
> use the pinctrl_pm_select_default_state(dev);
>
> I realize that this means that you will end up with a transient
> state where the pins are in "default" mode before probe
> commence. If this is a big problem, yeah, then you have to
> go for custom states.

In this case I think it could be a big problem.  When you're just
talking about a PWM backlight then it doesn't matter much, but I don't
like the idea of letting a regulator glitch to a random voltage during
probe.


> But then the "default" state should not
> be defined at all, instead call them something unique like
> "active" and "relaxed" or so, so you don't overrun states already
> used by the pinctrl core. Then all confusion is gone.
>
> The "default" state was added with the assumption that most
> drivers want their pins to be in "ready to go" mode by default.

What do you think about the "default to idle" boolean, then (and
corresponding device tree property)?  It would be a pretty small patch
to pinctrl_bind_pins(), I think.  I could make sure that "idle" is
defined even if no CONFIG_PM...


-Doug

P.S. Some of this might be moot point on my particular board.  It
looks likely that the firmware will want to leave this PWM running
when it starts the kernel and they kernel could really just get by
with not touching the pinctrl at all.  ...however, I think that the
points made in this patch are still valid and it would still be worth
addressing them (and you never know, someone might be running old
firmware).
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 7, 2014, 1:15 p.m. | #3
On Thu, Sep 25, 2014 at 10:26 PM, Doug Anderson <dianders@chromium.org> wrote:
> On Thu, Sep 25, 2014 at 12:31 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Wed, Sep 24, 2014 at 1:17 AM, Doug Anderson <dianders@chromium.org> wrote:

>> The device core will handle three states for you: "default", "sleep", "idle".
>
> Except it will handle only one state unless "CONFIG_PM" is defined,
> right?  In other words "sleep" and "idle" totally don't exist in the
> "!CONFIG_PM" case.

Yes. Because the intent of these settings is usually to save power.
So by disabling the PM config, you can test if the system works
without PM. Even hairy stuff like power-related pin control. It has
this nice orthogonal debug feature to it...

> Normally it should be considered "safe" (though high power)
> to disable CONFIG_PM, right?

Yeah. If your usecase makes it unsafe to have just "default",
you should not rely on these state names but invent new ones.

>> What I worry about in this patch is that you redefine the meaning
>> of "default" from what it used to be into "sleep", basically, that
>> you want the device to come up in a relaxed mode, whereas
>> the "default" state is usually defined as "ready to go", i.e. what
>> you refer to as "active".
>
> Yeah, I didn't like that either.  That's mostly why I put the RFC and
> "I'm gonna get yelled at" comment.  ;)
>
> Hrm, I wonder if another reasonable option is to add a boolean
> property somewhere saying that "pinctrl starts idle" would fix things
> for me.

That's a pretty good idea actually.

It has to be part of the state defintion and also the device tree
bindings then I guess?

Something in <linux/pinctrl/machine.h> needs patching.

>> Inspecting the helper code in drivers/base/pinctrl.c you will find
>> that if the "default" state is defined it will be retrieved and
>> activated on boot, so you don't have to do this yourself. Then
>> if "sleep" and "idle" exist, these will be grabbed and made
>> accessible using special helper functions.
>
> How I deal with this would depend on whether I require CONFIG_PM.
> What do you think?

No I don't have everything in my head, it depends on whether
you can live with the device being in the "default" state if
CONFIG_PM is disabled.

You could always have the driver do this in Kconfig:

depends on PM

but I don't know if that is desireable.

>> I realize that this means that you will end up with a transient
>> state where the pins are in "default" mode before probe
>> commence. If this is a big problem, yeah, then you have to
>> go for custom states.
>
> In this case I think it could be a big problem.  When you're just
> talking about a PWM backlight then it doesn't matter much, but I don't
> like the idea of letting a regulator glitch to a random voltage during
> probe.

OK something like tagging the device to go to "sleep"
when probing would work.

> What do you think about the "default to idle" boolean, then (and
> corresponding device tree property)?  It would be a pretty small patch
> to pinctrl_bind_pins(), I think.  I could make sure that "idle" is
> defined even if no CONFIG_PM...

Yeah I sort of like this idea.

Like you could override what is the actual preferred
probe state.

I would like something like that:

        /* For a client device requiring named states */
        device {
                pinctrl-names = "default", "sleep";
                pinctrl-0 = <&state_0_node_a>;
                pinctrl-1 = <&state_1_node_a &state_1_node_b>;
                pinctrl-probe-state = "sleep";
        };



Then patch the code in drivers/base/pinctrl.c to respect
this if DT is used and this is present, else just use "default".

Note that since it's just a string, you can specify any state
here.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson Oct. 7, 2014, 8:30 p.m. | #4
Linus,

On Tue, Oct 7, 2014 at 6:15 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Sep 25, 2014 at 10:26 PM, Doug Anderson <dianders@chromium.org> wrote:
>> On Thu, Sep 25, 2014 at 12:31 AM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>>> On Wed, Sep 24, 2014 at 1:17 AM, Doug Anderson <dianders@chromium.org> wrote:
>
>>> The device core will handle three states for you: "default", "sleep", "idle".
>>
>> Except it will handle only one state unless "CONFIG_PM" is defined,
>> right?  In other words "sleep" and "idle" totally don't exist in the
>> "!CONFIG_PM" case.
>
> Yes. Because the intent of these settings is usually to save power.
> So by disabling the PM config, you can test if the system works
> without PM. Even hairy stuff like power-related pin control. It has
> this nice orthogonal debug feature to it...
>
>> Normally it should be considered "safe" (though high power)
>> to disable CONFIG_PM, right?
>
> Yeah. If your usecase makes it unsafe to have just "default",
> you should not rely on these state names but invent new ones.

I thought about it a bit more and I decided that a new state made the
most sense.  I put another RFC up there at
<https://patchwork.kernel.org/patch/5049041/>.  Feel free to have a
look at it.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 966497d..12f86e0 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -245,6 +245,14 @@  int pwmchip_add(struct pwm_chip *chip)
 	if (ret < 0)
 		goto out;
 
+	chip->pinctrl = devm_pinctrl_get(chip->dev);
+	if (!IS_ERR_OR_NULL(chip->pinctrl)) {
+		chip->state_default = pinctrl_lookup_state(chip->pinctrl,
+							   "default");
+		chip->state_active = pinctrl_lookup_state(chip->pinctrl,
+							  "active");
+	}
+
 	chip->pwms = kzalloc(chip->npwm * sizeof(*pwm), GFP_KERNEL);
 	if (!chip->pwms) {
 		ret = -ENOMEM;
@@ -457,8 +465,17 @@  EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
-		return pwm->chip->ops->enable(pwm->chip, pwm);
+	int ret;
+
+	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+		ret = pwm->chip->ops->enable(pwm->chip, pwm);
+
+		if (!ret && !IS_ERR_OR_NULL(pwm->chip->state_active))
+			ret = pinctrl_select_state(pwm->chip->pinctrl,
+						   pwm->chip->state_active);
+
+		return ret;
+	}
 
 	return pwm ? 0 : -EINVAL;
 }
@@ -470,8 +487,19 @@  EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (pwm && test_and_clear_bit(PWMF_ENABLED, &pwm->flags))
+	int ret;
+
+	if (pwm && test_and_clear_bit(PWMF_ENABLED, &pwm->flags)) {
+		if (!IS_ERR_OR_NULL(pwm->chip->state_default)) {
+			ret = pinctrl_select_state(pwm->chip->pinctrl,
+						   pwm->chip->state_default);
+			if (ret)
+				dev_warn(pwm->chip->dev,
+					 "Couldn't set def state: %d\n", ret);
+		}
+
 		pwm->chip->ops->disable(pwm->chip, pwm);
+	}
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..039c898 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -162,6 +162,10 @@  struct pwm_ops {
  * @pwms: array of PWM devices allocated by the framework
  * @can_sleep: must be true if the .config(), .enable() or .disable()
  *             operations may sleep
+ *
+ * @pinctrl: Pointer to the a pinctrl for the dev
+ * @state_default: We'll apply this when a PWM is disabled
+ * @state_active: We'll apply this when a PWM is enabled
  */
 struct pwm_chip {
 	struct device		*dev;
@@ -176,6 +180,10 @@  struct pwm_chip {
 					    const struct of_phandle_args *args);
 	unsigned int		of_pwm_n_cells;
 	bool			can_sleep;
+
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *state_default;
+	struct pinctrl_state *state_active;
 };
 
 #if IS_ENABLED(CONFIG_PWM)