| Message ID | 1411514246-6163-1-git-send-email-dianders@chromium.org |
|---|---|
| State | Deferred |
| Headers | show |
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
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
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
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
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)
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(-)