| Message ID | 20200316103216.29383-2-geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: Renesas R-Car and TPU fixes | expand |
On Mon, Mar 16, 2020 at 11:32:14AM +0100, Geert Uytterhoeven wrote: > Runtime PM should be enabled before calling pwmchip_add(), as PWM users > can appear immediately after the PWM chip has been added. > Likewise, Runtime PM should be disabled after the removal of the PWM > chip. > > Fixes: ed6c1476bf7f16d5 ("pwm: Add support for R-Car PWM Timer") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/pwm/pwm-rcar.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > index 2685577b6dd45be7..7ab9eb6616d950cb 100644 > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -229,24 +229,28 @@ static int rcar_pwm_probe(struct platform_device *pdev) > rcar_pwm->chip.base = -1; > rcar_pwm->chip.npwm = 1; > > + pm_runtime_enable(&pdev->dev); > + > ret = pwmchip_add(&rcar_pwm->chip); > if (ret < 0) { > dev_err(&pdev->dev, "failed to register PWM chip: %d\n", ret); > + pm_runtime_disable(&pdev->dev); > return ret; > } > > - pm_runtime_enable(&pdev->dev); > - Wouldn't it be wiser to do the pm_runtime_enable in .request, or even in .apply when enabled=true? Best regards Uwe
Hi Uwe, On Mon, Mar 16, 2020 at 4:40 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 16, 2020 at 11:32:14AM +0100, Geert Uytterhoeven wrote: > > Runtime PM should be enabled before calling pwmchip_add(), as PWM users > > can appear immediately after the PWM chip has been added. > > Likewise, Runtime PM should be disabled after the removal of the PWM > > chip. > > > > Fixes: ed6c1476bf7f16d5 ("pwm: Add support for R-Car PWM Timer") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/pwm/pwm-rcar.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > > index 2685577b6dd45be7..7ab9eb6616d950cb 100644 > > --- a/drivers/pwm/pwm-rcar.c > > +++ b/drivers/pwm/pwm-rcar.c > > @@ -229,24 +229,28 @@ static int rcar_pwm_probe(struct platform_device *pdev) > > rcar_pwm->chip.base = -1; > > rcar_pwm->chip.npwm = 1; > > > > + pm_runtime_enable(&pdev->dev); > > + > > ret = pwmchip_add(&rcar_pwm->chip); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to register PWM chip: %d\n", ret); > > + pm_runtime_disable(&pdev->dev); > > return ret; > > } > > > > - pm_runtime_enable(&pdev->dev); > > - > > Wouldn't it be wiser to do the pm_runtime_enable in .request, or even in > .apply when enabled=true? Wouldn't that mean that the device cannot be powered down until the first time a PWM is used? Gr{oetje,eeting}s, Geert
Hello Geert, On Mon, Mar 16, 2020 at 04:42:31PM +0100, Geert Uytterhoeven wrote: > On Mon, Mar 16, 2020 at 4:40 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 16, 2020 at 11:32:14AM +0100, Geert Uytterhoeven wrote: > > > Runtime PM should be enabled before calling pwmchip_add(), as PWM users > > > can appear immediately after the PWM chip has been added. > > > Likewise, Runtime PM should be disabled after the removal of the PWM > > > chip. > > > > > > Fixes: ed6c1476bf7f16d5 ("pwm: Add support for R-Car PWM Timer") > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- > > > drivers/pwm/pwm-rcar.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > > > index 2685577b6dd45be7..7ab9eb6616d950cb 100644 > > > --- a/drivers/pwm/pwm-rcar.c > > > +++ b/drivers/pwm/pwm-rcar.c > > > @@ -229,24 +229,28 @@ static int rcar_pwm_probe(struct platform_device *pdev) > > > rcar_pwm->chip.base = -1; > > > rcar_pwm->chip.npwm = 1; > > > > > > + pm_runtime_enable(&pdev->dev); > > > + > > > ret = pwmchip_add(&rcar_pwm->chip); > > > if (ret < 0) { > > > dev_err(&pdev->dev, "failed to register PWM chip: %d\n", ret); > > > + pm_runtime_disable(&pdev->dev); > > > return ret; > > > } > > > > > > - pm_runtime_enable(&pdev->dev); > > > - > > > > Wouldn't it be wiser to do the pm_runtime_enable in .request, or even in > > .apply when enabled=true? > > Wouldn't that mean that the device cannot be powered down until the first > time a PWM is used? Ah, it seems I got the semantic of pm_runtime_enable() wrong. I confused it with pm_runtime_get(). Now with that corrected your fix is obviously right: Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe
Hi Geert, Thank you for the patch. On Mon, Mar 16, 2020 at 11:32:14AM +0100, Geert Uytterhoeven wrote: > Runtime PM should be enabled before calling pwmchip_add(), as PWM users > can appear immediately after the PWM chip has been added. > Likewise, Runtime PM should be disabled after the removal of the PWM > chip. > > Fixes: ed6c1476bf7f16d5 ("pwm: Add support for R-Car PWM Timer") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/pwm/pwm-rcar.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > index 2685577b6dd45be7..7ab9eb6616d950cb 100644 > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -229,24 +229,28 @@ static int rcar_pwm_probe(struct platform_device *pdev) > rcar_pwm->chip.base = -1; > rcar_pwm->chip.npwm = 1; > > + pm_runtime_enable(&pdev->dev); > + > ret = pwmchip_add(&rcar_pwm->chip); > if (ret < 0) { > dev_err(&pdev->dev, "failed to register PWM chip: %d\n", ret); > + pm_runtime_disable(&pdev->dev); > return ret; > } > > - pm_runtime_enable(&pdev->dev); > - > return 0; > } > > static int rcar_pwm_remove(struct platform_device *pdev) > { > struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); > + int ret; > + > + ret = pwmchip_remove(&rcar_pwm->chip); > > pm_runtime_disable(&pdev->dev); > > - return pwmchip_remove(&rcar_pwm->chip); > + return ret; > } > > static const struct of_device_id rcar_pwm_of_table[] = {
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c index 2685577b6dd45be7..7ab9eb6616d950cb 100644 --- a/drivers/pwm/pwm-rcar.c +++ b/drivers/pwm/pwm-rcar.c @@ -229,24 +229,28 @@ static int rcar_pwm_probe(struct platform_device *pdev) rcar_pwm->chip.base = -1; rcar_pwm->chip.npwm = 1; + pm_runtime_enable(&pdev->dev); + ret = pwmchip_add(&rcar_pwm->chip); if (ret < 0) { dev_err(&pdev->dev, "failed to register PWM chip: %d\n", ret); + pm_runtime_disable(&pdev->dev); return ret; } - pm_runtime_enable(&pdev->dev); - return 0; } static int rcar_pwm_remove(struct platform_device *pdev) { struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev); + int ret; + + ret = pwmchip_remove(&rcar_pwm->chip); pm_runtime_disable(&pdev->dev); - return pwmchip_remove(&rcar_pwm->chip); + return ret; } static const struct of_device_id rcar_pwm_of_table[] = {
Runtime PM should be enabled before calling pwmchip_add(), as PWM users can appear immediately after the PWM chip has been added. Likewise, Runtime PM should be disabled after the removal of the PWM chip. Fixes: ed6c1476bf7f16d5 ("pwm: Add support for R-Car PWM Timer") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/pwm/pwm-rcar.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)