Re: [PATCH 3/4] pwm: sysfs: Add suspend/resume support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4006 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]