[PATCH 0/3] spi: mt65xx: Convert to platform remove callback returning void

[PATCH 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

this series converts the spi-mt65xx driver to .remove_new(). While the
preparing patch that gets rid of an early error return is in many cases
a bug fix, it's not tragic here, as the early return only skips steps
that are not necessary after resume failed. Still rework the code flow
to prepare for conversion to .remove_new(). The 2nd patch actually
converts the driver. The third is a small nitpick cleanup the I noticed
while working on the driver.

Best regards
Uwe

Uwe Kleine-König (3):
  spi: mt65xx: Properly handle failures in .remove()
  spi: mt65xx: Convert to platform remove callback returning void
  spi: mt65xx: Don't disguise a "return 0" as "return ret"

 drivers/spi/spi-mt65xx.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.1

[PATCH 1/3] spi: mt65xx: Properly handle failures in .remove()

[Uwe Kleine-König]

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.
+	 */
+	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);
-- 
2.39.1

Re: [PATCH 1/3] spi: mt65xx: Properly handle failures in .remove()

[AngeloGioacchino Del Regno]

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

[PATCH 2/3] spi: mt65xx: Convert to platform remove callback returning void

[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/spi/spi-mt65xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index b1cf7bbb2c08..f744cb97aa87 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1269,7 +1269,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int mtk_spi_remove(struct platform_device *pdev)
+static void mtk_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
@@ -1293,8 +1293,6 @@ static int mtk_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1414,7 +1412,7 @@ static struct platform_driver mtk_spi_driver = {
 		.of_match_table = mtk_spi_of_match,
 	},
 	.probe = mtk_spi_probe,
-	.remove = mtk_spi_remove,
+	.remove_new = mtk_spi_remove,
 };
 
 module_platform_driver(mtk_spi_driver);
-- 
2.39.1

Re: [PATCH 2/3] spi: mt65xx: Convert to platform remove callback returning void

[AngeloGioacchino Del Regno]

Il 09/03/23 10:47, Uwe Kleine-König ha scritto:
> 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: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"

[Uwe Kleine-König]

Because of the earlier

	 if (ret)
		return ret;

ret is always zero at the end of mtk_spi_suspend(). Write it as explicit
return 0 for slightly improved clearness.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-mt65xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index f744cb97aa87..2216d4e00c7a 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1311,7 +1311,7 @@ static int mtk_spi_suspend(struct device *dev)
 		clk_disable_unprepare(mdata->spi_hclk);
 	}
 
-	return ret;
+	return 0;
 }
 
 static int mtk_spi_resume(struct device *dev)
-- 
2.39.1

Re: [PATCH 3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"

[AngeloGioacchino Del Regno]

Il 09/03/23 10:47, Uwe Kleine-König ha scritto:
> Because of the earlier
> 
> 	 if (ret)
> 		return ret;
> 
> ret is always zero at the end of mtk_spi_suspend(). Write it as explicit
> return 0 for slightly improved clearness.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Mark Brown]

On Thu, 09 Mar 2023 10:47:01 +0100, Uwe Kleine-König wrote:
> this series converts the spi-mt65xx driver to .remove_new(). While the
> preparing patch that gets rid of an early error return is in many cases
> a bug fix, it's not tragic here, as the early return only skips steps
> that are not necessary after resume failed. Still rework the code flow
> to prepare for conversion to .remove_new(). The 2nd patch actually
> converts the driver. The third is a small nitpick cleanup the I noticed
> while working on the driver.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: mt65xx: Properly handle failures in .remove()
      commit: 22f407278ea43df46f90cece6595e5e8a0d5447c
[2/3] spi: mt65xx: Convert to platform remove callback returning void
      commit: df7e47196fcef5d5611caa65f91d813578cf3efd
[3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"
      commit: 6f089e986778d3657247fdc2b38bd38de796732b

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark