[PATCH v3 0/3] Add PM support to STM32 LP Timer drivers

[PATCH v3 0/3] Add PM support to STM32 LP Timer drivers

[Fabrice Gasnier]

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 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                                 | 14 +++++++++---
 drivers/pwm/pwm-stm32-lp.c                         | 25 ++++++++++++++++++++++
 include/linux/pwm.h                                |  6 ++++--
 4 files changed, 46 insertions(+), 8 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] dt-bindings: pwm-stm32-lp: document pinctrl sleep state

[Fabrice Gasnier]

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

[PATCH v3 2/3] pwm: stm32-lp: Add power management support

[Fabrice Gasnier]

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

[PATCH v3 3/3] pwm: core: add consumer device link

[Fabrice Gasnier]

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_ 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 v3:
- add struct device to of_get_pwm() arguments to handle device link from
  there.
---
 drivers/pwm/core.c  | 14 +++++++++++---
 include/linux/pwm.h |  6 ++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6a..8cb5d4bc 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 
 /**
  * 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,7 +656,8 @@ 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;
@@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 	if (IS_ERR(pwm))
 		goto put;
 
+	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
+		pr_debug("%s(): device link not added\n", __func__);
+
 	/*
 	 * 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
@@ -771,7 +776,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
@@ -851,6 +856,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 	pwm->args.period = chosen->period;
 	pwm->args.polarity = chosen->polarity;
 
+	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
+		pr_debug("%s(): device link not added\n", __func__);
+
 	return pwm;
 }
 EXPORT_SYMBOL_GPL(pwm_get);
@@ -939,7 +947,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

Re: [PATCH v3 3/3] pwm: core: add consumer device link

[Thierry Reding]

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

On Wed, Feb 13, 2019 at 11:50:12AM +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_ 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 v3:
> - add struct device to of_get_pwm() arguments to handle device link from
>   there.
> ---
>  drivers/pwm/core.c  | 14 +++++++++++---
>  include/linux/pwm.h |  6 ++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 1581f6a..8cb5d4bc 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>  
>  /**
>   * 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,7 +656,8 @@ 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;
> @@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
>  	if (IS_ERR(pwm))
>  		goto put;
>  
> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> +		pr_debug("%s(): device link not added\n", __func__);

I think it's better to turn this into dev_dbg(dev, ...) and maybe
mention which supplier it failed to link to, something like:

	if (!device_link_add(...))
		dev_dbg(dev, "failed to create device link to %s\n",
			pwm->chip->dev);

Also, I wonder if this should perhaps be dev_err(). Under what
circumstances does this fail?

> +
>  	/*
>  	 * 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
> @@ -771,7 +776,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
> @@ -851,6 +856,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  	pwm->args.period = chosen->period;
>  	pwm->args.polarity = chosen->polarity;
>  
> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> +		pr_debug("%s(): device link not added\n", __func__);

Same here. Also: not sure if we really need to include __func__ in the
message.

> +
>  	return pwm;
>  }
>  EXPORT_SYMBOL_GPL(pwm_get);
> @@ -939,7 +947,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);

I'm slightly concerned about this. I think one of the reasons why this
was introduced was to allow code to get at the PWM if they didn't have
a struct device * available. However, it doesn't seem like there are any
users of that function, so this seems fine.

Thierry

>  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
> 

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

Re: [PATCH v3 3/3] pwm: core: add consumer device link

[Fabrice Gasnier]

On 2/13/19 1:53 PM, Thierry Reding wrote:
> On Wed, Feb 13, 2019 at 11:50:12AM +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_ 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 v3:
>> - add struct device to of_get_pwm() arguments to handle device link from
>>   there.
>> ---
>>  drivers/pwm/core.c  | 14 +++++++++++---
>>  include/linux/pwm.h |  6 ++++--
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 1581f6a..8cb5d4bc 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>>  
>>  /**
>>   * 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,7 +656,8 @@ 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;
>> @@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
>>  	if (IS_ERR(pwm))
>>  		goto put;
>>  
>> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
>> +		pr_debug("%s(): device link not added\n", __func__);
> 
> I think it's better to turn this into dev_dbg(dev, ...) and maybe
> mention which supplier it failed to link to, something like:
> 
> 	if (!device_link_add(...))
> 		dev_dbg(dev, "failed to create device link to %s\n",
> 			pwm->chip->dev);

Hi Thierry,

Thanks for reviewing.

I can update this: I used pr_debug() as there are pr_err() calls
elsewhere in this routine.
BTW, do you wish an additional patch to turn pr_err() into dev_err() in
of_pwm_get()?

> 
> Also, I wonder if this should perhaps be dev_err(). Under what
> circumstances does this fail?

Well, here is a comment from "device_link_add()" routine:
"
/*
 * If the supplier has not been fully registered yet or there is a
 * reverse dependency between the consumer and the supplier already in
 * the graph, return NULL.
 */
"

=> Here the PWM supplier is already registered. (It seems a probe defer
can be returned few lines above otherwise.)

Other possibilities: kzalloc() failed, no consumer or supplier has been
provided (or invalid flags, but this is hardcoded here.).

So, I see two case here:
1 - The caller provided a 'dev' for PWM consumer... So, NULL link is an
error when consumer & supplier has been passed correctly.
=> I can add a check on 'dev' for PWM consumer and report an error here:
   return -EINVAL

2 - The caller can't provide a 'dev' for PWM consumer as you mention
bellow: "to allow code to get at the PWM if they didn't have..."
=> We should probably add a dev_warn() here, with no error ?
Please see here after.

> 
>> +
>>  	/*
>>  	 * 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
>> @@ -771,7 +776,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
>> @@ -851,6 +856,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>>  	pwm->args.period = chosen->period;
>>  	pwm->args.polarity = chosen->polarity;
>>  
>> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
>> +		pr_debug("%s(): device link not added\n", __func__);
> 
> Same here. Also: not sure if we really need to include __func__ in the
> message.
> 
>> +
>>  	return pwm;
>>  }
>>  EXPORT_SYMBOL_GPL(pwm_get);
>> @@ -939,7 +947,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);
> 
> I'm slightly concerned about this. I think one of the reasons why this
> was introduced was to allow code to get at the PWM if they didn't have
> a struct device * available. However, it doesn't seem like there are any
> users of that function, so this seems fine.

The git blame pointed out commit 8eb961279960:
"
pwm: core: Rename of_pwm_request() to of_pwm_get() and export it

Allow client driver to use of_pwm_get() to get the PWM they need. This
is needed for drivers which handle more than one PWM separately, like
leds-pwm driver, which have:
"
...

For instance, I tested the leds-pwm driver. It uses the devm_* variant
now (as others), there is a struct device * available. So yes, it seems
fine.

The only thing maybe out of tree code? This is where I have a doubt on
having a mandatory struct device * to enforce consumer link creation...
or make it optional (e.g. behave as a 'legacy' API) and warn the caller.

Please let me know your feeling.
Best regards,
Fabrice

> 
> Thierry
> 
>>  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
>>

Re: [PATCH v3 3/3] pwm: core: add consumer device link

[Fabrice Gasnier]

On 2/13/19 4:17 PM, Fabrice Gasnier wrote:
> On 2/13/19 1:53 PM, Thierry Reding wrote:
>> On Wed, Feb 13, 2019 at 11:50:12AM +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_ 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 v3:
>>> - add struct device to of_get_pwm() arguments to handle device link from
>>>   there.
>>> ---
>>>  drivers/pwm/core.c  | 14 +++++++++++---
>>>  include/linux/pwm.h |  6 ++++--
>>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> index 1581f6a..8cb5d4bc 100644
>>> --- a/drivers/pwm/core.c
>>> +++ b/drivers/pwm/core.c
>>> @@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>>>  
>>>  /**
>>>   * 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,7 +656,8 @@ 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;
>>> @@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
>>>  	if (IS_ERR(pwm))
>>>  		goto put;
>>>  
>>> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
>>> +		pr_debug("%s(): device link not added\n", __func__);
>>
>> I think it's better to turn this into dev_dbg(dev, ...) and maybe
>> mention which supplier it failed to link to, something like:
>>
>> 	if (!device_link_add(...))
>> 		dev_dbg(dev, "failed to create device link to %s\n",
>> 			pwm->chip->dev);
> 
> Hi Thierry,
> 
> Thanks for reviewing.
> 
> I can update this: I used pr_debug() as there are pr_err() calls
> elsewhere in this routine.
> BTW, do you wish an additional patch to turn pr_err() into dev_err() in
> of_pwm_get()?
> 
>>
>> Also, I wonder if this should perhaps be dev_err(). Under what
>> circumstances does this fail?
> 
> Well, here is a comment from "device_link_add()" routine:
> "
> /*
>  * If the supplier has not been fully registered yet or there is a
>  * reverse dependency between the consumer and the supplier already in
>  * the graph, return NULL.
>  */
> "
> 
> => Here the PWM supplier is already registered. (It seems a probe defer
> can be returned few lines above otherwise.)
> 
> Other possibilities: kzalloc() failed, no consumer or supplier has been
> provided (or invalid flags, but this is hardcoded here.).
> 
> So, I see two case here:
> 1 - The caller provided a 'dev' for PWM consumer... So, NULL link is an
> error when consumer & supplier has been passed correctly.
> => I can add a check on 'dev' for PWM consumer and report an error here:
>    return -EINVAL
> 
> 2 - The caller can't provide a 'dev' for PWM consumer as you mention
> bellow: "to allow code to get at the PWM if they didn't have..."
> => We should probably add a dev_warn() here, with no error ?
> Please see here after.
> 
>>
>>> +
>>>  	/*
>>>  	 * 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
>>> @@ -771,7 +776,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
>>> @@ -851,6 +856,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>>>  	pwm->args.period = chosen->period;
>>>  	pwm->args.polarity = chosen->polarity;
>>>  
>>> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
>>> +		pr_debug("%s(): device link not added\n", __func__);
>>
>> Same here. Also: not sure if we really need to include __func__ in the
>> message.
>>
>>> +
>>>  	return pwm;
>>>  }
>>>  EXPORT_SYMBOL_GPL(pwm_get);
>>> @@ -939,7 +947,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);
>>
>> I'm slightly concerned about this. I think one of the reasons why this
>> was introduced was to allow code to get at the PWM if they didn't have
>> a struct device * available. However, it doesn't seem like there are any
>> users of that function, so this seems fine.
> 
> The git blame pointed out commit 8eb961279960:
> "
> pwm: core: Rename of_pwm_request() to of_pwm_get() and export it
> 
> Allow client driver to use of_pwm_get() to get the PWM they need. This
> is needed for drivers which handle more than one PWM separately, like
> leds-pwm driver, which have:
> "
> ...
> 
> For instance, I tested the leds-pwm driver. It uses the devm_* variant
> now (as others), there is a struct device * available. So yes, it seems
> fine.
> 
> The only thing maybe out of tree code? This is where I have a doubt on
> having a mandatory struct device * to enforce consumer link creation...
> or make it optional (e.g. behave as a 'legacy' API) and warn the caller.
> 
> Please let me know your feeling.

Hi Thierry,

I just sent a v4 to update the error handling following the cases
described above.

BR,
Fabrice
> Best regards,
> Fabrice
> 
>>
>> Thierry
>>
>>>  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
>>>

Re: [PATCH v3 3/3] pwm: core: add consumer device link

[Claudiu.Beznea]

On 13.02.2019 12:50, 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_ 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 v3:
> - add struct device to of_get_pwm() arguments to handle device link from
>   there.
> ---
>  drivers/pwm/core.c  | 14 +++++++++++---
>  include/linux/pwm.h |  6 ++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 1581f6a..8cb5d4bc 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>  
>  /**
>   * 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,7 +656,8 @@ 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;
> @@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
>  	if (IS_ERR(pwm))
>  		goto put;
>  
> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> +		pr_debug("%s(): device link not added\n", __func__);
> +

Just asking... will this mechanism also be working for PWMs requested via
sysfs? At a first look it seems not.

Thank you,
Claudiu Beznea

>  	/*
>  	 * 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
> @@ -771,7 +776,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
> @@ -851,6 +856,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  	pwm->args.period = chosen->period;
>  	pwm->args.polarity = chosen->polarity;
>  
> +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> +		pr_debug("%s(): device link not added\n", __func__);
> +
>  	return pwm;
>  }
>  EXPORT_SYMBOL_GPL(pwm_get);
> @@ -939,7 +947,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);
> 

Re: [PATCH v3 3/3] pwm: core: add consumer device link

[Uwe Kleine-König]

On Fri, Feb 15, 2019 at 09:23:48AM +0000, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 13.02.2019 12:50, 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_ 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 v3:
> > - add struct device to of_get_pwm() arguments to handle device link from
> >   there.
> > ---
> >  drivers/pwm/core.c  | 14 +++++++++++---
> >  include/linux/pwm.h |  6 ++++--
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 1581f6a..8cb5d4bc 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -638,6 +638,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
> >  
> >  /**
> >   * 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,7 +656,8 @@ 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;
> > @@ -689,6 +691,9 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
> >  	if (IS_ERR(pwm))
> >  		goto put;
> >  
> > +	if (!device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER))
> > +		pr_debug("%s(): device link not added\n", __func__);
> > +
> 
> Just asking... will this mechanism also be working for PWMs requested via
> sysfs? At a first look it seems not.

No, but I would not expect a problem because AFAIU userspace is
suspended before kernel devices. However as userspace might not notice
the suspend it probably fails to stop the pwm and so will likely prevent
the suspend.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |