Re: [PATCH 3/4] soc: mediatek: pwrap: Use devm_clk_get_[optional_]enabled()

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 29/06/24 11:19, Christophe JAILLET ha scritto:
> Use devm_clk_get_enabled() and devm_clk_get_optional_enabled() to simplify
> the code and to make sure that clk_disable_unprepare() is called if the
> driver is unloaded.
> 
> Fixes: 55924157da8c ("soc: mediatek: pwrap: add support for sys & tmr clocks")
> Fixes: 1f022d84bd19 ("soc: mediatek: Add PMIC wrapper for MT8135 and MT8173 SoCs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested-only
> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++--------------------
>   1 file changed, 25 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index d57553486383..6981d6a1ab93 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1366,10 +1366,6 @@ struct pmic_wrapper {
>   	struct regmap *regmap;
>   	const struct pmic_wrapper_type *master;
>   	const struct pwrap_slv_type *slave;
> -	struct clk *clk_spi;
> -	struct clk *clk_wrap;
> -	struct clk *clk_sys;
> -	struct clk *clk_tmr;
>   	struct reset_control *rstc;
>   
>   	struct reset_control *rstc_bridge;
> @@ -2471,6 +2467,7 @@ static int pwrap_probe(struct platform_device *pdev)
>   {
>   	int ret, irq;
>   	u32 mask_done;
> +	struct clk *clk;
>   	struct pmic_wrapper *wrp;
>   	struct device_node *np = pdev->dev.of_node;
>   	const struct of_device_id *of_slave_id = NULL;
> @@ -2521,50 +2518,34 @@ static int pwrap_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	wrp->clk_spi = devm_clk_get(wrp->dev, "spi");
> -	if (IS_ERR(wrp->clk_spi)) {
> +	clk = devm_clk_get_enabled(wrp->dev, "spi");

Uhm... in this case, it might be worth using devm_clk_bulk_get_all_enable()
instead... as anyway we're never turning off those clocks during operation.

I checked the devicetrees and the clocks are ordered the right way, but then
we can also consider the fact that the bindings are enforcing the clock order
so that's.. golden.

Practically, by using devm_clk_bulk_get_all_enable(), you're removing even
more lines from this driver, as those four clocks (and four times error checking)
will be reduced to just one call.....!

Thanks for cleaning up this driver, btw!

Cheers,
Angelo


> +	if (IS_ERR(clk)) {
>   		dev_dbg(wrp->dev, "failed to get clock: %ld\n",
> -			PTR_ERR(wrp->clk_spi));
> -		return PTR_ERR(wrp->clk_spi);
> +			PTR_ERR(clk));
> +		return PTR_ERR(clk);
>   	}
>   
> -	wrp->clk_wrap = devm_clk_get(wrp->dev, "wrap");
> -	if (IS_ERR(wrp->clk_wrap)) {
> +	clk = devm_clk_get_enabled(wrp->dev, "wrap");
> +	if (IS_ERR(clk)) {
>   		dev_dbg(wrp->dev, "failed to get clock: %ld\n",
> -			PTR_ERR(wrp->clk_wrap));
> -		return PTR_ERR(wrp->clk_wrap);
> +			PTR_ERR(clk));
> +		return PTR_ERR(clk);
>   	}
>   
> -	wrp->clk_sys = devm_clk_get_optional(wrp->dev, "sys");
> -	if (IS_ERR(wrp->clk_sys)) {
> -		return dev_err_probe(wrp->dev, PTR_ERR(wrp->clk_sys),
> +	clk = devm_clk_get_optional_enabled(wrp->dev, "sys");
> +	if (IS_ERR(clk)) {
> +		return dev_err_probe(wrp->dev, PTR_ERR(clk),
>   				     "failed to get clock: %pe\n",
> -				     wrp->clk_sys);
> +				     clk);
>   	}
>   
> -	wrp->clk_tmr = devm_clk_get_optional(wrp->dev, "tmr");
> -	if (IS_ERR(wrp->clk_tmr)) {
> -		return dev_err_probe(wrp->dev, PTR_ERR(wrp->clk_tmr),
> +	clk = devm_clk_get_optional_enabled(wrp->dev, "tmr");
> +	if (IS_ERR(clk)) {
> +		return dev_err_probe(wrp->dev, PTR_ERR(clk),
>   				     "failed to get clock: %pe\n",
> -				     wrp->clk_tmr);
> +				     clk);
>   	}
>   
> -	ret = clk_prepare_enable(wrp->clk_spi);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(wrp->clk_wrap);
> -	if (ret)
> -		goto err_out1;
> -
> -	ret = clk_prepare_enable(wrp->clk_sys);
> -	if (ret)
> -		goto err_out2;
> -
> -	ret = clk_prepare_enable(wrp->clk_tmr);
> -	if (ret)
> -		goto err_out3;
> -
>   	/* Enable internal dynamic clock */
>   	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_DCM)) {
>   		pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> @@ -2579,7 +2560,7 @@ static int pwrap_probe(struct platform_device *pdev)
>   		ret = pwrap_init(wrp);
>   		if (ret) {
>   			dev_dbg(wrp->dev, "init failed with %d\n", ret);
> -			goto err_out4;
> +			return ret;
>   		}
>   	}
>   
> @@ -2592,8 +2573,7 @@ static int pwrap_probe(struct platform_device *pdev)
>   
>   	if (!(pwrap_readl(wrp, PWRAP_WACS2_RDATA) & mask_done)) {
>   		dev_dbg(wrp->dev, "initialization isn't finished\n");
> -		ret = -ENODEV;
> -		goto err_out4;
> +		return -ENODEV;
>   	}
>   
>   	/* Initialize watchdog, may not be done by the bootloader */
> @@ -2622,42 +2602,27 @@ static int pwrap_probe(struct platform_device *pdev)
>   		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>   
>   	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		ret = irq;
> -		goto err_out2;
> -	}
> +	if (irq < 0)
> +		return irq;
>   
>   	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
>   			       IRQF_TRIGGER_HIGH,
>   			       "mt-pmic-pwrap", wrp);
>   	if (ret)
> -		goto err_out4;
> +		return ret;
>   
>   	wrp->regmap = devm_regmap_init(wrp->dev, NULL, wrp, wrp->slave->regops->regmap);
> -	if (IS_ERR(wrp->regmap)) {
> -		ret = PTR_ERR(wrp->regmap);
> -		goto err_out2;
> -	}
> +	if (IS_ERR(wrp->regmap))
> +		return PTR_ERR(wrp->regmap);
>   
>   	ret = of_platform_populate(np, NULL, NULL, wrp->dev);
>   	if (ret) {
>   		dev_dbg(wrp->dev, "failed to create child devices at %pOF\n",
>   				np);
> -		goto err_out4;
> +		return ret;
>   	}
>   
>   	return 0;
> -
> -err_out4:
> -	clk_disable_unprepare(wrp->clk_tmr);
> -err_out3:
> -	clk_disable_unprepare(wrp->clk_sys);
> -err_out2:
> -	clk_disable_unprepare(wrp->clk_wrap);
> -err_out1:
> -	clk_disable_unprepare(wrp->clk_spi);
> -
> -	return ret;
>   }
>   
>   static struct platform_driver pwrap_drv = {