* [PATCH v4 0/3] Add PM support to STM32 LP Timer drivers @ 2019-02-14 10:25 Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state Fabrice Gasnier ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-14 10:25 UTC (permalink / raw) To: thierry.reding, robh+dt Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, linux-pwm, mcoquelin.stm32, u.kleine-koenig, fabrice.gasnier, linux-stm32, linux-arm-kernel This patch series adds power management support for STM32 LP Timer: - PWM driver - Document the pinctrl states for sleep mode It also adds device link between the PWM consumer and the PWM provider. This allows proper sequencing for suspend/resume (e.g. user will likely do a pwm_disable() before the PWM provider suspend executes), see [1]. [1] https://lkml.org/lkml/2019/2/5/770 --- Changes in v4: - improve error handling when adding the PWM consumer device link. Changes in v3: - Move the device_link_add() call to of_pwm_get() as discussed with Uwe. Changes in v2: - Don't disable PWM channel in PWM provider: rather refuse to suspend and report an error as suggested by Uwe and Thierry. - Add patch 3/3 to propose device link addition. - No updates for STM32 LP Timer IIO driver. Patches can be send separately. Fabrice Gasnier (3): dt-bindings: pwm-stm32-lp: document pinctrl sleep state pwm: stm32-lp: Add power management support pwm: core: add consumer device link .../devicetree/bindings/pwm/pwm-stm32-lp.txt | 9 ++-- drivers/pwm/core.c | 50 ++++++++++++++++++++-- drivers/pwm/pwm-stm32-lp.c | 25 +++++++++++ include/linux/pwm.h | 6 ++- 4 files changed, 82 insertions(+), 8 deletions(-) -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state 2019-02-14 10:25 [PATCH v4 0/3] Add PM support to STM32 LP Timer drivers Fabrice Gasnier @ 2019-02-14 10:25 ` Fabrice Gasnier 2019-02-18 16:45 ` Rob Herring 2019-02-14 10:25 ` [PATCH v4 2/3] pwm: stm32-lp: Add power management support Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 3/3] pwm: core: add consumer device link Fabrice Gasnier 2 siblings, 1 reply; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-14 10:25 UTC (permalink / raw) To: thierry.reding, robh+dt Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, linux-pwm, mcoquelin.stm32, u.kleine-koenig, fabrice.gasnier, linux-stm32, linux-arm-kernel Add documentation for pinctrl sleep state on STM32 LPTimer PWM. Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> --- Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt index bd23302..6521bc4 100644 --- a/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt @@ -11,8 +11,10 @@ Required parameters: bindings defined in pwm.txt. Optional properties: -- pinctrl-names: Set to "default". -- pinctrl-0: Phandle pointing to pin configuration node for PWM. +- pinctrl-names: Set to "default". An additional "sleep" state can be + defined to set pins in sleep state when in low power. +- pinctrl-n: Phandle(s) pointing to pin configuration node for PWM, + respectively for "default" and "sleep" states. Example: timer@40002400 { @@ -21,7 +23,8 @@ Example: pwm { compatible = "st,stm32-pwm-lp"; #pwm-cells = <3>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&lppwm1_pins>; + pinctrl-1 = <&lppwm1_sleep_pins>; }; }; -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state 2019-02-14 10:25 ` [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state Fabrice Gasnier @ 2019-02-18 16:45 ` Rob Herring 0 siblings, 0 replies; 9+ messages in thread From: Rob Herring @ 2019-02-18 16:45 UTC (permalink / raw) To: Fabrice Gasnier Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, robh+dt, thierry.reding, linux-pwm, mcoquelin.stm32, u.kleine-koenig, fabrice.gasnier, linux-stm32, linux-arm-kernel On Thu, 14 Feb 2019 11:25:49 +0100, Fabrice Gasnier wrote: > Add documentation for pinctrl sleep state on STM32 LPTimer PWM. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > --- > Documentation/devicetree/bindings/pwm/pwm-stm32-lp.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 2/3] pwm: stm32-lp: Add power management support 2019-02-14 10:25 [PATCH v4 0/3] Add PM support to STM32 LP Timer drivers Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state Fabrice Gasnier @ 2019-02-14 10:25 ` Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 3/3] pwm: core: add consumer device link Fabrice Gasnier 2 siblings, 0 replies; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-14 10:25 UTC (permalink / raw) To: thierry.reding, robh+dt Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, linux-pwm, mcoquelin.stm32, u.kleine-koenig, fabrice.gasnier, linux-stm32, linux-arm-kernel Add suspend/resume PM sleep ops. When going to low power, enforce the PWM channel isn't active. Let the PWM consumers disable it during their own suspend sequence. Only perform a check here, and handle the pinctrl states. See [1]. [1] https://lkml.org/lkml/2019/2/5/770 Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> --- Changes in v2: - Don't disable PWM channel: let the PWM user disable it. Refuse to suspend in case it's been left enabled. - Drop the ifdefs, use __maybe_unused instead. --- drivers/pwm/pwm-stm32-lp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c index 0059b24c..2211a64 100644 --- a/drivers/pwm/pwm-stm32-lp.c +++ b/drivers/pwm/pwm-stm32-lp.c @@ -13,6 +13,7 @@ #include <linux/mfd/stm32-lptimer.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/pwm.h> @@ -223,6 +224,29 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) return pwmchip_remove(&priv->chip); } +static int __maybe_unused stm32_pwm_lp_suspend(struct device *dev) +{ + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); + struct pwm_state state; + + pwm_get_state(&priv->chip.pwms[0], &state); + if (state.enabled) { + dev_err(dev, "The consumer didn't stop us (%s)\n", + priv->chip.pwms[0].label); + return -EBUSY; + } + + return pinctrl_pm_select_sleep_state(dev); +} + +static int __maybe_unused stm32_pwm_lp_resume(struct device *dev) +{ + return pinctrl_pm_select_default_state(dev); +} + +static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend, + stm32_pwm_lp_resume); + static const struct of_device_id stm32_pwm_lp_of_match[] = { { .compatible = "st,stm32-pwm-lp", }, {}, @@ -235,6 +259,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) .driver = { .name = "stm32-pwm-lp", .of_match_table = of_match_ptr(stm32_pwm_lp_of_match), + .pm = &stm32_pwm_lp_pm_ops, }, }; module_platform_driver(stm32_pwm_lp_driver); -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 3/3] pwm: core: add consumer device link 2019-02-14 10:25 [PATCH v4 0/3] Add PM support to STM32 LP Timer drivers Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 2/3] pwm: stm32-lp: Add power management support Fabrice Gasnier @ 2019-02-14 10:25 ` Fabrice Gasnier 2019-02-18 17:22 ` Uwe Kleine-König 2 siblings, 1 reply; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-14 10:25 UTC (permalink / raw) To: thierry.reding, robh+dt Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, linux-pwm, mcoquelin.stm32, u.kleine-koenig, fabrice.gasnier, linux-stm32, linux-arm-kernel Add a device link between the PWM consumer and the PWM provider. This enforces the PWM user to get suspended before the PWM provider. It allows proper synchronization of suspend/resume sequences: the PWM user is responsible for properly stopping PWM, before the provider gets suspended: see [1]. Add the device link in: - of_pwm_get() - pwm_get() - devm_*pwm_get() variants as it requires a reference to the device for the PWM consumer. [1] https://lkml.org/lkml/2019/2/5/770 Suggested-by: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> --- Changes in v4: - rework error handling following Thierry's comments - turn/split pr_debug() into dev_err()/pr_warn(). Changes in v3: - add struct device to of_get_pwm() arguments to handle device link from there as discussed with Uwe. --- drivers/pwm/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- include/linux/pwm.h | 6 ++++-- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1581f6a..64e10a6 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -636,8 +636,35 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) return ERR_PTR(-EPROBE_DEFER); } +static struct device_link *pwm_device_link_add(struct device *dev, + struct pwm_device *pwm) +{ + struct device_link *dl; + + if (!dev) { + /* + * No device for the PWM consumer has been provided. It may + * impact the PM sequence ordering: the PWM supplier may get + * suspended before the consumer. + */ + pr_warn("no consumer dev, can't create device link to %s\n", + dev_name(pwm->chip->dev)); + return NULL; + } + + dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); + if (!dl) { + dev_err(dev, "failed to create device link to %s\n", + dev_name(pwm->chip->dev)); + return ERR_PTR(-EINVAL); + } + + return dl; +} + /** * of_pwm_get() - request a PWM via the PWM framework + * @dev: device for PWM consumer * @np: device node to get the PWM from * @con_id: consumer name * @@ -655,10 +682,12 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded * error code on failure. */ -struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) +struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np, + const char *con_id) { struct pwm_device *pwm = NULL; struct of_phandle_args args; + struct device_link *dl; struct pwm_chip *pc; int index = 0; int err; @@ -689,6 +718,14 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) if (IS_ERR(pwm)) goto put; + dl = pwm_device_link_add(dev, pwm); + if (IS_ERR(dl)) { + /* of_xlate ended up calling pwm_request_from_chip() */ + pwm_free(pwm); + pwm = ERR_CAST(dl); + goto put; + } + /* * If a consumer name was not given, try to look it up from the * "pwm-names" property if it exists. Otherwise use the name of @@ -764,6 +801,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) const char *dev_id = dev ? dev_name(dev) : NULL; struct pwm_device *pwm; struct pwm_chip *chip; + struct device_link *dl; unsigned int best = 0; struct pwm_lookup *p, *chosen = NULL; unsigned int match; @@ -771,7 +809,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) /* look up via DT first */ if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) - return of_pwm_get(dev->of_node, con_id); + return of_pwm_get(dev, dev->of_node, con_id); /* * We look up the provider in the static table typically provided by @@ -848,6 +886,12 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) if (IS_ERR(pwm)) return pwm; + dl = pwm_device_link_add(dev, pwm); + if (IS_ERR(dl)) { + pwm_free(pwm); + return ERR_CAST(dl); + } + pwm->args.period = chosen->period; pwm->args.polarity = chosen->polarity; @@ -939,7 +983,7 @@ struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np, if (!ptr) return ERR_PTR(-ENOMEM); - pwm = of_pwm_get(np, con_id); + pwm = of_pwm_get(dev, np, con_id); if (!IS_ERR(pwm)) { *ptr = pwm; devres_add(dev, ptr); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index d5199b5..895e074 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -406,7 +406,8 @@ struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args); struct pwm_device *pwm_get(struct device *dev, const char *con_id); -struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id); +struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np, + const char *con_id); void pwm_put(struct pwm_device *pwm); struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id); @@ -494,7 +495,8 @@ static inline struct pwm_device *pwm_get(struct device *dev, return ERR_PTR(-ENODEV); } -static inline struct pwm_device *of_pwm_get(struct device_node *np, +static inline struct pwm_device *of_pwm_get(struct device *dev, + struct device_node *np, const char *con_id) { return ERR_PTR(-ENODEV); -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] pwm: core: add consumer device link 2019-02-14 10:25 ` [PATCH v4 3/3] pwm: core: add consumer device link Fabrice Gasnier @ 2019-02-18 17:22 ` Uwe Kleine-König 2019-02-19 8:46 ` Fabrice Gasnier 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2019-02-18 17:22 UTC (permalink / raw) To: Fabrice Gasnier Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, robh+dt, thierry.reding, linux-pwm, mcoquelin.stm32, linux-stm32, linux-arm-kernel Hello, On Thu, Feb 14, 2019 at 11:25:51AM +0100, Fabrice Gasnier wrote: > Add a device link between the PWM consumer and the PWM provider. This > enforces the PWM user to get suspended before the PWM provider. It > allows proper synchronization of suspend/resume sequences: the PWM user > is responsible for properly stopping PWM, before the provider gets > suspended: see [1]. Add the device link in: > - of_pwm_get() > - pwm_get() > - devm_*pwm_get() variants > as it requires a reference to the device for the PWM consumer. > > [1] https://lkml.org/lkml/2019/2/5/770 > > Suggested-by: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > --- > Changes in v4: > - rework error handling following Thierry's comments > - turn/split pr_debug() into dev_err()/pr_warn(). > > Changes in v3: > - add struct device to of_get_pwm() arguments to handle device link from > there as discussed with Uwe. > --- > drivers/pwm/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- > include/linux/pwm.h | 6 ++++-- > 2 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 1581f6a..64e10a6 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -636,8 +636,35 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) > return ERR_PTR(-EPROBE_DEFER); > } > > +static struct device_link *pwm_device_link_add(struct device *dev, > + struct pwm_device *pwm) > +{ > + struct device_link *dl; > + > + if (!dev) { > + /* > + * No device for the PWM consumer has been provided. It may > + * impact the PM sequence ordering: the PWM supplier may get > + * suspended before the consumer. > + */ > + pr_warn("no consumer dev, can't create device link to %s\n", > + dev_name(pwm->chip->dev)); Maybe use dev_warn(pwm->chip->dev, ...) ? > + return NULL; > + } > + > + dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); > + if (!dl) { > + dev_err(dev, "failed to create device link to %s\n", > + dev_name(pwm->chip->dev)); > + return ERR_PTR(-EINVAL); broken indention. > + } > + > + return dl; > +} > + Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] pwm: core: add consumer device link 2019-02-18 17:22 ` Uwe Kleine-König @ 2019-02-19 8:46 ` Fabrice Gasnier 2019-02-19 8:55 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-19 8:46 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, robh+dt, thierry.reding, linux-pwm, mcoquelin.stm32, linux-stm32, linux-arm-kernel On 2/18/19 6:22 PM, Uwe Kleine-König wrote: > Hello, > > On Thu, Feb 14, 2019 at 11:25:51AM +0100, Fabrice Gasnier wrote: >> Add a device link between the PWM consumer and the PWM provider. This >> enforces the PWM user to get suspended before the PWM provider. It >> allows proper synchronization of suspend/resume sequences: the PWM user >> is responsible for properly stopping PWM, before the provider gets >> suspended: see [1]. Add the device link in: >> - of_pwm_get() >> - pwm_get() >> - devm_*pwm_get() variants >> as it requires a reference to the device for the PWM consumer. >> >> [1] https://lkml.org/lkml/2019/2/5/770 >> >> Suggested-by: Thierry Reding <thierry.reding@gmail.com> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >> --- >> Changes in v4: >> - rework error handling following Thierry's comments >> - turn/split pr_debug() into dev_err()/pr_warn(). >> >> Changes in v3: >> - add struct device to of_get_pwm() arguments to handle device link from >> there as discussed with Uwe. >> --- >> drivers/pwm/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- >> include/linux/pwm.h | 6 ++++-- >> 2 files changed, 51 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >> index 1581f6a..64e10a6 100644 >> --- a/drivers/pwm/core.c >> +++ b/drivers/pwm/core.c >> @@ -636,8 +636,35 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) >> return ERR_PTR(-EPROBE_DEFER); >> } >> >> +static struct device_link *pwm_device_link_add(struct device *dev, >> + struct pwm_device *pwm) >> +{ >> + struct device_link *dl; >> + >> + if (!dev) { >> + /* >> + * No device for the PWM consumer has been provided. It may >> + * impact the PM sequence ordering: the PWM supplier may get >> + * suspended before the consumer. >> + */ >> + pr_warn("no consumer dev, can't create device link to %s\n", >> + dev_name(pwm->chip->dev)); > > Maybe use dev_warn(pwm->chip->dev, ...) ? Hi Uwe, I'm wondering a bit about this: In this case, the caller that doesn't provide a struct device *, PWM provider isn't responsible for that. So I just hope this wouldn't be miss-leading ? > >> + return NULL; >> + } >> + >> + dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER); >> + if (!dl) { >> + dev_err(dev, "failed to create device link to %s\n", >> + dev_name(pwm->chip->dev)); >> + return ERR_PTR(-EINVAL); > > broken indention. Oops, I'll fix it. Thanks, Fabrice > >> + } >> + >> + return dl; >> +} >> + > > Best regards > Uwe > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] pwm: core: add consumer device link 2019-02-19 8:46 ` Fabrice Gasnier @ 2019-02-19 8:55 ` Uwe Kleine-König 2019-02-19 9:54 ` Fabrice Gasnier 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2019-02-19 8:55 UTC (permalink / raw) To: Fabrice Gasnier Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, robh+dt, thierry.reding, linux-pwm, mcoquelin.stm32, linux-stm32, linux-arm-kernel On Tue, Feb 19, 2019 at 09:46:32AM +0100, Fabrice Gasnier wrote: > On 2/18/19 6:22 PM, Uwe Kleine-König wrote: > > Hello, > > > > On Thu, Feb 14, 2019 at 11:25:51AM +0100, Fabrice Gasnier wrote: > >> Add a device link between the PWM consumer and the PWM provider. This > >> enforces the PWM user to get suspended before the PWM provider. It > >> allows proper synchronization of suspend/resume sequences: the PWM user > >> is responsible for properly stopping PWM, before the provider gets > >> suspended: see [1]. Add the device link in: > >> - of_pwm_get() > >> - pwm_get() > >> - devm_*pwm_get() variants > >> as it requires a reference to the device for the PWM consumer. > >> > >> [1] https://lkml.org/lkml/2019/2/5/770 > >> > >> Suggested-by: Thierry Reding <thierry.reding@gmail.com> > >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > >> --- > >> Changes in v4: > >> - rework error handling following Thierry's comments > >> - turn/split pr_debug() into dev_err()/pr_warn(). > >> > >> Changes in v3: > >> - add struct device to of_get_pwm() arguments to handle device link from > >> there as discussed with Uwe. > >> --- > >> drivers/pwm/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- > >> include/linux/pwm.h | 6 ++++-- > >> 2 files changed, 51 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > >> index 1581f6a..64e10a6 100644 > >> --- a/drivers/pwm/core.c > >> +++ b/drivers/pwm/core.c > >> @@ -636,8 +636,35 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) > >> return ERR_PTR(-EPROBE_DEFER); > >> } > >> > >> +static struct device_link *pwm_device_link_add(struct device *dev, > >> + struct pwm_device *pwm) > >> +{ > >> + struct device_link *dl; > >> + > >> + if (!dev) { > >> + /* > >> + * No device for the PWM consumer has been provided. It may > >> + * impact the PM sequence ordering: the PWM supplier may get > >> + * suspended before the consumer. > >> + */ > >> + pr_warn("no consumer dev, can't create device link to %s\n", > >> + dev_name(pwm->chip->dev)); > > > > Maybe use dev_warn(pwm->chip->dev, ...) ? > > Hi Uwe, > > I'm wondering a bit about this: In this case, the caller that doesn't > provide a struct device *, PWM provider isn't responsible for that. So I > just hope this wouldn't be miss-leading ? IMHO it's more the wording that might make the message misleading. If you use dev_warn(pwm->chip->dev, "No consumer device specified to create a device link to\n"); that's completely fine in my eyes. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] pwm: core: add consumer device link 2019-02-19 8:55 ` Uwe Kleine-König @ 2019-02-19 9:54 ` Fabrice Gasnier 0 siblings, 0 replies; 9+ messages in thread From: Fabrice Gasnier @ 2019-02-19 9:54 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, devicetree, alexandre.torgue, tduszyns, linux-kernel, robh+dt, thierry.reding, linux-pwm, mcoquelin.stm32, linux-stm32, linux-arm-kernel On 2/19/19 9:55 AM, Uwe Kleine-König wrote: > On Tue, Feb 19, 2019 at 09:46:32AM +0100, Fabrice Gasnier wrote: >> On 2/18/19 6:22 PM, Uwe Kleine-König wrote: >>> Hello, >>> >>> On Thu, Feb 14, 2019 at 11:25:51AM +0100, Fabrice Gasnier wrote: >>>> Add a device link between the PWM consumer and the PWM provider. This >>>> enforces the PWM user to get suspended before the PWM provider. It >>>> allows proper synchronization of suspend/resume sequences: the PWM user >>>> is responsible for properly stopping PWM, before the provider gets >>>> suspended: see [1]. Add the device link in: >>>> - of_pwm_get() >>>> - pwm_get() >>>> - devm_*pwm_get() variants >>>> as it requires a reference to the device for the PWM consumer. >>>> >>>> [1] https://lkml.org/lkml/2019/2/5/770 >>>> >>>> Suggested-by: Thierry Reding <thierry.reding@gmail.com> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >>>> --- >>>> Changes in v4: >>>> - rework error handling following Thierry's comments >>>> - turn/split pr_debug() into dev_err()/pr_warn(). >>>> >>>> Changes in v3: >>>> - add struct device to of_get_pwm() arguments to handle device link from >>>> there as discussed with Uwe. >>>> --- >>>> drivers/pwm/core.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- >>>> include/linux/pwm.h | 6 ++++-- >>>> 2 files changed, 51 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >>>> index 1581f6a..64e10a6 100644 >>>> --- a/drivers/pwm/core.c >>>> +++ b/drivers/pwm/core.c >>>> @@ -636,8 +636,35 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np) >>>> return ERR_PTR(-EPROBE_DEFER); >>>> } >>>> >>>> +static struct device_link *pwm_device_link_add(struct device *dev, >>>> + struct pwm_device *pwm) >>>> +{ >>>> + struct device_link *dl; >>>> + >>>> + if (!dev) { >>>> + /* >>>> + * No device for the PWM consumer has been provided. It may >>>> + * impact the PM sequence ordering: the PWM supplier may get >>>> + * suspended before the consumer. >>>> + */ >>>> + pr_warn("no consumer dev, can't create device link to %s\n", >>>> + dev_name(pwm->chip->dev)); >>> >>> Maybe use dev_warn(pwm->chip->dev, ...) ? >> >> Hi Uwe, >> >> I'm wondering a bit about this: In this case, the caller that doesn't >> provide a struct device *, PWM provider isn't responsible for that. So I >> just hope this wouldn't be miss-leading ? > > IMHO it's more the wording that might make the message misleading. If > you use > > dev_warn(pwm->chip->dev, "No consumer device specified to create a device link to\n"); > > that's completely fine in my eyes. Thanks for the suggestion, I'll update this as well in v5. Best regards, Fabrice > > Best regards > Uwe > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-19 9:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-14 10:25 [PATCH v4 0/3] Add PM support to STM32 LP Timer drivers Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state Fabrice Gasnier 2019-02-18 16:45 ` Rob Herring 2019-02-14 10:25 ` [PATCH v4 2/3] pwm: stm32-lp: Add power management support Fabrice Gasnier 2019-02-14 10:25 ` [PATCH v4 3/3] pwm: core: add consumer device link Fabrice Gasnier 2019-02-18 17:22 ` Uwe Kleine-König 2019-02-19 8:46 ` Fabrice Gasnier 2019-02-19 8:55 ` Uwe Kleine-König 2019-02-19 9:54 ` Fabrice Gasnier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).