[PATCH 0/2] Qcom Geni exit path cleanups

[PATCH 0/2] Qcom Geni exit path cleanups

[Andi Shyti]

Hi,

I am submitting two trivial cleanups in this series. The first 
replaces all instances of dev_err with dev_err_probe throughout 
the probe function for consistency. The second improves the error
exit path by introducing a single 'goto' label for better 
maintainability.

Thank you,  
Andi

Andi Shyti (2):
  i2c: qcom-geni: Use dev_err_probe in the probe function
  i2c: qcom-geni: Simplify error handling in probe function

 drivers/i2c/busses/i2c-qcom-geni.c | 53 ++++++++++++++----------------
 1 file changed, 25 insertions(+), 28 deletions(-)

-- 
2.45.2

[PATCH 1/2] i2c: qcom-geni: Use dev_err_probe in the probe function

[Andi Shyti]

Replace classical dev_err with dev_err_probe in the probe
function for better error reporting. Also, use dev_err_probe in
cases where the error number is clear (e.g., -EIO or -EINVAL) to
maintain consistency.

Additionally, remove redundant logging to simplify the code.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 33 +++++++++++++-----------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7a22e1f46e60..01db24188e29 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -823,11 +823,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return gi2c->irq;
 
 	ret = geni_i2c_clk_map_idx(gi2c);
-	if (ret) {
-		dev_err(dev, "Invalid clk frequency %d Hz: %d\n",
-			gi2c->clk_freq_out, ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Invalid clk frequency %d Hz\n",
+				     gi2c->clk_freq_out);
 
 	gi2c->adap.algo = &geni_i2c_algo;
 	init_completion(&gi2c->done);
@@ -837,11 +835,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	/* Keep interrupts disabled initially to allow for low-power modes */
 	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
 			       dev_name(dev), gi2c);
-	if (ret) {
-		dev_err(dev, "Request_irq failed:%d: err:%d\n",
-			gi2c->irq, ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Request_irq failed: %d\n", gi2c->irq);
+
 	i2c_set_adapdata(&gi2c->adap, gi2c);
 	gi2c->adap.dev.parent = dev;
 	gi2c->adap.dev.of_node = dev->of_node;
@@ -870,16 +867,14 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	ret = geni_se_resources_on(&gi2c->se);
 	if (ret) {
-		dev_err(dev, "Error turning on resources %d\n", ret);
 		clk_disable_unprepare(gi2c->core_clk);
-		return ret;
+		return dev_err_probe(dev, ret, "Error turning on resources\n");
 	}
 	proto = geni_se_read_proto(&gi2c->se);
 	if (proto != GENI_SE_I2C) {
-		dev_err(dev, "Invalid proto %d\n", proto);
 		geni_se_resources_off(&gi2c->se);
 		clk_disable_unprepare(gi2c->core_clk);
-		return -ENXIO;
+		return dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
 	}
 
 	if (desc && desc->no_dma_support)
@@ -894,7 +889,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		if (ret) {
 			geni_se_resources_off(&gi2c->se);
 			clk_disable_unprepare(gi2c->core_clk);
-			return dev_err_probe(dev, ret, "Failed to setup GPI DMA mode\n");
+			return ret;
 		}
 
 		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
@@ -907,10 +902,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
 			tx_depth = desc->tx_fifo_depth;
 
 		if (!tx_depth) {
-			dev_err(dev, "Invalid TX FIFO depth\n");
 			geni_se_resources_off(&gi2c->se);
 			clk_disable_unprepare(gi2c->core_clk);
-			return -EINVAL;
+			return dev_err_probe(dev, -EINVAL,
+					     "Invalid TX FIFO depth\n");
 		}
 
 		gi2c->tx_wm = tx_depth - 1;
@@ -924,7 +919,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	clk_disable_unprepare(gi2c->core_clk);
 	ret = geni_se_resources_off(&gi2c->se);
 	if (ret) {
-		dev_err(dev, "Error turning off resources %d\n", ret);
+		dev_err_probe(dev, ret, "Error turning off resources\n");
 		goto err_dma;
 	}
 
@@ -940,7 +935,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	ret = i2c_add_adapter(&gi2c->adap);
 	if (ret) {
-		dev_err(dev, "Error adding i2c adapter %d\n", ret);
+		dev_err_probe(dev, ret, "Error adding i2c adapter\n");
 		pm_runtime_disable(gi2c->se.dev);
 		goto err_dma;
 	}
-- 
2.45.2

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

[Andi Shyti]

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 | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 01db24188e29..3fc85595a4aa 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_off;
 	}
 
 	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_off;
 
 		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_off;
 		}
 
 		gi2c->tx_wm = tx_depth - 1;
@@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_off:
+	geni_se_resources_off(&gi2c->se);
+err_clk:
+	clk_disable_unprepare(gi2c->core_clk);
+
+	return ret;
+
 err_dma:
 	release_gpi_dma(gi2c);
+
 	return ret;
 }
 
-- 
2.45.2

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

[Mukesh Kumar Savaliya]

Thanks Andi for this change !

On 12/11/2024 4:40 AM, 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 | 30 ++++++++++++++++--------------
>   1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 01db24188e29..3fc85595a4aa 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);
Suggestive comment, can we make this second patch as first patch ? So 
that we can have both above lines reduced in this patch.
> +		goto err_off;
>   	}
>   
>   	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_off;
>   
>   		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_off;
>   		}
>   
>   		gi2c->tx_wm = tx_depth - 1;
> @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   
>   	return 0;
return ret here ? yes, we need to initialize ret = 0.
>   
> +err_off:
can we rename as 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;
>   }
>   

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

[Andi Shyti]

Hi Mukesh,

On Wed, Dec 11, 2024 at 09:26:53AM +0530, Mukesh Kumar Savaliya wrote:
> Thanks Andi for this change !

Thanks for looking into it.

...

> > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >   	return 0;
> return ret here ? yes, we need to initialize ret = 0.

here? It's returning '0', as it was before. I'm failing to see
where 'ret' is used uninitialized. What am I missing?

> > +err_off:
> can we rename as err_resources ?

yes, it's better, as meaning it's more aligned with the other
labels.

Thanks,
Andi

> > +	geni_se_resources_off(&gi2c->se);
> > +err_clk:
> > +	clk_disable_unprepare(gi2c->core_clk);
> > +
> > +	return ret;
> > +
> >   err_dma:
> >   	release_gpi_dma(gi2c);
> > +
> >   	return ret;
> >   }
> 

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

[Mukesh Kumar Savaliya]

Thanks Andi !

On 12/11/2024 12:26 PM, Andi Shyti wrote:
> Hi Mukesh,
> 
> On Wed, Dec 11, 2024 at 09:26:53AM +0530, Mukesh Kumar Savaliya wrote:
>> Thanks Andi for this change !
> 
> Thanks for looking into it.
> 
> ...
> 
>>> @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>>    	return 0;
>> return ret here ? yes, we need to initialize ret = 0.
> 
> here? It's returning '0', as it was before. I'm failing to see
> where 'ret' is used uninitialized. What am I missing?
> 
My point is - Except this place, rest of the places we are returning ret 
OR standard error code. If we return as ret with initializing 0 at the 
start of probe function, it would look good. As such No strict requirement.
>>> +err_off:
>> can we rename as err_resources ?
> 
> yes, it's better, as meaning it's more aligned with the other
> labels.
> 
> Thanks,
> Andi
> 
>>> +	geni_se_resources_off(&gi2c->se);
>>> +err_clk:
>>> +	clk_disable_unprepare(gi2c->core_clk);
>>> +
>>> +	return ret;
>>> +
>>>    err_dma:
>>>    	release_gpi_dma(gi2c);
>>> +
>>>    	return ret;
>>>    }
>>

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

[Andi Shyti]

> > > > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
> > > >    	return 0;
> > > return ret here ? yes, we need to initialize ret = 0.
> > 
> > here? It's returning '0', as it was before. I'm failing to see
> > where 'ret' is used uninitialized. What am I missing?
> > 
> My point is - Except this place, rest of the places we are returning ret OR
> standard error code. If we return as ret with initializing 0 at the start of
> probe function, it would look good. As such No strict requirement.

Ah, I see. Sure, I can do a "return ret" in my v2.

Thanks,
Andi

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

[Andi Shyti]

Hi Mukesh,

> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 01db24188e29..3fc85595a4aa 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);
> Suggestive comment, can we make this second patch as first patch ? So that
> we can have both above lines reduced in this patch.

I'm sorry, I missed this comment. I tried to swap it and there is
not much reduction in line changes[*].

The reason I chose this order is to avoid changing the same line
on both the patches, like here[**].

If it's not binding for you, I would keep the ordering.

Thanks again a lot for looking into this,
Andi

[*] https://paste.debian.net/1339486/
[**] https://paste.debian.net/1339488/

> > +		goto err_off;
> >   	}
> >   	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_off;
> >   		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_off;
> >   		}
> >   		gi2c->tx_wm = tx_depth - 1;
> > @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >   	return 0;
> return ret here ? yes, we need to initialize ret = 0.
> > +err_off:
> can we rename as 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;
> >   }
> 

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

[Mukesh Kumar Savaliya]

Thanks Andi !

On 12/12/2024 7:15 PM, Andi Shyti wrote:
> Hi Mukesh,
> 
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>>> index 01db24188e29..3fc85595a4aa 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);
>> Suggestive comment, can we make this second patch as first patch ? So that
>> we can have both above lines reduced in this patch.
> 
> I'm sorry, I missed this comment. I tried to swap it and there is
> not much reduction in line changes[*].
> 
> The reason I chose this order is to avoid changing the same line
> on both the patches, like here[**].
> 
> If it's not binding for you, I would keep the ordering.
> 
Sure, it's perfectly fine to me. No worries.
> Thanks again a lot for looking into this,
> Andi
> 
> [*] https://paste.debian.net/1339486/
> [**] https://paste.debian.net/1339488/
> 
>>> +		goto err_off;
>>>    	}
>>>    	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_off;
>>>    		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_off;
>>>    		}
>>>    		gi2c->tx_wm = tx_depth - 1;
>>> @@ -944,8 +938,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>>    	return 0;
>> return ret here ? yes, we need to initialize ret = 0.
>>> +err_off:
>> can we rename as 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;
>>>    }
>>