Re: [PATCH 5/7] memory: pl172: simplify with devm_clk_get_enabled()
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Date: 2024-08-23 11:51:59
Also in:
linux-omap, lkml
On Fri, 23 Aug 2024 12:16:00 +0200 Krzysztof Kozlowski [off-list ref] wrote:
Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and make code simpler. Change to dev_err_probe() in handling clk_get_rate() error to make it even simpler. Signed-off-by: Krzysztof Kozlowski <redacted>
Follow up suggestion inline + I'd use dev_err_probe() one more time. Anyhow fine either way Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
quoted hunk ↗ jump to hunk
--- drivers/memory/pl172.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c index 390012401f64..db5fbee34077 100644 --- a/drivers/memory/pl172.c +++ b/drivers/memory/pl172.c@@ -216,28 +216,20 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id) if (!pl172) return -ENOMEM; - pl172->clk = devm_clk_get(dev, "mpmcclk"); + pl172->clk = devm_clk_get_enabled(dev, "mpmcclk"); if (IS_ERR(pl172->clk)) return dev_err_probe(dev, PTR_ERR(pl172->clk), "no mpmcclk provided clock\n"); - ret = clk_prepare_enable(pl172->clk); - if (ret) { - dev_err(dev, "unable to mpmcclk enable clock\n"); - return ret; - } - pl172->rate = clk_get_rate(pl172->clk) / MSEC_PER_SEC; - if (!pl172->rate) { - dev_err(dev, "unable to get mpmcclk clock rate\n"); - ret = -EINVAL; - goto err_clk_enable; - } + if (!pl172->rate) + return dev_err_probe(dev, -EINVAL, + "unable to get mpmcclk clock rate\n"); ret = amba_request_regions(adev, NULL);
Seeing as you are here an obvious follow up... Use a devm_add_action_or_reset() and a local cleanup callback or spin up a devm_amba_request_regions() and apply that here and probably in other similar locations (a quick look suggested this would be generally useful). Then can drop remove entirely which is always nice to see + return directly in the remaining error path.
if (ret) {
dev_err(dev, "unable to request AMBA regions\n");
- goto err_clk_enable;
+ return ret;I'd make this a return dev_err_probe() as well.
quoted hunk ↗ jump to hunk
} pl172->base = devm_ioremap(dev, adev->res.start,@@ -267,16 +259,11 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id) err_no_ioremap: amba_release_regions(adev); -err_clk_enable: - clk_disable_unprepare(pl172->clk); return ret; } static void pl172_remove(struct amba_device *adev) { - struct pl172_data *pl172 = amba_get_drvdata(adev); - - clk_disable_unprepare(pl172->clk); amba_release_regions(adev); }