| Message ID | 20210407080155.55004-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] pwm: lpss: Don't modify HW state in .remove callback | expand |
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
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
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 */
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(-)