[1/5] mmc: dw_mmc: exynos: Obviously always return success in remove callback

dw_mci_pltfm_remove() 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/dw_mmc-exynos.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56

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

On Fri, 10 Jun 2022 at 14:13, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> dw_mci_pltfm_remove() 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>

This one and the other 4 patches in the series, applied for next. Thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/dw_mmc-exynos.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index ca5be4445ae0..9f20ac524c8b 100644
> --- a/drivers/mmc/host/dw_mmc-exynos.c
> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> @@ -670,7 +670,9 @@ static int dw_mci_exynos_remove(struct platform_device *pdev)
>         pm_runtime_set_suspended(&pdev->dev);
>         pm_runtime_put_noidle(&pdev->dev);
>
> -       return dw_mci_pltfm_remove(pdev);
> +       dw_mci_pltfm_remove(pdev);
> +
> +       return 0;
>  }
>
>  static const struct dev_pm_ops dw_mci_exynos_pmops = {
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> 2.36.1
>