[PATCH 0/9] memory: simplify with scoped/cleanup.h for device nodes

[PATCH 0/9] memory: simplify with scoped/cleanup.h for device nodes

[Krzysztof Kozlowski]

Make code a bit simpler and smaller by using cleanup.h when handling
device nodes.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (9):
      memory: atmel-ebi: use scoped device node handling to simplify error paths
      memory: atmel-ebi: simplify with scoped for each OF child loop
      memory: samsung: exynos5422-dmc: use scoped device node handling to simplify error paths
      memory: stm32-fmc2-ebi: simplify with scoped for each OF child loop
      memory: tegra-mc: simplify with scoped for each OF child loop
      memory: tegra124-emc: simplify with scoped for each OF child loop
      memory: tegra20-emc: simplify with scoped for each OF child loop
      memory: tegra30-emc: simplify with scoped for each OF child loop
      memory: ti-aemif: simplify with scoped for each OF child loop

 drivers/memory/atmel-ebi.c              | 35 +++++++++++----------------------
 drivers/memory/samsung/exynos5422-dmc.c | 31 +++++++++++------------------
 drivers/memory/stm32-fmc2-ebi.c         |  8 +-------
 drivers/memory/tegra/mc.c               | 11 +++--------
 drivers/memory/tegra/tegra124-emc.c     |  7 ++-----
 drivers/memory/tegra/tegra20-emc.c      |  7 ++-----
 drivers/memory/tegra/tegra30-emc.c      |  7 ++-----
 drivers/memory/ti-aemif.c               | 13 ++++--------
 8 files changed, 37 insertions(+), 82 deletions(-)
---
base-commit: cf4d89333014d387065aa296160aaec5cec04cc5
change-id: 20240812-cleanup-h-of-node-put-memory-dd6de1b92917

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

[PATCH 1/9] memory: atmel-ebi: use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

Obtain the device node reference with scoped/cleanup.h to reduce error
handling and make the code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/atmel-ebi.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index e8bb5f37f5cb..fcbfc2655d8d 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
@@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
 static int atmel_ebi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *child, *np = dev->of_node, *smc_np;
+	struct device_node *child, *np = dev->of_node;
 	struct atmel_ebi *ebi;
 	int ret, reg_cells;
 	struct clk *clk;
@@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 
 	ebi->clk = clk;
 
-	smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0);
+	struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node,
+									  "atmel,smc", 0);
 
 	ebi->smc.regmap = syscon_node_to_regmap(smc_np);
-	if (IS_ERR(ebi->smc.regmap)) {
-		ret = PTR_ERR(ebi->smc.regmap);
-		goto put_node;
-	}
+	if (IS_ERR(ebi->smc.regmap))
+		return PTR_ERR(ebi->smc.regmap);
 
 	ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np);
-	if (IS_ERR(ebi->smc.layout)) {
-		ret = PTR_ERR(ebi->smc.layout);
-		goto put_node;
-	}
+	if (IS_ERR(ebi->smc.layout))
+		return PTR_ERR(ebi->smc.layout);
 
 	ebi->smc.clk = of_clk_get(smc_np, 0);
 	if (IS_ERR(ebi->smc.clk)) {
-		if (PTR_ERR(ebi->smc.clk) != -ENOENT) {
-			ret = PTR_ERR(ebi->smc.clk);
-			goto put_node;
-		}
+		if (PTR_ERR(ebi->smc.clk) != -ENOENT)
+			return PTR_ERR(ebi->smc.clk);
 
 		ebi->smc.clk = NULL;
 	}
-	of_node_put(smc_np);
 	ret = clk_prepare_enable(ebi->smc.clk);
 	if (ret)
 		return ret;
@@ -615,10 +610,6 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 	}
 
 	return of_platform_populate(np, NULL, NULL, dev);
-
-put_node:
-	of_node_put(smc_np);
-	return ret;
 }
 
 static __maybe_unused int atmel_ebi_resume(struct device *dev)

-- 
2.43.0

Re: [PATCH 1/9] memory: atmel-ebi: use scoped device node handling to simplify error paths

[Jonathan Cameron]

On Mon, 12 Aug 2024 15:33:55 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Obtain the device node reference with scoped/cleanup.h to reduce error
> handling and make the code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Hi,

Comments inline.
> ---
>  drivers/memory/atmel-ebi.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
> index e8bb5f37f5cb..fcbfc2655d8d 100644
> --- a/drivers/memory/atmel-ebi.c
> +++ b/drivers/memory/atmel-ebi.c
> @@ -6,6 +6,7 @@
>   * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/mfd/syscon.h>
> @@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
>  static int atmel_ebi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *child, *np = dev->of_node, *smc_np;
> +	struct device_node *child, *np = dev->of_node;
>  	struct atmel_ebi *ebi;
>  	int ret, reg_cells;
>  	struct clk *clk;
> @@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev)
>  
>  	ebi->clk = clk;
>  
> -	smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0);
> +	struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node,
> +									  "atmel,smc", 0);
Trivial:
I'd line break this as
> +	struct device_node *smc_np __free(device_node) =
		of_parse_phandle(dev->of_node, "atmel,smc", 0);

>  
>  	ebi->smc.regmap = syscon_node_to_regmap(smc_np);
> -	if (IS_ERR(ebi->smc.regmap)) {
> -		ret = PTR_ERR(ebi->smc.regmap);
> -		goto put_node;
> -	}
> +	if (IS_ERR(ebi->smc.regmap))
> +		return PTR_ERR(ebi->smc.regmap);
>  
>  	ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np);
> -	if (IS_ERR(ebi->smc.layout)) {
> -		ret = PTR_ERR(ebi->smc.layout);
> -		goto put_node;
> -	}
> +	if (IS_ERR(ebi->smc.layout))
> +		return PTR_ERR(ebi->smc.layout);
>  
>  	ebi->smc.clk = of_clk_get(smc_np, 0);
>  	if (IS_ERR(ebi->smc.clk)) {
> -		if (PTR_ERR(ebi->smc.clk) != -ENOENT) {
> -			ret = PTR_ERR(ebi->smc.clk);
> -			goto put_node;
> -		}
> +		if (PTR_ERR(ebi->smc.clk) != -ENOENT)
> +			return PTR_ERR(ebi->smc.clk);
>  
>  		ebi->smc.clk = NULL;
>  	}
> -	of_node_put(smc_np);

The large change in scope is a bit inelegant as it now hangs on to
the smc_np much longer than before.

Maybe it's worth pulling out the modified code as a 
atem_eb_probe_smc(struct device_node *smc_np, struct atmel_ebi_smc *smc )

or something like with a struct_group to define the atmel_ebi_smc

That would keep the tight scope for the data and generally simplify it
a bit.

>  	ret = clk_prepare_enable(ebi->smc.clk);
>  	if (ret)
>  		return ret;
> @@ -615,10 +610,6 @@ static int atmel_ebi_probe(struct platform_device *pdev)
>  	}
>  
>  	return of_platform_populate(np, NULL, NULL, dev);
> -
> -put_node:
> -	of_node_put(smc_np);
> -	return ret;
>  }
>  
>  static __maybe_unused int atmel_ebi_resume(struct device *dev)
> 

Re: [PATCH 1/9] memory: atmel-ebi: use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

On 14/08/2024 18:38, Jonathan Cameron wrote:
> On Mon, 12 Aug 2024 15:33:55 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Obtain the device node reference with scoped/cleanup.h to reduce error
>> handling and make the code a bit simpler.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Hi,
> 
> Comments inline.
>> ---
>>  drivers/memory/atmel-ebi.c | 29 ++++++++++-------------------
>>  1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
>> index e8bb5f37f5cb..fcbfc2655d8d 100644
>> --- a/drivers/memory/atmel-ebi.c
>> +++ b/drivers/memory/atmel-ebi.c
>> @@ -6,6 +6,7 @@
>>   * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>   */
>>  
>> +#include <linux/cleanup.h>
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>>  #include <linux/mfd/syscon.h>
>> @@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
>>  static int atmel_ebi_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> -	struct device_node *child, *np = dev->of_node, *smc_np;
>> +	struct device_node *child, *np = dev->of_node;
>>  	struct atmel_ebi *ebi;
>>  	int ret, reg_cells;
>>  	struct clk *clk;
>> @@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev)
>>  
>>  	ebi->clk = clk;
>>  
>> -	smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0);
>> +	struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node,
>> +									  "atmel,smc", 0);
> Trivial:
> I'd line break this as
>> +	struct device_node *smc_np __free(device_node) =
> 		of_parse_phandle(dev->of_node, "atmel,smc", 0);

Yeah, I have troubles with this constructor+destructor syntaxes. They
are way past 80 and 100 column, so maybe indeed should be wrapped at '='.

> 
>>  
>>  	ebi->smc.regmap = syscon_node_to_regmap(smc_np);
>> -	if (IS_ERR(ebi->smc.regmap)) {
>> -		ret = PTR_ERR(ebi->smc.regmap);
>> -		goto put_node;
>> -	}
>> +	if (IS_ERR(ebi->smc.regmap))
>> +		return PTR_ERR(ebi->smc.regmap);
>>  
>>  	ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np);
>> -	if (IS_ERR(ebi->smc.layout)) {
>> -		ret = PTR_ERR(ebi->smc.layout);
>> -		goto put_node;
>> -	}
>> +	if (IS_ERR(ebi->smc.layout))
>> +		return PTR_ERR(ebi->smc.layout);
>>  
>>  	ebi->smc.clk = of_clk_get(smc_np, 0);
>>  	if (IS_ERR(ebi->smc.clk)) {
>> -		if (PTR_ERR(ebi->smc.clk) != -ENOENT) {
>> -			ret = PTR_ERR(ebi->smc.clk);
>> -			goto put_node;
>> -		}
>> +		if (PTR_ERR(ebi->smc.clk) != -ENOENT)
>> +			return PTR_ERR(ebi->smc.clk);
>>  
>>  		ebi->smc.clk = NULL;
>>  	}
>> -	of_node_put(smc_np);
> 
> The large change in scope is a bit inelegant as it now hangs on to
> the smc_np much longer than before.
> 
> Maybe it's worth pulling out the modified code as a 
> atem_eb_probe_smc(struct device_node *smc_np, struct atmel_ebi_smc *smc )
> 
> or something like with a struct_group to define the atmel_ebi_smc
> 
> That would keep the tight scope for the data and generally simplify it
> a bit.


Are you speaking about any particular code optimization/performance
concerns or readability? Because scope in the latter, I believe, is not
the problem here. The entire point of __free() is that you do not care
about scope of variable or destructor. You know it will be freed, sooner
or later. If it happens later - no problem, anyway we don't have to
"think" about this variable or cleaning up because of __free().

Best regards,
Krzysztof

[PATCH 2/9] memory: atmel-ebi: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_available_child_of_node_scoped() when iterating over
device nodes to make code a bit simpler.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index fcbfc2655d8d..abc6c067b5f1 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -518,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
 static int atmel_ebi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *child, *np = dev->of_node;
+	struct device_node *np = dev->of_node;
 	struct atmel_ebi *ebi;
 	int ret, reg_cells;
 	struct clk *clk;
@@ -592,7 +592,7 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 
 	reg_cells += val;
 
-	for_each_available_child_of_node(np, child) {
+	for_each_available_child_of_node_scoped(np, child) {
 		if (!of_property_present(child, "reg"))
 			continue;
 
@@ -602,10 +602,8 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 				child);
 
 			ret = atmel_ebi_dev_disable(ebi, child);
-			if (ret) {
-				of_node_put(child);
+			if (ret)
 				return ret;
-			}
 		}
 	}
 

-- 
2.43.0

Re: [PATCH 2/9] memory: atmel-ebi: simplify with scoped for each OF child loop

[Jonathan Cameron]

On Mon, 12 Aug 2024 15:33:56 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use scoped for_each_available_child_of_node_scoped() when iterating over
> device nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 3/9] memory: samsung: exynos5422-dmc: use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

Obtain the device node reference with scoped/cleanup.h to reduce error
handling and make the code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/samsung/exynos5422-dmc.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
index da7ecd921c72..d3ae4d95a3ba 100644
--- a/drivers/memory/samsung/exynos5422-dmc.c
+++ b/drivers/memory/samsung/exynos5422-dmc.c
@@ -4,6 +4,7 @@
  * Author: Lukasz Luba <l.luba@partner.samsung.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/devfreq.h>
 #include <linux/devfreq-event.h>
@@ -1176,10 +1177,10 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
 {
 	int ret = 0;
 	int idx;
-	struct device_node *np_ddr;
 	u32 freq_mhz, clk_period_ps;
 
-	np_ddr = of_parse_phandle(dmc->dev->of_node, "device-handle", 0);
+	struct device_node *np_ddr __free(device_node) = of_parse_phandle(dmc->dev->of_node,
+									  "device-handle", 0);
 	if (!np_ddr) {
 		dev_warn(dmc->dev, "could not find 'device-handle' in DT\n");
 		return -EINVAL;
@@ -1187,39 +1188,31 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
 
 	dmc->timing_row = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
 					     sizeof(u32), GFP_KERNEL);
-	if (!dmc->timing_row) {
-		ret = -ENOMEM;
-		goto put_node;
-	}
+	if (!dmc->timing_row)
+		return -ENOMEM;
 
 	dmc->timing_data = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
 					      sizeof(u32), GFP_KERNEL);
-	if (!dmc->timing_data) {
-		ret = -ENOMEM;
-		goto put_node;
-	}
+	if (!dmc->timing_data)
+		return -ENOMEM;
 
 	dmc->timing_power = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
 					       sizeof(u32), GFP_KERNEL);
-	if (!dmc->timing_power) {
-		ret = -ENOMEM;
-		goto put_node;
-	}
+	if (!dmc->timing_power)
+		return -ENOMEM;
 
 	dmc->timings = of_lpddr3_get_ddr_timings(np_ddr, dmc->dev,
 						 DDR_TYPE_LPDDR3,
 						 &dmc->timings_arr_size);
 	if (!dmc->timings) {
 		dev_warn(dmc->dev, "could not get timings from DT\n");
-		ret = -EINVAL;
-		goto put_node;
+		return -EINVAL;
 	}
 
 	dmc->min_tck = of_lpddr3_get_min_tck(np_ddr, dmc->dev);
 	if (!dmc->min_tck) {
 		dev_warn(dmc->dev, "could not get tck from DT\n");
-		ret = -EINVAL;
-		goto put_node;
+		return -EINVAL;
 	}
 
 	/* Sorted array of OPPs with frequency ascending */
@@ -1239,8 +1232,6 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
 	dmc->bypass_timing_data = dmc->timing_data[idx - 1];
 	dmc->bypass_timing_power = dmc->timing_power[idx - 1];
 
-put_node:
-	of_node_put(np_ddr);
 	return ret;
 }
 

-- 
2.43.0

Re: [PATCH 3/9] memory: samsung: exynos5422-dmc: use scoped device node handling to simplify error paths

[Jonathan Cameron]

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

> Obtain the device node reference with scoped/cleanup.h to reduce error
> handling and make the code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Trivial comments inline
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/memory/samsung/exynos5422-dmc.c | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
> index da7ecd921c72..d3ae4d95a3ba 100644
> --- a/drivers/memory/samsung/exynos5422-dmc.c
> +++ b/drivers/memory/samsung/exynos5422-dmc.c
> @@ -4,6 +4,7 @@
>   * Author: Lukasz Luba <l.luba@partner.samsung.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/devfreq.h>
>  #include <linux/devfreq-event.h>
> @@ -1176,10 +1177,10 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
>  {
>  	int ret = 0;
>  	int idx;
> -	struct device_node *np_ddr;

This would definitely benefit from a
struct device *dev = dmc->dev;

>  	u32 freq_mhz, clk_period_ps;
>  
> -	np_ddr = of_parse_phandle(dmc->dev->of_node, "device-handle", 0);
> +	struct device_node *np_ddr __free(device_node) = of_parse_phandle(dmc->dev->of_node,
> +									  "device-handle", 0);
Trivial. Maybe consider the wrap suggested in patch 1.
> +	struct device_node *np_ddr __free(device_node) =
		of_parse_phandle(dmc->dev->of_node, "device-handle", 0);

>  	if (!np_ddr) {
>  		dev_warn(dmc->dev, "could not find 'device-handle' in DT\n");
>  		return -EINVAL;
> @@ -1187,39 +1188,31 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
>  
>  	dmc->timing_row = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
>  					     sizeof(u32), GFP_KERNEL);
> -	if (!dmc->timing_row) {
> -		ret = -ENOMEM;
> -		goto put_node;
> -	}
> +	if (!dmc->timing_row)
> +		return -ENOMEM;
>  
>  	dmc->timing_data = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
>  					      sizeof(u32), GFP_KERNEL);
> -	if (!dmc->timing_data) {
> -		ret = -ENOMEM;
> -		goto put_node;
> -	}
> +	if (!dmc->timing_data)
> +		return -ENOMEM;
>  
>  	dmc->timing_power = devm_kmalloc_array(dmc->dev, TIMING_COUNT,
>  					       sizeof(u32), GFP_KERNEL);
> -	if (!dmc->timing_power) {
> -		ret = -ENOMEM;
> -		goto put_node;
> -	}
> +	if (!dmc->timing_power)
> +		return -ENOMEM;
>  
>  	dmc->timings = of_lpddr3_get_ddr_timings(np_ddr, dmc->dev,
>  						 DDR_TYPE_LPDDR3,
>  						 &dmc->timings_arr_size);
>  	if (!dmc->timings) {
>  		dev_warn(dmc->dev, "could not get timings from DT\n");
> -		ret = -EINVAL;
> -		goto put_node;
> +		return -EINVAL;
>  	}
>  
>  	dmc->min_tck = of_lpddr3_get_min_tck(np_ddr, dmc->dev);
>  	if (!dmc->min_tck) {
>  		dev_warn(dmc->dev, "could not get tck from DT\n");
> -		ret = -EINVAL;
> -		goto put_node;
> +		return -EINVAL;
>  	}
>  
>  	/* Sorted array of OPPs with frequency ascending */
> @@ -1239,8 +1232,6 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
>  	dmc->bypass_timing_data = dmc->timing_data[idx - 1];
>  	dmc->bypass_timing_power = dmc->timing_power[idx - 1];
>  
> -put_node:
> -	of_node_put(np_ddr);
>  	return ret;
>  }
>  
> 

Re: [PATCH 3/9] memory: samsung: exynos5422-dmc: use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

On 14/08/2024 18:42, Jonathan Cameron wrote:
> On Mon, 12 Aug 2024 15:33:57 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Obtain the device node reference with scoped/cleanup.h to reduce error
>> handling and make the code a bit simpler.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Trivial comments inline
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>  drivers/memory/samsung/exynos5422-dmc.c | 31 +++++++++++--------------------
>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
>> index da7ecd921c72..d3ae4d95a3ba 100644
>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>> @@ -4,6 +4,7 @@
>>   * Author: Lukasz Luba <l.luba@partner.samsung.com>
>>   */
>>  
>> +#include <linux/cleanup.h>
>>  #include <linux/clk.h>
>>  #include <linux/devfreq.h>
>>  #include <linux/devfreq-event.h>
>> @@ -1176,10 +1177,10 @@ static int of_get_dram_timings(struct exynos5_dmc *dmc)
>>  {
>>  	int ret = 0;
>>  	int idx;
>> -	struct device_node *np_ddr;
> 
> This would definitely benefit from a
> struct device *dev = dmc->dev;

True, I'll do it in separate patch.

> 
>>  	u32 freq_mhz, clk_period_ps;
>>  
>> -	np_ddr = of_parse_phandle(dmc->dev->of_node, "device-handle", 0);
>> +	struct device_node *np_ddr __free(device_node) = of_parse_phandle(dmc->dev->of_node,
>> +									  "device-handle", 0);
> Trivial. Maybe consider the wrap suggested in patch 1.
>> +	struct device_node *np_ddr __free(device_node) =
> 		of_parse_phandle(dmc->dev->of_node, "device-handle", 0);

Ack.



Best regards,
Krzysztof

[PATCH 4/9] memory: stm32-fmc2-ebi: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_available_child_of_node_scoped() when iterating over
device nodes to make code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/stm32-fmc2-ebi.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index 1c63eeacd071..7167e1da56d3 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -1573,29 +1573,25 @@ static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
 static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
 {
 	struct device *dev = ebi->dev;
-	struct device_node *child;
 	bool child_found = false;
 	u32 bank;
 	int ret;
 
-	for_each_available_child_of_node(dev->of_node, child) {
+	for_each_available_child_of_node_scoped(dev->of_node, child) {
 		ret = of_property_read_u32(child, "reg", &bank);
 		if (ret) {
 			dev_err(dev, "could not retrieve reg property: %d\n",
 				ret);
-			of_node_put(child);
 			return ret;
 		}
 
 		if (bank >= FMC2_MAX_BANKS) {
 			dev_err(dev, "invalid reg value: %d\n", bank);
-			of_node_put(child);
 			return -EINVAL;
 		}
 
 		if (ebi->bank_assigned & BIT(bank)) {
 			dev_err(dev, "bank already assigned: %d\n", bank);
-			of_node_put(child);
 			return -EINVAL;
 		}
 
@@ -1603,7 +1599,6 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
 			ret = ebi->data->check_rif(ebi, bank + 1);
 			if (ret) {
 				dev_err(dev, "bank access failed: %d\n", bank);
-				of_node_put(child);
 				return ret;
 			}
 		}
@@ -1613,7 +1608,6 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
 			if (ret) {
 				dev_err(dev, "setup chip select %d failed: %d\n",
 					bank, ret);
-				of_node_put(child);
 				return ret;
 			}
 		}

-- 
2.43.0

Re: [PATCH 4/9] memory: stm32-fmc2-ebi: simplify with scoped for each OF child loop

[Jonathan Cameron]

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

> Use scoped for_each_available_child_of_node_scoped() when iterating over
> device nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Might be worth using dev_err_probe() in here. Otherwise LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/memory/stm32-fmc2-ebi.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
> index 1c63eeacd071..7167e1da56d3 100644
> --- a/drivers/memory/stm32-fmc2-ebi.c
> +++ b/drivers/memory/stm32-fmc2-ebi.c
> @@ -1573,29 +1573,25 @@ static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
>  static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>  {
>  	struct device *dev = ebi->dev;
> -	struct device_node *child;
>  	bool child_found = false;
>  	u32 bank;
>  	int ret;
>  
> -	for_each_available_child_of_node(dev->of_node, child) {
> +	for_each_available_child_of_node_scoped(dev->of_node, child) {
>  		ret = of_property_read_u32(child, "reg", &bank);
>  		if (ret) {
>  			dev_err(dev, "could not retrieve reg property: %d\n",
>  				ret);
> -			of_node_put(child);
>  			return ret;
			return dev_err_probe(dev, "could not retrieve reg property\n");
perhaps?
>  		}
>  
>  		if (bank >= FMC2_MAX_BANKS) {
>  			dev_err(dev, "invalid reg value: %d\n", bank);
> -			of_node_put(child);
>  			return -EINVAL;
>  		}
>  
>  		if (ebi->bank_assigned & BIT(bank)) {
>  			dev_err(dev, "bank already assigned: %d\n", bank);
> -			of_node_put(child);
>  			return -EINVAL;
>  		}
>  
> @@ -1603,7 +1599,6 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>  			ret = ebi->data->check_rif(ebi, bank + 1);
>  			if (ret) {
>  				dev_err(dev, "bank access failed: %d\n", bank);
> -				of_node_put(child);
>  				return ret;
>  			}
>  		}
> @@ -1613,7 +1608,6 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>  			if (ret) {
>  				dev_err(dev, "setup chip select %d failed: %d\n",
>  					bank, ret);
> -				of_node_put(child);
>  				return ret;
>  			}
>  		}
> 

Re: [PATCH 4/9] memory: stm32-fmc2-ebi: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

On 14/08/2024 18:45, Jonathan Cameron wrote:
> On Mon, 12 Aug 2024 15:33:58 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Use scoped for_each_available_child_of_node_scoped() when iterating over
>> device nodes to make code a bit simpler.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Might be worth using dev_err_probe() in here. Otherwise LGTM
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>  drivers/memory/stm32-fmc2-ebi.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
>> index 1c63eeacd071..7167e1da56d3 100644
>> --- a/drivers/memory/stm32-fmc2-ebi.c
>> +++ b/drivers/memory/stm32-fmc2-ebi.c
>> @@ -1573,29 +1573,25 @@ static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
>>  static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>>  {
>>  	struct device *dev = ebi->dev;
>> -	struct device_node *child;
>>  	bool child_found = false;
>>  	u32 bank;
>>  	int ret;
>>  
>> -	for_each_available_child_of_node(dev->of_node, child) {
>> +	for_each_available_child_of_node_scoped(dev->of_node, child) {
>>  		ret = of_property_read_u32(child, "reg", &bank);
>>  		if (ret) {
>>  			dev_err(dev, "could not retrieve reg property: %d\n",
>>  				ret);
>> -			of_node_put(child);
>>  			return ret;
> 			return dev_err_probe(dev, "could not retrieve reg property\n");
> perhaps?

New patch for that... but just mind that deferred probe cannot happen
here, so only part of dev_err_probe() benefits would be used.

Best regards,
Krzysztof

[PATCH 5/9] memory: tegra-mc: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

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

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 224b488794e5..bd5b58f1fd42 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -450,7 +450,6 @@ static int load_one_timing(struct tegra_mc *mc,
 
 static int load_timings(struct tegra_mc *mc, struct device_node *node)
 {
-	struct device_node *child;
 	struct tegra_mc_timing *timing;
 	int child_count = of_get_child_count(node);
 	int i = 0, err;
@@ -462,14 +461,12 @@ static int load_timings(struct tegra_mc *mc, struct device_node *node)
 
 	mc->num_timings = child_count;
 
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		timing = &mc->timings[i++];
 
 		err = load_one_timing(mc, timing, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}
 
 	return 0;
@@ -477,7 +474,6 @@ static int load_timings(struct tegra_mc *mc, struct device_node *node)
 
 static int tegra_mc_setup_timings(struct tegra_mc *mc)
 {
-	struct device_node *node;
 	u32 ram_code, node_ram_code;
 	int err;
 
@@ -485,14 +481,13 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
 
 	mc->num_timings = 0;
 
-	for_each_child_of_node(mc->dev->of_node, node) {
+	for_each_child_of_node_scoped(mc->dev->of_node, node) {
 		err = of_property_read_u32(node, "nvidia,ram-code",
 					   &node_ram_code);
 		if (err || (node_ram_code != ram_code))
 			continue;
 
 		err = load_timings(mc, node);
-		of_node_put(node);
 		if (err)
 			return err;
 		break;

-- 
2.43.0

Re: [PATCH 5/9] memory: tegra-mc: simplify with scoped for each OF child loop

[Jonathan Cameron]

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

> Use scoped for_each_child_of_node_scoped() when iterating over device
> nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
The final block is already a weird code structure, but your changes look fine
to me.

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

> ---
>  drivers/memory/tegra/mc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 224b488794e5..bd5b58f1fd42 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -450,7 +450,6 @@ static int load_one_timing(struct tegra_mc *mc,
>  
>  static int load_timings(struct tegra_mc *mc, struct device_node *node)
>  {
> -	struct device_node *child;
>  	struct tegra_mc_timing *timing;
>  	int child_count = of_get_child_count(node);
>  	int i = 0, err;
> @@ -462,14 +461,12 @@ static int load_timings(struct tegra_mc *mc, struct device_node *node)
>  
>  	mc->num_timings = child_count;
>  
> -	for_each_child_of_node(node, child) {
> +	for_each_child_of_node_scoped(node, child) {
>  		timing = &mc->timings[i++];
>  
>  		err = load_one_timing(mc, timing, child);
> -		if (err) {
> -			of_node_put(child);
> +		if (err)
>  			return err;
> -		}
>  	}
>  
>  	return 0;
> @@ -477,7 +474,6 @@ static int load_timings(struct tegra_mc *mc, struct device_node *node)
>  
>  static int tegra_mc_setup_timings(struct tegra_mc *mc)
>  {
> -	struct device_node *node;
>  	u32 ram_code, node_ram_code;
>  	int err;
>  
> @@ -485,14 +481,13 @@ static int tegra_mc_setup_timings(struct tegra_mc *mc)
>  
>  	mc->num_timings = 0;
>  
> -	for_each_child_of_node(mc->dev->of_node, node) {
> +	for_each_child_of_node_scoped(mc->dev->of_node, node) {
>  		err = of_property_read_u32(node, "nvidia,ram-code",
>  					   &node_ram_code);
>  		if (err || (node_ram_code != ram_code))
>  			continue;
>  
>  		err = load_timings(mc, node);
> -		of_node_put(node);
>  		if (err)
>  			return err;
>  		break;
> 

[PATCH 6/9] memory: tegra124-emc: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/tegra124-emc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
index 47c0c19e13fd..03f1daa2d132 100644
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -992,7 +992,6 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
 					  struct device_node *node)
 {
 	int child_count = of_get_child_count(node);
-	struct device_node *child;
 	struct emc_timing *timing;
 	unsigned int i = 0;
 	int err;
@@ -1004,14 +1003,12 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
 
 	emc->num_timings = child_count;
 
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		timing = &emc->timings[i++];
 
 		err = load_one_timing_from_dt(emc, timing, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}
 
 	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,

-- 
2.43.0

Re: [PATCH 6/9] memory: tegra124-emc: simplify with scoped for each OF child loop

[Jonathan Cameron]

On Mon, 12 Aug 2024 15:34:00 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use scoped for_each_child_of_node_scoped() when iterating over device
> nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 7/9] memory: tegra20-emc: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/tegra20-emc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index 97cf59523b0b..7193f848d17e 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -410,7 +410,6 @@ static int cmp_timings(const void *_a, const void *_b)
 static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
 					  struct device_node *node)
 {
-	struct device_node *child;
 	struct emc_timing *timing;
 	int child_count;
 	int err;
@@ -428,15 +427,13 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
 
 	timing = emc->timings;
 
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		if (of_node_name_eq(child, "lpddr2"))
 			continue;
 
 		err = load_one_timing_from_dt(emc, timing++, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 
 		emc->num_timings++;
 	}

-- 
2.43.0

Re: [PATCH 7/9] memory: tegra20-emc: simplify with scoped for each OF child loop

[Jonathan Cameron]

On Mon, 12 Aug 2024 15:34:01 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use scoped for_each_child_of_node_scoped() when iterating over device
> nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 8/9] memory: tegra30-emc: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/tegra30-emc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index d7b0a23c2d7d..921dce1b8bc6 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -979,7 +979,6 @@ static int emc_check_mc_timings(struct tegra_emc *emc)
 static int emc_load_timings_from_dt(struct tegra_emc *emc,
 				    struct device_node *node)
 {
-	struct device_node *child;
 	struct emc_timing *timing;
 	int child_count;
 	int err;
@@ -998,12 +997,10 @@ static int emc_load_timings_from_dt(struct tegra_emc *emc,
 	emc->num_timings = child_count;
 	timing = emc->timings;
 
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		err = load_one_timing_from_dt(emc, timing++, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}
 
 	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,

-- 
2.43.0

[PATCH 9/9] memory: ti-aemif: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_available_child_of_node_scoped() when iterating over
device nodes to make code a bit simpler.

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

diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
index e192db9e0e4b..cd2945d4ec18 100644
--- a/drivers/memory/ti-aemif.c
+++ b/drivers/memory/ti-aemif.c
@@ -330,7 +330,6 @@ static int aemif_probe(struct platform_device *pdev)
 	int ret = -ENODEV;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *child_np;
 	struct aemif_device *aemif;
 	struct aemif_platform_data *pdata;
 	struct of_dev_auxdata *dev_lookup;
@@ -374,12 +373,10 @@ static int aemif_probe(struct platform_device *pdev)
 		 * functions iterate over these nodes and update the cs data
 		 * array.
 		 */
-		for_each_available_child_of_node(np, child_np) {
+		for_each_available_child_of_node_scoped(np, child_np) {
 			ret = of_aemif_parse_abus_config(pdev, child_np);
-			if (ret < 0) {
-				of_node_put(child_np);
+			if (ret < 0)
 				goto error;
-			}
 		}
 	} else if (pdata && pdata->num_abus_data > 0) {
 		for (i = 0; i < pdata->num_abus_data; i++, aemif->num_cs++) {
@@ -402,13 +399,11 @@ static int aemif_probe(struct platform_device *pdev)
 	 * child will be probed after the AEMIF timing parameters are set.
 	 */
 	if (np) {
-		for_each_available_child_of_node(np, child_np) {
+		for_each_available_child_of_node_scoped(np, child_np) {
 			ret = of_platform_populate(child_np, NULL,
 						   dev_lookup, dev);
-			if (ret < 0) {
-				of_node_put(child_np);
+			if (ret < 0)
 				goto error;
-			}
 		}
 	} else if (pdata) {
 		for (i = 0; i < pdata->num_sub_devices; i++) {

-- 
2.43.0

Re: [PATCH 9/9] memory: ti-aemif: simplify with scoped for each OF child loop

[Jonathan Cameron]

On Mon, 12 Aug 2024 15:34:03 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use scoped for_each_available_child_of_node_scoped() when iterating over
> device nodes to make code a bit simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Nothing wrong with this patch, but I think you can add a precusor
that will make this neater.

Jonathan

> ---
>  drivers/memory/ti-aemif.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
> index e192db9e0e4b..cd2945d4ec18 100644
> --- a/drivers/memory/ti-aemif.c
> +++ b/drivers/memory/ti-aemif.c
> @@ -330,7 +330,6 @@ static int aemif_probe(struct platform_device *pdev)
>  	int ret = -ENODEV;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *child_np;
>  	struct aemif_device *aemif;
>  	struct aemif_platform_data *pdata;
>  	struct of_dev_auxdata *dev_lookup;
> @@ -374,12 +373,10 @@ static int aemif_probe(struct platform_device *pdev)
>  		 * functions iterate over these nodes and update the cs data
>  		 * array.
>  		 */
> -		for_each_available_child_of_node(np, child_np) {
> +		for_each_available_child_of_node_scoped(np, child_np) {
>  			ret = of_aemif_parse_abus_config(pdev, child_np);
> -			if (ret < 0) {
> -				of_node_put(child_np);
> +			if (ret < 0)
>  				goto error;
I'd precede this patch with use of
devm_clk_get_enabled()

That would avoid what looks like potential mixed devm and not issues
and let you return here.


> -			}
>  		}
>  	} else if (pdata && pdata->num_abus_data > 0) {
>  		for (i = 0; i < pdata->num_abus_data; i++, aemif->num_cs++) {
> @@ -402,13 +399,11 @@ static int aemif_probe(struct platform_device *pdev)
>  	 * child will be probed after the AEMIF timing parameters are set.
>  	 */
>  	if (np) {
> -		for_each_available_child_of_node(np, child_np) {
> +		for_each_available_child_of_node_scoped(np, child_np) {
>  			ret = of_platform_populate(child_np, NULL,
>  						   dev_lookup, dev);
> -			if (ret < 0) {
> -				of_node_put(child_np);
> +			if (ret < 0)
>  				goto error;
> -			}
>  		}
>  	} else if (pdata) {
>  		for (i = 0; i < pdata->num_sub_devices; i++) {
> 

Re: [PATCH 9/9] memory: ti-aemif: simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

On 14/08/2024 18:55, Jonathan Cameron wrote:
> On Mon, 12 Aug 2024 15:34:03 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Use scoped for_each_available_child_of_node_scoped() when iterating over
>> device nodes to make code a bit simpler.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Nothing wrong with this patch, but I think you can add a precusor
> that will make this neater.
> 
> Jonathan
> 
>> ---
>>  drivers/memory/ti-aemif.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
>> index e192db9e0e4b..cd2945d4ec18 100644
>> --- a/drivers/memory/ti-aemif.c
>> +++ b/drivers/memory/ti-aemif.c
>> @@ -330,7 +330,6 @@ static int aemif_probe(struct platform_device *pdev)
>>  	int ret = -ENODEV;
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *np = dev->of_node;
>> -	struct device_node *child_np;
>>  	struct aemif_device *aemif;
>>  	struct aemif_platform_data *pdata;
>>  	struct of_dev_auxdata *dev_lookup;
>> @@ -374,12 +373,10 @@ static int aemif_probe(struct platform_device *pdev)
>>  		 * functions iterate over these nodes and update the cs data
>>  		 * array.
>>  		 */
>> -		for_each_available_child_of_node(np, child_np) {
>> +		for_each_available_child_of_node_scoped(np, child_np) {
>>  			ret = of_aemif_parse_abus_config(pdev, child_np);
>> -			if (ret < 0) {
>> -				of_node_put(child_np);
>> +			if (ret < 0)
>>  				goto error;
> I'd precede this patch with use of
> devm_clk_get_enabled()
> 
> That would avoid what looks like potential mixed devm and not issues
> and let you return here.

Yep, that would be useful.

Best regards,
Krzysztof