| Message ID | 1559116082-9851-4-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pwm: add power management on sysfs and switch to SPDX | expand |
On Wed, May 29, 2019 at 04:48:01PM +0900, Yoshihiro Shimoda 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> > --- > drivers/pwm/sysfs.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c > index 7eb4a13..72dafdd 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; > + bool enabled_in_suspend; How about if we save the complete state here? Something like: struct pwm_state suspend; Or similar? Then we can just pwm_get_state() into that and then disable the PWM like you do. > }; > > static struct pwm_export *child_to_pwm_export(struct device *child) > @@ -372,10 +373,73 @@ static struct attribute *pwm_chip_attrs[] = { > }; > ATTRIBUTE_GROUPS(pwm_chip); > > +static int pwm_class_suspend_resume(struct device *parent, bool suspend) I would prefer if these were separate functions. I think the kind of conditionals that you have below isn't worth the few lines that you may save by fusing suspend/resume into one function. Also, if you store struct pwm_state suspend during suspend, then both implementations will end up being fairly different, so reusing the code isn't going to be much of an advantage. > +{ > + 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 device *child; > + struct pwm_export *export; > + struct pwm_state state; > + > + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) > + continue; > + > + child = device_find_child(parent, pwm, pwm_unexport_match); > + if (!child) > + goto rollback; > + > + export = child_to_pwm_export(child); > + put_device(child); /* for device_find_child() */ > + if (!export) > + goto rollback; Con this even happen? I have a hard time seeing how. > + > + mutex_lock(&export->lock); > + pwm_get_state(pwm, &state); All of the above is shared code, so perhaps it'd be worth putting that into a separate helper function to achieve the code reuse that you otherwise get from sharing the function. > + if (suspend) { > + if (state.enabled) > + export->enabled_in_suspend = true; > + state.enabled = false; > + } else if (export->enabled_in_suspend) { > + state.enabled = true; > + export->enabled_in_suspend = false; > + } This in particular is what I mean. I think the two levels of conditionals here make this more complicated to understand than necessary. > + ret = pwm_apply_state(pwm, &state); > + mutex_unlock(&export->lock); > + if (ret < 0) > + goto rollback; > + } > + > + return ret; > + > +rollback: > + /* roll back only when suspend */ > + if (suspend) > + pwm_class_suspend_resume(parent, false); And then there's stuff like this where you need to explain what's going on just to save a couple of lines of code. Other than that, looks really nice. Thierry > + > + return ret; > +} > + > +static int pwm_class_suspend(struct device *parent) > +{ > + return pwm_class_suspend_resume(parent, true); > +} > + > +static int pwm_class_resume(struct device *parent) > +{ > + return pwm_class_suspend_resume(parent, false); > +} > + > +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) > -- > 2.7.4 >
Hi Thierry, > From: Thierry Reding, Sent: Wednesday, May 29, 2019 10:38 PM > > On Wed, May 29, 2019 at 04:48:01PM +0900, Yoshihiro Shimoda 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> > > --- > > drivers/pwm/sysfs.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 64 insertions(+) > > > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c > > index 7eb4a13..72dafdd 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; > > + bool enabled_in_suspend; > > How about if we save the complete state here? Something like: > > struct pwm_state suspend; > > Or similar? Then we can just pwm_get_state() into that and then disable > the PWM like you do. I got it. I'll fix it on v2. > > }; > > > > static struct pwm_export *child_to_pwm_export(struct device *child) > > @@ -372,10 +373,73 @@ static struct attribute *pwm_chip_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(pwm_chip); > > > > +static int pwm_class_suspend_resume(struct device *parent, bool suspend) > > I would prefer if these were separate functions. I think the kind of > conditionals that you have below isn't worth the few lines that you may > save by fusing suspend/resume into one function. > > Also, if you store struct pwm_state suspend during suspend, then both > implementations will end up being fairly different, so reusing the code > isn't going to be much of an advantage. I got it. As you said, separate functions are better for the code readability. > > +{ > > + 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 device *child; > > + struct pwm_export *export; > > + struct pwm_state state; > > + > > + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) > > + continue; > > + > > + child = device_find_child(parent, pwm, pwm_unexport_match); > > + if (!child) > > + goto rollback; > > + > > + export = child_to_pwm_export(child); > > + put_device(child); /* for device_find_child() */ > > + if (!export) > > + goto rollback; > > Con this even happen? I have a hard time seeing how. Oops! This condition is unnecessary. I'll remove it. > > + > > + mutex_lock(&export->lock); > > + pwm_get_state(pwm, &state); > > All of the above is shared code, so perhaps it'd be worth putting that > into a separate helper function to achieve the code reuse that you > otherwise get from sharing the function. I got it. I'll make such a helper function on v2. > > + if (suspend) { > > + if (state.enabled) > > + export->enabled_in_suspend = true; > > + state.enabled = false; > > + } else if (export->enabled_in_suspend) { > > + state.enabled = true; > > + export->enabled_in_suspend = false; > > + } > > This in particular is what I mean. I think the two levels of > conditionals here make this more complicated to understand than > necessary. I think so. > > + ret = pwm_apply_state(pwm, &state); > > + mutex_unlock(&export->lock); > > + if (ret < 0) > > + goto rollback; > > + } > > + > > + return ret; > > + > > +rollback: > > + /* roll back only when suspend */ > > + if (suspend) > > + pwm_class_suspend_resume(parent, false); > > And then there's stuff like this where you need to explain what's going > on just to save a couple of lines of code. I'll add a comment on v2. > Other than that, looks really nice. Thank you for your review! Best regards, Yoshihiro Shimoda > Thierry > > > + > > + return ret; > > +} > > + > > +static int pwm_class_suspend(struct device *parent) > > +{ > > + return pwm_class_suspend_resume(parent, true); > > +} > > + > > +static int pwm_class_resume(struct device *parent) > > +{ > > + return pwm_class_suspend_resume(parent, false); > > +} > > + > > +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) > > -- > > 2.7.4 > >
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 7eb4a13..72dafdd 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; + bool enabled_in_suspend; }; static struct pwm_export *child_to_pwm_export(struct device *child) @@ -372,10 +373,73 @@ static struct attribute *pwm_chip_attrs[] = { }; ATTRIBUTE_GROUPS(pwm_chip); +static int pwm_class_suspend_resume(struct device *parent, bool suspend) +{ + 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 device *child; + struct pwm_export *export; + struct pwm_state state; + + if (!test_bit(PWMF_EXPORTED, &pwm->flags)) + continue; + + child = device_find_child(parent, pwm, pwm_unexport_match); + if (!child) + goto rollback; + + export = child_to_pwm_export(child); + put_device(child); /* for device_find_child() */ + if (!export) + goto rollback; + + mutex_lock(&export->lock); + pwm_get_state(pwm, &state); + if (suspend) { + if (state.enabled) + export->enabled_in_suspend = true; + state.enabled = false; + } else if (export->enabled_in_suspend) { + state.enabled = true; + export->enabled_in_suspend = false; + } + ret = pwm_apply_state(pwm, &state); + mutex_unlock(&export->lock); + if (ret < 0) + goto rollback; + } + + return ret; + +rollback: + /* roll back only when suspend */ + if (suspend) + pwm_class_suspend_resume(parent, false); + + return ret; +} + +static int pwm_class_suspend(struct device *parent) +{ + return pwm_class_suspend_resume(parent, true); +} + +static int pwm_class_resume(struct device *parent) +{ + return pwm_class_suspend_resume(parent, false); +} + +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 | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)