| Message ID | 20240823-b4-cleanup-h-guard-v1-5-01668915bd55@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: simplify with devm() and guard() | expand |
On Fri, 23 Aug 2024 12:16:00 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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 <krzysztof.kozlowski@linaro.org> 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> > --- > 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. > } > > 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); > } > >
On 8/23/24 13:16, Krzysztof Kozlowski 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 <krzysztof.kozlowski@linaro.org> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> -- Best wishes, Vladimir
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); if (ret) { dev_err(dev, "unable to request AMBA regions\n"); - goto err_clk_enable; + return ret; } 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); }
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 <krzysztof.kozlowski@linaro.org> --- drivers/memory/pl172.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)