[PATCH 0/4] pwm: add power management on sysfs and switch to SPDX

[PATCH 0/4] pwm: add power management on sysfs and switch to SPDX

[Yoshihiro Shimoda]

This patch series is affected from the following email:
https://marc.info/?l=linux-renesas-soc&m=155896668906609&w=2

- The patch 1 adds descriptions into Documentation/pwm.txt.
- The patch 2 is not related to the topic though, switches to
  SPDX identifier of sysfs.c.
- The patch 3 adds suspend/resume support into sysfs.c.
- The patch 4 removes suspend/resume support from pwm-rcar.c.

This is the first trial so that I didn't make patches for other
PWM drivers to remove suspend/resume support.

Yoshihiro Shimoda (4):
  pwm: Add power management descriptions
  pwm: sysfs: Switch to SPDX identifier
  pwm: sysfs: Add suspend/resume support
  pwm: rcar: Remove suspend/resume support

 Documentation/pwm.txt  |  7 +++++
 drivers/pwm/pwm-rcar.c | 39 --------------------------
 drivers/pwm/sysfs.c    | 75 +++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 72 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH 1/4] pwm: Add power management descriptions

[Yoshihiro Shimoda]

This patch adds power management descriptions that consumers should
implement it.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/pwm.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 8fbf0aa..996e5ea 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -65,6 +65,10 @@ period). struct pwm_args contains 2 fields (period and polarity) and should
 be used to set the initial PWM config (usually done in the probe function
 of the PWM user). PWM arguments are retrieved with pwm_get_args().
 
+All consumers should really be reconfiguring the PWM upon resume as
+appropriate. This is the only way to ensure that everything is resumed in
+the proper order.
+
 Using PWMs with the sysfs interface
 -----------------------------------
 
@@ -141,6 +145,9 @@ The implementation of ->get_state() (a method used to retrieve initial PWM
 state) is also encouraged for the same reason: letting the PWM user know
 about the current PWM state would allow him to avoid glitches.
 
+Drivers should not implement any power management. In other words,
+consumers should implement it as described as the "Using PWMs" section.
+
 Locking
 -------
 
-- 
2.7.4

[PATCH 2/4] pwm: sysfs: Switch to SPDX identifier

[Yoshihiro Shimoda]

Adopt the SPDX license identifier headers to ease license compliance
management.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/sysfs.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 719f8fa..7eb4a13 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -1,19 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * A simple sysfs interface for the generic PWM framework
  *
  * Copyright (C) 2013 H Hartley Sweeten <hsweeten@visionengravers.com>
  *
  * Based on previous work by Lars Poeschel <poeschel@lemonage.de>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2, or (at your option)
- * any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #include <linux/device.h>
-- 
2.7.4

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

[Yoshihiro Shimoda]

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;
 };
 
 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)
-- 
2.7.4

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

[Thierry Reding]

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

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

[Yoshihiro Shimoda]

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
> >

[PATCH 4/4] pwm: rcar: Remove suspend/resume support

[Yoshihiro Shimoda]

According to the Documentation/pwm.txt, all PWM consumers should
implement power management instead of the PWM driver. So, this
patch removes suspend/resume support.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 39 ---------------------------------------
 1 file changed, 39 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index cfe7dd1..5b2b8ec 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -254,50 +254,11 @@ static const struct of_device_id rcar_pwm_of_table[] = {
 };
 MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);
 
-#ifdef CONFIG_PM_SLEEP
-static struct pwm_device *rcar_pwm_dev_to_pwm_dev(struct device *dev)
-{
-	struct rcar_pwm_chip *rcar_pwm = dev_get_drvdata(dev);
-	struct pwm_chip *chip = &rcar_pwm->chip;
-
-	return &chip->pwms[0];
-}
-
-static int rcar_pwm_suspend(struct device *dev)
-{
-	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
-
-	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
-		return 0;
-
-	pm_runtime_put(dev);
-
-	return 0;
-}
-
-static int rcar_pwm_resume(struct device *dev)
-{
-	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
-	struct pwm_state state;
-
-	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
-		return 0;
-
-	pm_runtime_get_sync(dev);
-
-	pwm_get_state(pwm, &state);
-
-	return rcar_pwm_apply(pwm->chip, pwm, &state);
-}
-#endif /* CONFIG_PM_SLEEP */
-static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume);
-
 static struct platform_driver rcar_pwm_driver = {
 	.probe = rcar_pwm_probe,
 	.remove = rcar_pwm_remove,
 	.driver = {
 		.name = "pwm-rcar",
-		.pm	= &rcar_pwm_pm_ops,
 		.of_match_table = of_match_ptr(rcar_pwm_of_table),
 	}
 };
-- 
2.7.4

Re: [PATCH 4/4] pwm: rcar: Remove suspend/resume support

[Thierry Reding]

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

On Wed, May 29, 2019 at 04:48:02PM +0900, Yoshihiro Shimoda wrote:
> According to the Documentation/pwm.txt, all PWM consumers should
> implement power management instead of the PWM driver. So, this
> patch removes suspend/resume support.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 39 ---------------------------------------
>  1 file changed, 39 deletions(-)

Yes, I like that very much. =)

Thierry

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

Re: [PATCH 0/4] pwm: add power management on sysfs and switch to SPDX

[Thierry Reding]

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

On Wed, May 29, 2019 at 04:47:58PM +0900, Yoshihiro Shimoda wrote:
> This patch series is affected from the following email:
> https://marc.info/?l=linux-renesas-soc&m=155896668906609&w=2
> 
> - The patch 1 adds descriptions into Documentation/pwm.txt.
> - The patch 2 is not related to the topic though, switches to
>   SPDX identifier of sysfs.c.
> - The patch 3 adds suspend/resume support into sysfs.c.
> - The patch 4 removes suspend/resume support from pwm-rcar.c.
> 
> This is the first trial so that I didn't make patches for other
> PWM drivers to remove suspend/resume support.

Thanks for doing this. It's looking already pretty good. Just a couple
of minor comments in the individual patches.

Thierry

> 
> Yoshihiro Shimoda (4):
>   pwm: Add power management descriptions
>   pwm: sysfs: Switch to SPDX identifier
>   pwm: sysfs: Add suspend/resume support
>   pwm: rcar: Remove suspend/resume support
> 
>  Documentation/pwm.txt  |  7 +++++
>  drivers/pwm/pwm-rcar.c | 39 --------------------------
>  drivers/pwm/sysfs.c    | 75 +++++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 72 insertions(+), 49 deletions(-)
> 
> -- 
> 2.7.4
> 

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