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

[Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>]

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