| Message ID | 20200316103216.29383-3-geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: Renesas R-Car and TPU fixes | expand |
Hello Geert, On Mon, Mar 16, 2020 at 11:32:15AM +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 always be disabled after the removal of the > PWM chip, even if the latter failed. > > Fixes: 99b82abb0a35b073 ("pwm: Add Renesas TPU PWM driver") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/pwm/pwm-renesas-tpu.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > index 4a855a21b782dea3..8032acc84161a9dd 100644 > --- a/drivers/pwm/pwm-renesas-tpu.c > +++ b/drivers/pwm/pwm-renesas-tpu.c > @@ -415,16 +415,17 @@ static int tpu_probe(struct platform_device *pdev) > tpu->chip.base = -1; > tpu->chip.npwm = TPU_CHANNEL_MAX; > > + pm_runtime_enable(&pdev->dev); > + > ret = pwmchip_add(&tpu->chip); > if (ret < 0) { > dev_err(&pdev->dev, "failed to register PWM chip\n"); > + pm_runtime_disable(&pdev->dev); > return ret; > } > > dev_info(&pdev->dev, "TPU PWM %d registered\n", tpu->pdev->id); > > - pm_runtime_enable(&pdev->dev); > - > return 0; > } > > @@ -434,12 +435,10 @@ static int tpu_remove(struct platform_device *pdev) > int ret; > > ret = pwmchip_remove(&tpu->chip); > - if (ret) > - return ret; > > pm_runtime_disable(&pdev->dev); > > - return 0; > + return ret; > } Maybe I was a bit quick with my reply to the previous patch. I wonder if it is right to call pm_runtime_disable if pwmchip_remove failed? Best regards Uwe
Hi Geert, Thank you for the patch. On Mon, Mar 16, 2020 at 05:01:08PM +0100, Uwe Kleine-König wrote: > On Mon, Mar 16, 2020 at 11:32:15AM +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 always be disabled after the removal of the > > PWM chip, even if the latter failed. > > Fixes: 99b82abb0a35b073 ("pwm: Add Renesas TPU PWM driver") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/pwm/pwm-renesas-tpu.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > > index 4a855a21b782dea3..8032acc84161a9dd 100644 > > --- a/drivers/pwm/pwm-renesas-tpu.c > > +++ b/drivers/pwm/pwm-renesas-tpu.c > > @@ -415,16 +415,17 @@ static int tpu_probe(struct platform_device *pdev) > > tpu->chip.base = -1; > > tpu->chip.npwm = TPU_CHANNEL_MAX; > > > > + pm_runtime_enable(&pdev->dev); > > + > > ret = pwmchip_add(&tpu->chip); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to register PWM chip\n"); > > + pm_runtime_disable(&pdev->dev); > > return ret; > > } > > > > dev_info(&pdev->dev, "TPU PWM %d registered\n", tpu->pdev->id); > > > > - pm_runtime_enable(&pdev->dev); > > - > > return 0; > > } This part looks good to me. > > > > @@ -434,12 +435,10 @@ static int tpu_remove(struct platform_device *pdev) > > int ret; > > > > ret = pwmchip_remove(&tpu->chip); > > - if (ret) > > - return ret; > > > > pm_runtime_disable(&pdev->dev); > > > > - return 0; > > + return ret; > > } > > Maybe I was a bit quick with my reply to the previous patch. I wonder if > it is right to call pm_runtime_disable if pwmchip_remove failed? It should at least be explained in the commit message why this is the right thing to do.
Hi Uwe, On Mon, Mar 16, 2020 at 5:01 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Mar 16, 2020 at 11:32:15AM +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 always be disabled after the removal of the > > PWM chip, even if the latter failed. > > > > Fixes: 99b82abb0a35b073 ("pwm: Add Renesas TPU PWM driver") > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/pwm/pwm-renesas-tpu.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > > index 4a855a21b782dea3..8032acc84161a9dd 100644 > > --- a/drivers/pwm/pwm-renesas-tpu.c > > +++ b/drivers/pwm/pwm-renesas-tpu.c > > @@ -415,16 +415,17 @@ static int tpu_probe(struct platform_device *pdev) > > tpu->chip.base = -1; > > tpu->chip.npwm = TPU_CHANNEL_MAX; > > > > + pm_runtime_enable(&pdev->dev); > > + > > ret = pwmchip_add(&tpu->chip); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to register PWM chip\n"); > > + pm_runtime_disable(&pdev->dev); > > return ret; > > } > > > > dev_info(&pdev->dev, "TPU PWM %d registered\n", tpu->pdev->id); > > > > - pm_runtime_enable(&pdev->dev); > > - > > return 0; > > } > > > > @@ -434,12 +435,10 @@ static int tpu_remove(struct platform_device *pdev) > > int ret; > > > > ret = pwmchip_remove(&tpu->chip); > > - if (ret) > > - return ret; > > > > pm_runtime_disable(&pdev->dev); > > > > - return 0; > > + return ret; > > } > > Maybe I was a bit quick with my reply to the previous patch. I wonder if > it is right to call pm_runtime_disable if pwmchip_remove failed? While the pwmchip may still exist, the hardware is unmapped and no longer accessible. Gr{oetje,eeting}s, Geert
Hi Geert, On Mon, Mar 16, 2020 at 05:06:12PM +0100, Geert Uytterhoeven wrote: > On Mon, Mar 16, 2020 at 5:01 PM Uwe Kleine-König wrote: > > On Mon, Mar 16, 2020 at 11:32:15AM +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 always be disabled after the removal of the > > > PWM chip, even if the latter failed. > > > > > > Fixes: 99b82abb0a35b073 ("pwm: Add Renesas TPU PWM driver") > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- > > > drivers/pwm/pwm-renesas-tpu.c | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > > > index 4a855a21b782dea3..8032acc84161a9dd 100644 > > > --- a/drivers/pwm/pwm-renesas-tpu.c > > > +++ b/drivers/pwm/pwm-renesas-tpu.c > > > @@ -415,16 +415,17 @@ static int tpu_probe(struct platform_device *pdev) > > > tpu->chip.base = -1; > > > tpu->chip.npwm = TPU_CHANNEL_MAX; > > > > > > + pm_runtime_enable(&pdev->dev); > > > + > > > ret = pwmchip_add(&tpu->chip); > > > if (ret < 0) { > > > dev_err(&pdev->dev, "failed to register PWM chip\n"); > > > + pm_runtime_disable(&pdev->dev); > > > return ret; > > > } > > > > > > dev_info(&pdev->dev, "TPU PWM %d registered\n", tpu->pdev->id); > > > > > > - pm_runtime_enable(&pdev->dev); > > > - > > > return 0; > > > } > > > > > > @@ -434,12 +435,10 @@ static int tpu_remove(struct platform_device *pdev) > > > int ret; > > > > > > ret = pwmchip_remove(&tpu->chip); > > > - if (ret) > > > - return ret; > > > > > > pm_runtime_disable(&pdev->dev); > > > > > > - return 0; > > > + return ret; > > > } > > > > Maybe I was a bit quick with my reply to the previous patch. I wonder if > > it is right to call pm_runtime_disable if pwmchip_remove failed? > > While the pwmchip may still exist, the hardware is unmapped and no > longer accessible. Is it the case on module unload, doesn't a .remove() failure prevent the module from being unloaded, and keeps the device associated with the driver ? I haven't actually checked myself.
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 4a855a21b782dea3..8032acc84161a9dd 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -415,16 +415,17 @@ static int tpu_probe(struct platform_device *pdev) tpu->chip.base = -1; tpu->chip.npwm = TPU_CHANNEL_MAX; + pm_runtime_enable(&pdev->dev); + ret = pwmchip_add(&tpu->chip); if (ret < 0) { dev_err(&pdev->dev, "failed to register PWM chip\n"); + pm_runtime_disable(&pdev->dev); return ret; } dev_info(&pdev->dev, "TPU PWM %d registered\n", tpu->pdev->id); - pm_runtime_enable(&pdev->dev); - return 0; } @@ -434,12 +435,10 @@ static int tpu_remove(struct platform_device *pdev) int ret; ret = pwmchip_remove(&tpu->chip); - if (ret) - return ret; pm_runtime_disable(&pdev->dev); - return 0; + return ret; } #ifdef CONFIG_OF
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 always be disabled after the removal of the PWM chip, even if the latter failed. Fixes: 99b82abb0a35b073 ("pwm: Add Renesas TPU PWM driver") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/pwm/pwm-renesas-tpu.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)