[PATCH 0/4] mtd: st_spi_fsm: Some fixes and improvements

[PATCH 0/4] mtd: st_spi_fsm: Some fixes and improvements

[Uwe Kleine-König]

Hello,

my intention was to just resend
https://lore.kernel.org/linux-mtd/20220603210758.148493-5-u.kleine-koenig@pengutronix.de
because after a change that was merged for 5.19-rc1 this patch didn't
apply any more. While preparing this update, I looked a bit too deep
into the driver and identified a few more patch opportunities.

I considered swapping the order of patches #2 and #3 because #3 is a fix
which #2 isn't. But doing would require a helper variable added to
.remove() in #2 that then is removed again in #3. If you feel strong
here (e.g. because you think #3 should be backported to stable but #2
shouldn't), I can rework accordingly.

Best regards
Uwe

Uwe Kleine-König (4):
  mtd: st_spi_fsm: Add a clk_disable_unprepare() in .probe()'s error path
  mtd: st_spi_fsm: Warn about failure to unregister mtd device
  mtd: st_spi_fsm: Disable clock only after device was unregistered
  mtd: st_spi_fsm: Simplify error checking in .probe() a bit

 drivers/mtd/devices/st_spi_fsm.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/4] mtd: st_spi_fsm: Add a clk_disable_unprepare() in .probe()'s error path

[Uwe Kleine-König]

For all but one error path clk_disable_unprepare() is already there. Add
it to the one location where it's missing.

Fixes: 481815a6193b ("mtd: st_spi_fsm: Handle clk_prepare_enable/clk_disable_unprepare.")
Fixes: 69d5af8d016c ("mtd: st_spi_fsm: Obtain and use EMI clock")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/devices/st_spi_fsm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index d3377b10fc0f..52a799cae402 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -2115,10 +2115,12 @@ static int stfsm_probe(struct platform_device *pdev)
 		(long long)fsm->mtd.size, (long long)(fsm->mtd.size >> 20),
 		fsm->mtd.erasesize, (fsm->mtd.erasesize >> 10));
 
-	return mtd_device_register(&fsm->mtd, NULL, 0);
-
+	ret = mtd_device_register(&fsm->mtd, NULL, 0);
+	if (ret) {
 err_clk_unprepare:
-	clk_disable_unprepare(fsm->clk);
+		clk_disable_unprepare(fsm->clk);
+	}
+
 	return ret;
 }
 
-- 
2.36.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: st_spi_fsm: Add a clk_disable_unprepare() in .probe()'s error path

[Miquel Raynal]

On Tue, 2022-06-07 at 15:24:55 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= wrote:
> For all but one error path clk_disable_unprepare() is already there. Add
> it to the one location where it's missing.
> 
> Fixes: 481815a6193b ("mtd: st_spi_fsm: Handle clk_prepare_enable/clk_disable_unprepare.")
> Fixes: 69d5af8d016c ("mtd: st_spi_fsm: Obtain and use EMI clock")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] mtd: st_spi_fsm: Warn about failure to unregister mtd device

[Uwe Kleine-König]

mtd_device_unregister() shouldn't fail. Wail loudly if it does anyhow.

This matches how other drivers (e.g. nand/raw/nandsim.c) use
mtd_device_unregister().

By returning 0 in the platform remove callback a generic error message
by the device core is suppressed, nothing else changes.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/devices/st_spi_fsm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 52a799cae402..a5a4b612480c 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -2130,7 +2130,9 @@ static int stfsm_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(fsm->clk);
 
-	return mtd_device_unregister(&fsm->mtd);
+	WARN_ON(mtd_device_unregister(&fsm->mtd));
+
+	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.36.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: st_spi_fsm: Warn about failure to unregister mtd device

[Miquel Raynal]

On Tue, 2022-06-07 at 15:24:56 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= wrote:
> mtd_device_unregister() shouldn't fail. Wail loudly if it does anyhow.
> 
> This matches how other drivers (e.g. nand/raw/nandsim.c) use
> mtd_device_unregister().
> 
> By returning 0 in the platform remove callback a generic error message
> by the device core is suppressed, nothing else changes.
> 
> This is a preparation for making platform remove callbacks return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/4] mtd: st_spi_fsm: Disable clock only after device was unregistered

[Uwe Kleine-König]

Until mtd_device_unregister() returns the device is expected to be
operational. So only disable the clock after the mtd is unregistered.

Fixes: 1fefc8ecb834 ("mtd: st_spi_fsm: add missing clk_disable_unprepare() in stfsm_remove()")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/devices/st_spi_fsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index a5a4b612480c..9f6d4dd8bade 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -2128,10 +2128,10 @@ static int stfsm_remove(struct platform_device *pdev)
 {
 	struct stfsm *fsm = platform_get_drvdata(pdev);
 
-	clk_disable_unprepare(fsm->clk);
-
 	WARN_ON(mtd_device_unregister(&fsm->mtd));
 
+	clk_disable_unprepare(fsm->clk);
+
 	return 0;
 }
 
-- 
2.36.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: st_spi_fsm: Disable clock only after device was unregistered

[Miquel Raynal]

On Tue, 2022-06-07 at 15:24:57 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= wrote:
> Until mtd_device_unregister() returns the device is expected to be
> operational. So only disable the clock after the mtd is unregistered.
> 
> Fixes: 1fefc8ecb834 ("mtd: st_spi_fsm: add missing clk_disable_unprepare() in stfsm_remove()")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/4] mtd: st_spi_fsm: Simplify error checking in .probe() a bit

[Uwe Kleine-König]

Instead of ending each if branch with the same check, do it once
unconditionally after the if block.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Not entirely sure this is an objective improvement, but I like it better
this way.

It could be shorted one step further by doing

	ret = (info->config ?: stfsm_prepare_rwe_seqs_default)(fsm);
	if (ret)
		goto err_clk_unprepare;

but IMHO readability suffers here.

 drivers/mtd/devices/st_spi_fsm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 9f6d4dd8bade..54861d889c30 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -2084,15 +2084,12 @@ static int stfsm_probe(struct platform_device *pdev)
 	 * Configure READ/WRITE/ERASE sequences according to platform and
 	 * device flags.
 	 */
-	if (info->config) {
+	if (info->config)
 		ret = info->config(fsm);
-		if (ret)
-			goto err_clk_unprepare;
-	} else {
+	else
 		ret = stfsm_prepare_rwe_seqs_default(fsm);
-		if (ret)
-			goto err_clk_unprepare;
-	}
+	if (ret)
+		goto err_clk_unprepare;
 
 	fsm->mtd.name		= info->name;
 	fsm->mtd.dev.parent	= &pdev->dev;
-- 
2.36.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: st_spi_fsm: Simplify error checking in .probe() a bit

[Miquel Raynal]

Hi Uwe,

u.kleine-koenig@pengutronix.de wrote on Tue,  7 Jun 2022 17:24:58 +0200:

> Instead of ending each if branch with the same check, do it once
> unconditionally after the if block.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Not entirely sure this is an objective improvement, but I like it better
> this way.
> 
> It could be shorted one step further by doing
> 
> 	ret = (info->config ?: stfsm_prepare_rwe_seqs_default)(fsm);
> 	if (ret)
> 		goto err_clk_unprepare;
> 
> but IMHO readability suffers here.

Clearly, yes, I think you opted-out for the right optimization in the
end.

> 
>  drivers/mtd/devices/st_spi_fsm.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index 9f6d4dd8bade..54861d889c30 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -2084,15 +2084,12 @@ static int stfsm_probe(struct platform_device *pdev)
>  	 * Configure READ/WRITE/ERASE sequences according to platform and
>  	 * device flags.
>  	 */
> -	if (info->config) {
> +	if (info->config)
>  		ret = info->config(fsm);
> -		if (ret)
> -			goto err_clk_unprepare;
> -	} else {
> +	else
>  		ret = stfsm_prepare_rwe_seqs_default(fsm);
> -		if (ret)
> -			goto err_clk_unprepare;
> -	}
> +	if (ret)
> +		goto err_clk_unprepare;
>  
>  	fsm->mtd.name		= info->name;
>  	fsm->mtd.dev.parent	= &pdev->dev;


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: st_spi_fsm: Simplify error checking in .probe() a bit

[Miquel Raynal]

On Tue, 2022-06-07 at 15:24:58 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= wrote:
> Instead of ending each if branch with the same check, do it once
> unconditionally after the if block.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/