[PATCH 1/3] pwm: lpss: Don't modify HW state in .remove callback

[PATCH 1/3] pwm: lpss: Don't modify HW state in .remove callback

[Uwe Kleine-König]

A consumer is expected to disable a PWM before calling pwm_put(). And if
they didn't there is hopefully a good reason (or the consumer needs
fixing). Also if disabling an enabled PWM was the right thing to do,
this should better be done in the framework instead of in each low level
driver.

So drop the hardware modification from the .remove() callback.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 939de93c157b..c81cb0ef984a 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -255,12 +255,6 @@ EXPORT_SYMBOL_GPL(pwm_lpss_probe);
 
 int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
 {
-	int i;
-
-	for (i = 0; i < lpwm->info->npwm; i++) {
-		if (pwm_is_enabled(&lpwm->chip.pwms[i]))
-			pm_runtime_put(lpwm->chip.dev);
-	}
 	return pwmchip_remove(&lpwm->chip);
 }
 EXPORT_SYMBOL_GPL(pwm_lpss_remove);

base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
-- 
2.30.2

[PATCH 2/3] pwm: Add a devm managed function to add pwm_chips

[Uwe Kleine-König]

This potentially simplifies pwm lowlevel drivers.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  | 19 +++++++++++++++++++
 include/linux/pwm.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a8eff4b3ee36..2ffceb69e00b 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -383,6 +383,25 @@ int pwmchip_remove(struct pwm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
+static void devm_pwmchip_remove(void *data)
+{
+	struct pwm_chip *chip = data;
+
+	pwmchip_remove(chip);
+}
+
+int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
+{
+	int ret;
+
+	ret = pwmchip_add(chip);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pwmchip_remove, chip);
+}
+EXPORT_SYMBOL_GPL(devm_pwmchip_add);
+
 /**
  * pwm_request() - request a PWM device
  * @pwm: global PWM device index
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e4d84d4db293..00adafbbed9e 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -396,6 +396,9 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 			      enum pwm_polarity polarity);
 int pwmchip_add(struct pwm_chip *chip);
 int pwmchip_remove(struct pwm_chip *chip);
+
+int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip);
+
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
-- 
2.30.2

[PATCH 3/3] pwm: lpss: Simplify using devm_pwmchip_add

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpss-pci.c      | 4 ----
 drivers/pwm/pwm-lpss-platform.c | 4 +---
 drivers/pwm/pwm-lpss.c          | 8 +-------
 drivers/pwm/pwm-lpss.h          | 1 -
 4 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
index cf749ea0de9f..c893ec3d2fb4 100644
--- a/drivers/pwm/pwm-lpss-pci.c
+++ b/drivers/pwm/pwm-lpss-pci.c
@@ -69,12 +69,8 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
 
 static void pwm_lpss_remove_pci(struct pci_dev *pdev)
 {
-	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
-
 	pm_runtime_forbid(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
-
-	pwm_lpss_remove(lpwm);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 986786be1e49..928570430cef 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -85,10 +85,8 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev)
 
 static int pwm_lpss_remove_platform(struct platform_device *pdev)
 {
-	struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev);
-
 	pm_runtime_disable(&pdev->dev);
-	return pwm_lpss_remove(lpwm);
+	return 0;
 }
 
 static const struct acpi_device_id pwm_lpss_acpi_match[] = {
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index c81cb0ef984a..b73ae5542d93 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -237,7 +237,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 	lpwm->chip.base = -1;
 	lpwm->chip.npwm = info->npwm;
 
-	ret = pwmchip_add(&lpwm->chip);
+	ret = devm_pwmchip_add(dev, &lpwm->chip);
 	if (ret) {
 		dev_err(dev, "failed to add PWM chip: %d\n", ret);
 		return ERR_PTR(ret);
@@ -253,12 +253,6 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 }
 EXPORT_SYMBOL_GPL(pwm_lpss_probe);
 
-int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
-{
-	return pwmchip_remove(&lpwm->chip);
-}
-EXPORT_SYMBOL_GPL(pwm_lpss_remove);
-
 MODULE_DESCRIPTION("PWM driver for Intel LPSS");
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 70db7e389d66..8b3476f25e06 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -35,6 +35,5 @@ struct pwm_lpss_boardinfo {
 
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
 				     const struct pwm_lpss_boardinfo *info);
-int pwm_lpss_remove(struct pwm_lpss_chip *lpwm);
 
 #endif	/* __PWM_LPSS_H */
-- 
2.30.2

Re: [PATCH 1/3] pwm: lpss: Don't modify HW state in .remove callback

[Thierry Reding]

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

On Wed, Apr 07, 2021 at 10:01:53AM +0200, Uwe Kleine-König wrote:
> A consumer is expected to disable a PWM before calling pwm_put(). And if
> they didn't there is hopefully a good reason (or the consumer needs
> fixing). Also if disabling an enabled PWM was the right thing to do,
> this should better be done in the framework instead of in each low level
> driver.
> 
> So drop the hardware modification from the .remove() callback.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-lpss.c | 6 ------
>  1 file changed, 6 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 3/3] pwm: lpss: Simplify using devm_pwmchip_add

[Thierry Reding]

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

On Wed, Apr 07, 2021 at 10:01:55AM +0200, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-lpss-pci.c      | 4 ----
>  drivers/pwm/pwm-lpss-platform.c | 4 +---
>  drivers/pwm/pwm-lpss.c          | 8 +-------
>  drivers/pwm/pwm-lpss.h          | 1 -
>  4 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
> index cf749ea0de9f..c893ec3d2fb4 100644
> --- a/drivers/pwm/pwm-lpss-pci.c
> +++ b/drivers/pwm/pwm-lpss-pci.c
> @@ -69,12 +69,8 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
>  
>  static void pwm_lpss_remove_pci(struct pci_dev *pdev)
>  {
> -	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
> -
>  	pm_runtime_forbid(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
> -
> -	pwm_lpss_remove(lpwm);
>  }

Isn't this going to defeat your quest to make all drivers release
resources only after pwmchip_remove() was called? Before this patch
you'd be able to fix that by moving pwm_lpss_remove() before the runtime
PM stuff, but after using devm_pwmchip_add() the pwmchip_remove() will
always get called after the driver's ->remove() returns.

Granted, in this case it's perhaps not the best example because this
driver is actually grabbing a runtime PM reference, so that should be
safe. However, I'm thinking in general device-managed PWM chip addition
and removal becomes less useful because of the ordering requirements.

Thierry

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

Re: [PATCH 3/3] pwm: lpss: Simplify using devm_pwmchip_add

[Uwe Kleine-König]

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

On Fri, Apr 09, 2021 at 03:28:07PM +0200, Thierry Reding wrote:
> On Wed, Apr 07, 2021 at 10:01:55AM +0200, Uwe Kleine-König wrote:
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-lpss-pci.c      | 4 ----
> >  drivers/pwm/pwm-lpss-platform.c | 4 +---
> >  drivers/pwm/pwm-lpss.c          | 8 +-------
> >  drivers/pwm/pwm-lpss.h          | 1 -
> >  4 files changed, 2 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c
> > index cf749ea0de9f..c893ec3d2fb4 100644
> > --- a/drivers/pwm/pwm-lpss-pci.c
> > +++ b/drivers/pwm/pwm-lpss-pci.c
> > @@ -69,12 +69,8 @@ static int pwm_lpss_probe_pci(struct pci_dev *pdev,
> >  
> >  static void pwm_lpss_remove_pci(struct pci_dev *pdev)
> >  {
> > -	struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev);
> > -
> >  	pm_runtime_forbid(&pdev->dev);
> >  	pm_runtime_get_sync(&pdev->dev);
> > -
> > -	pwm_lpss_remove(lpwm);
> >  }
> 
> Isn't this going to defeat your quest to make all drivers release
> resources only after pwmchip_remove() was called? Before this patch
> you'd be able to fix that by moving pwm_lpss_remove() before the runtime
> PM stuff, but after using devm_pwmchip_add() the pwmchip_remove() will
> always get called after the driver's ->remove() returns.
> 
> Granted, in this case it's perhaps not the best example because this
> driver is actually grabbing a runtime PM reference, so that should be
> safe. However, I'm thinking in general device-managed PWM chip addition
> and removal becomes less useful because of the ordering requirements.

The conversion to devm_pwmchip_add is obviously only correct if all
other resources needed by the PWM are also devm managed (ab8500,
bcm-kona). Once I got the clk maintainers to apply
https://lore.kernel.org/r/20210330181755.204339-1-u.kleine-koenig@pengutronix.de
this should be possible for some more drivers. (pwm-atmel, atmel_hlcdc,
bcm2835, bcm-iproc, berlin, brcmstb (stopped checking after b*))

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/3] pwm: lpss: Don't modify HW state in .remove callback

[Uwe Kleine-König]

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

On Fri, Apr 09, 2021 at 03:21:40PM +0200, Thierry Reding wrote:
> On Wed, Apr 07, 2021 at 10:01:53AM +0200, Uwe Kleine-König wrote:
> > A consumer is expected to disable a PWM before calling pwm_put(). And if
> > they didn't there is hopefully a good reason (or the consumer needs
> > fixing). Also if disabling an enabled PWM was the right thing to do,
> > this should better be done in the framework instead of in each low level
> > driver.
> > 
> > So drop the hardware modification from the .remove() callback.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-lpss.c | 6 ------
> >  1 file changed, 6 deletions(-)
> 
> Applied, thanks.

Great, but you didn't push yet. Is this still going through your own CI,
or did you forget to push?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/3] pwm: lpss: Don't modify HW state in .remove callback

[Uwe Kleine-König]

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

On Sat, Apr 10, 2021 at 03:46:01PM +0200, Uwe Kleine-König wrote:
> On Fri, Apr 09, 2021 at 03:21:40PM +0200, Thierry Reding wrote:
> > On Wed, Apr 07, 2021 at 10:01:53AM +0200, Uwe Kleine-König wrote:
> > > A consumer is expected to disable a PWM before calling pwm_put(). And if
> > > they didn't there is hopefully a good reason (or the consumer needs
> > > fixing). Also if disabling an enabled PWM was the right thing to do,
> > > this should better be done in the framework instead of in each low level
> > > driver.
> > > 
> > > So drop the hardware modification from the .remove() callback.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/pwm/pwm-lpss.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > 
> > Applied, thanks.
> 
> Great, but you didn't push yet. Is this still going through your own CI,
> or did you forget to push?

Oh, I thought you took the devm_pwmchip_add patch. I withdraw my
question.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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