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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

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;
>>>    }
>>