| Message ID | 20231110100726.2930218-2-vigneshr@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: rti_wdt: Disable module when unused | expand |
On 11/10/23 02:07, Vignesh Raghavendra wrote: > Switch to devm_pm_runtime_enable() to simplify error handling in driver > probe. > This also replaces the call to pm_runtime_resume_and_get() without explanation. Worse, the next patch conditionally re-introduces pm_runtime_put_sync() on the probe function. > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> > --- > drivers/watchdog/rti_wdt.c | 30 ++++++++---------------------- > 1 file changed, 8 insertions(+), 22 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 8e1be7ba0103..163bdeb6929a 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device *pdev) > if (wdt->freq < 32768) > wdt->freq = wdt->freq * 9 / 10; > > - pm_runtime_enable(dev); > - ret = pm_runtime_resume_and_get(dev); > - if (ret < 0) { > - pm_runtime_disable(&pdev->dev); > - return dev_err_probe(dev, ret, "runtime pm failed\n"); > - } > + devm_pm_runtime_enable(dev); devm_pm_runtime_enable() returns an error code. I don't think ignoring it is a good idea. > + pm_runtime_get_noresume(dev); Is this functionally identical to pm_runtime_resume_and_get() ? That would require further explanation. Why is it not necessary to resume here ? Also, doesn't the next patch fix or at least modify this ? > > platform_set_drvdata(pdev, wdt); > > @@ -260,7 +256,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > wdt->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(wdt->base)) { > ret = PTR_ERR(wdt->base); > - goto err_iomap; > + return ret; > } > > if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { > @@ -286,7 +282,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > ret = rti_wdt_setup_hw_hb(wdd, wsize); > if (ret) { > dev_err(dev, "bad window size.\n"); > - goto err_iomap; > + return ret; > } > > last_ping = heartbeat_ms - time_left_ms; > @@ -301,7 +297,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > ret = of_address_to_resource(node, 0, &res); > if (ret) { > dev_err(dev, "No memory address assigned to the region.\n"); > - goto err_iomap; > + return ret; > } > > /* > @@ -312,15 +308,13 @@ static int rti_wdt_probe(struct platform_device *pdev) > reserved_mem_size = resource_size(&res); > if (reserved_mem_size < RESERVED_MEM_MIN_SIZE) { > dev_err(dev, "The size of reserved memory is too small.\n"); > - ret = -EINVAL; > - goto err_iomap; > + return -EINVAL; > } > > vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB); > if (!vaddr) { > dev_err(dev, "Failed to map memory-region.\n"); > - ret = -ENOMEM; > - goto err_iomap; > + return -ENOMEM; > } > > if (vaddr[0] == PON_REASON_SOF_NUM && > @@ -337,19 +331,13 @@ static int rti_wdt_probe(struct platform_device *pdev) > ret = watchdog_register_device(wdd); > if (ret) { > dev_err(dev, "cannot register watchdog device\n"); > - goto err_iomap; > + return ret; > } > > if (last_ping) > watchdog_set_last_hw_keepalive(wdd, last_ping); > > return 0; > - > -err_iomap: > - pm_runtime_put_sync(&pdev->dev); > - pm_runtime_disable(&pdev->dev); > - > - return ret; > } > > static void rti_wdt_remove(struct platform_device *pdev) > @@ -357,8 +345,6 @@ static void rti_wdt_remove(struct platform_device *pdev) > struct rti_wdt_device *wdt = platform_get_drvdata(pdev); > > watchdog_unregister_device(&wdt->wdd); > - pm_runtime_put(&pdev->dev); > - pm_runtime_disable(&pdev->dev); > } > > static const struct of_device_id rti_wdt_of_match[] = {
Hi, On 10/11/23 20:33, Guenter Roeck wrote: > On 11/10/23 02:07, Vignesh Raghavendra wrote: >> Switch to devm_pm_runtime_enable() to simplify error handling in driver >> probe. >> > > This also replaces the call to pm_runtime_resume_and_get() without > explanation. > Worse, the next patch conditionally re-introduces pm_runtime_put_sync() > on the probe function. > >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> >> --- >> drivers/watchdog/rti_wdt.c | 30 ++++++++---------------------- >> 1 file changed, 8 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >> index 8e1be7ba0103..163bdeb6929a 100644 >> --- a/drivers/watchdog/rti_wdt.c >> +++ b/drivers/watchdog/rti_wdt.c >> @@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device >> *pdev) >> if (wdt->freq < 32768) >> wdt->freq = wdt->freq * 9 / 10; >> - pm_runtime_enable(dev); >> - ret = pm_runtime_resume_and_get(dev); >> - if (ret < 0) { >> - pm_runtime_disable(&pdev->dev); >> - return dev_err_probe(dev, ret, "runtime pm failed\n"); >> - } >> + devm_pm_runtime_enable(dev); > > devm_pm_runtime_enable() returns an error code. I don't think ignoring it > is a good idea. > Oops, yes... >> + pm_runtime_get_noresume(dev); > > Is this functionally identical to pm_runtime_resume_and_get() ? > That would require further explanation. Why is it not necessary > to resume here ? include/linux/pm_runtime.h :: pm_runtime_resume_and_get - Bump up usage counter of a device and resume it. vs pm_runtime_get_noresume - Bump up runtime PM usage counter of a device. During probe, device is already active. Hence, there is really no need to call driver level runtime_resume() callback as there is really no context to resume from. Driver currently doesnt have runtime_pm calls which I intend to add as a later patch. I guess, its probably better to move this patch to same series. Also, missed a call to pm_runtime_put_noidle() in the error path. So for now I will respin 2/2 as standalone fix and repost along with runtime_pm support. Apologies, for the delayed response!
diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 8e1be7ba0103..163bdeb6929a 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device *pdev) if (wdt->freq < 32768) wdt->freq = wdt->freq * 9 / 10; - pm_runtime_enable(dev); - ret = pm_runtime_resume_and_get(dev); - if (ret < 0) { - pm_runtime_disable(&pdev->dev); - return dev_err_probe(dev, ret, "runtime pm failed\n"); - } + devm_pm_runtime_enable(dev); + pm_runtime_get_noresume(dev); platform_set_drvdata(pdev, wdt); @@ -260,7 +256,7 @@ static int rti_wdt_probe(struct platform_device *pdev) wdt->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(wdt->base)) { ret = PTR_ERR(wdt->base); - goto err_iomap; + return ret; } if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { @@ -286,7 +282,7 @@ static int rti_wdt_probe(struct platform_device *pdev) ret = rti_wdt_setup_hw_hb(wdd, wsize); if (ret) { dev_err(dev, "bad window size.\n"); - goto err_iomap; + return ret; } last_ping = heartbeat_ms - time_left_ms; @@ -301,7 +297,7 @@ static int rti_wdt_probe(struct platform_device *pdev) ret = of_address_to_resource(node, 0, &res); if (ret) { dev_err(dev, "No memory address assigned to the region.\n"); - goto err_iomap; + return ret; } /* @@ -312,15 +308,13 @@ static int rti_wdt_probe(struct platform_device *pdev) reserved_mem_size = resource_size(&res); if (reserved_mem_size < RESERVED_MEM_MIN_SIZE) { dev_err(dev, "The size of reserved memory is too small.\n"); - ret = -EINVAL; - goto err_iomap; + return -EINVAL; } vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB); if (!vaddr) { dev_err(dev, "Failed to map memory-region.\n"); - ret = -ENOMEM; - goto err_iomap; + return -ENOMEM; } if (vaddr[0] == PON_REASON_SOF_NUM && @@ -337,19 +331,13 @@ static int rti_wdt_probe(struct platform_device *pdev) ret = watchdog_register_device(wdd); if (ret) { dev_err(dev, "cannot register watchdog device\n"); - goto err_iomap; + return ret; } if (last_ping) watchdog_set_last_hw_keepalive(wdd, last_ping); return 0; - -err_iomap: - pm_runtime_put_sync(&pdev->dev); - pm_runtime_disable(&pdev->dev); - - return ret; } static void rti_wdt_remove(struct platform_device *pdev) @@ -357,8 +345,6 @@ static void rti_wdt_remove(struct platform_device *pdev) struct rti_wdt_device *wdt = platform_get_drvdata(pdev); watchdog_unregister_device(&wdt->wdd); - pm_runtime_put(&pdev->dev); - pm_runtime_disable(&pdev->dev); } static const struct of_device_id rti_wdt_of_match[] = {
Switch to devm_pm_runtime_enable() to simplify error handling in driver probe. Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> --- drivers/watchdog/rti_wdt.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)