[PATCH 00/14] power: reset: Drop platform_driver_probe() and convert to platform remove callback returning void

[PATCH 00/14] power: reset: Drop platform_driver_probe() and convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

there are two different types of patches here that would justify to
different series. But as the patches are not independant I chose to put
them in a single series.

The first three patches drop usage of platform_driver_probe(). This is a
concept that isn't so relevant any more today. I didn't check, but it
saves typically only a few 100k and there are thoughts to deprecate it
to simplify the core. Getting the usage right is not trivial though the
at91 drivers got it nearly right. The alternative to these patches is to
add __ref to the driver struct ideally with a comment describing the
need like is e.g. done in commit 5b44abbc39ca ("platform/x86: hp-wmi::
Mark driver struct with __refdata to prevent section mismatch warning").

The remaining patches convert the platform drivers to .remove_new(), see
commit 5c5a7680e67b ("platform: Provide a remove callback that returns
no value") for an extended explanation and the eventual goal. All
conversions but one are trivial as the remove functions return zero
unconditionally. The only exception is the tps65086-restart driver.

Best regards
Uwe

Uwe Kleine-König (14):
  power: reset: at91-poweroff: Stop using module_platform_driver_probe()
  power: reset: at91-reset:: Stop using module_platform_driver_probe()
  power: reset: at91-sama5d2_shdwc: Stop using
    module_platform_driver_probe()
  power: reset: as3722-poweroff: Convert to platform remove callback
    returning void
  power: reset: at91-poweroff: Convert to platform remove callback
    returning void
  power: reset: atc260x-poweroff: Convert to platform remove callback
    returning void
  power: reset: ltc2952-poweroff: Convert to platform remove callback
    returning void
  power: reset: mt6323-poweroff: Convert to platform remove callback
    returning void
  power: reset: qnap-poweroff: Convert to platform remove callback
    returning void
  power: reset: regulator-poweroff: Convert to platform remove callback
    returning void
  power: reset: restart-poweroff: Convert to platform remove callback
    returning void
  power: reset: rmobile-reset: Convert to platform remove callback
    returning void
  power: reset: syscon-poweroff: Convert to platform remove callback
    returning void
  power: reset: tps65086-restart: Convert to platform remove callback
    returning void

 drivers/power/reset/as3722-poweroff.c    |  6 ++----
 drivers/power/reset/at91-poweroff.c      | 11 +++++------
 drivers/power/reset/at91-reset.c         |  9 +++++----
 drivers/power/reset/at91-sama5d2_shdwc.c |  9 +++++----
 drivers/power/reset/atc260x-poweroff.c   |  6 ++----
 drivers/power/reset/ltc2952-poweroff.c   |  5 ++---
 drivers/power/reset/mt6323-poweroff.c    |  6 ++----
 drivers/power/reset/qnap-poweroff.c      |  5 ++---
 drivers/power/reset/regulator-poweroff.c |  6 ++----
 drivers/power/reset/restart-poweroff.c   |  6 ++----
 drivers/power/reset/rmobile-reset.c      |  5 ++---
 drivers/power/reset/syscon-poweroff.c    |  6 ++----
 drivers/power/reset/tps65086-restart.c   | 12 +++++++-----
 13 files changed, 40 insertions(+), 52 deletions(-)


base-commit: e27090b1413ff236ca1aec26d6b022149115de2c
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/14] power: reset: at91-poweroff: Stop using module_platform_driver_probe()

[Uwe Kleine-König]

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.

The downside of platform_driver_probe() is that the driver cannot be
bound and unbound at runtime which is ancient and so slightly
complicates testing. There are also thoughts to deprecate
platform_driver_probe() because it adds some complexity in the driver
core for little gain. Also many drivers don't use it correctly. This
driver for example misses to mark the driver struct with __ref which is
needed to suppress a (W=1) modpost warning.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/power/reset/at91-poweroff.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index dd5399785b69..83567428ab43 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -149,7 +149,7 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev)
 	writel(wakeup_mode | mode, at91_shdwc.shdwc_base + AT91_SHDW_MR);
 }
 
-static int __init at91_poweroff_probe(struct platform_device *pdev)
+static int at91_poweroff_probe(struct platform_device *pdev)
 {
 	struct device_node *np;
 	u32 ddr_type;
@@ -202,7 +202,7 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit at91_poweroff_remove(struct platform_device *pdev)
+static int at91_poweroff_remove(struct platform_device *pdev)
 {
 	if (pm_power_off == at91_poweroff)
 		pm_power_off = NULL;
@@ -224,13 +224,14 @@ static const struct of_device_id at91_poweroff_of_match[] = {
 MODULE_DEVICE_TABLE(of, at91_poweroff_of_match);
 
 static struct platform_driver at91_poweroff_driver = {
-	.remove = __exit_p(at91_poweroff_remove),
+	.probe = at91_poweroff_probe,
+	.remove = at91_poweroff_remove,
 	.driver = {
 		.name = "at91-poweroff",
 		.of_match_table = at91_poweroff_of_match,
 	},
 };
-module_platform_driver_probe(at91_poweroff_driver, at91_poweroff_probe);
+module_platform_driver(at91_poweroff_driver);
 
 MODULE_AUTHOR("Atmel Corporation");
 MODULE_DESCRIPTION("Shutdown driver for Atmel SoCs");
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/14] power: reset: at91-reset:: Stop using module_platform_driver_probe()

[Uwe Kleine-König]

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.

The downside of platform_driver_probe() is that the driver cannot be
bound and unbound at runtime which is ancient and so slightly
complicates testing. There are also thoughts to deprecate
platform_driver_probe() because it adds some complexity in the driver
core for little gain. Also many drivers don't use it correctly. This
driver for example misses to mark the driver struct with __ref which is
needed to suppress a (W=1) modpost warning.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/power/reset/at91-reset.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index aa9b012d3d00..af85f2f929ba 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -337,7 +337,7 @@ static int at91_rcdev_init(struct at91_reset *reset,
 	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
 }
 
-static int __init at91_reset_probe(struct platform_device *pdev)
+static int at91_reset_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	struct at91_reset *reset;
@@ -417,7 +417,7 @@ static int __init at91_reset_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit at91_reset_remove(struct platform_device *pdev)
+static int at91_reset_remove(struct platform_device *pdev)
 {
 	struct at91_reset *reset = platform_get_drvdata(pdev);
 
@@ -428,13 +428,14 @@ static int __exit at91_reset_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver at91_reset_driver = {
-	.remove = __exit_p(at91_reset_remove),
+	.probe = at91_reset_probe,
+	.remove = at91_reset_remove,
 	.driver = {
 		.name = "at91-reset",
 		.of_match_table = at91_reset_of_match,
 	},
 };
-module_platform_driver_probe(at91_reset_driver, at91_reset_probe);
+module_platform_driver(at91_reset_driver);
 
 MODULE_AUTHOR("Atmel Corporation");
 MODULE_DESCRIPTION("Reset driver for Atmel SoCs");
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/14] power: reset: at91-sama5d2_shdwc: Stop using module_platform_driver_probe()

[Uwe Kleine-König]

On today's platforms the benefit of platform_driver_probe() isn't that
relevant any more. It allows to drop some code after booting (or module
loading) for .probe() and discard the .remove() function completely if
the driver is built-in. This typically saves a few 100k.

The downside of platform_driver_probe() is that the driver cannot be
bound and unbound at runtime which is ancient and so slightly
complicates testing. There are also thoughts to deprecate
platform_driver_probe() because it adds some complexity in the driver
core for little gain. Also many drivers don't use it correctly. This
driver for example misses to mark the driver struct with __ref which is
needed to suppress a (W=1) modpost warning.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/power/reset/at91-sama5d2_shdwc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
index e76b102b57b1..ef8add623363 100644
--- a/drivers/power/reset/at91-sama5d2_shdwc.c
+++ b/drivers/power/reset/at91-sama5d2_shdwc.c
@@ -329,7 +329,7 @@ static const struct of_device_id at91_pmc_ids[] = {
 	{ /* Sentinel. */ }
 };
 
-static int __init at91_shdwc_probe(struct platform_device *pdev)
+static int at91_shdwc_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	struct device_node *np;
@@ -421,7 +421,7 @@ static int __init at91_shdwc_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit at91_shdwc_remove(struct platform_device *pdev)
+static int at91_shdwc_remove(struct platform_device *pdev)
 {
 	struct shdwc *shdw = platform_get_drvdata(pdev);
 
@@ -442,13 +442,14 @@ static int __exit at91_shdwc_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver at91_shdwc_driver = {
-	.remove = __exit_p(at91_shdwc_remove),
+	.probe = at91_shdwc_probe,
+	.remove = at91_shdwc_remove,
 	.driver = {
 		.name = "at91-shdwc",
 		.of_match_table = at91_shdwc_of_match,
 	},
 };
-module_platform_driver_probe(at91_shdwc_driver, at91_shdwc_probe);
+module_platform_driver(at91_shdwc_driver);
 
 MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>");
 MODULE_DESCRIPTION("Atmel shutdown controller driver");
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/14] power: reset: at91-poweroff: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/power/reset/at91-poweroff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
index 83567428ab43..126e774e210c 100644
--- a/drivers/power/reset/at91-poweroff.c
+++ b/drivers/power/reset/at91-poweroff.c
@@ -202,7 +202,7 @@ static int at91_poweroff_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int at91_poweroff_remove(struct platform_device *pdev)
+static void at91_poweroff_remove(struct platform_device *pdev)
 {
 	if (pm_power_off == at91_poweroff)
 		pm_power_off = NULL;
@@ -211,8 +211,6 @@ static int at91_poweroff_remove(struct platform_device *pdev)
 		iounmap(at91_shdwc.mpddrc_base);
 
 	clk_disable_unprepare(at91_shdwc.sclk);
-
-	return 0;
 }
 
 static const struct of_device_id at91_poweroff_of_match[] = {
@@ -225,7 +223,7 @@ MODULE_DEVICE_TABLE(of, at91_poweroff_of_match);
 
 static struct platform_driver at91_poweroff_driver = {
 	.probe = at91_poweroff_probe,
-	.remove = at91_poweroff_remove,
+	.remove_new = at91_poweroff_remove,
 	.driver = {
 		.name = "at91-poweroff",
 		.of_match_table = at91_poweroff_of_match,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/14] power: reset: mt6323-poweroff: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/power/reset/mt6323-poweroff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/mt6323-poweroff.c b/drivers/power/reset/mt6323-poweroff.c
index 108167f7738b..57a63c0ab7fb 100644
--- a/drivers/power/reset/mt6323-poweroff.c
+++ b/drivers/power/reset/mt6323-poweroff.c
@@ -70,12 +70,10 @@ static int mt6323_pwrc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int mt6323_pwrc_remove(struct platform_device *pdev)
+static void mt6323_pwrc_remove(struct platform_device *pdev)
 {
 	if (pm_power_off == &mt6323_do_pwroff)
 		pm_power_off = NULL;
-
-	return 0;
 }
 
 static const struct of_device_id mt6323_pwrc_dt_match[] = {
@@ -86,7 +84,7 @@ MODULE_DEVICE_TABLE(of, mt6323_pwrc_dt_match);
 
 static struct platform_driver mt6323_pwrc_driver = {
 	.probe          = mt6323_pwrc_probe,
-	.remove         = mt6323_pwrc_remove,
+	.remove_new     = mt6323_pwrc_remove,
 	.driver         = {
 		.name   = "mt6323-pwrc",
 		.of_match_table = mt6323_pwrc_dt_match,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 15/14] power: reset: at91-reset: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/power/reset/at91-reset.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index af85f2f929ba..16512654295f 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -417,19 +417,17 @@ static int at91_reset_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int at91_reset_remove(struct platform_device *pdev)
+static void at91_reset_remove(struct platform_device *pdev)
 {
 	struct at91_reset *reset = platform_get_drvdata(pdev);
 
 	unregister_restart_handler(&reset->nb);
 	clk_disable_unprepare(reset->sclk);
-
-	return 0;
 }
 
 static struct platform_driver at91_reset_driver = {
 	.probe = at91_reset_probe,
-	.remove = at91_reset_remove,
+	.remove_new = at91_reset_remove,
 	.driver = {
 		.name = "at91-reset",
 		.of_match_table = at91_reset_of_match,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 16/14] power: reset: at91-sama5d2_shdwc: 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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/power/reset/at91-sama5d2_shdwc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
index ef8add623363..af95c7b39cb3 100644
--- a/drivers/power/reset/at91-sama5d2_shdwc.c
+++ b/drivers/power/reset/at91-sama5d2_shdwc.c
@@ -421,7 +421,7 @@ static int at91_shdwc_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int at91_shdwc_remove(struct platform_device *pdev)
+static void at91_shdwc_remove(struct platform_device *pdev)
 {
 	struct shdwc *shdw = platform_get_drvdata(pdev);
 
@@ -437,13 +437,11 @@ static int at91_shdwc_remove(struct platform_device *pdev)
 	iounmap(shdw->pmc_base);
 
 	clk_disable_unprepare(shdw->sclk);
-
-	return 0;
 }
 
 static struct platform_driver at91_shdwc_driver = {
 	.probe = at91_shdwc_probe,
-	.remove = at91_shdwc_remove,
+	.remove_new = at91_shdwc_remove,
 	.driver = {
 		.name = "at91-shdwc",
 		.of_match_table = at91_shdwc_of_match,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/14] power: reset: Drop platform_driver_probe() and convert to platform remove callback returning void

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1908 bytes --]

Hello Sebastian,

On Sat, Nov 04, 2023 at 10:15:02PM +0100, Uwe Kleine-König wrote:
> there are two different types of patches here that would justify to
> different series. But as the patches are not independant I chose to put
> them in a single series.
> 
> The first three patches drop usage of platform_driver_probe(). This is a
> concept that isn't so relevant any more today. I didn't check, but it
> saves typically only a few 100k and there are thoughts to deprecate it
> to simplify the core. Getting the usage right is not trivial though the
> at91 drivers got it nearly right. The alternative to these patches is to
> add __ref to the driver struct ideally with a comment describing the
> need like is e.g. done in commit 5b44abbc39ca ("platform/x86: hp-wmi::
> Mark driver struct with __refdata to prevent section mismatch warning").
> 
> The remaining patches convert the platform drivers to .remove_new(), see
> commit 5c5a7680e67b ("platform: Provide a remove callback that returns
> no value") for an extended explanation and the eventual goal. All
> conversions but one are trivial as the remove functions return zero
> unconditionally. The only exception is the tps65086-restart driver.

I forgot to convert two of the drivers that used platform_driver_probe()
before. I added them as patches 15 and 16 to this series in the hope
that b4 (or whatever tool you use to apply patches) gets that right.

> Uwe Kleine-König (14):
>   power: reset: at91-poweroff: Stop using module_platform_driver_probe()
>   power: reset: at91-reset:: Stop using module_platform_driver_probe()

If you could do s/::/:/ on that patch description that would be great.
(Or should I resend for that?)

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/14] power: reset: mt6323-poweroff: Convert to platform remove callback returning void

[AngeloGioacchino Del Regno]

Il 04/11/23 22:15, 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/14] power: reset: at91-poweroff: Stop using module_platform_driver_probe()

[claudiu beznea]

On 04.11.2023 23:15, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and so slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __ref which is
> needed to suppress a (W=1) modpost warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>  drivers/power/reset/at91-poweroff.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index dd5399785b69..83567428ab43 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -149,7 +149,7 @@ static void at91_poweroff_dt_set_wakeup_mode(struct platform_device *pdev)
>  	writel(wakeup_mode | mode, at91_shdwc.shdwc_base + AT91_SHDW_MR);
>  }
>  
> -static int __init at91_poweroff_probe(struct platform_device *pdev)
> +static int at91_poweroff_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np;
>  	u32 ddr_type;
> @@ -202,7 +202,7 @@ static int __init at91_poweroff_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int __exit at91_poweroff_remove(struct platform_device *pdev)
> +static int at91_poweroff_remove(struct platform_device *pdev)
>  {
>  	if (pm_power_off == at91_poweroff)
>  		pm_power_off = NULL;
> @@ -224,13 +224,14 @@ static const struct of_device_id at91_poweroff_of_match[] = {
>  MODULE_DEVICE_TABLE(of, at91_poweroff_of_match);
>  
>  static struct platform_driver at91_poweroff_driver = {
> -	.remove = __exit_p(at91_poweroff_remove),
> +	.probe = at91_poweroff_probe,
> +	.remove = at91_poweroff_remove,
>  	.driver = {
>  		.name = "at91-poweroff",
>  		.of_match_table = at91_poweroff_of_match,
>  	},
>  };
> -module_platform_driver_probe(at91_poweroff_driver, at91_poweroff_probe);
> +module_platform_driver(at91_poweroff_driver);
>  
>  MODULE_AUTHOR("Atmel Corporation");
>  MODULE_DESCRIPTION("Shutdown driver for Atmel SoCs");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/14] power: reset: at91-reset:: Stop using module_platform_driver_probe()

[claudiu beznea]

On 04.11.2023 23:15, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and so slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __ref which is
> needed to suppress a (W=1) modpost warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Other than "::" in title:

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>  drivers/power/reset/at91-reset.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index aa9b012d3d00..af85f2f929ba 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -337,7 +337,7 @@ static int at91_rcdev_init(struct at91_reset *reset,
>  	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>  }
>  
> -static int __init at91_reset_probe(struct platform_device *pdev)
> +static int at91_reset_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;
>  	struct at91_reset *reset;
> @@ -417,7 +417,7 @@ static int __init at91_reset_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int __exit at91_reset_remove(struct platform_device *pdev)
> +static int at91_reset_remove(struct platform_device *pdev)
>  {
>  	struct at91_reset *reset = platform_get_drvdata(pdev);
>  
> @@ -428,13 +428,14 @@ static int __exit at91_reset_remove(struct platform_device *pdev)
>  }
>  
>  static struct platform_driver at91_reset_driver = {
> -	.remove = __exit_p(at91_reset_remove),
> +	.probe = at91_reset_probe,
> +	.remove = at91_reset_remove,
>  	.driver = {
>  		.name = "at91-reset",
>  		.of_match_table = at91_reset_of_match,
>  	},
>  };
> -module_platform_driver_probe(at91_reset_driver, at91_reset_probe);
> +module_platform_driver(at91_reset_driver);
>  
>  MODULE_AUTHOR("Atmel Corporation");
>  MODULE_DESCRIPTION("Reset driver for Atmel SoCs");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/14] power: reset: at91-sama5d2_shdwc: Stop using module_platform_driver_probe()

[claudiu beznea]

On 04.11.2023 23:15, Uwe Kleine-König wrote:
> On today's platforms the benefit of platform_driver_probe() isn't that
> relevant any more. It allows to drop some code after booting (or module
> loading) for .probe() and discard the .remove() function completely if
> the driver is built-in. This typically saves a few 100k.
> 
> The downside of platform_driver_probe() is that the driver cannot be
> bound and unbound at runtime which is ancient and so slightly
> complicates testing. There are also thoughts to deprecate
> platform_driver_probe() because it adds some complexity in the driver
> core for little gain. Also many drivers don't use it correctly. This
> driver for example misses to mark the driver struct with __ref which is
> needed to suppress a (W=1) modpost warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>  drivers/power/reset/at91-sama5d2_shdwc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
> index e76b102b57b1..ef8add623363 100644
> --- a/drivers/power/reset/at91-sama5d2_shdwc.c
> +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
> @@ -329,7 +329,7 @@ static const struct of_device_id at91_pmc_ids[] = {
>  	{ /* Sentinel. */ }
>  };
>  
> -static int __init at91_shdwc_probe(struct platform_device *pdev)
> +static int at91_shdwc_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;
>  	struct device_node *np;
> @@ -421,7 +421,7 @@ static int __init at91_shdwc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int __exit at91_shdwc_remove(struct platform_device *pdev)
> +static int at91_shdwc_remove(struct platform_device *pdev)
>  {
>  	struct shdwc *shdw = platform_get_drvdata(pdev);
>  
> @@ -442,13 +442,14 @@ static int __exit at91_shdwc_remove(struct platform_device *pdev)
>  }
>  
>  static struct platform_driver at91_shdwc_driver = {
> -	.remove = __exit_p(at91_shdwc_remove),
> +	.probe = at91_shdwc_probe,
> +	.remove = at91_shdwc_remove,
>  	.driver = {
>  		.name = "at91-shdwc",
>  		.of_match_table = at91_shdwc_of_match,
>  	},
>  };
> -module_platform_driver_probe(at91_shdwc_driver, at91_shdwc_probe);
> +module_platform_driver(at91_shdwc_driver);
>  
>  MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>");
>  MODULE_DESCRIPTION("Atmel shutdown controller driver");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/14] power: reset: at91-poweroff: Convert to platform remove callback returning void

[claudiu beznea]

On 04.11.2023 23:15, Uwe Kleine-König wrote:
> 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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: Claudiu Beznea <claudiu.beznea@tuxon.dev>


> ---
>  drivers/power/reset/at91-poweroff.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-poweroff.c b/drivers/power/reset/at91-poweroff.c
> index 83567428ab43..126e774e210c 100644
> --- a/drivers/power/reset/at91-poweroff.c
> +++ b/drivers/power/reset/at91-poweroff.c
> @@ -202,7 +202,7 @@ static int at91_poweroff_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int at91_poweroff_remove(struct platform_device *pdev)
> +static void at91_poweroff_remove(struct platform_device *pdev)
>  {
>  	if (pm_power_off == at91_poweroff)
>  		pm_power_off = NULL;
> @@ -211,8 +211,6 @@ static int at91_poweroff_remove(struct platform_device *pdev)
>  		iounmap(at91_shdwc.mpddrc_base);
>  
>  	clk_disable_unprepare(at91_shdwc.sclk);
> -
> -	return 0;
>  }
>  
>  static const struct of_device_id at91_poweroff_of_match[] = {
> @@ -225,7 +223,7 @@ MODULE_DEVICE_TABLE(of, at91_poweroff_of_match);
>  
>  static struct platform_driver at91_poweroff_driver = {
>  	.probe = at91_poweroff_probe,
> -	.remove = at91_poweroff_remove,
> +	.remove_new = at91_poweroff_remove,
>  	.driver = {
>  		.name = "at91-poweroff",
>  		.of_match_table = at91_poweroff_of_match,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 15/14] power: reset: at91-reset: Convert to platform remove callback returning void

[claudiu beznea]

On 05.11.2023 11:47, Uwe Kleine-König wrote:
> 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>  drivers/power/reset/at91-reset.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index af85f2f929ba..16512654295f 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -417,19 +417,17 @@ static int at91_reset_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int at91_reset_remove(struct platform_device *pdev)
> +static void at91_reset_remove(struct platform_device *pdev)
>  {
>  	struct at91_reset *reset = platform_get_drvdata(pdev);
>  
>  	unregister_restart_handler(&reset->nb);
>  	clk_disable_unprepare(reset->sclk);
> -
> -	return 0;
>  }
>  
>  static struct platform_driver at91_reset_driver = {
>  	.probe = at91_reset_probe,
> -	.remove = at91_reset_remove,
> +	.remove_new = at91_reset_remove,
>  	.driver = {
>  		.name = "at91-reset",
>  		.of_match_table = at91_reset_of_match,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 16/14] power: reset: at91-sama5d2_shdwc: Convert to platform remove callback returning void

[claudiu beznea]

On 05.11.2023 11:47, Uwe Kleine-König wrote:
> 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>  drivers/power/reset/at91-sama5d2_shdwc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
> index ef8add623363..af95c7b39cb3 100644
> --- a/drivers/power/reset/at91-sama5d2_shdwc.c
> +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
> @@ -421,7 +421,7 @@ static int at91_shdwc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int at91_shdwc_remove(struct platform_device *pdev)
> +static void at91_shdwc_remove(struct platform_device *pdev)
>  {
>  	struct shdwc *shdw = platform_get_drvdata(pdev);
>  
> @@ -437,13 +437,11 @@ static int at91_shdwc_remove(struct platform_device *pdev)
>  	iounmap(shdw->pmc_base);
>  
>  	clk_disable_unprepare(shdw->sclk);
> -
> -	return 0;
>  }
>  
>  static struct platform_driver at91_shdwc_driver = {
>  	.probe = at91_shdwc_probe,
> -	.remove = at91_shdwc_remove,
> +	.remove_new = at91_shdwc_remove,
>  	.driver = {
>  		.name = "at91-shdwc",
>  		.of_match_table = at91_shdwc_of_match,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/14] power: reset: Drop platform_driver_probe() and convert to platform remove callback returning void

[Sebastian Reichel]

On Sat, 04 Nov 2023 22:15:02 +0100, Uwe Kleine-König wrote:
> there are two different types of patches here that would justify to
> different series. But as the patches are not independant I chose to put
> them in a single series.
> 
> The first three patches drop usage of platform_driver_probe(). This is a
> concept that isn't so relevant any more today. I didn't check, but it
> saves typically only a few 100k and there are thoughts to deprecate it
> to simplify the core. Getting the usage right is not trivial though the
> at91 drivers got it nearly right. The alternative to these patches is to
> add __ref to the driver struct ideally with a comment describing the
> need like is e.g. done in commit 5b44abbc39ca ("platform/x86: hp-wmi::
> Mark driver struct with __refdata to prevent section mismatch warning").
> 
> [...]

Applied, thanks!

[01/14] power: reset: at91-poweroff: Stop using module_platform_driver_probe()
        commit: 099806de68b75a0fe114376b1ee162fdff572ecc
[02/14] power: reset: at91-reset:: Stop using module_platform_driver_probe()
        commit: 12389c657b623da34ba9b30306e13919d0b42f3a
[03/14] power: reset: at91-sama5d2_shdwc: Stop using module_platform_driver_probe()
        commit: dde74a5de817e0a011e4783cf26295d7f6fdca26
[04/14] power: reset: as3722-poweroff: Convert to platform remove callback returning void
        commit: 904e582f0c7282b3d7c76c73c06f3ad3b0910335
[05/14] power: reset: at91-poweroff: Convert to platform remove callback returning void
        commit: a31438ece3ec27057c77822b0b0bc0614798c425
[06/14] power: reset: atc260x-poweroff: Convert to platform remove callback returning void
        commit: 6f539f3151721f1c90fcdafa2962c19a8efc1afc
[07/14] power: reset: ltc2952-poweroff: Convert to platform remove callback returning void
        commit: 6642b13206b2225b0d75451f5dd763574a2c8bb0
[08/14] power: reset: mt6323-poweroff: Convert to platform remove callback returning void
        commit: 99f7fa6c7cc573ebe2529959723b71b4d12ec2f5
[09/14] power: reset: qnap-poweroff: Convert to platform remove callback returning void
        commit: 1a0457ab2ce81d92c2077a79080141d924884c5f
[10/14] power: reset: regulator-poweroff: Convert to platform remove callback returning void
        commit: 6f7be7b2f15a654a5f08b7d37df282c171b9380b
[11/14] power: reset: restart-poweroff: Convert to platform remove callback returning void
        commit: aedd4da0aa27fe9d0b03b3fbf62376aebbfc385b
[12/14] power: reset: rmobile-reset: Convert to platform remove callback returning void
        commit: 30d26d2be83de0d036f59f384b805dd3bf4bf5ef
[13/14] power: reset: syscon-poweroff: Convert to platform remove callback returning void
        commit: 2973706c4160ed8c1e695b4dfef4bdb3124c5753
[14/14] power: reset: tps65086-restart: Convert to platform remove callback returning void
        commit: 0bf7207e09673144df6425b2cad8bcb015e3501a
[15/15] power: reset: at91-reset: Convert to platform remove callback returning void
        commit: 20cea2b59abe33b80536b936b6729c88d1de2624
[16/16] power: reset: at91-sama5d2_shdwc: Convert to platform remove callback returning void
        commit: 054eb2377523530404fb64c24c8747feb022c5b4

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel