[PATCH 1/4] amba: reorder functions

[PATCH 1/4] amba: reorder functions

[Uwe Kleine-König]

Put helpers (here: amba_get_enable_pclk and amba_put_disable_pclk) at
the top of the file and then define callbacks directly before the
structs they are used in; in the same order.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/amba/bus.c | 77 +++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index ecc304149067..8658e0533b67 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -56,31 +56,28 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
 	return NULL;
 }
 
-static int amba_match(struct device *dev, struct device_driver *drv)
+static int amba_get_enable_pclk(struct amba_device *pcdev)
 {
-	struct amba_device *pcdev = to_amba_device(dev);
-	struct amba_driver *pcdrv = to_amba_driver(drv);
+	int ret;
 
-	/* When driver_override is set, only bind to the matching driver */
-	if (pcdev->driver_override)
-		return !strcmp(pcdev->driver_override, drv->name);
+	pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
+	if (IS_ERR(pcdev->pclk))
+		return PTR_ERR(pcdev->pclk);
 
-	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
+	ret = clk_prepare_enable(pcdev->pclk);
+	if (ret)
+		clk_put(pcdev->pclk);
+
+	return ret;
 }
 
-static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
+static void amba_put_disable_pclk(struct amba_device *pcdev)
 {
-	struct amba_device *pcdev = to_amba_device(dev);
-	int retval = 0;
-
-	retval = add_uevent_var(env, "AMBA_ID=%08x", pcdev->periphid);
-	if (retval)
-		return retval;
-
-	retval = add_uevent_var(env, "MODALIAS=amba:d%08X", pcdev->periphid);
-	return retval;
+	clk_disable_unprepare(pcdev->pclk);
+	clk_put(pcdev->pclk);
 }
 
+
 static ssize_t driver_override_show(struct device *_dev,
 				    struct device_attribute *attr, char *buf)
 {
@@ -152,6 +149,31 @@ static struct attribute *amba_dev_attrs[] = {
 };
 ATTRIBUTE_GROUPS(amba_dev);
 
+static int amba_match(struct device *dev, struct device_driver *drv)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	struct amba_driver *pcdrv = to_amba_driver(drv);
+
+	/* When driver_override is set, only bind to the matching driver */
+	if (pcdev->driver_override)
+		return !strcmp(pcdev->driver_override, drv->name);
+
+	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
+}
+
+static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int retval = 0;
+
+	retval = add_uevent_var(env, "AMBA_ID=%08x", pcdev->periphid);
+	if (retval)
+		return retval;
+
+	retval = add_uevent_var(env, "MODALIAS=amba:d%08X", pcdev->periphid);
+	return retval;
+}
+
 #ifdef CONFIG_PM
 /*
  * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
@@ -229,27 +251,6 @@ static int __init amba_init(void)
 
 postcore_initcall(amba_init);
 
-static int amba_get_enable_pclk(struct amba_device *pcdev)
-{
-	int ret;
-
-	pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
-	if (IS_ERR(pcdev->pclk))
-		return PTR_ERR(pcdev->pclk);
-
-	ret = clk_prepare_enable(pcdev->pclk);
-	if (ret)
-		clk_put(pcdev->pclk);
-
-	return ret;
-}
-
-static void amba_put_disable_pclk(struct amba_device *pcdev)
-{
-	clk_disable_unprepare(pcdev->pclk);
-	clk_put(pcdev->pclk);
-}
-
 /*
  * These are the device model conversion veneers; they convert the
  * device model structures to our more specific structures.

base-commit: 95065cb54210eba86bed10cb2118041524d54573
-- 
2.29.2

[PATCH 2/4] amba: Fix resource leak for drivers without .remove

[Uwe Kleine-König]

Consider an amba driver with a .probe but without a .remove callback (e.g.
pl061_gpio_driver). The function amba_probe() is called to bind a device
and so dev_pm_domain_attach() and others are called. As there is no remove
callback amba_remove() isn't called at unbind time however and so calling
dev_pm_domain_detach() is missed and the pm domain keeps active.

To fix this always use the core driver callbacks and handle missing amba
callbacks there. For probe refuse registration as a driver without probe
doesn't make sense.

Fixes: 7cfe249475fd ("ARM: AMBA: Add pclk support to AMBA bus infrastructure")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/amba/bus.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 8658e0533b67..8c4a42df47c6 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -300,10 +300,11 @@ static int amba_remove(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	int ret;
+	int ret = 0;
 
 	pm_runtime_get_sync(dev);
-	ret = drv->remove(pcdev);
+	if (drv->remove)
+		ret = drv->remove(pcdev);
 	pm_runtime_put_noidle(dev);
 
 	/* Undo the runtime PM settings in amba_probe() */
@@ -320,7 +321,9 @@ static int amba_remove(struct device *dev)
 static void amba_shutdown(struct device *dev)
 {
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	drv->shutdown(to_amba_device(dev));
+
+	if (drv->shutdown)
+		drv->shutdown(to_amba_device(dev));
 }
 
 /**
@@ -333,12 +336,13 @@ static void amba_shutdown(struct device *dev)
  */
 int amba_driver_register(struct amba_driver *drv)
 {
-	drv->drv.bus = &amba_bustype;
+	if (!drv->probe)
+		return -EINVAL;
 
-#define SETFN(fn)	if (drv->fn) drv->drv.fn = amba_##fn
-	SETFN(probe);
-	SETFN(remove);
-	SETFN(shutdown);
+	drv->drv.bus = &amba_bustype;
+	drv->drv.probe = amba_probe;
+	drv->drv.remove = amba_remove;
+	drv->drv.shutdown = amba_shutdown;
 
 	return driver_register(&drv->drv);
 }
-- 
2.29.2

Re: [PATCH 2/4] amba: Fix resource leak for drivers without .remove

[Ulf Hansson]

On Tue, 24 Nov 2020 at 11:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Consider an amba driver with a .probe but without a .remove callback (e.g.
> pl061_gpio_driver). The function amba_probe() is called to bind a device
> and so dev_pm_domain_attach() and others are called. As there is no remove
> callback amba_remove() isn't called at unbind time however and so calling
> dev_pm_domain_detach() is missed and the pm domain keeps active.
>
> To fix this always use the core driver callbacks and handle missing amba
> callbacks there. For probe refuse registration as a driver without probe
> doesn't make sense.
>
> Fixes: 7cfe249475fd ("ARM: AMBA: Add pclk support to AMBA bus infrastructure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>


> ---
>  drivers/amba/bus.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 8658e0533b67..8c4a42df47c6 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -300,10 +300,11 @@ static int amba_remove(struct device *dev)
>  {
>         struct amba_device *pcdev = to_amba_device(dev);
>         struct amba_driver *drv = to_amba_driver(dev->driver);
> -       int ret;
> +       int ret = 0;
>
>         pm_runtime_get_sync(dev);
> -       ret = drv->remove(pcdev);
> +       if (drv->remove)
> +               ret = drv->remove(pcdev);
>         pm_runtime_put_noidle(dev);
>
>         /* Undo the runtime PM settings in amba_probe() */
> @@ -320,7 +321,9 @@ static int amba_remove(struct device *dev)
>  static void amba_shutdown(struct device *dev)
>  {
>         struct amba_driver *drv = to_amba_driver(dev->driver);
> -       drv->shutdown(to_amba_device(dev));
> +
> +       if (drv->shutdown)
> +               drv->shutdown(to_amba_device(dev));
>  }
>
>  /**
> @@ -333,12 +336,13 @@ static void amba_shutdown(struct device *dev)
>   */
>  int amba_driver_register(struct amba_driver *drv)
>  {
> -       drv->drv.bus = &amba_bustype;
> +       if (!drv->probe)
> +               return -EINVAL;
>
> -#define SETFN(fn)      if (drv->fn) drv->drv.fn = amba_##fn
> -       SETFN(probe);
> -       SETFN(remove);
> -       SETFN(shutdown);
> +       drv->drv.bus = &amba_bustype;
> +       drv->drv.probe = amba_probe;
> +       drv->drv.remove = amba_remove;
> +       drv->drv.shutdown = amba_shutdown;
>
>         return driver_register(&drv->drv);
>  }
> --
> 2.29.2
>

[PATCH 3/4] amba: Make the remove callback return void

[Uwe Kleine-König]

Most amba drivers return 0 in their remove callback. Together with the
driver core ignoring the return value anyhow, it doesn't make sense to
return a value here.

So add a warning to the only driver that could return an error code and
change the remove prototype to return void, which makes it explicit that
returning an error value doesn't work as expected. This simplifies changing
the core remove callback to return void, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/amba/bus.c                                 | 5 ++---
 drivers/char/hw_random/nomadik-rng.c               | 3 +--
 drivers/dma/pl330.c                                | 3 +--
 drivers/gpu/drm/pl111/pl111_drv.c                  | 4 +---
 drivers/hwtracing/coresight/coresight-catu.c       | 3 +--
 drivers/hwtracing/coresight/coresight-cpu-debug.c  | 4 +---
 drivers/hwtracing/coresight/coresight-cti-core.c   | 4 +---
 drivers/hwtracing/coresight/coresight-etb10.c      | 4 +---
 drivers/hwtracing/coresight/coresight-etm3x-core.c | 4 +---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +---
 drivers/hwtracing/coresight/coresight-funnel.c     | 4 ++--
 drivers/hwtracing/coresight/coresight-replicator.c | 2 +-
 drivers/hwtracing/coresight/coresight-stm.c        | 4 +---
 drivers/hwtracing/coresight/coresight-tmc-core.c   | 4 +---
 drivers/hwtracing/coresight/coresight-tpiu.c       | 4 +---
 drivers/i2c/busses/i2c-nomadik.c                   | 4 +---
 drivers/input/serio/ambakmi.c                      | 3 +--
 drivers/memory/pl172.c                             | 4 +---
 drivers/memory/pl353-smc.c                         | 4 +---
 drivers/mmc/host/mmci.c                            | 4 +---
 drivers/rtc/rtc-pl030.c                            | 4 +---
 drivers/rtc/rtc-pl031.c                            | 4 +---
 drivers/spi/spi-pl022.c                            | 5 ++---
 drivers/tty/serial/amba-pl010.c                    | 4 +---
 drivers/tty/serial/amba-pl011.c                    | 3 +--
 drivers/vfio/platform/vfio_amba.c                  | 7 +++----
 drivers/video/fbdev/amba-clcd.c                    | 4 +---
 drivers/watchdog/sp805_wdt.c                       | 4 +---
 include/linux/amba/bus.h                           | 2 +-
 sound/arm/aaci.c                                   | 4 +---
 30 files changed, 35 insertions(+), 81 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 8c4a42df47c6..48b5d4b4e889 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -300,11 +300,10 @@ static int amba_remove(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	int ret = 0;
 
 	pm_runtime_get_sync(dev);
 	if (drv->remove)
-		ret = drv->remove(pcdev);
+		drv->remove(pcdev);
 	pm_runtime_put_noidle(dev);
 
 	/* Undo the runtime PM settings in amba_probe() */
@@ -315,7 +314,7 @@ static int amba_remove(struct device *dev)
 	amba_put_disable_pclk(pcdev);
 	dev_pm_domain_detach(dev, true);
 
-	return ret;
+	return 0;
 }
 
 static void amba_shutdown(struct device *dev)
diff --git a/drivers/char/hw_random/nomadik-rng.c b/drivers/char/hw_random/nomadik-rng.c
index b0ded41eb865..67947a19aa22 100644
--- a/drivers/char/hw_random/nomadik-rng.c
+++ b/drivers/char/hw_random/nomadik-rng.c
@@ -69,11 +69,10 @@ static int nmk_rng_probe(struct amba_device *dev, const struct amba_id *id)
 	return ret;
 }
 
-static int nmk_rng_remove(struct amba_device *dev)
+static void nmk_rng_remove(struct amba_device *dev)
 {
 	amba_release_regions(dev);
 	clk_disable(rng_clk);
-	return 0;
 }
 
 static const struct amba_id nmk_rng_ids[] = {
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index bc0f66af0f11..fd8d2bc3be9f 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3195,7 +3195,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int pl330_remove(struct amba_device *adev)
+static void pl330_remove(struct amba_device *adev)
 {
 	struct pl330_dmac *pl330 = amba_get_drvdata(adev);
 	struct dma_pl330_chan *pch, *_p;
@@ -3235,7 +3235,6 @@ static int pl330_remove(struct amba_device *adev)
 
 	if (pl330->rstc)
 		reset_control_assert(pl330->rstc);
-	return 0;
 }
 
 static const struct amba_id pl330_ids[] = {
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 40e6708fbbe2..1fb5eacefd2d 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -320,7 +320,7 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	return ret;
 }
 
-static int pl111_amba_remove(struct amba_device *amba_dev)
+static void pl111_amba_remove(struct amba_device *amba_dev)
 {
 	struct device *dev = &amba_dev->dev;
 	struct drm_device *drm = amba_get_drvdata(amba_dev);
@@ -331,8 +331,6 @@ static int pl111_amba_remove(struct amba_device *amba_dev)
 		drm_panel_bridge_remove(priv->bridge);
 	drm_dev_put(drm);
 	of_reserved_mem_device_release(dev);
-
-	return 0;
 }
 
 /*
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 99430f6cf5a5..3ea6a5f7f93d 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -567,12 +567,11 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int __exit catu_remove(struct amba_device *adev)
+static void __exit catu_remove(struct amba_device *adev)
 {
 	struct catu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
 	coresight_unregister(drvdata->csdev);
-	return 0;
 }
 
 static struct amba_id catu_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index e1d232411d8d..2dcf13de751f 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -627,7 +627,7 @@ static int debug_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int debug_remove(struct amba_device *adev)
+static void debug_remove(struct amba_device *adev)
 {
 	struct device *dev = &adev->dev;
 	struct debug_drvdata *drvdata = amba_get_drvdata(adev);
@@ -642,8 +642,6 @@ static int debug_remove(struct amba_device *adev)
 
 	if (!--debug_count)
 		debug_func_exit();
-
-	return 0;
 }
 
 static const struct amba_cs_uci_id uci_id_debug[] = {
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
index d28eae93e55c..d25c6f0150ad 100644
--- a/drivers/hwtracing/coresight/coresight-cti-core.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -836,7 +836,7 @@ static void cti_device_release(struct device *dev)
 	if (drvdata->csdev_release)
 		drvdata->csdev_release(dev);
 }
-static int __exit cti_remove(struct amba_device *adev)
+static void __exit cti_remove(struct amba_device *adev)
 {
 	struct cti_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -845,8 +845,6 @@ static int __exit cti_remove(struct amba_device *adev)
 	mutex_unlock(&ect_mutex);
 
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 static int cti_probe(struct amba_device *adev, const struct amba_id *id)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 1b320ab581ca..0f9ec21a9eda 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -803,7 +803,7 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int __exit etb_remove(struct amba_device *adev)
+static void __exit etb_remove(struct amba_device *adev)
 {
 	struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -814,8 +814,6 @@ static int __exit etb_remove(struct amba_device *adev)
 	 */
 	misc_deregister(&drvdata->miscdev);
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 47f610b1c2b1..5f7a008a1af2 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -909,7 +909,7 @@ static void __exit clear_etmdrvdata(void *info)
 	etmdrvdata[cpu] = NULL;
 }
 
-static int __exit etm_remove(struct amba_device *adev)
+static void __exit etm_remove(struct amba_device *adev)
 {
 	struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -932,8 +932,6 @@ static int __exit etm_remove(struct amba_device *adev)
 	cpus_read_unlock();
 
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d78a37b6592c..40dd59233810 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1582,7 +1582,7 @@ static void __exit clear_etmdrvdata(void *info)
 	etmdrvdata[cpu] = NULL;
 }
 
-static int __exit etm4_remove(struct amba_device *adev)
+static void __exit etm4_remove(struct amba_device *adev)
 {
 	struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -1605,8 +1605,6 @@ static int __exit etm4_remove(struct amba_device *adev)
 	cpus_read_unlock();
 
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 static const struct amba_id etm4_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 39be46b74dfe..43431001fd72 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -370,9 +370,9 @@ static int dynamic_funnel_probe(struct amba_device *adev,
 	return funnel_probe(&adev->dev, &adev->res);
 }
 
-static int __exit dynamic_funnel_remove(struct amba_device *adev)
+static void __exit dynamic_funnel_remove(struct amba_device *adev)
 {
-	return funnel_remove(&adev->dev);
+	funnel_remove(&adev->dev);
 }
 
 static const struct amba_id dynamic_funnel_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 6772f23e5c4b..de71058d131d 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -390,7 +390,7 @@ static int dynamic_replicator_probe(struct amba_device *adev,
 
 static int __exit dynamic_replicator_remove(struct amba_device *adev)
 {
-	return replicator_remove(&adev->dev);
+	replicator_remove(&adev->dev);
 }
 
 static const struct amba_id dynamic_replicator_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index b0ad912651a9..33c5b3080c81 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -951,15 +951,13 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int __exit stm_remove(struct amba_device *adev)
+static void __exit stm_remove(struct amba_device *adev)
 {
 	struct stm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
 	coresight_unregister(drvdata->csdev);
 
 	stm_unregister_device(&drvdata->stm);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 5653e0945c74..bd159905dccd 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -559,7 +559,7 @@ static void tmc_shutdown(struct amba_device *adev)
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 }
 
-static int __exit tmc_remove(struct amba_device *adev)
+static void __exit tmc_remove(struct amba_device *adev)
 {
 	struct tmc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
@@ -570,8 +570,6 @@ static int __exit tmc_remove(struct amba_device *adev)
 	 */
 	misc_deregister(&drvdata->miscdev);
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 static const struct amba_id tmc_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 566c57e03596..934eee4b0e60 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -173,13 +173,11 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
 	return PTR_ERR(drvdata->csdev);
 }
 
-static int __exit tpiu_remove(struct amba_device *adev)
+static void __exit tpiu_remove(struct amba_device *adev)
 {
 	struct tpiu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
 
 	coresight_unregister(drvdata->csdev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index d4b1b0865f67..a3363b20f168 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -1055,7 +1055,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int nmk_i2c_remove(struct amba_device *adev)
+static void nmk_i2c_remove(struct amba_device *adev)
 {
 	struct resource *res = &adev->res;
 	struct nmk_i2c_dev *dev = amba_get_drvdata(adev);
@@ -1068,8 +1068,6 @@ static int nmk_i2c_remove(struct amba_device *adev)
 	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
 	clk_disable_unprepare(dev->clk);
 	release_mem_region(res->start, resource_size(res));
-
-	return 0;
 }
 
 static struct i2c_vendor_data vendor_stn8815 = {
diff --git a/drivers/input/serio/ambakmi.c b/drivers/input/serio/ambakmi.c
index ecdeca147ed7..4408245b61d2 100644
--- a/drivers/input/serio/ambakmi.c
+++ b/drivers/input/serio/ambakmi.c
@@ -159,7 +159,7 @@ static int amba_kmi_probe(struct amba_device *dev,
 	return ret;
 }
 
-static int amba_kmi_remove(struct amba_device *dev)
+static void amba_kmi_remove(struct amba_device *dev)
 {
 	struct amba_kmi_port *kmi = amba_get_drvdata(dev);
 
@@ -168,7 +168,6 @@ static int amba_kmi_remove(struct amba_device *dev)
 	iounmap(kmi->base);
 	kfree(kmi);
 	amba_release_regions(dev);
-	return 0;
 }
 
 static int __maybe_unused amba_kmi_resume(struct device *dev)
diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c
index 575fadbffa30..9eb8cc7de494 100644
--- a/drivers/memory/pl172.c
+++ b/drivers/memory/pl172.c
@@ -273,14 +273,12 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int pl172_remove(struct amba_device *adev)
+static void pl172_remove(struct amba_device *adev)
 {
 	struct pl172_data *pl172 = amba_get_drvdata(adev);
 
 	clk_disable_unprepare(pl172->clk);
 	amba_release_regions(adev);
-
-	return 0;
 }
 
 static const struct amba_id pl172_ids[] = {
diff --git a/drivers/memory/pl353-smc.c b/drivers/memory/pl353-smc.c
index 73bd3023202f..3b5b1045edd9 100644
--- a/drivers/memory/pl353-smc.c
+++ b/drivers/memory/pl353-smc.c
@@ -426,14 +426,12 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
 	return err;
 }
 
-static int pl353_smc_remove(struct amba_device *adev)
+static void pl353_smc_remove(struct amba_device *adev)
 {
 	struct pl353_smc_data *pl353_smc = amba_get_drvdata(adev);
 
 	clk_disable_unprepare(pl353_smc->memclk);
 	clk_disable_unprepare(pl353_smc->aclk);
-
-	return 0;
 }
 
 static const struct amba_id pl353_ids[] = {
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b5a41a7ce165..32f52d070bbd 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -2195,7 +2195,7 @@ static int mmci_probe(struct amba_device *dev,
 	return ret;
 }
 
-static int mmci_remove(struct amba_device *dev)
+static void mmci_remove(struct amba_device *dev)
 {
 	struct mmc_host *mmc = amba_get_drvdata(dev);
 
@@ -2223,8 +2223,6 @@ static int mmci_remove(struct amba_device *dev)
 		clk_disable_unprepare(host->clk);
 		mmc_free_host(mmc);
 	}
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
index 5a880516f3e8..39038c0754ee 100644
--- a/drivers/rtc/rtc-pl030.c
+++ b/drivers/rtc/rtc-pl030.c
@@ -137,7 +137,7 @@ static int pl030_probe(struct amba_device *dev, const struct amba_id *id)
 	return ret;
 }
 
-static int pl030_remove(struct amba_device *dev)
+static void pl030_remove(struct amba_device *dev)
 {
 	struct pl030_rtc *rtc = amba_get_drvdata(dev);
 
@@ -146,8 +146,6 @@ static int pl030_remove(struct amba_device *dev)
 	free_irq(dev->irq[0], rtc);
 	iounmap(rtc->base);
 	amba_release_regions(dev);
-
-	return 0;
 }
 
 static struct amba_id pl030_ids[] = {
diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
index 224bbf096262..620c8dc33647 100644
--- a/drivers/rtc/rtc-pl031.c
+++ b/drivers/rtc/rtc-pl031.c
@@ -280,7 +280,7 @@ static int pl031_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	return 0;
 }
 
-static int pl031_remove(struct amba_device *adev)
+static void pl031_remove(struct amba_device *adev)
 {
 	struct pl031_local *ldata = dev_get_drvdata(&adev->dev);
 
@@ -289,8 +289,6 @@ static int pl031_remove(struct amba_device *adev)
 	if (adev->irq[0])
 		free_irq(adev->irq[0], ldata);
 	amba_release_regions(adev);
-
-	return 0;
 }
 
 static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index d1776fea287e..fd74ddfbb686 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2314,13 +2314,13 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	return status;
 }
 
-static int
+static void
 pl022_remove(struct amba_device *adev)
 {
 	struct pl022 *pl022 = amba_get_drvdata(adev);
 
 	if (!pl022)
-		return 0;
+		return;
 
 	/*
 	 * undo pm_runtime_put() in probe.  I assume that we're not
@@ -2335,7 +2335,6 @@ pl022_remove(struct amba_device *adev)
 	clk_disable_unprepare(pl022->clk);
 	amba_release_regions(adev);
 	tasklet_disable(&pl022->pump_transfers);
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index 3284f34e9dfe..3f96edfe569c 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -754,7 +754,7 @@ static int pl010_probe(struct amba_device *dev, const struct amba_id *id)
 	return ret;
 }
 
-static int pl010_remove(struct amba_device *dev)
+static void pl010_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 	int i;
@@ -770,8 +770,6 @@ static int pl010_remove(struct amba_device *dev)
 
 	if (!busy)
 		uart_unregister_driver(&amba_reg);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index c255476cce28..4ead0c9048a8 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2679,13 +2679,12 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	return pl011_register_port(uap);
 }
 
-static int pl011_remove(struct amba_device *dev)
+static void pl011_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 
 	uart_remove_one_port(&amba_reg, &uap->port);
 	pl011_unregister_port(uap);
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index 9636a2afaecd..47cf8d869289 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -71,7 +71,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int vfio_amba_remove(struct amba_device *adev)
+static void vfio_amba_remove(struct amba_device *adev)
 {
 	struct vfio_platform_device *vdev;
 
@@ -79,10 +79,9 @@ static int vfio_amba_remove(struct amba_device *adev)
 	if (vdev) {
 		kfree(vdev->name);
 		kfree(vdev);
-		return 0;
+	} else {
+		dev_warn(&adev->dev, "removing device failed\n");
 	}
-
-	return -EINVAL;
 }
 
 static const struct amba_id pl330_ids[] = {
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index b7682de412d8..33595cc4778e 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -925,7 +925,7 @@ static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
 	return ret;
 }
 
-static int clcdfb_remove(struct amba_device *dev)
+static void clcdfb_remove(struct amba_device *dev)
 {
 	struct clcd_fb *fb = amba_get_drvdata(dev);
 
@@ -942,8 +942,6 @@ static int clcdfb_remove(struct amba_device *dev)
 	kfree(fb);
 
 	amba_release_regions(dev);
-
-	return 0;
 }
 
 static const struct amba_id clcdfb_id_table[] = {
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 190d26e2e75f..2815f78d22bb 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -304,14 +304,12 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
-static int sp805_wdt_remove(struct amba_device *adev)
+static void sp805_wdt_remove(struct amba_device *adev)
 {
 	struct sp805_wdt *wdt = amba_get_drvdata(adev);
 
 	watchdog_unregister_device(&wdt->wdd);
 	watchdog_set_drvdata(&wdt->wdd, NULL);
-
-	return 0;
 }
 
 static int __maybe_unused sp805_wdt_suspend(struct device *dev)
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 0bbfd647f5c6..6cc93ab5b809 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -76,7 +76,7 @@ struct amba_device {
 struct amba_driver {
 	struct device_driver	drv;
 	int			(*probe)(struct amba_device *, const struct amba_id *);
-	int			(*remove)(struct amba_device *);
+	void			(*remove)(struct amba_device *);
 	void			(*shutdown)(struct amba_device *);
 	const struct amba_id	*id_table;
 };
diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index a0996c47e58f..b326a5f5f0d5 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -1055,7 +1055,7 @@ static int aaci_probe(struct amba_device *dev,
 	return ret;
 }
 
-static int aaci_remove(struct amba_device *dev)
+static void aaci_remove(struct amba_device *dev)
 {
 	struct snd_card *card = amba_get_drvdata(dev);
 
@@ -1066,8 +1066,6 @@ static int aaci_remove(struct amba_device *dev)
 		snd_card_free(card);
 		amba_release_regions(dev);
 	}
-
-	return 0;
 }
 
 static struct amba_id aaci_ids[] = {
-- 
2.29.2

Re: [PATCH 3/4] amba: Make the remove callback return void

[Ulf Hansson]

On Tue, 24 Nov 2020 at 11:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Most amba drivers return 0 in their remove callback. Together with the
> driver core ignoring the return value anyhow, it doesn't make sense to
> return a value here.
>
> So add a warning to the only driver that could return an error code and
> change the remove prototype to return void, which makes it explicit that
> returning an error value doesn't work as expected. This simplifies changing
> the core remove callback to return void, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

[...]

> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index 6772f23e5c4b..de71058d131d 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -390,7 +390,7 @@ static int dynamic_replicator_probe(struct amba_device *adev,
>
>  static int __exit dynamic_replicator_remove(struct amba_device *adev)

Looks like you missed to convert this to void.

>  {
> -       return replicator_remove(&adev->dev);
> +       replicator_remove(&adev->dev);
>  }

Other the above, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

Kind regards
Uffe

Re: [PATCH 3/4] amba: Make the remove callback return void

[Arnd Bergmann]

On Tue, Nov 24, 2020 at 11:32 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Most amba drivers return 0 in their remove callback. Together with the
> driver core ignoring the return value anyhow, it doesn't make sense to
> return a value here.
>
> So add a warning to the only driver that could return an error code and
> change the remove prototype to return void, which makes it explicit that
> returning an error value doesn't work as expected. This simplifies changing
> the core remove callback to return void, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Makes sense, I checked that the vfio driver cannot actually return an error
either, the 'if (vdev)' check seems to be done out of unnecessary caution,
as the probe would never have succeeded if that was NULL.

         Arnd

Re: [PATCH 3/4] amba: Make the remove callback return void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On Tue, Nov 24, 2020 at 12:48:05PM +0100, Arnd Bergmann wrote:
> On Tue, Nov 24, 2020 at 11:32 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Most amba drivers return 0 in their remove callback. Together with the
> > driver core ignoring the return value anyhow, it doesn't make sense to
> > return a value here.
> >
> > So add a warning to the only driver that could return an error code and
> > change the remove prototype to return void, which makes it explicit that
> > returning an error value doesn't work as expected. This simplifies changing
> > the core remove callback to return void, too.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Makes sense, I checked that the vfio driver cannot actually return an error
> either, the 'if (vdev)' check seems to be done out of unnecessary caution,
> as the probe would never have succeeded if that was NULL.

Yes I thought this possible, but didn't tried to wrap my head around the
logic there. IMHO this warrants a cleanup, will address this in v2.

Thanks for your feedback,
Uwe

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

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

[PATCH 4/4] amba: Make use of bus_type functions

[Uwe Kleine-König]

Instead of assigning the needed functions for each driver separately do it
only once in amba_bustype. Move the definition of the functions to their
proper place among the other callbacks used there.

This prepares getting rid of these callbacks in struct device_driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/amba/bus.c | 154 ++++++++++++++++++++++-----------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 48b5d4b4e889..2f3799ac3edb 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -174,6 +174,80 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return retval;
 }
 
+/*
+ * These are the device model conversion veneers; they convert the
+ * device model structures to our more specific structures.
+ */
+static int amba_probe(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
+	const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
+	int ret;
+
+	do {
+		ret = of_clk_set_defaults(dev->of_node, false);
+		if (ret < 0)
+			break;
+
+		ret = dev_pm_domain_attach(dev, true);
+		if (ret)
+			break;
+
+		ret = amba_get_enable_pclk(pcdev);
+		if (ret) {
+			dev_pm_domain_detach(dev, true);
+			break;
+		}
+
+		pm_runtime_get_noresume(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+
+		ret = pcdrv->probe(pcdev, id);
+		if (ret == 0)
+			break;
+
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_put_noidle(dev);
+
+		amba_put_disable_pclk(pcdev);
+		dev_pm_domain_detach(dev, true);
+	} while (0);
+
+	return ret;
+}
+
+static int amba_remove(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	struct amba_driver *drv = to_amba_driver(dev->driver);
+
+	pm_runtime_get_sync(dev);
+	if (drv->remove)
+		drv->remove(pcdev);
+	pm_runtime_put_noidle(dev);
+
+	/* Undo the runtime PM settings in amba_probe() */
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+
+	amba_put_disable_pclk(pcdev);
+	dev_pm_domain_detach(dev, true);
+
+	return 0;
+}
+
+static void amba_shutdown(struct device *dev)
+{
+	struct amba_driver *drv = to_amba_driver(dev->driver);
+
+	if (drv->shutdown)
+		drv->shutdown(to_amba_device(dev));
+}
+
 #ifdef CONFIG_PM
 /*
  * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
@@ -239,6 +313,9 @@ struct bus_type amba_bustype = {
 	.dev_groups	= amba_dev_groups,
 	.match		= amba_match,
 	.uevent		= amba_uevent,
+	.probe		= amba_probe,
+	.remove		= amba_remove,
+	.shutdown	= amba_shutdown,
 	.dma_configure	= platform_dma_configure,
 	.pm		= &amba_pm,
 };
@@ -251,80 +328,6 @@ static int __init amba_init(void)
 
 postcore_initcall(amba_init);
 
-/*
- * These are the device model conversion veneers; they convert the
- * device model structures to our more specific structures.
- */
-static int amba_probe(struct device *dev)
-{
-	struct amba_device *pcdev = to_amba_device(dev);
-	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
-	const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
-	int ret;
-
-	do {
-		ret = of_clk_set_defaults(dev->of_node, false);
-		if (ret < 0)
-			break;
-
-		ret = dev_pm_domain_attach(dev, true);
-		if (ret)
-			break;
-
-		ret = amba_get_enable_pclk(pcdev);
-		if (ret) {
-			dev_pm_domain_detach(dev, true);
-			break;
-		}
-
-		pm_runtime_get_noresume(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-
-		ret = pcdrv->probe(pcdev, id);
-		if (ret == 0)
-			break;
-
-		pm_runtime_disable(dev);
-		pm_runtime_set_suspended(dev);
-		pm_runtime_put_noidle(dev);
-
-		amba_put_disable_pclk(pcdev);
-		dev_pm_domain_detach(dev, true);
-	} while (0);
-
-	return ret;
-}
-
-static int amba_remove(struct device *dev)
-{
-	struct amba_device *pcdev = to_amba_device(dev);
-	struct amba_driver *drv = to_amba_driver(dev->driver);
-
-	pm_runtime_get_sync(dev);
-	if (drv->remove)
-		drv->remove(pcdev);
-	pm_runtime_put_noidle(dev);
-
-	/* Undo the runtime PM settings in amba_probe() */
-	pm_runtime_disable(dev);
-	pm_runtime_set_suspended(dev);
-	pm_runtime_put_noidle(dev);
-
-	amba_put_disable_pclk(pcdev);
-	dev_pm_domain_detach(dev, true);
-
-	return 0;
-}
-
-static void amba_shutdown(struct device *dev)
-{
-	struct amba_driver *drv = to_amba_driver(dev->driver);
-
-	if (drv->shutdown)
-		drv->shutdown(to_amba_device(dev));
-}
-
 /**
  *	amba_driver_register - register an AMBA device driver
  *	@drv: amba device driver structure
@@ -339,9 +342,6 @@ int amba_driver_register(struct amba_driver *drv)
 		return -EINVAL;
 
 	drv->drv.bus = &amba_bustype;
-	drv->drv.probe = amba_probe;
-	drv->drv.remove = amba_remove;
-	drv->drv.shutdown = amba_shutdown;
 
 	return driver_register(&drv->drv);
 }
-- 
2.29.2

Re: [PATCH 4/4] amba: Make use of bus_type functions

[Ulf Hansson]

On Tue, 24 Nov 2020 at 11:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Instead of assigning the needed functions for each driver separately do it
> only once in amba_bustype. Move the definition of the functions to their
> proper place among the other callbacks used there.
>
> This prepares getting rid of these callbacks in struct device_driver.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/amba/bus.c | 154 ++++++++++++++++++++++-----------------------
>  1 file changed, 77 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 48b5d4b4e889..2f3799ac3edb 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -174,6 +174,80 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
>         return retval;
>  }
>
> +/*
> + * These are the device model conversion veneers; they convert the
> + * device model structures to our more specific structures.
> + */
> +static int amba_probe(struct device *dev)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +       struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> +       const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> +       int ret;
> +
> +       do {
> +               ret = of_clk_set_defaults(dev->of_node, false);
> +               if (ret < 0)
> +                       break;
> +
> +               ret = dev_pm_domain_attach(dev, true);
> +               if (ret)
> +                       break;
> +
> +               ret = amba_get_enable_pclk(pcdev);
> +               if (ret) {
> +                       dev_pm_domain_detach(dev, true);
> +                       break;
> +               }
> +
> +               pm_runtime_get_noresume(dev);
> +               pm_runtime_set_active(dev);
> +               pm_runtime_enable(dev);
> +
> +               ret = pcdrv->probe(pcdev, id);
> +               if (ret == 0)
> +                       break;
> +
> +               pm_runtime_disable(dev);
> +               pm_runtime_set_suspended(dev);
> +               pm_runtime_put_noidle(dev);
> +
> +               amba_put_disable_pclk(pcdev);
> +               dev_pm_domain_detach(dev, true);
> +       } while (0);
> +
> +       return ret;
> +}
> +
> +static int amba_remove(struct device *dev)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +       struct amba_driver *drv = to_amba_driver(dev->driver);
> +
> +       pm_runtime_get_sync(dev);
> +       if (drv->remove)
> +               drv->remove(pcdev);
> +       pm_runtime_put_noidle(dev);
> +
> +       /* Undo the runtime PM settings in amba_probe() */
> +       pm_runtime_disable(dev);
> +       pm_runtime_set_suspended(dev);
> +       pm_runtime_put_noidle(dev);
> +
> +       amba_put_disable_pclk(pcdev);
> +       dev_pm_domain_detach(dev, true);
> +
> +       return 0;
> +}
> +
> +static void amba_shutdown(struct device *dev)
> +{
> +       struct amba_driver *drv = to_amba_driver(dev->driver);
> +
> +       if (drv->shutdown)
> +               drv->shutdown(to_amba_device(dev));
> +}
> +
>  #ifdef CONFIG_PM
>  /*
>   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> @@ -239,6 +313,9 @@ struct bus_type amba_bustype = {
>         .dev_groups     = amba_dev_groups,
>         .match          = amba_match,
>         .uevent         = amba_uevent,
> +       .probe          = amba_probe,
> +       .remove         = amba_remove,
> +       .shutdown       = amba_shutdown,
>         .dma_configure  = platform_dma_configure,
>         .pm             = &amba_pm,
>  };
> @@ -251,80 +328,6 @@ static int __init amba_init(void)
>
>  postcore_initcall(amba_init);
>
> -/*
> - * These are the device model conversion veneers; they convert the
> - * device model structures to our more specific structures.
> - */
> -static int amba_probe(struct device *dev)
> -{
> -       struct amba_device *pcdev = to_amba_device(dev);
> -       struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> -       const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> -       int ret;
> -
> -       do {
> -               ret = of_clk_set_defaults(dev->of_node, false);
> -               if (ret < 0)
> -                       break;
> -
> -               ret = dev_pm_domain_attach(dev, true);
> -               if (ret)
> -                       break;
> -
> -               ret = amba_get_enable_pclk(pcdev);
> -               if (ret) {
> -                       dev_pm_domain_detach(dev, true);
> -                       break;
> -               }
> -
> -               pm_runtime_get_noresume(dev);
> -               pm_runtime_set_active(dev);
> -               pm_runtime_enable(dev);
> -
> -               ret = pcdrv->probe(pcdev, id);
> -               if (ret == 0)
> -                       break;
> -
> -               pm_runtime_disable(dev);
> -               pm_runtime_set_suspended(dev);
> -               pm_runtime_put_noidle(dev);
> -
> -               amba_put_disable_pclk(pcdev);
> -               dev_pm_domain_detach(dev, true);
> -       } while (0);
> -
> -       return ret;
> -}
> -
> -static int amba_remove(struct device *dev)
> -{
> -       struct amba_device *pcdev = to_amba_device(dev);
> -       struct amba_driver *drv = to_amba_driver(dev->driver);
> -
> -       pm_runtime_get_sync(dev);
> -       if (drv->remove)
> -               drv->remove(pcdev);
> -       pm_runtime_put_noidle(dev);
> -
> -       /* Undo the runtime PM settings in amba_probe() */
> -       pm_runtime_disable(dev);
> -       pm_runtime_set_suspended(dev);
> -       pm_runtime_put_noidle(dev);
> -
> -       amba_put_disable_pclk(pcdev);
> -       dev_pm_domain_detach(dev, true);
> -
> -       return 0;
> -}
> -
> -static void amba_shutdown(struct device *dev)
> -{
> -       struct amba_driver *drv = to_amba_driver(dev->driver);
> -
> -       if (drv->shutdown)
> -               drv->shutdown(to_amba_device(dev));
> -}
> -
>  /**
>   *     amba_driver_register - register an AMBA device driver
>   *     @drv: amba device driver structure
> @@ -339,9 +342,6 @@ int amba_driver_register(struct amba_driver *drv)
>                 return -EINVAL;
>
>         drv->drv.bus = &amba_bustype;
> -       drv->drv.probe = amba_probe;
> -       drv->drv.remove = amba_remove;
> -       drv->drv.shutdown = amba_shutdown;
>
>         return driver_register(&drv->drv);
>  }
> --
> 2.29.2
>

Re: [PATCH 1/4] amba: reorder functions

[Ulf Hansson]

On Tue, 24 Nov 2020 at 11:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Put helpers (here: amba_get_enable_pclk and amba_put_disable_pclk) at
> the top of the file and then define callbacks directly before the
> structs they are used in; in the same order.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>


> ---
>  drivers/amba/bus.c | 77 +++++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index ecc304149067..8658e0533b67 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -56,31 +56,28 @@ amba_lookup(const struct amba_id *table, struct amba_device *dev)
>         return NULL;
>  }
>
> -static int amba_match(struct device *dev, struct device_driver *drv)
> +static int amba_get_enable_pclk(struct amba_device *pcdev)
>  {
> -       struct amba_device *pcdev = to_amba_device(dev);
> -       struct amba_driver *pcdrv = to_amba_driver(drv);
> +       int ret;
>
> -       /* When driver_override is set, only bind to the matching driver */
> -       if (pcdev->driver_override)
> -               return !strcmp(pcdev->driver_override, drv->name);
> +       pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
> +       if (IS_ERR(pcdev->pclk))
> +               return PTR_ERR(pcdev->pclk);
>
> -       return amba_lookup(pcdrv->id_table, pcdev) != NULL;
> +       ret = clk_prepare_enable(pcdev->pclk);
> +       if (ret)
> +               clk_put(pcdev->pclk);
> +
> +       return ret;
>  }
>
> -static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
> +static void amba_put_disable_pclk(struct amba_device *pcdev)
>  {
> -       struct amba_device *pcdev = to_amba_device(dev);
> -       int retval = 0;
> -
> -       retval = add_uevent_var(env, "AMBA_ID=%08x", pcdev->periphid);
> -       if (retval)
> -               return retval;
> -
> -       retval = add_uevent_var(env, "MODALIAS=amba:d%08X", pcdev->periphid);
> -       return retval;
> +       clk_disable_unprepare(pcdev->pclk);
> +       clk_put(pcdev->pclk);
>  }
>
> +
>  static ssize_t driver_override_show(struct device *_dev,
>                                     struct device_attribute *attr, char *buf)
>  {
> @@ -152,6 +149,31 @@ static struct attribute *amba_dev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(amba_dev);
>
> +static int amba_match(struct device *dev, struct device_driver *drv)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +       struct amba_driver *pcdrv = to_amba_driver(drv);
> +
> +       /* When driver_override is set, only bind to the matching driver */
> +       if (pcdev->driver_override)
> +               return !strcmp(pcdev->driver_override, drv->name);
> +
> +       return amba_lookup(pcdrv->id_table, pcdev) != NULL;
> +}
> +
> +static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +       struct amba_device *pcdev = to_amba_device(dev);
> +       int retval = 0;
> +
> +       retval = add_uevent_var(env, "AMBA_ID=%08x", pcdev->periphid);
> +       if (retval)
> +               return retval;
> +
> +       retval = add_uevent_var(env, "MODALIAS=amba:d%08X", pcdev->periphid);
> +       return retval;
> +}
> +
>  #ifdef CONFIG_PM
>  /*
>   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> @@ -229,27 +251,6 @@ static int __init amba_init(void)
>
>  postcore_initcall(amba_init);
>
> -static int amba_get_enable_pclk(struct amba_device *pcdev)
> -{
> -       int ret;
> -
> -       pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
> -       if (IS_ERR(pcdev->pclk))
> -               return PTR_ERR(pcdev->pclk);
> -
> -       ret = clk_prepare_enable(pcdev->pclk);
> -       if (ret)
> -               clk_put(pcdev->pclk);
> -
> -       return ret;
> -}
> -
> -static void amba_put_disable_pclk(struct amba_device *pcdev)
> -{
> -       clk_disable_unprepare(pcdev->pclk);
> -       clk_put(pcdev->pclk);
> -}
> -
>  /*
>   * These are the device model conversion veneers; they convert the
>   * device model structures to our more specific structures.
>
> base-commit: 95065cb54210eba86bed10cb2118041524d54573
> --
> 2.29.2
>

Re: [PATCH 1/4] amba: reorder functions

[Arnd Bergmann]

On Tue, Nov 24, 2020 at 11:32 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Put helpers (here: amba_get_enable_pclk and amba_put_disable_pclk) at
> the top of the file and then define callbacks directly before the
> structs they are used in; in the same order.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Looks good, I checked the whole series and this seems like a useful
cleanup. All four patches:

Reviewed-by: Arnd Bergmann <arnd@arndb.de>