[PATCH 0/7] memory: simplify with devm() and guard()

[PATCH 0/7] memory: simplify with devm() and guard()

[Krzysztof Kozlowski]

Few cleanups/simplifications, of which second patch "memory: emif:
simplify locking with guard()" might have actual impact (RFT).

Best regards,
Krzysztof

---
Krzysztof Kozlowski (7):
      memory: emif: drop unused 'irq_state' member
      memory: emif: simplify locking with guard()
      memory: omap-gpmc: simplify locking with guard()
      memory: pl172: simplify with dev_err_probe()
      memory: pl172: simplify with devm_clk_get_enabled()
      memory: pl353-smc: simplify with dev_err_probe()
      memory: pl353-smc: simplify with devm_clk_get_enabled()

 drivers/memory/emif.c      | 18 ++++++++---------
 drivers/memory/omap-gpmc.c | 24 ++++++++++------------
 drivers/memory/pl172.c     | 30 ++++++++--------------------
 drivers/memory/pl353-smc.c | 50 +++++++++-------------------------------------
 4 files changed, 36 insertions(+), 86 deletions(-)
---
base-commit: 0aa21a46c484770ef30207ed77a9fd3f5a6a32cf
change-id: 20240823-b4-cleanup-h-guard-c30a42f76f23

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH 1/7] memory: emif: drop unused 'irq_state' member

[Krzysztof Kozlowski]

Driver does not use 'emif_data.irq_state'.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/emif.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/memory/emif.c b/drivers/memory/emif.c
index 974ed641473e..b32f3a8f9d71 100644
--- a/drivers/memory/emif.c
+++ b/drivers/memory/emif.c
@@ -57,7 +57,6 @@ struct emif_data {
 	u8				temperature_level;
 	u8				lpmode;
 	struct list_head		node;
-	unsigned long			irq_state;
 	void __iomem			*base;
 	struct device			*dev;
 	struct emif_regs		*regs_cache[EMIF_MAX_NUM_FREQUENCIES];

-- 
2.43.0

[PATCH 2/7] memory: emif: simplify locking with guard()

[Krzysztof Kozlowski]

Simplify error handling (less gotos) over locks with guard().

The driver used file-scope variable 'irq_state' for storing IRQ state
with spin_lock_irqsave, so move it into respective local scopes.  This
should be equivalent, but more readable (less global variables).

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Not tested on hardware, although I don't think that moving 'irq_state'
scope would affect anything.
---
 drivers/memory/emif.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/memory/emif.c b/drivers/memory/emif.c
index b32f3a8f9d71..99eb7d1baa5f 100644
--- a/drivers/memory/emif.c
+++ b/drivers/memory/emif.c
@@ -7,6 +7,7 @@
  * Aneesh V <aneesh@ti.com>
  * Santosh Shilimkar <santosh.shilimkar@ti.com>
  */
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/reboot.h>
@@ -68,7 +69,6 @@ struct emif_data {
 
 static struct emif_data *emif1;
 static DEFINE_SPINLOCK(emif_lock);
-static unsigned long	irq_state;
 static LIST_HEAD(device_list);
 
 static void do_emif_regdump_show(struct seq_file *s, struct emif_data *emif,
@@ -522,18 +522,18 @@ static void setup_temperature_sensitive_regs(struct emif_data *emif,
 static irqreturn_t handle_temp_alert(void __iomem *base, struct emif_data *emif)
 {
 	u32		old_temp_level;
-	irqreturn_t	ret = IRQ_HANDLED;
+	irqreturn_t	ret;
 	struct emif_custom_configs *custom_configs;
 
-	spin_lock_irqsave(&emif_lock, irq_state);
+	guard(spinlock_irqsave)(&emif_lock);
 	old_temp_level = emif->temperature_level;
 	get_temperature_level(emif);
 
 	if (unlikely(emif->temperature_level == old_temp_level)) {
-		goto out;
+		return IRQ_HANDLED;
 	} else if (!emif->curr_regs) {
 		dev_err(emif->dev, "temperature alert before registers are calculated, not de-rating timings\n");
-		goto out;
+		return IRQ_HANDLED;
 	}
 
 	custom_configs = emif->plat_data->custom_configs;
@@ -553,8 +553,7 @@ static irqreturn_t handle_temp_alert(void __iomem *base, struct emif_data *emif)
 			 * from thread context
 			 */
 			emif->temperature_level = SDRAM_TEMP_VERY_HIGH_SHUTDOWN;
-			ret = IRQ_WAKE_THREAD;
-			goto out;
+			return IRQ_WAKE_THREAD;
 		}
 	}
 
@@ -570,10 +569,9 @@ static irqreturn_t handle_temp_alert(void __iomem *base, struct emif_data *emif)
 		/* Temperature is going up - handle immediately */
 		setup_temperature_sensitive_regs(emif, emif->curr_regs);
 		do_freq_update();
+		ret = IRQ_HANDLED;
 	}
 
-out:
-	spin_unlock_irqrestore(&emif_lock, irq_state);
 	return ret;
 }
 
@@ -616,6 +614,7 @@ static irqreturn_t emif_interrupt_handler(int irq, void *dev_id)
 static irqreturn_t emif_threaded_isr(int irq, void *dev_id)
 {
 	struct emif_data	*emif = dev_id;
+	unsigned long		irq_state;
 
 	if (emif->temperature_level == SDRAM_TEMP_VERY_HIGH_SHUTDOWN) {
 		dev_emerg(emif->dev, "SDRAM temperature exceeds operating limit.. Needs shut down!!!\n");

-- 
2.43.0

Re: [PATCH 2/7] memory: emif: simplify locking with guard()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:15:57 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Simplify error handling (less gotos) over locks with guard().
> 
> The driver used file-scope variable 'irq_state' for storing IRQ state
> with spin_lock_irqsave, so move it into respective local scopes.  This
> should be equivalent, but more readable (less global variables).
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
LGTM. File scoped irq_state is certainly unusual...

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>

[PATCH 3/7] memory: omap-gpmc: simplify locking with guard()

[Krzysztof Kozlowski]

Simplify error handling (less gotos) over locks with guard().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/omap-gpmc.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 80d038884207..c8a0d82f9c27 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2009 Texas Instruments
  * Added OMAP4 support - Santosh Shilimkar <santosh.shilimkar@ti.com>
  */
+#include <linux/cleanup.h>
 #include <linux/cpu_pm.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
@@ -989,18 +990,18 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
 	if (size > (1 << GPMC_SECTION_SHIFT))
 		return -ENOMEM;
 
-	spin_lock(&gpmc_mem_lock);
-	if (gpmc_cs_reserved(cs)) {
-		r = -EBUSY;
-		goto out;
-	}
+	guard(spinlock)(&gpmc_mem_lock);
+
+	if (gpmc_cs_reserved(cs))
+		return -EBUSY;
+
 	if (gpmc_cs_mem_enabled(cs))
 		r = adjust_resource(res, res->start & ~(size - 1), size);
 	if (r < 0)
 		r = allocate_resource(&gpmc_mem_root, res, size, 0, ~0,
 				      size, NULL, NULL);
 	if (r < 0)
-		goto out;
+		return r;
 
 	/* Disable CS while changing base address and size mask */
 	gpmc_cs_disable_mem(cs);
@@ -1008,16 +1009,15 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
 	r = gpmc_cs_set_memconf(cs, res->start, resource_size(res));
 	if (r < 0) {
 		release_resource(res);
-		goto out;
+		return r;
 	}
 
 	/* Enable CS */
 	gpmc_cs_enable_mem(cs);
 	*base = res->start;
 	gpmc_cs_set_reserved(cs, 1);
-out:
-	spin_unlock(&gpmc_mem_lock);
-	return r;
+
+	return 0;
 }
 EXPORT_SYMBOL(gpmc_cs_request);
 
@@ -1026,10 +1026,9 @@ void gpmc_cs_free(int cs)
 	struct gpmc_cs_data *gpmc;
 	struct resource *res;
 
-	spin_lock(&gpmc_mem_lock);
+	guard(spinlock)(&gpmc_mem_lock);
 	if (cs >= gpmc_cs_num || cs < 0 || !gpmc_cs_reserved(cs)) {
 		WARN(1, "Trying to free non-reserved GPMC CS%d\n", cs);
-		spin_unlock(&gpmc_mem_lock);
 		return;
 	}
 	gpmc = &gpmc_cs[cs];
@@ -1039,7 +1038,6 @@ void gpmc_cs_free(int cs)
 	if (res->flags)
 		release_resource(res);
 	gpmc_cs_set_reserved(cs, 0);
-	spin_unlock(&gpmc_mem_lock);
 }
 EXPORT_SYMBOL(gpmc_cs_free);
 

-- 
2.43.0

Re: [PATCH 3/7] memory: omap-gpmc: simplify locking with guard()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:15:58 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Simplify error handling (less gotos) over locks with guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Simple one. LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 4/7] memory: pl172: simplify with dev_err_probe()

[Krzysztof Kozlowski]

Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
the code also simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/pl172.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c
index 9eb8cc7de494..390012401f64 100644
--- a/drivers/memory/pl172.c
+++ b/drivers/memory/pl172.c
@@ -217,10 +217,9 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
 		return -ENOMEM;
 
 	pl172->clk = devm_clk_get(dev, "mpmcclk");
-	if (IS_ERR(pl172->clk)) {
-		dev_err(dev, "no mpmcclk provided clock\n");
-		return PTR_ERR(pl172->clk);
-	}
+	if (IS_ERR(pl172->clk))
+		return dev_err_probe(dev, PTR_ERR(pl172->clk),
+				     "no mpmcclk provided clock\n");
 
 	ret = clk_prepare_enable(pl172->clk);
 	if (ret) {

-- 
2.43.0

Re: [PATCH 4/7] memory: pl172: simplify with dev_err_probe()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:15:59 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
> ---
>  drivers/memory/pl172.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c
> index 9eb8cc7de494..390012401f64 100644
> --- a/drivers/memory/pl172.c
> +++ b/drivers/memory/pl172.c
> @@ -217,10 +217,9 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
>  		return -ENOMEM;
>  
>  	pl172->clk = devm_clk_get(dev, "mpmcclk");
> -	if (IS_ERR(pl172->clk)) {
> -		dev_err(dev, "no mpmcclk provided clock\n");
> -		return PTR_ERR(pl172->clk);
> -	}
> +	if (IS_ERR(pl172->clk))
> +		return dev_err_probe(dev, PTR_ERR(pl172->clk),
> +				     "no mpmcclk provided clock\n");
>  
>  	ret = clk_prepare_enable(pl172->clk);
>  	if (ret) {
> 

Re: [PATCH 4/7] memory: pl172: simplify with dev_err_probe()

[Vladimir Zapolskiy]

On 8/23/24 13:15, Krzysztof Kozlowski wrote:
> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
Best wishes,
Vladimir

[PATCH 5/7] memory: pl172: simplify with devm_clk_get_enabled()

[Krzysztof Kozlowski]

Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
make code simpler.  Change to dev_err_probe() in handling clk_get_rate()
error to make it even simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/pl172.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c
index 390012401f64..db5fbee34077 100644
--- a/drivers/memory/pl172.c
+++ b/drivers/memory/pl172.c
@@ -216,28 +216,20 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
 	if (!pl172)
 		return -ENOMEM;
 
-	pl172->clk = devm_clk_get(dev, "mpmcclk");
+	pl172->clk = devm_clk_get_enabled(dev, "mpmcclk");
 	if (IS_ERR(pl172->clk))
 		return dev_err_probe(dev, PTR_ERR(pl172->clk),
 				     "no mpmcclk provided clock\n");
 
-	ret = clk_prepare_enable(pl172->clk);
-	if (ret) {
-		dev_err(dev, "unable to mpmcclk enable clock\n");
-		return ret;
-	}
-
 	pl172->rate = clk_get_rate(pl172->clk) / MSEC_PER_SEC;
-	if (!pl172->rate) {
-		dev_err(dev, "unable to get mpmcclk clock rate\n");
-		ret = -EINVAL;
-		goto err_clk_enable;
-	}
+	if (!pl172->rate)
+		return dev_err_probe(dev, -EINVAL,
+				     "unable to get mpmcclk clock rate\n");
 
 	ret = amba_request_regions(adev, NULL);
 	if (ret) {
 		dev_err(dev, "unable to request AMBA regions\n");
-		goto err_clk_enable;
+		return ret;
 	}
 
 	pl172->base = devm_ioremap(dev, adev->res.start,
@@ -267,16 +259,11 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
 
 err_no_ioremap:
 	amba_release_regions(adev);
-err_clk_enable:
-	clk_disable_unprepare(pl172->clk);
 	return ret;
 }
 
 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);
 }
 

-- 
2.43.0

Re: [PATCH 5/7] memory: pl172: simplify with devm_clk_get_enabled()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:16:00 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.  Change to dev_err_probe() in handling clk_get_rate()
> error to make it even simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Follow up suggestion inline + I'd use dev_err_probe() one more
time.

Anyhow fine either way
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/memory/pl172.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/memory/pl172.c b/drivers/memory/pl172.c
> index 390012401f64..db5fbee34077 100644
> --- a/drivers/memory/pl172.c
> +++ b/drivers/memory/pl172.c
> @@ -216,28 +216,20 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
>  	if (!pl172)
>  		return -ENOMEM;
>  
> -	pl172->clk = devm_clk_get(dev, "mpmcclk");
> +	pl172->clk = devm_clk_get_enabled(dev, "mpmcclk");
>  	if (IS_ERR(pl172->clk))
>  		return dev_err_probe(dev, PTR_ERR(pl172->clk),
>  				     "no mpmcclk provided clock\n");
>  
> -	ret = clk_prepare_enable(pl172->clk);
> -	if (ret) {
> -		dev_err(dev, "unable to mpmcclk enable clock\n");
> -		return ret;
> -	}
> -
>  	pl172->rate = clk_get_rate(pl172->clk) / MSEC_PER_SEC;
> -	if (!pl172->rate) {
> -		dev_err(dev, "unable to get mpmcclk clock rate\n");
> -		ret = -EINVAL;
> -		goto err_clk_enable;
> -	}
> +	if (!pl172->rate)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "unable to get mpmcclk clock rate\n");
>  
>  	ret = amba_request_regions(adev, NULL);
Seeing as you are here an obvious follow up...

Use a devm_add_action_or_reset() and a local cleanup callback
or spin up a devm_amba_request_regions() and apply that here
and probably in other similar locations (a quick look suggested
this would be generally useful).

Then can drop remove entirely which is always nice to see + return
directly in the remaining error path.

>  	if (ret) {
>  		dev_err(dev, "unable to request AMBA regions\n");
> -		goto err_clk_enable;
> +		return ret;
I'd make this a return dev_err_probe() as well.

>  	}
>  
>  	pl172->base = devm_ioremap(dev, adev->res.start,
> @@ -267,16 +259,11 @@ static int pl172_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  err_no_ioremap:
>  	amba_release_regions(adev);
> -err_clk_enable:
> -	clk_disable_unprepare(pl172->clk);
>  	return ret;
>  }
>  
>  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);
>  }
>  
> 

Re: [PATCH 5/7] memory: pl172: simplify with devm_clk_get_enabled()

[Vladimir Zapolskiy]

On 8/23/24 13:16, Krzysztof Kozlowski wrote:
> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.  Change to dev_err_probe() in handling clk_get_rate()
> error to make it even simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
Best wishes,
Vladimir

[PATCH 6/7] memory: pl353-smc: simplify with dev_err_probe()

[Krzysztof Kozlowski]

Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
the code also simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/pl353-smc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/memory/pl353-smc.c b/drivers/memory/pl353-smc.c
index 56e51737c81f..c75b99e49970 100644
--- a/drivers/memory/pl353-smc.c
+++ b/drivers/memory/pl353-smc.c
@@ -82,16 +82,14 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
 		return -ENOMEM;
 
 	pl353_smc->aclk = devm_clk_get(&adev->dev, "apb_pclk");
-	if (IS_ERR(pl353_smc->aclk)) {
-		dev_err(&adev->dev, "aclk clock not found.\n");
-		return PTR_ERR(pl353_smc->aclk);
-	}
+	if (IS_ERR(pl353_smc->aclk))
+		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->aclk),
+				     "aclk clock not found.\n");
 
 	pl353_smc->memclk = devm_clk_get(&adev->dev, "memclk");
-	if (IS_ERR(pl353_smc->memclk)) {
-		dev_err(&adev->dev, "memclk clock not found.\n");
-		return PTR_ERR(pl353_smc->memclk);
-	}
+	if (IS_ERR(pl353_smc->memclk))
+		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->memclk),
+				     "memclk clock not found.\n");
 
 	err = clk_prepare_enable(pl353_smc->aclk);
 	if (err) {

-- 
2.43.0

Re: [PATCH 6/7] memory: pl353-smc: simplify with dev_err_probe()

[Miquel Raynal]

Hi Krzysztof,

krzysztof.kozlowski@linaro.org wrote on Fri, 23 Aug 2024 12:16:01 +0200:

> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH 6/7] memory: pl353-smc: simplify with dev_err_probe()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:16:01 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use dev_err_probe() to avoid dmesg flood on actual defer.  This makes
> the code also simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()

[Krzysztof Kozlowski]

Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
make code simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/pl353-smc.c | 36 +++---------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/memory/pl353-smc.c b/drivers/memory/pl353-smc.c
index c75b99e49970..994c7a792e34 100644
--- a/drivers/memory/pl353-smc.c
+++ b/drivers/memory/pl353-smc.c
@@ -75,34 +75,21 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
 	const struct of_device_id *match = NULL;
 	struct pl353_smc_data *pl353_smc;
 	struct device_node *child;
-	int err;
 
 	pl353_smc = devm_kzalloc(&adev->dev, sizeof(*pl353_smc), GFP_KERNEL);
 	if (!pl353_smc)
 		return -ENOMEM;
 
-	pl353_smc->aclk = devm_clk_get(&adev->dev, "apb_pclk");
+	pl353_smc->aclk = devm_clk_get_enabled(&adev->dev, "apb_pclk");
 	if (IS_ERR(pl353_smc->aclk))
 		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->aclk),
 				     "aclk clock not found.\n");
 
-	pl353_smc->memclk = devm_clk_get(&adev->dev, "memclk");
+	pl353_smc->memclk = devm_clk_get_enabled(&adev->dev, "memclk");
 	if (IS_ERR(pl353_smc->memclk))
 		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->memclk),
 				     "memclk clock not found.\n");
 
-	err = clk_prepare_enable(pl353_smc->aclk);
-	if (err) {
-		dev_err(&adev->dev, "Unable to enable AXI clock.\n");
-		return err;
-	}
-
-	err = clk_prepare_enable(pl353_smc->memclk);
-	if (err) {
-		dev_err(&adev->dev, "Unable to enable memory clock.\n");
-		goto disable_axi_clk;
-	}
-
 	amba_set_drvdata(adev, pl353_smc);
 
 	/* Find compatible children. Only a single child is supported */
@@ -115,30 +102,14 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
 		break;
 	}
 	if (!match) {
-		err = -ENODEV;
 		dev_err(&adev->dev, "no matching children\n");
-		goto disable_mem_clk;
+		return -ENODEV;
 	}
 
 	of_platform_device_create(child, NULL, &adev->dev);
 	of_node_put(child);
 
 	return 0;
-
-disable_mem_clk:
-	clk_disable_unprepare(pl353_smc->memclk);
-disable_axi_clk:
-	clk_disable_unprepare(pl353_smc->aclk);
-
-	return err;
-}
-
-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);
 }
 
 static const struct amba_id pl353_ids[] = {
@@ -157,7 +128,6 @@ static struct amba_driver pl353_smc_driver = {
 	},
 	.id_table = pl353_ids,
 	.probe = pl353_smc_probe,
-	.remove = pl353_smc_remove,
 };
 
 module_amba_driver(pl353_smc_driver);

-- 
2.43.0

Re: [PATCH 7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()

[Miquel Raynal]

Hi Krzysztof,

krzysztof.kozlowski@linaro.org wrote on Fri, 23 Aug 2024 12:16:02 +0200:

> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH 7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()

[Jonathan Cameron]

On Fri, 23 Aug 2024 12:16:02 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use devm_clk_get_enabled() to drop clock prepare/unprepare parts and
> make code simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
LGTM.
Follow up suggestion inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/memory/pl353-smc.c | 36 +++---------------------------------
>  1 file changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/memory/pl353-smc.c b/drivers/memory/pl353-smc.c
> index c75b99e49970..994c7a792e34 100644
> --- a/drivers/memory/pl353-smc.c
> +++ b/drivers/memory/pl353-smc.c
> @@ -75,34 +75,21 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
>  	const struct of_device_id *match = NULL;
>  	struct pl353_smc_data *pl353_smc;
>  	struct device_node *child;
> -	int err;
>  
>  	pl353_smc = devm_kzalloc(&adev->dev, sizeof(*pl353_smc), GFP_KERNEL);
>  	if (!pl353_smc)
>  		return -ENOMEM;
>  
> -	pl353_smc->aclk = devm_clk_get(&adev->dev, "apb_pclk");
> +	pl353_smc->aclk = devm_clk_get_enabled(&adev->dev, "apb_pclk");
>  	if (IS_ERR(pl353_smc->aclk))
>  		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->aclk),
>  				     "aclk clock not found.\n");
>  
> -	pl353_smc->memclk = devm_clk_get(&adev->dev, "memclk");
> +	pl353_smc->memclk = devm_clk_get_enabled(&adev->dev, "memclk");
>  	if (IS_ERR(pl353_smc->memclk))
>  		return dev_err_probe(&adev->dev, PTR_ERR(pl353_smc->memclk),
>  				     "memclk clock not found.\n");
>  
> -	err = clk_prepare_enable(pl353_smc->aclk);
> -	if (err) {
> -		dev_err(&adev->dev, "Unable to enable AXI clock.\n");
> -		return err;
> -	}
> -
> -	err = clk_prepare_enable(pl353_smc->memclk);
> -	if (err) {
> -		dev_err(&adev->dev, "Unable to enable memory clock.\n");
> -		goto disable_axi_clk;
> -	}
> -
>  	amba_set_drvdata(adev, pl353_smc);
>  
>  	/* Find compatible children. Only a single child is supported */
> @@ -115,30 +102,14 @@ static int pl353_smc_probe(struct amba_device *adev, const struct amba_id *id)
>  		break;
>  	}
>  	if (!match) {
With change below this becomes unconditional as we'll have already
returned in the loop for the good path.

Might as well use dev_err_probe() here as well to save a few lines.

> -		err = -ENODEV;
>  		dev_err(&adev->dev, "no matching children\n");
> -		goto disable_mem_clk;
> +		return -ENODEV;
>  	}
>  
>  	of_platform_device_create(child, NULL, &adev->dev);
>  	of_node_put(child);

An additional cleanup looks sensible here.

Push this last bit into the loop and use 

for_each_available_child_of_node_scoped()

Assuming you don't already have a patch doing that :)

>  
>  	return 0;
> -
> -disable_mem_clk:
> -	clk_disable_unprepare(pl353_smc->memclk);
> -disable_axi_clk:
> -	clk_disable_unprepare(pl353_smc->aclk);
> -
> -	return err;
> -}
> -
> -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);
>  }
>  
>  static const struct amba_id pl353_ids[] = {
> @@ -157,7 +128,6 @@ static struct amba_driver pl353_smc_driver = {
>  	},
>  	.id_table = pl353_ids,
>  	.probe = pl353_smc_probe,
> -	.remove = pl353_smc_remove,
>  };
>  
>  module_amba_driver(pl353_smc_driver);
> 

Re: [PATCH 7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()

[Krzysztof Kozlowski]

On 23/08/2024 13:56, Jonathan Cameron wrote:
> With change below this becomes unconditional as we'll have already
> returned in the loop for the good path.
> 
> Might as well use dev_err_probe() here as well to save a few lines.
> 
>> -		err = -ENODEV;
>>  		dev_err(&adev->dev, "no matching children\n");
>> -		goto disable_mem_clk;
>> +		return -ENODEV;
>>  	}
>>  
>>  	of_platform_device_create(child, NULL, &adev->dev);
>>  	of_node_put(child);
> 
> An additional cleanup looks sensible here.
> 
> Push this last bit into the loop and use 
> 
> for_each_available_child_of_node_scoped()
> 
> Assuming you don't already have a patch doing that :)

Yes, this could be simplified, thanks for the hint.

Best regards,
Krzysztof

Re: [PATCH 0/7] memory: simplify with devm() and guard()

[Krzysztof Kozlowski]

On Fri, 23 Aug 2024 12:15:55 +0200, Krzysztof Kozlowski wrote:
> Few cleanups/simplifications, of which second patch "memory: emif:
> simplify locking with guard()" might have actual impact (RFT).
> 
> Best regards,
> Krzysztof
> 

Applied, thanks!

[1/7] memory: emif: drop unused 'irq_state' member
      https://git.kernel.org/krzk/linux-mem-ctrl/c/99602b4d30359f971247f8438afac57cbd1832f0
[2/7] memory: emif: simplify locking with guard()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/f1619986d7e996eb6e53c7fe8ab32e66b17e1cf1
[3/7] memory: omap-gpmc: simplify locking with guard()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/c93ad423edd8a42a6666241595043f9193469f9e
[4/7] memory: pl172: simplify with dev_err_probe()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/8f3cb397cbc1e3b18081738af87171f960bd112e
[5/7] memory: pl172: simplify with devm_clk_get_enabled()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/610395de8496c34919f827dd194bf41af20c2bca
[6/7] memory: pl353-smc: simplify with dev_err_probe()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/49ee2e842a40dec16ed4933bf2cd993c709f12a3
[7/7] memory: pl353-smc: simplify with devm_clk_get_enabled()
      https://git.kernel.org/krzk/linux-mem-ctrl/c/ce536578e5652ec4da10ec69436d2ae2548bd619

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>