| Message ID | 20230309094704.2568531-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: mt65xx: Convert to platform remove callback returning void | expand |
Il 09/03/23 10:47, Uwe Kleine-König ha scritto: > Returning an error code in a platform driver's remove function is wrong > most of the time and there is an effort to make the callback return > void. To prepare this rework the function not to exit early. > > There wasn't a real problem because if pm runtime resume failed the only > step missing was pm_runtime_disable() which isn't an issue. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/spi/spi-mt65xx.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c > index 9eab6c20dbc5..b1cf7bbb2c08 100644 > --- a/drivers/spi/spi-mt65xx.c > +++ b/drivers/spi/spi-mt65xx.c > @@ -1275,15 +1275,20 @@ static int mtk_spi_remove(struct platform_device *pdev) > struct mtk_spi *mdata = spi_master_get_devdata(master); > int ret; > > - ret = pm_runtime_resume_and_get(&pdev->dev); > - if (ret < 0) > - return ret; > - > - mtk_spi_reset(mdata); > + ret = pm_runtime_get_sync(&pdev->dev); > + /* > + * If pm runtime resume failed, clks are disabled and unprepared. So > + * don't access the hardware and skip clk unpreparing. > + */ This comment is counter-intuitive: you're saying in words "if this failed" but in code "if this didn't fail". Please, either invert the branch so that it looks either like /* if negative */ if (negative) warn else /* positive */ or like if (ret >= 0) { do_things() } else { /* if it failed ... */ warn } Functionally, though, looks good to me. Cheers, Angelo
diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c index 9eab6c20dbc5..b1cf7bbb2c08 100644 --- a/drivers/spi/spi-mt65xx.c +++ b/drivers/spi/spi-mt65xx.c @@ -1275,15 +1275,20 @@ static int mtk_spi_remove(struct platform_device *pdev) struct mtk_spi *mdata = spi_master_get_devdata(master); int ret; - ret = pm_runtime_resume_and_get(&pdev->dev); - if (ret < 0) - return ret; - - mtk_spi_reset(mdata); + ret = pm_runtime_get_sync(&pdev->dev); + /* + * If pm runtime resume failed, clks are disabled and unprepared. So + * don't access the hardware and skip clk unpreparing. + */ + if (ret >= 0) { + mtk_spi_reset(mdata); - if (mdata->dev_comp->no_need_unprepare) { - clk_unprepare(mdata->spi_clk); - clk_unprepare(mdata->spi_hclk); + if (mdata->dev_comp->no_need_unprepare) { + clk_unprepare(mdata->spi_clk); + clk_unprepare(mdata->spi_hclk); + } + } else { + dev_warn(&pdev->dev, "Failed to resume hardware (%pe)\n", ERR_PTR(ret)); } pm_runtime_put_noidle(&pdev->dev);
Returning an error code in a platform driver's remove function is wrong most of the time and there is an effort to make the callback return void. To prepare this rework the function not to exit early. There wasn't a real problem because if pm runtime resume failed the only step missing was pm_runtime_disable() which isn't an issue. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/spi/spi-mt65xx.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)