Re: [Linux-stm32] [PATCH v4 13/17] spi: stm32: Simplify clock handling with devm_clk_get_enabled()

[Amelie Delaunay <amelie.delaunay@foss.st.com>]

Hi,

On 3/18/26 03:40, Pei Xiao wrote:
> Replace devm_clk_get() followed by clk_prepare_enable() with
> devm_clk_get_enabled() for the clock. This removes the need for
> explicit clock enable and disable calls, as the managed API automatically
> handles clock disabling on device removal or probe failure.
> 
> Remove the now-unnecessary clk_disable_unprepare() calls from the probe
> error paths and the remove callback. Also simplify error handling by
> using dev_err_probe().
> 
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>   drivers/spi/spi-stm32.c | 61 ++++++++++++-----------------------------
>   1 file changed, 18 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c
> index 8a7f5a10d4af..ee5092bc5956 100644
> --- a/drivers/spi/spi-stm32.c
> +++ b/drivers/spi/spi-stm32.c
> @@ -2360,25 +2360,21 @@ static int stm32_spi_probe(struct platform_device *pdev)
>   	int ret;
>   
>   	cfg = of_device_get_match_data(&pdev->dev);
> -	if (!cfg) {
> -		dev_err(&pdev->dev, "Failed to get match data for platform\n");
> -		return -ENODEV;
> -	}
> +	if (!cfg)
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "Failed to get match data for platform\n");
>   
>   	device_mode = of_property_read_bool(np, "spi-slave");
> -	if (!cfg->has_device_mode && device_mode) {
> -		dev_err(&pdev->dev, "spi-slave not supported\n");
> -		return -EPERM;
> -	}
> +	if (!cfg->has_device_mode && device_mode)
> +		return dev_err_probe(&pdev->dev, -EPERM, "spi-slave not supported\n");
>   
>   	if (device_mode)
>   		ctrl = devm_spi_alloc_target(&pdev->dev, sizeof(struct stm32_spi));
>   	else
>   		ctrl = devm_spi_alloc_host(&pdev->dev, sizeof(struct stm32_spi));
> -	if (!ctrl) {
> -		dev_err(&pdev->dev, "spi controller allocation failed\n");
> -		return -ENOMEM;
> -	}
> +	if (!ctrl)
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +				     "spi controller allocation failed\n");

it fits on single line
+		return dev_err_probe(&pdev->dev, -ENOMEM, "spi controller allocation 
failed\n");

>   	platform_set_drvdata(pdev, ctrl);
>   
>   	spi = spi_controller_get_devdata(ctrl);
> @@ -2409,32 +2405,19 @@ static int stm32_spi_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	spi->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(spi->clk)) {
> -		ret = PTR_ERR(spi->clk);
> -		dev_err(&pdev->dev, "clk get failed: %d\n", ret);
> -		return ret;
> -	}
> +	spi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(spi->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(spi->clk), "clk enabled failed\n");
>   
> -	ret = clk_prepare_enable(spi->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "clk enable failed: %d\n", ret);
> -		return ret;
> -	}
>   	spi->clk_rate = clk_get_rate(spi->clk);
> -	if (!spi->clk_rate) {
> -		dev_err(&pdev->dev, "clk rate = 0\n");
> -		ret = -EINVAL;
> -		goto err_clk_disable;
> -	}
> +	if (!spi->clk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "clk rate = 0\n");
>   
>   	rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
>   	if (rst) {
> -		if (IS_ERR(rst)) {
> +		if (IS_ERR(rst))
>   			ret = dev_err_probe(&pdev->dev, PTR_ERR(rst),
>   					    "failed to get reset\n");
> -			goto err_clk_disable;
> -		}

Here you change the behavior: it should return dev_err_probe(...) (on 
single line too)
+		if (IS_ERR(rst))
+   			return dev_err_probe(&pdev->dev, PTR_ERR(rst), "failed to get 
reset\n");

>   
>   		reset_control_assert(rst);
>   		udelay(2);
> @@ -2461,11 +2444,9 @@ static int stm32_spi_probe(struct platform_device *pdev)
>   	dev_dbg(spi->dev, "one message max size %d\n", spi->t_size_max);
>   
>   	ret = spi->cfg->config(spi);
> -	if (ret) {
> -		dev_err(&pdev->dev, "controller configuration failed: %d\n",
> -			ret);
> -		goto err_clk_disable;
> -	}
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "controller configuration failed: %d\n", ret);

it fits on single line too here.

>   
>   	ctrl->auto_runtime_pm = true;
>   	ctrl->bus_num = pdev->id;
> @@ -2490,8 +2471,7 @@ static int stm32_spi_probe(struct platform_device *pdev)
>   			dev_info(&pdev->dev, "tx dma disabled\n");
>   			spi->dma_tx = NULL;
>   		} else {
> -			dev_err_probe(&pdev->dev, ret, "failed to request tx dma channel\n");
> -			goto err_clk_disable;
> +			return dev_err_probe(&pdev->dev, ret, "failed to request tx dma channel\n");
>   		}
>   	} else {
>   		ctrl->dma_tx = spi->dma_tx;
> @@ -2579,8 +2559,6 @@ static int stm32_spi_probe(struct platform_device *pdev)
>   err_dma_tx_release:
>   	if (spi->dma_tx)
>   		dma_release_channel(spi->dma_tx);
> -err_clk_disable:
> -	clk_disable_unprepare(spi->clk);
>   
>   	return ret;
>   }
> @@ -2610,9 +2588,6 @@ static void stm32_spi_remove(struct platform_device *pdev)
>   		gen_pool_free(spi->sram_pool, (unsigned long)spi->sram_rx_buf,
>   			      spi->sram_rx_buf_size);
>   
> -	clk_disable_unprepare(spi->clk);
> -
> -
>   	pinctrl_pm_select_sleep_state(&pdev->dev);
>   }
>