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@xxxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
