On 10/1/19 9:04 AM, Uwe Kleine-König wrote: > Hello Fabrice, > > On Mon, Sep 30, 2019 at 05:39:11PM +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@xxxxxx> >> --- >> drivers/pwm/pwm-stm32.c | 82 +++++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 62 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c >> index 740e2de..9bcd73a 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> >> >> @@ -19,6 +20,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 +33,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 +515,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; >> +} > > Can you explain what the effect of this function is? This is something > that is lost during suspend? Hi Uwe, Yes, that's what I explain in the commit message: ...registers content may be lost when going to low power mode. Do you think I need to rephrase ? > > I wonder why the patch is so big. There are some rearrangements that > should have no effect and I think it would be beneficial for > reviewability to split this patch in a patch that only does the > restructuring and than on top of that add the PM stuff. I can split this to ease the review. > >> + >> +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 +546,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 +623,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; >> >> @@ -647,6 +656,38 @@ 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); >> + struct pwm_state state; >> + unsigned int i; >> + >> + for (i = 0; i < priv->chip.npwm; i++) { >> + pwm_get_state(&priv->chip.pwms[i], &state); > > pwm_get_state is a function designed to be used by PWM consumers. I > would prefer to check the hardware registers here instead. It's also useful for PWM provider: This PWM driver is part of a MFD that also take care of IIO trigger (can be used simultaneously). Simply reading a register doesn't tell us that the timer is used/configured as a PWM here. That's the reason to use this helper to read pwm->state. Do you wish I add a comment to clarify this here ? > > What if there is no consumer and the PWM just happens to be enabled by > the bootloader? Or is this too minor an issue to be worth consideration? That's the purpose of returning -EBUSY: "PWM should not stop if the PWM user didn't call pwm_disable()" ... "to avoid situation where the PWM is actually suspended before the user". This has been enforced in later series with the device_link_add(). See our previous discussions here: https://lkml.org/lkml/2019/2/5/770 So, I guess this would point exactly a lack for a PWM user to manage it after the boot stage, in the kernel. Could you please clarify, provide an example here ? Thanks for reviewing, BR, Fabrice > > Best regards > Uwe >
