Re: [PATCH v2 2/2] i2c: qcom-geni: Simplify error handling in probe function

[Vladimir Zapolskiy <vz@mleia.com>]

On 12/12/24 15:54, Andi Shyti wrote:
> Avoid repeating the error handling pattern:
> 
>          geni_se_resources_off(&gi2c->se);
>          clk_disable_unprepare(gi2c->core_clk);
>          return;
> 
> Introduce a single 'goto' exit label for cleanup in case of
> errors. While there are currently two distinct exit points, there
> is no overlap in their handling, allowing both branches to
> coexist cleanly.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 32 ++++++++++++++++--------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 01db24188e29..88709b97f86d 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -867,14 +867,13 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   
>   	ret = geni_se_resources_on(&gi2c->se);
>   	if (ret) {
> -		clk_disable_unprepare(gi2c->core_clk);
> -		return dev_err_probe(dev, ret, "Error turning on resources\n");
> +		dev_err_probe(dev, ret, "Error turning on resources\n");
> +		goto err_clk;
>   	}
>   	proto = geni_se_read_proto(&gi2c->se);
>   	if (proto != GENI_SE_I2C) {
> -		geni_se_resources_off(&gi2c->se);
> -		clk_disable_unprepare(gi2c->core_clk);
> -		return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> +		dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> +		goto err_resources;

What's about setting ret = -ENXIO before bailing out?

>   	}
>   
>   	if (desc && desc->no_dma_support)
> @@ -886,11 +885,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   		/* FIFO is disabled, so we can only use GPI DMA */
>   		gi2c->gpi_mode = true;
>   		ret = setup_gpi_dma(gi2c);
> -		if (ret) {
> -			geni_se_resources_off(&gi2c->se);
> -			clk_disable_unprepare(gi2c->core_clk);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_resources;
>   
>   		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>   	} else {
> @@ -902,10 +898,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   			tx_depth = desc->tx_fifo_depth;
>   
>   		if (!tx_depth) {
> -			geni_se_resources_off(&gi2c->se);
> -			clk_disable_unprepare(gi2c->core_clk);
> -			return dev_err_probe(dev, -EINVAL,
> -					     "Invalid TX FIFO depth\n");
> +			dev_err_probe(dev, -EINVAL, "Invalid TX FIFO depth\n");
> +			goto err_resources;

Same comment as above.

>   		}
>   
>   		gi2c->tx_wm = tx_depth - 1;
> @@ -942,10 +936,18 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   
>   	dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
>   
> -	return 0;
> +	return ret;
> +
> +err_resources:
> +	geni_se_resources_off(&gi2c->se);
> +err_clk:
> +	clk_disable_unprepare(gi2c->core_clk);
> +
> +	return ret;
>   
>   err_dma:
>   	release_gpi_dma(gi2c);
> +
>   	return ret;
>   }
>   

--
Best wishes,
Vladimir