linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX
@ 2019-05-30 10:16 Yoshihiro Shimoda
  2019-05-30 10:16 ` [PATCH v2 1/4] pwm: Add power management descriptions Yoshihiro Shimoda
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-30 10:16 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-pwm, linux-renesas-soc, 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.

I'll make for other PWM drivers to remove suspend/resume support
after this patch series is applied.

Changes from v1 [1]:
 - Save pwm_state of suspend instead of own bool value on the patch 3.
 - Separate functions for suspend and resume on the patch 3.
 - Remove unnecessary condition of child_to_pwm_export() on the patch 3.
 - Add helper functions for suspend/resume into sysfs.c on the patch 3.
 - Add comment about the "roll back" on the patch 3.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=124235

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    | 111 ++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 108 insertions(+), 49 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] pwm: Add power management descriptions
  2019-05-30 10:16 [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX Yoshihiro Shimoda
@ 2019-05-30 10:16 ` Yoshihiro Shimoda
  2019-05-31  7:20   ` Geert Uytterhoeven
  2019-05-30 10:16 ` [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier Yoshihiro Shimoda
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-30 10:16 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-pwm, linux-renesas-soc, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier
  2019-05-30 10:16 [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX Yoshihiro Shimoda
  2019-05-30 10:16 ` [PATCH v2 1/4] pwm: Add power management descriptions Yoshihiro Shimoda
@ 2019-05-30 10:16 ` Yoshihiro Shimoda
  2019-05-31  7:20   ` Geert Uytterhoeven
  2019-05-30 10:16 ` [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support Yoshihiro Shimoda
  2019-05-30 10:16 ` [PATCH v2 4/4] pwm: rcar: Remove " Yoshihiro Shimoda
  3 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-30 10:16 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-pwm, linux-renesas-soc, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support
  2019-05-30 10:16 [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX Yoshihiro Shimoda
  2019-05-30 10:16 ` [PATCH v2 1/4] pwm: Add power management descriptions Yoshihiro Shimoda
  2019-05-30 10:16 ` [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier Yoshihiro Shimoda
@ 2019-05-30 10:16 ` Yoshihiro Shimoda
  2019-05-31  7:34   ` Geert Uytterhoeven
  2019-05-30 10:16 ` [PATCH v2 4/4] pwm: rcar: Remove " Yoshihiro Shimoda
  3 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-30 10:16 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-pwm, linux-renesas-soc, 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 | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 7eb4a13..2cbfb20 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;
+	struct pwm_state suspend;
 };
 
 static struct pwm_export *child_to_pwm_export(struct device *child)
@@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = {
 };
 ATTRIBUTE_GROUPS(pwm_chip);
 
+static struct pwm_export *pwm_class_get_state(struct device *parent,
+					      struct pwm_device *pwm,
+					      struct pwm_state *state)
+{
+	struct device *child;
+	struct pwm_export *export;
+
+	if (!test_bit(PWMF_EXPORTED, &pwm->flags))
+		return NULL;
+
+	child = device_find_child(parent, pwm, pwm_unexport_match);
+	if (!child)
+		return NULL;
+
+	export = child_to_pwm_export(child);
+	put_device(child);	/* for device_find_child() */
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, state);
+
+	return export;
+}
+
+static int pwm_class_apply_state(struct pwm_export *export,
+				 struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	int ret = pwm_apply_state(pwm, state);
+
+	mutex_unlock(&export->lock);
+
+	return ret;
+}
+
+static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
+		struct pwm_state state;
+		struct pwm_export *export;
+
+		export = pwm_class_get_state(parent, pwm, &state);
+		if (!export)
+			continue;
+
+		state.enabled = export->suspend.enabled;
+		ret = pwm_class_apply_state(export, pwm, &state);
+		if (ret < 0)
+			break;
+	}
+
+	return ret;
+}
+
+static int pwm_class_resume(struct device *parent)
+{
+	struct pwm_chip *chip = dev_get_drvdata(parent);
+
+	return pwm_class_resume_npwm(parent, chip->npwm);
+}
+
+static int pwm_class_suspend(struct device *parent)
+{
+	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 pwm_state state;
+		struct pwm_export *export;
+
+		export = pwm_class_get_state(parent, pwm, &state);
+		if (!export)
+			continue;
+
+		export->suspend = state;
+		state.enabled = false;
+		ret = pwm_class_apply_state(export, pwm, &state);
+		if (ret < 0) {
+			/*
+			 * roll back the PWM devices that were disabled by
+			 * this suspend function.
+			 */
+			pwm_class_resume_npwm(parent, i);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] pwm: rcar: Remove suspend/resume support
  2019-05-30 10:16 [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX Yoshihiro Shimoda
                   ` (2 preceding siblings ...)
  2019-05-30 10:16 ` [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support Yoshihiro Shimoda
@ 2019-05-30 10:16 ` Yoshihiro Shimoda
  2019-05-31  7:35   ` Geert Uytterhoeven
  3 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-30 10:16 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-pwm, linux-renesas-soc, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] pwm: Add power management descriptions
  2019-05-30 10:16 ` [PATCH v2 1/4] pwm: Add power management descriptions Yoshihiro Shimoda
@ 2019-05-31  7:20   ` Geert Uytterhoeven
  2019-05-31  9:22     ` Yoshihiro Shimoda
  0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-31  7:20 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Shimoda-san,

On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> This patch adds power management descriptions that consumers should
> implement it.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/Documentation/pwm.txt
> +++ b/Documentation/pwm.txt

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

s/as/in/

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier
  2019-05-30 10:16 ` [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier Yoshihiro Shimoda
@ 2019-05-31  7:20   ` Geert Uytterhoeven
  0 siblings, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-31  7:20 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Adopt the SPDX license identifier headers to ease license compliance
> management.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support
  2019-05-30 10:16 ` [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support Yoshihiro Shimoda
@ 2019-05-31  7:34   ` Geert Uytterhoeven
  2019-05-31  9:24     ` Yoshihiro Shimoda
  0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-31  7:34 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Shimoda-san,

On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c

> @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(pwm_chip);
>

You may want to add a comment "takes export->lock on success" here...

> +static struct pwm_export *pwm_class_get_state(struct device *parent,
> +                                             struct pwm_device *pwm,
> +                                             struct pwm_state *state)
> +{
> +       struct device *child;
> +       struct pwm_export *export;
> +
> +       if (!test_bit(PWMF_EXPORTED, &pwm->flags))
> +               return NULL;
> +
> +       child = device_find_child(parent, pwm, pwm_unexport_match);
> +       if (!child)
> +               return NULL;
> +
> +       export = child_to_pwm_export(child);
> +       put_device(child);      /* for device_find_child() */
> +
> +       mutex_lock(&export->lock);
> +       pwm_get_state(pwm, state);
> +
> +       return export;
> +}
> +
> +static int pwm_class_apply_state(struct pwm_export *export,
> +                                struct pwm_device *pwm,
> +                                struct pwm_state *state)
> +{
> +       int ret = pwm_apply_state(pwm, state);
> +

... and "release lock taken in pwm_class_get_state()" here.

> +       mutex_unlock(&export->lock);
> +
> +       return ret;
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] pwm: rcar: Remove suspend/resume support
  2019-05-30 10:16 ` [PATCH v2 4/4] pwm: rcar: Remove " Yoshihiro Shimoda
@ 2019-05-31  7:35   ` Geert Uytterhoeven
  0 siblings, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-31  7:35 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 1/4] pwm: Add power management descriptions
  2019-05-31  7:20   ` Geert Uytterhoeven
@ 2019-05-31  9:22     ` Yoshihiro Shimoda
  2019-05-31  9:28       ` Geert Uytterhoeven
  0 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-31  9:22 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Friday, May 31, 2019 4:20 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > This patch adds power management descriptions that consumers should
> > implement it.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/pwm.txt
> > +++ b/Documentation/pwm.txt
> 
> > @@ -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.
> 
> s/as/in/

So, "... in described as ..." ?

I'm not a native speaker, so that any cases below seem not so different to me :)

1. "... as described as ..."
2. "... in described as ..."
3. "... as described in ..."
4. "... in described in ..."

> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support
  2019-05-31  7:34   ` Geert Uytterhoeven
@ 2019-05-31  9:24     ` Yoshihiro Shimoda
  0 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-31  9:24 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, May 31, 2019 4:34 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> 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>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for your review!
I'll add comments you described on v3 patch.

Best regards,
Yoshihiro Shimoda

> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> 
> > @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(pwm_chip);
> >
> 
> You may want to add a comment "takes export->lock on success" here...
> 
> > +static struct pwm_export *pwm_class_get_state(struct device *parent,
> > +                                             struct pwm_device *pwm,
> > +                                             struct pwm_state *state)
> > +{
> > +       struct device *child;
> > +       struct pwm_export *export;
> > +
> > +       if (!test_bit(PWMF_EXPORTED, &pwm->flags))
> > +               return NULL;
> > +
> > +       child = device_find_child(parent, pwm, pwm_unexport_match);
> > +       if (!child)
> > +               return NULL;
> > +
> > +       export = child_to_pwm_export(child);
> > +       put_device(child);      /* for device_find_child() */
> > +
> > +       mutex_lock(&export->lock);
> > +       pwm_get_state(pwm, state);
> > +
> > +       return export;
> > +}
> > +
> > +static int pwm_class_apply_state(struct pwm_export *export,
> > +                                struct pwm_device *pwm,
> > +                                struct pwm_state *state)
> > +{
> > +       int ret = pwm_apply_state(pwm, state);
> > +
> 
> ... and "release lock taken in pwm_class_get_state()" here.
> 
> > +       mutex_unlock(&export->lock);
> > +
> > +       return ret;
> > +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] pwm: Add power management descriptions
  2019-05-31  9:22     ` Yoshihiro Shimoda
@ 2019-05-31  9:28       ` Geert Uytterhoeven
  2019-05-31  9:31         ` Yoshihiro Shimoda
  0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-31  9:28 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Shimoda-san,

On Fri, May 31, 2019 at 11:22 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > This patch adds power management descriptions that consumers should
> > > implement it.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/Documentation/pwm.txt
> > > +++ b/Documentation/pwm.txt
> >
> > > @@ -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.
> >
> > s/as/in/
>
> So, "... in described as ..." ?
>
> I'm not a native speaker, so that any cases below seem not so different to me :)
>
> 1. "... as described as ..."
> 2. "... in described as ..."
> 3. "... as described in ..."
> 4. "... in described in ..."

Sorry for being unclear: "... as described in ..."

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v2 1/4] pwm: Add power management descriptions
  2019-05-31  9:28       ` Geert Uytterhoeven
@ 2019-05-31  9:31         ` Yoshihiro Shimoda
  0 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-31  9:31 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Thierry Reding, Linux PWM List, Linux-Renesas

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, May 31, 2019 6:29 PM
> 
> Hi Shimoda-san,
> 
> On Fri, May 31, 2019 at 11:22 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > This patch adds power management descriptions that consumers should
> > > > implement it.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/Documentation/pwm.txt
> > > > +++ b/Documentation/pwm.txt
> > >
> > > > @@ -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.
> > >
> > > s/as/in/
> >
> > So, "... in described as ..." ?
> >
> > I'm not a native speaker, so that any cases below seem not so different to me :)
> >
> > 1. "... as described as ..."
> > 2. "... in described as ..."
> > 3. "... as described in ..."
> > 4. "... in described in ..."
> 
> Sorry for being unclear: "... as described in ..."

Thank you for the reply! I'll revise it.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-05-31  9:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-30 10:16 [PATCH v2 0/4] pwm: add power management on sysfs and switch to SPDX Yoshihiro Shimoda
2019-05-30 10:16 ` [PATCH v2 1/4] pwm: Add power management descriptions Yoshihiro Shimoda
2019-05-31  7:20   ` Geert Uytterhoeven
2019-05-31  9:22     ` Yoshihiro Shimoda
2019-05-31  9:28       ` Geert Uytterhoeven
2019-05-31  9:31         ` Yoshihiro Shimoda
2019-05-30 10:16 ` [PATCH v2 2/4] pwm: sysfs: Switch to SPDX identifier Yoshihiro Shimoda
2019-05-31  7:20   ` Geert Uytterhoeven
2019-05-30 10:16 ` [PATCH v2 3/4] pwm: sysfs: Add suspend/resume support Yoshihiro Shimoda
2019-05-31  7:34   ` Geert Uytterhoeven
2019-05-31  9:24     ` Yoshihiro Shimoda
2019-05-30 10:16 ` [PATCH v2 4/4] pwm: rcar: Remove " Yoshihiro Shimoda
2019-05-31  7:35   ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).