| Message ID | 20220610211257.102071-4-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] mmc: dw_mmc: exynos: Obviously always return success in remove callback | expand |
On 11/06/22 00:12, Uwe Kleine-König wrote: > sdhci_pltfm_unregister() returns 0 unconditionally and returning an > error in a platform remove callback isn't very sensible. (The only > effect of the latter is that the device core emits a generic warning and > then removes the device anyhow.) > > So return 0 unconditionally to make it obvious there is no error > forwarded to the upper layers. > > This is a preparation for making platform remove callbacks return void. This preparation seems a bit unnatural. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 757801dfc308..3997cad1f793 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -1733,7 +1733,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > > static int sdhci_arasan_remove(struct platform_device *pdev) > { > - int ret; > struct sdhci_host *host = platform_get_drvdata(pdev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > @@ -1747,11 +1746,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev) > > sdhci_arasan_unregister_sdclk(&pdev->dev); > > - ret = sdhci_pltfm_unregister(pdev); > + sdhci_pltfm_unregister(pdev); > > clk_disable_unprepare(clk_ahb); > > - return ret; > + return 0; > } > > static struct platform_driver sdhci_arasan_driver = {
On Mon, Jun 13, 2022 at 10:13:16AM +0300, Adrian Hunter wrote: > On 11/06/22 00:12, Uwe Kleine-König wrote: > > sdhci_pltfm_unregister() returns 0 unconditionally and returning an > > error in a platform remove callback isn't very sensible. (The only > > effect of the latter is that the device core emits a generic warning and > > then removes the device anyhow.) > > > > So return 0 unconditionally to make it obvious there is no error > > forwarded to the upper layers. > > > > This is a preparation for making platform remove callbacks return void. > > This preparation seems a bit unnatural. IMHO it's not. I have to adapt all ~4800 platform drivers together in a single commit to change the prototype of the return callback. Now assume you want to review that commit and make sure there are no relevant changes in behaviour. Without the preparation in the commit under discussion, the change to sdhci-of-arasan.c would look as follows: diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -1733,7 +1733,4 @@ static int sdhci_arasan_probe(struct platform_device *pdev) static int sdhci_arasan_remove(struct platform_device *pdev) { - int ret; struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); @@ -1747,11 +1746,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev) sdhci_arasan_unregister_sdclk(&pdev->dev); - ret = sdhci_pltfm_unregister(pdev); + sdhci_pltfm_unregister(pdev); clk_disable_unprepare(clk_ahb); - - return ret; } static struct platform_driver sdhci_arasan_driver = { So you have to look up then what sdhci_pltfm_unregister() does and if it's ok to ignore the return value. Should I mention that in the commit log? What about the other drivers? If however the change to the sdhci-arasan driver is only diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 3997cad1f793..84c949bd99c8 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -1749,8 +1749,6 @@ static int sdhci_arasan_remove(struct platform_device *pdev) sdhci_pltfm_unregister(pdev); clk_disable_unprepare(clk_ahb); - - return 0; } static struct platform_driver sdhci_arasan_driver = { , it's trivial to see that there is no relevant driver specific change. Best regards Uwe
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 757801dfc308..3997cad1f793 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -1733,7 +1733,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev) static int sdhci_arasan_remove(struct platform_device *pdev) { - int ret; struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); @@ -1747,11 +1746,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev) sdhci_arasan_unregister_sdclk(&pdev->dev); - ret = sdhci_pltfm_unregister(pdev); + sdhci_pltfm_unregister(pdev); clk_disable_unprepare(clk_ahb); - return ret; + return 0; } static struct platform_driver sdhci_arasan_driver = {
sdhci_pltfm_unregister() returns 0 unconditionally and returning an error in a platform remove callback isn't very sensible. (The only effect of the latter is that the device core emits a generic warning and then removes the device anyhow.) So return 0 unconditionally to make it obvious there is no error forwarded to the upper layers. This is a preparation for making platform remove callbacks return void. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/mmc/host/sdhci-of-arasan.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)