| Message ID | 20241212135416.244504-3-andi.shyti@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Qcom Geni exit path cleanups | expand |
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
> > 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? Oh yes, sorry, my bad... I snap changed this patch without using my brain. It was better in v1 :-) Thanks Vladimir, Andi
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; } 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; } 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; }
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(-)