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 <[email protected]>
> > ---
> > 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
> >
