Message ID | 20230508205306.1474415-1-u.kleine-koenig@pengutronix.de |
---|---|
Headers | show |
Series | i2c: Convert to platform remove callback returning void | expand |
On Monday, May 8, 2023 9:52 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The .remove() callback for a platform driver returns an int which makes many > driver authors wrongly assume it's possible to do error handling by returning > an error code. However the value returned is (mostly) ignored and this > typically results in resource leaks. To improve here there is a quest to make the > remove callback return void. In the first step of this quest all drivers are > converted to .remove_new() which already returns void. > > Trivially convert this driver from always returning zero in the remove callback > to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Chris Pringle <chris.pringle@phabrix.com> Thanks, Chris
On 8.05.2023 22:52, Uwe Kleine-König wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > drivers/i2c/busses/i2c-qup.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c > index 2e153f2f71b6..6eef1dbd00de 100644 > --- a/drivers/i2c/busses/i2c-qup.c > +++ b/drivers/i2c/busses/i2c-qup.c > @@ -1904,7 +1904,7 @@ static int qup_i2c_probe(struct platform_device *pdev) > return ret; > } > > -static int qup_i2c_remove(struct platform_device *pdev) > +static void qup_i2c_remove(struct platform_device *pdev) > { > struct qup_i2c_dev *qup = platform_get_drvdata(pdev); > > @@ -1918,7 +1918,6 @@ static int qup_i2c_remove(struct platform_device *pdev) > i2c_del_adapter(&qup->adap); > pm_runtime_disable(qup->dev); > pm_runtime_set_suspended(qup->dev); > - return 0; > } > > #ifdef CONFIG_PM > @@ -1978,7 +1977,7 @@ MODULE_DEVICE_TABLE(of, qup_i2c_dt_match); > > static struct platform_driver qup_i2c_driver = { > .probe = qup_i2c_probe, > - .remove = qup_i2c_remove, > + .remove_new = qup_i2c_remove, > .driver = { > .name = "i2c_qup", > .pm = &qup_i2c_qup_pm_ops,
On 08/05/2023 22:51, Uwe Kleine-König wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 08/05/2023 22:52, Uwe Kleine-König wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/i2c/busses/i2c-mt65xx.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c > index a43c4d77739a..7ca3f2221ba6 100644 > --- a/drivers/i2c/busses/i2c-mt65xx.c > +++ b/drivers/i2c/busses/i2c-mt65xx.c > @@ -1505,15 +1505,13 @@ static int mtk_i2c_probe(struct platform_device *pdev) > return ret; > } > > -static int mtk_i2c_remove(struct platform_device *pdev) > +static void mtk_i2c_remove(struct platform_device *pdev) > { > struct mtk_i2c *i2c = platform_get_drvdata(pdev); > > i2c_del_adapter(&i2c->adap); > > clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks); > - > - return 0; > } > > #ifdef CONFIG_PM_SLEEP > @@ -1555,7 +1553,7 @@ static const struct dev_pm_ops mtk_i2c_pm = { > > static struct platform_driver mtk_i2c_driver = { > .probe = mtk_i2c_probe, > - .remove = mtk_i2c_remove, > + .remove_new = mtk_i2c_remove, > .driver = { > .name = I2C_DRV_NAME, > .pm = &mtk_i2c_pm,
On 08/05/2023 22:52, Uwe Kleine-König wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/i2c/busses/i2c-mt7621.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c > index 20eda5738ac4..f9c294e2bd3c 100644 > --- a/drivers/i2c/busses/i2c-mt7621.c > +++ b/drivers/i2c/busses/i2c-mt7621.c > @@ -332,19 +332,17 @@ static int mtk_i2c_probe(struct platform_device *pdev) > return ret; > } > > -static int mtk_i2c_remove(struct platform_device *pdev) > +static void mtk_i2c_remove(struct platform_device *pdev) > { > struct mtk_i2c *i2c = platform_get_drvdata(pdev); > > clk_disable_unprepare(i2c->clk); > i2c_del_adapter(&i2c->adap); > - > - return 0; > } > > static struct platform_driver mtk_i2c_driver = { > .probe = mtk_i2c_probe, > - .remove = mtk_i2c_remove, > + .remove_new = mtk_i2c_remove, > .driver = { > .name = "i2c-mt7621", > .of_match_table = i2c_mtk_dt_ids,
Dne ponedeljek, 08. maj 2023 ob 22:52:48 CEST je Uwe Kleine-König napisal(a): > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej
On Mon, May 8, 2023 at 10:53 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > If pm_runtime_get() fails in .remove() the driver used to return the > error to the driver core. The only effect of this (compared to returning > zero) is a generic warning that the error value is ignored. (The device > is unbound then anyhow.) > > Emit a better warning and return zero to suppress the generic (and > little helpful) message. Also disable runtime PM in the error case. > > This prepares changing platform device remove callbacks to return void. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/i2c/busses/i2c-davinci.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c > index 9750310f2c96..7ba7e6cbcc97 100644 > --- a/drivers/i2c/busses/i2c-davinci.c > +++ b/drivers/i2c/busses/i2c-davinci.c > @@ -894,11 +894,11 @@ static int davinci_i2c_remove(struct platform_device *pdev) > > i2c_del_adapter(&dev->adapter); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > if (ret < 0) > - return ret; > - > - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0); > + dev_err(&pdev->dev, "Failed to resume device\n"); > + else > + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0); > > pm_runtime_dont_use_autosuspend(dev->dev); > pm_runtime_put_sync(dev->dev); > -- > 2.39.2 > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
On Mon, May 8, 2023 at 10:53 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/i2c/busses/i2c-davinci.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c > index 7ba7e6cbcc97..b77f9288c0de 100644 > --- a/drivers/i2c/busses/i2c-davinci.c > +++ b/drivers/i2c/busses/i2c-davinci.c > @@ -885,7 +885,7 @@ static int davinci_i2c_probe(struct platform_device *pdev) > return r; > } > > -static int davinci_i2c_remove(struct platform_device *pdev) > +static void davinci_i2c_remove(struct platform_device *pdev) > { > struct davinci_i2c_dev *dev = platform_get_drvdata(pdev); > int ret; > @@ -903,8 +903,6 @@ static int davinci_i2c_remove(struct platform_device *pdev) > pm_runtime_dont_use_autosuspend(dev->dev); > pm_runtime_put_sync(dev->dev); > pm_runtime_disable(dev->dev); > - > - return 0; > } > > #ifdef CONFIG_PM > @@ -945,7 +943,7 @@ MODULE_ALIAS("platform:i2c_davinci"); > > static struct platform_driver davinci_i2c_driver = { > .probe = davinci_i2c_probe, > - .remove = davinci_i2c_remove, > + .remove_new = davinci_i2c_remove, > .driver = { > .name = "i2c_davinci", > .pm = davinci_i2c_pm_ops, > -- > 2.39.2 > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Acked-by: Jochen Friedrich <jochen@scram.de> Am 08.05.2023 um 22:51 schrieb Uwe Kleine-König: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is (mostly) ignored > and this typically results in resource leaks. To improve here there is a > quest to make the remove callback return void. In the first step of this > quest all drivers are converted to .remove_new() which already returns > void. > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/i2c/busses/i2c-cpm.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c > index 24d584a1c9a7..732daf6a932b 100644 > --- a/drivers/i2c/busses/i2c-cpm.c > +++ b/drivers/i2c/busses/i2c-cpm.c > @@ -676,7 +676,7 @@ static int cpm_i2c_probe(struct platform_device *ofdev) > return result; > } > > -static int cpm_i2c_remove(struct platform_device *ofdev) > +static void cpm_i2c_remove(struct platform_device *ofdev) > { > struct cpm_i2c *cpm = platform_get_drvdata(ofdev); > > @@ -685,8 +685,6 @@ static int cpm_i2c_remove(struct platform_device *ofdev) > cpm_i2c_shutdown(cpm); > > kfree(cpm); > - > - return 0; > } > > static const struct of_device_id cpm_i2c_match[] = { > @@ -703,7 +701,7 @@ MODULE_DEVICE_TABLE(of, cpm_i2c_match); > > static struct platform_driver cpm_i2c_driver = { > .probe = cpm_i2c_probe, > - .remove = cpm_i2c_remove, > + .remove_new = cpm_i2c_remove, > .driver = { > .name = "fsl-i2c-cpm", > .of_match_table = cpm_i2c_match,
> Subject: Re: [PATCH 00/89] i2c: Convert to platform remove callback > returning void > > [Dropped Phil Edworthy from recipents as his email address has problems] Phil no longer works with Renesas. Adding Fabrizio who is taking care of RZ/V2M I2C driver. Cheers, Biju
> I wonder how this series will go in. My expectation was that Wolfram > picks up the whole series via his tree?! Will do. I am currently super-busy, though.
On Thu, Jun 01, 2023 at 03:54:50PM +0200, Wolfram Sang wrote: > > > I wonder how this series will go in. My expectation was that Wolfram > > picks up the whole series via his tree?! > > Will do. I am currently super-busy, though. Whole series applied to for-next. I squashed all the commits into one. These are mostly simple changes which we won't revert anyhow, but fix incrementally if we ever find an issue.