| Message ID | 1559211367-25106-4-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: add power management on sysfs and switch to SPDX | expand |
Hi Shimoda-san, On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > According to the Documentation/pwm.txt, all PWM consumers should have > power management. Since this sysfs interface is one of consumers so that > this patch adds suspend/resume support. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/pwm/sysfs.c > +++ b/drivers/pwm/sysfs.c > @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = { > }; > ATTRIBUTE_GROUPS(pwm_chip); > You may want to add a comment "takes export->lock on success" here... > +static struct pwm_export *pwm_class_get_state(struct device *parent, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct device *child; > + struct pwm_export *export; > + > + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) > + return NULL; > + > + child = device_find_child(parent, pwm, pwm_unexport_match); > + if (!child) > + return NULL; > + > + export = child_to_pwm_export(child); > + put_device(child); /* for device_find_child() */ > + > + mutex_lock(&export->lock); > + pwm_get_state(pwm, state); > + > + return export; > +} > + > +static int pwm_class_apply_state(struct pwm_export *export, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + int ret = pwm_apply_state(pwm, state); > + ... and "release lock taken in pwm_class_get_state()" here. > + mutex_unlock(&export->lock); > + > + return ret; > +} Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, May 31, 2019 4:34 PM > > Hi Shimoda-san, > > On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > According to the Documentation/pwm.txt, all PWM consumers should have > > power management. Since this sysfs interface is one of consumers so that > > this patch adds suspend/resume support. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thank you for your review! I'll add comments you described on v3 patch. Best regards, Yoshihiro Shimoda > > --- a/drivers/pwm/sysfs.c > > +++ b/drivers/pwm/sysfs.c > > > @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(pwm_chip); > > > > You may want to add a comment "takes export->lock on success" here... > > > +static struct pwm_export *pwm_class_get_state(struct device *parent, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct device *child; > > + struct pwm_export *export; > > + > > + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) > > + return NULL; > > + > > + child = device_find_child(parent, pwm, pwm_unexport_match); > > + if (!child) > > + return NULL; > > + > > + export = child_to_pwm_export(child); > > + put_device(child); /* for device_find_child() */ > > + > > + mutex_lock(&export->lock); > > + pwm_get_state(pwm, state); > > + > > + return export; > > +} > > + > > +static int pwm_class_apply_state(struct pwm_export *export, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + int ret = pwm_apply_state(pwm, state); > > + > > ... and "release lock taken in pwm_class_get_state()" here. > > > + mutex_unlock(&export->lock); > > + > > + return ret; > > +} > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 7eb4a13..2cbfb20 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -18,6 +18,7 @@ struct pwm_export { struct device child; struct pwm_device *pwm; struct mutex lock; + struct pwm_state suspend; }; static struct pwm_export *child_to_pwm_export(struct device *child) @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = { }; ATTRIBUTE_GROUPS(pwm_chip); +static struct pwm_export *pwm_class_get_state(struct device *parent, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct device *child; + struct pwm_export *export; + + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) + return NULL; + + child = device_find_child(parent, pwm, pwm_unexport_match); + if (!child) + return NULL; + + export = child_to_pwm_export(child); + put_device(child); /* for device_find_child() */ + + mutex_lock(&export->lock); + pwm_get_state(pwm, state); + + return export; +} + +static int pwm_class_apply_state(struct pwm_export *export, + struct pwm_device *pwm, + struct pwm_state *state) +{ + int ret = pwm_apply_state(pwm, state); + + mutex_unlock(&export->lock); + + return ret; +} + +static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) +{ + struct pwm_chip *chip = dev_get_drvdata(parent); + unsigned int i; + int ret = 0; + + for (i = 0; i < npwm; i++) { + struct pwm_device *pwm = &chip->pwms[i]; + struct pwm_state state; + struct pwm_export *export; + + export = pwm_class_get_state(parent, pwm, &state); + if (!export) + continue; + + state.enabled = export->suspend.enabled; + ret = pwm_class_apply_state(export, pwm, &state); + if (ret < 0) + break; + } + + return ret; +} + +static int pwm_class_resume(struct device *parent) +{ + struct pwm_chip *chip = dev_get_drvdata(parent); + + return pwm_class_resume_npwm(parent, chip->npwm); +} + +static int pwm_class_suspend(struct device *parent) +{ + struct pwm_chip *chip = dev_get_drvdata(parent); + unsigned int i; + int ret = 0; + + for (i = 0; i < chip->npwm; i++) { + struct pwm_device *pwm = &chip->pwms[i]; + struct pwm_state state; + struct pwm_export *export; + + export = pwm_class_get_state(parent, pwm, &state); + if (!export) + continue; + + export->suspend = state; + state.enabled = false; + ret = pwm_class_apply_state(export, pwm, &state); + if (ret < 0) { + /* + * roll back the PWM devices that were disabled by + * this suspend function. + */ + pwm_class_resume_npwm(parent, i); + break; + } + } + + return ret; +} + +static SIMPLE_DEV_PM_OPS(pwm_class_pm_ops, pwm_class_suspend, pwm_class_resume); + static struct class pwm_class = { .name = "pwm", .owner = THIS_MODULE, .dev_groups = pwm_chip_groups, + .pm = &pwm_class_pm_ops, }; static int pwmchip_sysfs_match(struct device *parent, const void *data)
According to the Documentation/pwm.txt, all PWM consumers should have power management. Since this sysfs interface is one of consumers so that this patch adds suspend/resume support. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pwm/sysfs.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)