linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Add PM support to STM32 Timer PWM
@ 2019-10-04 12:53 Fabrice Gasnier
  2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Fabrice Gasnier @ 2019-10-04 12:53 UTC (permalink / raw)
  To: thierry.reding, robh+dt, u.kleine-koenig
  Cc: alexandre.torgue, mark.rutland, mcoquelin.stm32, fabrice.gasnier,
	devicetree, linux-arm-kernel, linux-kernel, linux-pwm,
	benjamin.gaignard, linux-stm32

This patch series adds power management support for STM32 Timer PWM:
- Document the pinctrl sleep state for STM32 Timer PWM
- STM32 Timer PWM driver

---
Changes in v2:
Follow Uwe suggestions/remarks:
- Add a precursor patch to ease reviewing
- Use registers read instead of pwm_get_state
- Add a comment to mention registers content may be lost in low power mode

Fabrice Gasnier (3):
  dt-bindings: pwm-stm32: document pinctrl sleep state
  pwm: stm32: split breakinput apply routine to ease PM support
  pwm: stm32: add power management support

 .../devicetree/bindings/pwm/pwm-stm32.txt          |  8 +-
 drivers/pwm/pwm-stm32.c                            | 86 +++++++++++++++++-----
 2 files changed, 71 insertions(+), 23 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state
  2019-10-04 12:53 [PATCH v2 0/3] Add PM support to STM32 Timer PWM Fabrice Gasnier
@ 2019-10-04 12:53 ` Fabrice Gasnier
  2019-10-11 15:10   ` Rob Herring
  2019-10-16  6:59   ` Thierry Reding
  2019-10-04 12:53 ` [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support Fabrice Gasnier
  2019-10-04 12:53 ` [PATCH v2 3/3] pwm: stm32: add power management support Fabrice Gasnier
  2 siblings, 2 replies; 9+ messages in thread
From: Fabrice Gasnier @ 2019-10-04 12:53 UTC (permalink / raw)
  To: thierry.reding, robh+dt, u.kleine-koenig
  Cc: alexandre.torgue, mark.rutland, mcoquelin.stm32, fabrice.gasnier,
	devicetree, linux-arm-kernel, linux-kernel, linux-pwm,
	benjamin.gaignard, linux-stm32

Add documentation for pinctrl sleep state that can be used by
STM32 timers PWM.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/devicetree/bindings/pwm/pwm-stm32.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
index a8690bf..f1620c1 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
@@ -5,8 +5,9 @@ See ../mfd/stm32-timers.txt for details about the parent node.
 
 Required parameters:
 - compatible:		Must be "st,stm32-pwm".
-- pinctrl-names: 	Set to "default".
-- pinctrl-0: 		List of phandles pointing to pin configuration nodes for PWM module.
+- pinctrl-names: 	Set to "default". An additional "sleep" state can be
+			defined to set pins in sleep state when in low power.
+- pinctrl-n: 		List of phandles pointing to pin configuration nodes for PWM module.
 			For Pinctrl properties see ../pinctrl/pinctrl-bindings.txt
 - #pwm-cells:		Should be set to 3. This PWM chip uses the default 3 cells
 			bindings defined in pwm.txt.
@@ -32,7 +33,8 @@ Example:
 			compatible = "st,stm32-pwm";
 			#pwm-cells = <3>;
 			pinctrl-0	= <&pwm1_pins>;
-			pinctrl-names	= "default";
+			pinctrl-1	= <&pwm1_sleep_pins>;
+			pinctrl-names	= "default", "sleep";
 			st,breakinput = <0 1 5>;
 		};
 	};
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support
  2019-10-04 12:53 [PATCH v2 0/3] Add PM support to STM32 Timer PWM Fabrice Gasnier
  2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
@ 2019-10-04 12:53 ` Fabrice Gasnier
  2019-10-16  7:03   ` Thierry Reding
  2019-10-04 12:53 ` [PATCH v2 3/3] pwm: stm32: add power management support Fabrice Gasnier
  2 siblings, 1 reply; 9+ messages in thread
From: Fabrice Gasnier @ 2019-10-04 12:53 UTC (permalink / raw)
  To: thierry.reding, robh+dt, u.kleine-koenig
  Cc: alexandre.torgue, mark.rutland, mcoquelin.stm32, fabrice.gasnier,
	devicetree, linux-arm-kernel, linux-kernel, linux-pwm,
	benjamin.gaignard, linux-stm32

Split breakinput routine that configures STM32 timers 'break' safety
feature upon probe, into two routines:
- stm32_pwm_apply_breakinputs() sets all the break inputs into registers.
- stm32_pwm_probe_breakinputs() probes the device tree break input settings
  before calling stm32_pwm_apply_breakinputs()

This is a precursor patch to ease PM support. Registers content may get
lost during low power. So, break input settings applied upon probe need
to be restored upon resume (e.g. by calling stm32_pwm_apply_breakinputs()).

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/pwm/pwm-stm32.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 359b085..cf8658c 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -19,6 +19,12 @@
 #define CCMR_CHANNEL_MASK  0xFF
 #define MAX_BREAKINPUT 2
 
+struct stm32_breakinput {
+	u32 index;
+	u32 level;
+	u32 filter;
+};
+
 struct stm32_pwm {
 	struct pwm_chip chip;
 	struct mutex lock; /* protect pwm config/enable */
@@ -26,15 +32,11 @@ struct stm32_pwm {
 	struct regmap *regmap;
 	u32 max_arr;
 	bool have_complementary_output;
+	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
+	unsigned int nbreakinput;
 	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
 };
 
-struct stm32_breakinput {
-	u32 index;
-	u32 level;
-	u32 filter;
-};
-
 static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
 {
 	return container_of(chip, struct stm32_pwm, chip);
@@ -512,15 +514,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
 	return (bdtr & bke) ? 0 : -EINVAL;
 }
 
-static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
+static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < priv->nbreakinput && !ret; i++) {
+		ret = stm32_pwm_set_breakinput(priv,
+					       priv->breakinput[i].index,
+					       priv->breakinput[i].level,
+					       priv->breakinput[i].filter);
+	}
+
+	return ret;
+}
+
+static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
 				       struct device_node *np)
 {
-	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
-	int nb, ret, i, array_size;
+	int nb, ret, array_size;
 
 	nb = of_property_count_elems_of_size(np, "st,breakinput",
 					     sizeof(struct stm32_breakinput));
-
 	/*
 	 * Because "st,breakinput" parameter is optional do not make probe
 	 * failed if it doesn't exist.
@@ -531,20 +545,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
 	if (nb > MAX_BREAKINPUT)
 		return -EINVAL;
 
+	priv->nbreakinput = nb;
 	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
 	ret = of_property_read_u32_array(np, "st,breakinput",
-					 (u32 *)breakinput, array_size);
+					 (u32 *)priv->breakinput, array_size);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < nb && !ret; i++) {
-		ret = stm32_pwm_set_breakinput(priv,
-					       breakinput[i].index,
-					       breakinput[i].level,
-					       breakinput[i].filter);
-	}
-
-	return ret;
+	return stm32_pwm_apply_breakinputs(priv);
 }
 
 static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
@@ -614,7 +622,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 	if (!priv->regmap || !priv->clk)
 		return -EINVAL;
 
-	ret = stm32_pwm_apply_breakinputs(priv, np);
+	ret = stm32_pwm_probe_breakinputs(priv, np);
 	if (ret)
 		return ret;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] pwm: stm32: add power management support
  2019-10-04 12:53 [PATCH v2 0/3] Add PM support to STM32 Timer PWM Fabrice Gasnier
  2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
  2019-10-04 12:53 ` [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support Fabrice Gasnier
@ 2019-10-04 12:53 ` Fabrice Gasnier
  2019-10-16  7:06   ` Thierry Reding
  2 siblings, 1 reply; 9+ messages in thread
From: Fabrice Gasnier @ 2019-10-04 12:53 UTC (permalink / raw)
  To: thierry.reding, robh+dt, u.kleine-koenig
  Cc: alexandre.torgue, mark.rutland, mcoquelin.stm32, fabrice.gasnier,
	devicetree, linux-arm-kernel, linux-kernel, linux-pwm,
	benjamin.gaignard, linux-stm32

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, see [1]. So, perform a check here, and handle the
pinctrl states. Also restore the break inputs upon resume, as registers
content may be lost when going to low power mode.

[1] https://lkml.org/lkml/2019/2/5/770

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
Follow Uwe suggestions/remarks:
- Add a precursor patch to ease reviewing
- Use registers read instead of pwm_get_state
- Add a comment to mention registers content may be lost in low power mode
---
 drivers/pwm/pwm-stm32.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index cf8658c..546b661 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -12,6 +12,7 @@
 #include <linux/mfd/stm32-timers.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 
@@ -655,6 +656,42 @@ static int stm32_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused stm32_pwm_suspend(struct device *dev)
+{
+	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	unsigned int ch;
+	u32 ccer, mask;
+
+	/* Look for active channels */
+	ccer = active_channels(priv);
+
+	for (ch = 0; ch < priv->chip.npwm; ch++) {
+		mask = TIM_CCER_CC1E << (ch * 4);
+		if (ccer & mask) {
+			dev_err(dev, "The consumer didn't stop us (%s)\n",
+				priv->chip.pwms[ch].label);
+			return -EBUSY;
+		}
+	}
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int __maybe_unused stm32_pwm_resume(struct device *dev)
+{
+	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
+	/* restore breakinput registers that may have been lost in low power */
+	return stm32_pwm_apply_breakinputs(priv);
+}
+
+static SIMPLE_DEV_PM_OPS(stm32_pwm_pm_ops, stm32_pwm_suspend, stm32_pwm_resume);
+
 static const struct of_device_id stm32_pwm_of_match[] = {
 	{ .compatible = "st,stm32-pwm",	},
 	{ /* end node */ },
@@ -667,6 +704,7 @@ static struct platform_driver stm32_pwm_driver = {
 	.driver	= {
 		.name = "stm32-pwm",
 		.of_match_table = stm32_pwm_of_match,
+		.pm = &stm32_pwm_pm_ops,
 	},
 };
 module_platform_driver(stm32_pwm_driver);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state
  2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
@ 2019-10-11 15:10   ` Rob Herring
  2019-10-16  6:59   ` Thierry Reding
  1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring @ 2019-10-11 15:10 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: thierry.reding, robh+dt, u.kleine-koenig, alexandre.torgue,
	mark.rutland, mcoquelin.stm32, fabrice.gasnier, devicetree,
	linux-arm-kernel, linux-kernel, linux-pwm, benjamin.gaignard,
	linux-stm32

On Fri, 4 Oct 2019 14:53:51 +0200, Fabrice Gasnier wrote:
> Add documentation for pinctrl sleep state that can be used by
> STM32 timers PWM.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-stm32.txt | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state
  2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
  2019-10-11 15:10   ` Rob Herring
@ 2019-10-16  6:59   ` Thierry Reding
  1 sibling, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2019-10-16  6:59 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: robh+dt, u.kleine-koenig, alexandre.torgue, mark.rutland,
	mcoquelin.stm32, devicetree, linux-arm-kernel, linux-kernel,
	linux-pwm, benjamin.gaignard, linux-stm32

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

On Fri, Oct 04, 2019 at 02:53:51PM +0200, Fabrice Gasnier wrote:
> Add documentation for pinctrl sleep state that can be used by
> STM32 timers PWM.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-stm32.txt | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Applied, thanks.

Thierry

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support
  2019-10-04 12:53 ` [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support Fabrice Gasnier
@ 2019-10-16  7:03   ` Thierry Reding
  0 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2019-10-16  7:03 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: robh+dt, u.kleine-koenig, alexandre.torgue, mark.rutland,
	mcoquelin.stm32, devicetree, linux-arm-kernel, linux-kernel,
	linux-pwm, benjamin.gaignard, linux-stm32

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

On Fri, Oct 04, 2019 at 02:53:52PM +0200, Fabrice Gasnier wrote:
> Split breakinput routine that configures STM32 timers 'break' safety
> feature upon probe, into two routines:
> - stm32_pwm_apply_breakinputs() sets all the break inputs into registers.
> - stm32_pwm_probe_breakinputs() probes the device tree break input settings
>   before calling stm32_pwm_apply_breakinputs()
> 
> This is a precursor patch to ease PM support. Registers content may get
> lost during low power. So, break input settings applied upon probe need
> to be restored upon resume (e.g. by calling stm32_pwm_apply_breakinputs()).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
>  drivers/pwm/pwm-stm32.c | 48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Applied, thanks. I've made some minor changes, mostly for consistency
with other drivers and the PWM core. See below.

> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 359b085..cf8658c 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -19,6 +19,12 @@
>  #define CCMR_CHANNEL_MASK  0xFF
>  #define MAX_BREAKINPUT 2
>  
> +struct stm32_breakinput {
> +	u32 index;
> +	u32 level;
> +	u32 filter;
> +};
> +
>  struct stm32_pwm {
>  	struct pwm_chip chip;
>  	struct mutex lock; /* protect pwm config/enable */
> @@ -26,15 +32,11 @@ struct stm32_pwm {
>  	struct regmap *regmap;
>  	u32 max_arr;
>  	bool have_complementary_output;
> +	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> +	unsigned int nbreakinput;

I changed these to breakinputs and num_breakinputs since they are
slightly more consistent with the naming elsewhere in PWM.

>  	u32 capture[4] ____cacheline_aligned; /* DMA'able buffer */
>  };
>  
> -struct stm32_breakinput {
> -	u32 index;
> -	u32 level;
> -	u32 filter;
> -};
> -
>  static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
>  {
>  	return container_of(chip, struct stm32_pwm, chip);
> @@ -512,15 +514,27 @@ static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
>  	return (bdtr & bke) ? 0 : -EINVAL;
>  }
>  
> -static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
> +static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv)
> +{
> +	int i, ret = 0;

Made i unsigned int.

> +
> +	for (i = 0; i < priv->nbreakinput && !ret; i++) {
> +		ret = stm32_pwm_set_breakinput(priv,
> +					       priv->breakinput[i].index,
> +					       priv->breakinput[i].level,
> +					       priv->breakinput[i].filter);
> +	}

I thought this was a little odd, so I changed it to explicitly check the
value of ret and return on error.

> +
> +	return ret;

And then this became "return 0;"

> +}
> +
> +static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>  				       struct device_node *np)
>  {
> -	struct stm32_breakinput breakinput[MAX_BREAKINPUT];
> -	int nb, ret, i, array_size;
> +	int nb, ret, array_size;
>  
>  	nb = of_property_count_elems_of_size(np, "st,breakinput",
>  					     sizeof(struct stm32_breakinput));
> -

Dropped this since it made the code look cluttered.

Thierry

>  	/*
>  	 * Because "st,breakinput" parameter is optional do not make probe
>  	 * failed if it doesn't exist.
> @@ -531,20 +545,14 @@ static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,
>  	if (nb > MAX_BREAKINPUT)
>  		return -EINVAL;
>  
> +	priv->nbreakinput = nb;
>  	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
>  	ret = of_property_read_u32_array(np, "st,breakinput",
> -					 (u32 *)breakinput, array_size);
> +					 (u32 *)priv->breakinput, array_size);
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < nb && !ret; i++) {
> -		ret = stm32_pwm_set_breakinput(priv,
> -					       breakinput[i].index,
> -					       breakinput[i].level,
> -					       breakinput[i].filter);
> -	}
> -
> -	return ret;
> +	return stm32_pwm_apply_breakinputs(priv);
>  }
>  
>  static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> @@ -614,7 +622,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
>  	if (!priv->regmap || !priv->clk)
>  		return -EINVAL;
>  
> -	ret = stm32_pwm_apply_breakinputs(priv, np);
> +	ret = stm32_pwm_probe_breakinputs(priv, np);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.7.4
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] pwm: stm32: add power management support
  2019-10-04 12:53 ` [PATCH v2 3/3] pwm: stm32: add power management support Fabrice Gasnier
@ 2019-10-16  7:06   ` Thierry Reding
  2019-10-16 10:08     ` Fabrice Gasnier
  0 siblings, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2019-10-16  7:06 UTC (permalink / raw)
  To: Fabrice Gasnier
  Cc: robh+dt, u.kleine-koenig, alexandre.torgue, mark.rutland,
	mcoquelin.stm32, devicetree, linux-arm-kernel, linux-kernel,
	linux-pwm, benjamin.gaignard, linux-stm32

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

On Fri, Oct 04, 2019 at 02:53:53PM +0200, Fabrice Gasnier wrote:
> 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, see [1]. So, perform a check here, and handle the
> pinctrl states. Also restore the break inputs upon resume, as registers
> content may be lost when going to low power mode.
> 
> [1] https://lkml.org/lkml/2019/2/5/770
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
> Changes in v2:
> Follow Uwe suggestions/remarks:
> - Add a precursor patch to ease reviewing
> - Use registers read instead of pwm_get_state
> - Add a comment to mention registers content may be lost in low power mode
> ---
>  drivers/pwm/pwm-stm32.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)

Applied, thanks. I made two minor changes, though, see below.

> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index cf8658c..546b661 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -12,6 +12,7 @@
>  #include <linux/mfd/stm32-timers.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  
> @@ -655,6 +656,42 @@ static int stm32_pwm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> +{
> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
> +	unsigned int ch;

I renamed this to just "i", which is more idiomatic for loop variables.
The function is small enough not to need to differentiate between loop
variables.

> +	u32 ccer, mask;
> +
> +	/* Look for active channels */
> +	ccer = active_channels(priv);
> +
> +	for (ch = 0; ch < priv->chip.npwm; ch++) {
> +		mask = TIM_CCER_CC1E << (ch * 4);
> +		if (ccer & mask) {
> +			dev_err(dev, "The consumer didn't stop us (%s)\n",
> +				priv->chip.pwms[ch].label);

Changed this to:

	"PWM %u still in use by consumer %s\n", i, priv->chip.pwms[i].label

I think that might help clarify which PWM is still enabled in case the
consumers don't set a label.

Thierry

> +			return -EBUSY;
> +		}
> +	}
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused stm32_pwm_resume(struct device *dev)
> +{
> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* restore breakinput registers that may have been lost in low power */
> +	return stm32_pwm_apply_breakinputs(priv);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(stm32_pwm_pm_ops, stm32_pwm_suspend, stm32_pwm_resume);
> +
>  static const struct of_device_id stm32_pwm_of_match[] = {
>  	{ .compatible = "st,stm32-pwm",	},
>  	{ /* end node */ },
> @@ -667,6 +704,7 @@ static struct platform_driver stm32_pwm_driver = {
>  	.driver	= {
>  		.name = "stm32-pwm",
>  		.of_match_table = stm32_pwm_of_match,
> +		.pm = &stm32_pwm_pm_ops,
>  	},
>  };
>  module_platform_driver(stm32_pwm_driver);
> -- 
> 2.7.4
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] pwm: stm32: add power management support
  2019-10-16  7:06   ` Thierry Reding
@ 2019-10-16 10:08     ` Fabrice Gasnier
  0 siblings, 0 replies; 9+ messages in thread
From: Fabrice Gasnier @ 2019-10-16 10:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: robh+dt, u.kleine-koenig, alexandre.torgue, mark.rutland,
	mcoquelin.stm32, devicetree, linux-arm-kernel, linux-kernel,
	linux-pwm, benjamin.gaignard, linux-stm32

On 10/16/19 9:06 AM, Thierry Reding wrote:
> On Fri, Oct 04, 2019 at 02:53:53PM +0200, Fabrice Gasnier wrote:
>> 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, see [1]. So, perform a check here, and handle the
>> pinctrl states. Also restore the break inputs upon resume, as registers
>> content may be lost when going to low power mode.
>>
>> [1] https://lkml.org/lkml/2019/2/5/770
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> ---
>> Changes in v2:
>> Follow Uwe suggestions/remarks:
>> - Add a precursor patch to ease reviewing
>> - Use registers read instead of pwm_get_state
>> - Add a comment to mention registers content may be lost in low power mode
>> ---
>>  drivers/pwm/pwm-stm32.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
> 
> Applied, thanks. I made two minor changes, though, see below.
> 
>>
>> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
>> index cf8658c..546b661 100644
>> --- a/drivers/pwm/pwm-stm32.c
>> +++ b/drivers/pwm/pwm-stm32.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/mfd/stm32-timers.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/pinctrl/consumer.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pwm.h>
>>  
>> @@ -655,6 +656,42 @@ static int stm32_pwm_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static int __maybe_unused stm32_pwm_suspend(struct device *dev)
>> +{
>> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
>> +	unsigned int ch;
> 
> I renamed this to just "i", which is more idiomatic for loop variables.
> The function is small enough not to need to differentiate between loop
> variables.
> 
>> +	u32 ccer, mask;
>> +
>> +	/* Look for active channels */
>> +	ccer = active_channels(priv);
>> +
>> +	for (ch = 0; ch < priv->chip.npwm; ch++) {
>> +		mask = TIM_CCER_CC1E << (ch * 4);
>> +		if (ccer & mask) {
>> +			dev_err(dev, "The consumer didn't stop us (%s)\n",
>> +				priv->chip.pwms[ch].label);
> 
> Changed this to:
> 
> 	"PWM %u still in use by consumer %s\n", i, priv->chip.pwms[i].label
> 
> I think that might help clarify which PWM is still enabled in case the
> consumers don't set a label.

Hi Thierry,

Many thanks for all the improvements on this series!

Best Regards,
Fabrice

> 
> Thierry
> 
>> +			return -EBUSY;
>> +		}
>> +	}
>> +
>> +	return pinctrl_pm_select_sleep_state(dev);
>> +}
>> +
>> +static int __maybe_unused stm32_pwm_resume(struct device *dev)
>> +{
>> +	struct stm32_pwm *priv = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = pinctrl_pm_select_default_state(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* restore breakinput registers that may have been lost in low power */
>> +	return stm32_pwm_apply_breakinputs(priv);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(stm32_pwm_pm_ops, stm32_pwm_suspend, stm32_pwm_resume);
>> +
>>  static const struct of_device_id stm32_pwm_of_match[] = {
>>  	{ .compatible = "st,stm32-pwm",	},
>>  	{ /* end node */ },
>> @@ -667,6 +704,7 @@ static struct platform_driver stm32_pwm_driver = {
>>  	.driver	= {
>>  		.name = "stm32-pwm",
>>  		.of_match_table = stm32_pwm_of_match,
>> +		.pm = &stm32_pwm_pm_ops,
>>  	},
>>  };
>>  module_platform_driver(stm32_pwm_driver);
>> -- 
>> 2.7.4
>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-10-16 10:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 12:53 [PATCH v2 0/3] Add PM support to STM32 Timer PWM Fabrice Gasnier
2019-10-04 12:53 ` [PATCH v2 1/3] dt-bindings: pwm-stm32: document pinctrl sleep state Fabrice Gasnier
2019-10-11 15:10   ` Rob Herring
2019-10-16  6:59   ` Thierry Reding
2019-10-04 12:53 ` [PATCH v2 2/3] pwm: stm32: split breakinput apply routine to ease PM support Fabrice Gasnier
2019-10-16  7:03   ` Thierry Reding
2019-10-04 12:53 ` [PATCH v2 3/3] pwm: stm32: add power management support Fabrice Gasnier
2019-10-16  7:06   ` Thierry Reding
2019-10-16 10:08     ` 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).