[PATCH 00/10] pmdomain: Simplify with cleanup.h

[PATCH 00/10] pmdomain: Simplify with cleanup.h

[Krzysztof Kozlowski]

Simplify the code with scoped loops, guards and __free().

Best regards,
Krzysztof

---
Krzysztof Kozlowski (10):
      pmdomain: rockchip: Simplify with scoped for each OF child loop
      pmdomain: rockchip: Simplify locking with guard()
      pmdomain: imx: gpc: Simplify with scoped for each OF child loop
      pmdomain: imx: gpcv2: Simplify with scoped for each OF child loop
      pmdomain: qcom: cpr: Simplify with dev_err_probe()
      pmdomain: qcom: cpr: Simplify locking with guard()
      pmdomain: qcom: rpmhpd: Simplify locking with guard()
      pmdomain: qcom: rpmpd: Simplify locking with guard()
      pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths
      pmdomain: renesas: rcar-sysc: Use scoped device node handling to simplify error paths

 drivers/pmdomain/imx/gpc.c                | 14 +++-----
 drivers/pmdomain/imx/gpcv2.c              |  8 ++---
 drivers/pmdomain/qcom/cpr.c               | 58 ++++++++++++-------------------
 drivers/pmdomain/qcom/rpmhpd.c            | 11 +++---
 drivers/pmdomain/qcom/rpmpd.c             | 20 ++++-------
 drivers/pmdomain/renesas/rcar-gen4-sysc.c | 26 ++++++--------
 drivers/pmdomain/renesas/rcar-sysc.c      | 28 ++++++---------
 drivers/pmdomain/rockchip/pm-domains.c    | 25 ++++---------
 8 files changed, 68 insertions(+), 122 deletions(-)
---
base-commit: e188fd67a69319f3d105d9b90e424b8d1ff9580c
change-id: 20240823-cleanup-h-guard-pm-domain-35eb491f35f9

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

[PATCH 01/10] pmdomain: rockchip: Simplify with scoped for each OF child loop

[Krzysztof Kozlowski]

Use scoped for_each_available_child_of_node_scoped() and
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/pmdomain/rockchip/pm-domains.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 64b4d7120d83..5679ad336a11 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -804,11 +804,10 @@ static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
 static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 				     struct device_node *parent)
 {
-	struct device_node *np;
 	struct generic_pm_domain *child_domain, *parent_domain;
 	int error;
 
-	for_each_child_of_node(parent, np) {
+	for_each_child_of_node_scoped(parent, np) {
 		u32 idx;
 
 		error = of_property_read_u32(parent, "reg", &idx);
@@ -816,7 +815,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 			dev_err(pmu->dev,
 				"%pOFn: failed to retrieve domain id (reg): %d\n",
 				parent, error);
-			goto err_out;
+			return error;
 		}
 		parent_domain = pmu->genpd_data.domains[idx];
 
@@ -824,7 +823,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 		if (error) {
 			dev_err(pmu->dev, "failed to handle node %pOFn: %d\n",
 				np, error);
-			goto err_out;
+			return error;
 		}
 
 		error = of_property_read_u32(np, "reg", &idx);
@@ -832,7 +831,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 			dev_err(pmu->dev,
 				"%pOFn: failed to retrieve domain id (reg): %d\n",
 				np, error);
-			goto err_out;
+			return error;
 		}
 		child_domain = pmu->genpd_data.domains[idx];
 
@@ -840,7 +839,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 		if (error) {
 			dev_err(pmu->dev, "%s failed to add subdomain %s: %d\n",
 				parent_domain->name, child_domain->name, error);
-			goto err_out;
+			return error;
 		} else {
 			dev_dbg(pmu->dev, "%s add subdomain: %s\n",
 				parent_domain->name, child_domain->name);
@@ -850,17 +849,12 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 	}
 
 	return 0;
-
-err_out:
-	of_node_put(np);
-	return error;
 }
 
 static int rockchip_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *node;
 	struct device *parent;
 	struct rockchip_pmu *pmu;
 	const struct rockchip_pmu_info *pmu_info;
@@ -918,12 +912,11 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	 */
 	mutex_lock(&dmc_pmu_mutex);
 
-	for_each_available_child_of_node(np, node) {
+	for_each_available_child_of_node_scoped(np, node) {
 		error = rockchip_pm_add_one_domain(pmu, node);
 		if (error) {
 			dev_err(dev, "failed to handle node %pOFn: %d\n",
 				node, error);
-			of_node_put(node);
 			goto err_out;
 		}
 
@@ -931,7 +924,6 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 		if (error < 0) {
 			dev_err(dev, "failed to handle subdomain node %pOFn: %d\n",
 				node, error);
-			of_node_put(node);
 			goto err_out;
 		}
 	}

-- 
2.43.0

Re: [PATCH 01/10] pmdomain: rockchip: Simplify with scoped for each OF child loop

[Jonathan Cameron]

On Fri, 23 Aug 2024 14:51:05 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Use scoped for_each_available_child_of_node_scoped() and
> 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>
Ripe for some dev_err_probe() usage that will make this even nicer
as a cleanup.

Looks good as it stands
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pmdomain/rockchip/pm-domains.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index 64b4d7120d83..5679ad336a11 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -804,11 +804,10 @@ static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
>  static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  				     struct device_node *parent)
>  {
> -	struct device_node *np;
>  	struct generic_pm_domain *child_domain, *parent_domain;
>  	int error;
>  
> -	for_each_child_of_node(parent, np) {
> +	for_each_child_of_node_scoped(parent, np) {
>  		u32 idx;
>  
>  		error = of_property_read_u32(parent, "reg", &idx);
> @@ -816,7 +815,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  			dev_err(pmu->dev,
>  				"%pOFn: failed to retrieve domain id (reg): %d\n",
>  				parent, error);
> -			goto err_out;
> +			return error;
>  		}
>  		parent_domain = pmu->genpd_data.domains[idx];
>  
> @@ -824,7 +823,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  		if (error) {
>  			dev_err(pmu->dev, "failed to handle node %pOFn: %d\n",
>  				np, error);
> -			goto err_out;
> +			return error;
>  		}
>  
>  		error = of_property_read_u32(np, "reg", &idx);
> @@ -832,7 +831,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  			dev_err(pmu->dev,
>  				"%pOFn: failed to retrieve domain id (reg): %d\n",
>  				np, error);
> -			goto err_out;
> +			return error;
>  		}
>  		child_domain = pmu->genpd_data.domains[idx];
>  
> @@ -840,7 +839,7 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  		if (error) {
>  			dev_err(pmu->dev, "%s failed to add subdomain %s: %d\n",
>  				parent_domain->name, child_domain->name, error);
> -			goto err_out;
> +			return error;
>  		} else {
>  			dev_dbg(pmu->dev, "%s add subdomain: %s\n",
>  				parent_domain->name, child_domain->name);
> @@ -850,17 +849,12 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  	}
>  
>  	return 0;
> -
> -err_out:
> -	of_node_put(np);
> -	return error;
>  }
>  
>  static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *node;
>  	struct device *parent;
>  	struct rockchip_pmu *pmu;
>  	const struct rockchip_pmu_info *pmu_info;
> @@ -918,12 +912,11 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  	 */
>  	mutex_lock(&dmc_pmu_mutex);
>  
> -	for_each_available_child_of_node(np, node) {
> +	for_each_available_child_of_node_scoped(np, node) {
>  		error = rockchip_pm_add_one_domain(pmu, node);
>  		if (error) {
>  			dev_err(dev, "failed to handle node %pOFn: %d\n",
>  				node, error);
> -			of_node_put(node);
>  			goto err_out;
>  		}
>  
> @@ -931,7 +924,6 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  		if (error < 0) {
>  			dev_err(dev, "failed to handle subdomain node %pOFn: %d\n",
>  				node, error);
> -			of_node_put(node);
>  			goto err_out;
>  		}
>  	}
> 

[PATCH 02/10] pmdomain: rockchip: Simplify locking with guard()

[Krzysztof Kozlowski]

Simplify error handling (smaller error handling) over locks with
guard().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/pmdomain/rockchip/pm-domains.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 5679ad336a11..538dde58d924 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -910,7 +910,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	 * Prevent any rockchip_pmu_block() from racing with the remainder of
 	 * setup (clocks, register initialization).
 	 */
-	mutex_lock(&dmc_pmu_mutex);
+	guard(mutex)(&dmc_pmu_mutex);
 
 	for_each_available_child_of_node_scoped(np, node) {
 		error = rockchip_pm_add_one_domain(pmu, node);
@@ -943,13 +943,10 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	if (!WARN_ON_ONCE(dmc_pmu))
 		dmc_pmu = pmu;
 
-	mutex_unlock(&dmc_pmu_mutex);
-
 	return 0;
 
 err_out:
 	rockchip_pm_domain_cleanup(pmu);
-	mutex_unlock(&dmc_pmu_mutex);
 	return error;
 }
 

-- 
2.43.0

Re: [PATCH 02/10] pmdomain: rockchip: Simplify locking with guard()

[Jonathan Cameron]

On Fri, 23 Aug 2024 14:51:06 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Simplify error handling (smaller error handling) over locks with
> guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Musing inline.

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


> ---
>  drivers/pmdomain/rockchip/pm-domains.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index 5679ad336a11..538dde58d924 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -910,7 +910,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  	 * Prevent any rockchip_pmu_block() from racing with the remainder of
>  	 * setup (clocks, register initialization).
>  	 */
> -	mutex_lock(&dmc_pmu_mutex);
> +	guard(mutex)(&dmc_pmu_mutex);
>  
>  	for_each_available_child_of_node_scoped(np, node) {
>  		error = rockchip_pm_add_one_domain(pmu, node);
> @@ -943,13 +943,10 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  	if (!WARN_ON_ONCE(dmc_pmu))
>  		dmc_pmu = pmu;
>  
> -	mutex_unlock(&dmc_pmu_mutex);
> -
>  	return 0;
>  
>  err_out:
>  	rockchip_pm_domain_cleanup(pmu);

I wonder.  Could you use a devm_add_action_or_reset for this and allow early
returns throughout?

Would need to take the lock again perhaps and I haven't checked if there
is any issue in dropping and retaking the mutex however.
The block logic is non obvious so I couldn't quickly figure this out.
 
> -	mutex_unlock(&dmc_pmu_mutex);
>  	return error;
>  }
>  
> 

Re: [PATCH 02/10] pmdomain: rockchip: Simplify locking with guard()

[Krzysztof Kozlowski]

On 27/08/2024 11:59, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 14:51:06 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Simplify error handling (smaller error handling) over locks with
>> guard().
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Musing inline.
> 
> LGTM
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
>> ---
>>  drivers/pmdomain/rockchip/pm-domains.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
>> index 5679ad336a11..538dde58d924 100644
>> --- a/drivers/pmdomain/rockchip/pm-domains.c
>> +++ b/drivers/pmdomain/rockchip/pm-domains.c
>> @@ -910,7 +910,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>>  	 * Prevent any rockchip_pmu_block() from racing with the remainder of
>>  	 * setup (clocks, register initialization).
>>  	 */
>> -	mutex_lock(&dmc_pmu_mutex);
>> +	guard(mutex)(&dmc_pmu_mutex);
>>  
>>  	for_each_available_child_of_node_scoped(np, node) {
>>  		error = rockchip_pm_add_one_domain(pmu, node);
>> @@ -943,13 +943,10 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>>  	if (!WARN_ON_ONCE(dmc_pmu))
>>  		dmc_pmu = pmu;
>>  
>> -	mutex_unlock(&dmc_pmu_mutex);
>> -
>>  	return 0;
>>  
>>  err_out:
>>  	rockchip_pm_domain_cleanup(pmu);
> 
> I wonder.  Could you use a devm_add_action_or_reset for this and allow early
> returns throughout?
> 
> Would need to take the lock again perhaps and I haven't checked if there
> is any issue in dropping and retaking the mutex however.
> The block logic is non obvious so I couldn't quickly figure this out.

I will take a look, but as you already pointed out it is a bit further
from trivial functionally-equivalent cleanup. I might mess with the locks.

Best regards,
Krzysztof

[PATCH 03/10] pmdomain: imx: gpc: 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/pmdomain/imx/gpc.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c
index 9517cce93d8a..80a4dcc77199 100644
--- a/drivers/pmdomain/imx/gpc.c
+++ b/drivers/pmdomain/imx/gpc.c
@@ -455,7 +455,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
 	} else {
 		struct imx_pm_domain *domain;
 		struct platform_device *pd_pdev;
-		struct device_node *np;
 		struct clk *ipg_clk;
 		unsigned int ipg_rate_mhz;
 		int domain_index;
@@ -465,28 +464,24 @@ static int imx_gpc_probe(struct platform_device *pdev)
 			return PTR_ERR(ipg_clk);
 		ipg_rate_mhz = clk_get_rate(ipg_clk) / 1000000;
 
-		for_each_child_of_node(pgc_node, np) {
+		for_each_child_of_node_scoped(pgc_node, np) {
 			ret = of_property_read_u32(np, "reg", &domain_index);
-			if (ret) {
-				of_node_put(np);
+			if (ret)
 				return ret;
-			}
+
 			if (domain_index >= of_id_data->num_domains)
 				continue;
 
 			pd_pdev = platform_device_alloc("imx-pgc-power-domain",
 							domain_index);
-			if (!pd_pdev) {
-				of_node_put(np);
+			if (!pd_pdev)
 				return -ENOMEM;
-			}
 
 			ret = platform_device_add_data(pd_pdev,
 						       &imx_gpc_domains[domain_index],
 						       sizeof(imx_gpc_domains[domain_index]));
 			if (ret) {
 				platform_device_put(pd_pdev);
-				of_node_put(np);
 				return ret;
 			}
 			domain = pd_pdev->dev.platform_data;
@@ -500,7 +495,6 @@ static int imx_gpc_probe(struct platform_device *pdev)
 			ret = platform_device_add(pd_pdev);
 			if (ret) {
 				platform_device_put(pd_pdev);
-				of_node_put(np);
 				return ret;
 			}
 		}

-- 
2.43.0

[PATCH 04/10] pmdomain: imx: gpcv2: 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/pmdomain/imx/gpcv2.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pmdomain/imx/gpcv2.c b/drivers/pmdomain/imx/gpcv2.c
index 856eaac0ec14..963d61c5af6d 100644
--- a/drivers/pmdomain/imx/gpcv2.c
+++ b/drivers/pmdomain/imx/gpcv2.c
@@ -1458,7 +1458,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 		.max_register   = SZ_4K,
 	};
 	struct device *dev = &pdev->dev;
-	struct device_node *pgc_np, *np;
+	struct device_node *pgc_np;
 	struct regmap *regmap;
 	void __iomem *base;
 	int ret;
@@ -1480,7 +1480,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	for_each_child_of_node(pgc_np, np) {
+	for_each_child_of_node_scoped(pgc_np, np) {
 		struct platform_device *pd_pdev;
 		struct imx_pgc_domain *domain;
 		u32 domain_index;
@@ -1491,7 +1491,6 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 		ret = of_property_read_u32(np, "reg", &domain_index);
 		if (ret) {
 			dev_err(dev, "Failed to read 'reg' property\n");
-			of_node_put(np);
 			return ret;
 		}
 
@@ -1506,7 +1505,6 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 						domain_index);
 		if (!pd_pdev) {
 			dev_err(dev, "Failed to allocate platform device\n");
-			of_node_put(np);
 			return -ENOMEM;
 		}
 
@@ -1515,7 +1513,6 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 					       sizeof(domain_data->domains[domain_index]));
 		if (ret) {
 			platform_device_put(pd_pdev);
-			of_node_put(np);
 			return ret;
 		}
 
@@ -1532,7 +1529,6 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
 		ret = platform_device_add(pd_pdev);
 		if (ret) {
 			platform_device_put(pd_pdev);
-			of_node_put(np);
 			return ret;
 		}
 	}

-- 
2.43.0

[PATCH 05/10] pmdomain: qcom: cpr: Simplify with dev_err_probe()

[Krzysztof Kozlowski]

Use dev_err_probe() to make defer code handling simpler.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/pmdomain/qcom/cpr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pmdomain/qcom/cpr.c b/drivers/pmdomain/qcom/cpr.c
index c64e84a27cc7..1bef89712188 100644
--- a/drivers/pmdomain/qcom/cpr.c
+++ b/drivers/pmdomain/qcom/cpr.c
@@ -1470,9 +1470,8 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 	 */
 	drv->cpu_clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(drv->cpu_clk)) {
-		ret = PTR_ERR(drv->cpu_clk);
-		if (ret != -EPROBE_DEFER)
-			dev_err(drv->dev, "could not get cpu clk: %d\n", ret);
+		ret = dev_err_probe(drv->dev, PTR_ERR(drv->cpu_clk),
+				    "could not get cpu clk\n");
 		goto unlock;
 	}
 	drv->attached_cpu_dev = dev;

-- 
2.43.0

Re: [PATCH 05/10] pmdomain: qcom: cpr: Simplify with dev_err_probe()

[Konrad Dybcio]

On 23.08.2024 2:51 PM, Krzysztof Kozlowski wrote:
> Use dev_err_probe() to make defer code handling simpler.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>

Konrad

[PATCH 06/10] pmdomain: qcom: cpr: 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/pmdomain/qcom/cpr.c | 57 ++++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 34 deletions(-)

diff --git a/drivers/pmdomain/qcom/cpr.c b/drivers/pmdomain/qcom/cpr.c
index 1bef89712188..a616a3ec3d46 100644
--- a/drivers/pmdomain/qcom/cpr.c
+++ b/drivers/pmdomain/qcom/cpr.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2019, Linaro Limited
  */
 
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/debugfs.h>
@@ -747,9 +748,9 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
 	struct cpr_drv *drv = container_of(domain, struct cpr_drv, pd);
 	struct corner *corner, *end;
 	enum voltage_change_dir dir;
-	int ret = 0, new_uV;
+	int ret, new_uV;
 
-	mutex_lock(&drv->lock);
+	guard(mutex)(&drv->lock);
 
 	dev_dbg(drv->dev, "%s: setting perf state: %u (prev state: %u)\n",
 		__func__, state, cpr_get_cur_perf_state(drv));
@@ -760,10 +761,8 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
 	 */
 	corner = drv->corners + state - 1;
 	end = &drv->corners[drv->num_corners - 1];
-	if (corner > end || corner < drv->corners) {
-		ret = -EINVAL;
-		goto unlock;
-	}
+	if (corner > end || corner < drv->corners)
+		return -EINVAL;
 
 	/* Determine direction */
 	if (drv->corner > corner)
@@ -783,7 +782,7 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
 
 	ret = cpr_scale_voltage(drv, corner, new_uV, dir);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	if (cpr_is_allowed(drv)) {
 		cpr_irq_clr(drv);
@@ -794,10 +793,7 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
 
 	drv->corner = corner;
 
-unlock:
-	mutex_unlock(&drv->lock);
-
-	return ret;
+	return 0;
 }
 
 static int
@@ -1443,9 +1439,9 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 {
 	struct cpr_drv *drv = container_of(domain, struct cpr_drv, pd);
 	const struct acc_desc *acc_desc = drv->acc_desc;
-	int ret = 0;
+	int ret;
 
-	mutex_lock(&drv->lock);
+	guard(mutex)(&drv->lock);
 
 	dev_dbg(drv->dev, "attach callback for: %s\n", dev_name(dev));
 
@@ -1457,7 +1453,7 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 	 * additional initialization when further CPUs get attached.
 	 */
 	if (drv->attached_cpu_dev)
-		goto unlock;
+		return 0;
 
 	/*
 	 * cpr_scale_voltage() requires the direction (if we are changing
@@ -1469,11 +1465,10 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 	 * the first time cpr_set_performance_state() is called.
 	 */
 	drv->cpu_clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(drv->cpu_clk)) {
-		ret = dev_err_probe(drv->dev, PTR_ERR(drv->cpu_clk),
-				    "could not get cpu clk\n");
-		goto unlock;
-	}
+	if (IS_ERR(drv->cpu_clk))
+		return dev_err_probe(drv->dev, PTR_ERR(drv->cpu_clk),
+				     "could not get cpu clk\n");
+
 	drv->attached_cpu_dev = dev;
 
 	dev_dbg(drv->dev, "using cpu clk from: %s\n",
@@ -1490,42 +1485,39 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 	ret = dev_pm_opp_get_opp_count(&drv->pd.dev);
 	if (ret < 0) {
 		dev_err(drv->dev, "could not get OPP count\n");
-		goto unlock;
+		return ret;
 	}
 	drv->num_corners = ret;
 
 	if (drv->num_corners < 2) {
 		dev_err(drv->dev, "need at least 2 OPPs to use CPR\n");
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	drv->corners = devm_kcalloc(drv->dev, drv->num_corners,
 				    sizeof(*drv->corners),
 				    GFP_KERNEL);
-	if (!drv->corners) {
-		ret = -ENOMEM;
-		goto unlock;
-	}
+	if (!drv->corners)
+		return -ENOMEM;
 
 	ret = cpr_corner_init(drv);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	cpr_set_loop_allowed(drv);
 
 	ret = cpr_init_parameters(drv);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	/* Configure CPR HW but keep it disabled */
 	ret = cpr_config(drv);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = cpr_find_initial_corner(drv);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	if (acc_desc->config)
 		regmap_multi_reg_write(drv->tcsr, acc_desc->config,
@@ -1540,10 +1532,7 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain,
 	dev_info(drv->dev, "driver initialized with %u OPPs\n",
 		 drv->num_corners);
 
-unlock:
-	mutex_unlock(&drv->lock);
-
-	return ret;
+	return 0;
 }
 
 static int cpr_debug_info_show(struct seq_file *s, void *unused)

-- 
2.43.0

Re: [PATCH 06/10] pmdomain: qcom: cpr: Simplify locking with guard()

[Konrad Dybcio]

On 23.08.2024 2:51 PM, Krzysztof Kozlowski wrote:
> Simplify error handling (less gotos) over locks with guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>

Konrad

[PATCH 07/10] pmdomain: qcom: rpmhpd: 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/pmdomain/qcom/rpmhpd.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pmdomain/qcom/rpmhpd.c b/drivers/pmdomain/qcom/rpmhpd.c
index d2cb4271a1ca..65505e1e2219 100644
--- a/drivers/pmdomain/qcom/rpmhpd.c
+++ b/drivers/pmdomain/qcom/rpmhpd.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -775,9 +776,9 @@ static int rpmhpd_set_performance_state(struct generic_pm_domain *domain,
 					unsigned int level)
 {
 	struct rpmhpd *pd = domain_to_rpmhpd(domain);
-	int ret = 0, i;
+	int ret, i;
 
-	mutex_lock(&rpmhpd_lock);
+	guard(mutex)(&rpmhpd_lock);
 
 	for (i = 0; i < pd->level_count; i++)
 		if (level <= pd->level[i])
@@ -797,14 +798,12 @@ static int rpmhpd_set_performance_state(struct generic_pm_domain *domain,
 
 		ret = rpmhpd_aggregate_corner(pd, i);
 		if (ret)
-			goto out;
+			return ret;
 	}
 
 	pd->corner = i;
-out:
-	mutex_unlock(&rpmhpd_lock);
 
-	return ret;
+	return 0;
 }
 
 static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)

-- 
2.43.0

Re: [PATCH 07/10] pmdomain: qcom: rpmhpd: Simplify locking with guard()

[Konrad Dybcio]

On 23.08.2024 2:51 PM, Krzysztof Kozlowski wrote:
> Simplify error handling (less gotos) over locks with guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>

Konrad

[PATCH 08/10] pmdomain: qcom: rpmpd: 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/pmdomain/qcom/rpmpd.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/pmdomain/qcom/rpmpd.c b/drivers/pmdomain/qcom/rpmpd.c
index 5e6280b4cf70..0be6b3026e3a 100644
--- a/drivers/pmdomain/qcom/rpmpd.c
+++ b/drivers/pmdomain/qcom/rpmpd.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -1024,20 +1025,17 @@ static int rpmpd_power_on(struct generic_pm_domain *domain)
 	int ret;
 	struct rpmpd *pd = domain_to_rpmpd(domain);
 
-	mutex_lock(&rpmpd_lock);
+	guard(mutex)(&rpmpd_lock);
 
 	ret = rpmpd_send_enable(pd, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	pd->enabled = true;
 
 	if (pd->corner)
 		ret = rpmpd_aggregate_corner(pd);
 
-out:
-	mutex_unlock(&rpmpd_lock);
-
 	return ret;
 }
 
@@ -1060,27 +1058,21 @@ static int rpmpd_power_off(struct generic_pm_domain *domain)
 static int rpmpd_set_performance(struct generic_pm_domain *domain,
 				 unsigned int state)
 {
-	int ret = 0;
 	struct rpmpd *pd = domain_to_rpmpd(domain);
 
 	if (state > pd->max_state)
 		state = pd->max_state;
 
-	mutex_lock(&rpmpd_lock);
+	guard(mutex)(&rpmpd_lock);
 
 	pd->corner = state;
 
 	/* Always send updates for vfc and vfl */
 	if (!pd->enabled && pd->key != cpu_to_le32(KEY_FLOOR_CORNER) &&
 	    pd->key != cpu_to_le32(KEY_FLOOR_LEVEL))
-		goto out;
+		return 0;
 
-	ret = rpmpd_aggregate_corner(pd);
-
-out:
-	mutex_unlock(&rpmpd_lock);
-
-	return ret;
+	return rpmpd_aggregate_corner(pd);
 }
 
 static int rpmpd_probe(struct platform_device *pdev)

-- 
2.43.0

Re: [PATCH 08/10] pmdomain: qcom: rpmpd: Simplify locking with guard()

[Konrad Dybcio]

On 23.08.2024 2:51 PM, Krzysztof Kozlowski wrote:
> Simplify error handling (less gotos) over locks with guard().
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>

Thanks for taking care of this!

Konrad

[PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: 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/pmdomain/renesas/rcar-gen4-sysc.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/pmdomain/renesas/rcar-gen4-sysc.c b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
index 66409cff2083..4ca85dbdedc2 100644
--- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
+++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/clk/renesas.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
 	const struct rcar_gen4_sysc_info *info;
 	const struct of_device_id *match;
 	struct rcar_gen4_pm_domains *domains;
-	struct device_node *np;
 	void __iomem *base;
 	unsigned int i;
 	int error;
 
-	np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
+	struct device_node *np __free(device_node) =
+		of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
 	if (!np)
 		return -ENODEV;
 
@@ -317,17 +318,14 @@ static int __init rcar_gen4_sysc_pd_init(void)
 	base = of_iomap(np, 0);
 	if (!base) {
 		pr_warn("%pOF: Cannot map regs\n", np);
-		error = -ENOMEM;
-		goto out_put;
+		return -ENOMEM;
 	}
 
 	rcar_gen4_sysc_base = base;
 
 	domains = kzalloc(sizeof(*domains), GFP_KERNEL);
-	if (!domains) {
-		error = -ENOMEM;
-		goto out_put;
-	}
+	if (!domains)
+		return -ENOMEM;
 
 	domains->onecell_data.domains = domains->domains;
 	domains->onecell_data.num_domains = ARRAY_SIZE(domains->domains);
@@ -345,10 +343,8 @@ static int __init rcar_gen4_sysc_pd_init(void)
 
 		n = strlen(area->name) + 1;
 		pd = kzalloc(sizeof(*pd) + n, GFP_KERNEL);
-		if (!pd) {
-			error = -ENOMEM;
-			goto out_put;
-		}
+		if (!pd)
+			return -ENOMEM;
 
 		memcpy(pd->name, area->name, n);
 		pd->genpd.name = pd->name;
@@ -357,7 +353,7 @@ static int __init rcar_gen4_sysc_pd_init(void)
 
 		error = rcar_gen4_sysc_pd_setup(pd);
 		if (error)
-			goto out_put;
+			return error;
 
 		domains->domains[area->pdr] = &pd->genpd;
 
@@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
 		if (error) {
 			pr_warn("Failed to add PM subdomain %s to parent %u\n",
 				area->name, area->parent);
-			goto out_put;
+			return error;
 		}
 	}
 
 	error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
 
-out_put:
-	of_node_put(np);
 	return error;
 }
 early_initcall(rcar_gen4_sysc_pd_init);

-- 
2.43.0

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Geert Uytterhoeven]

Hi Krzysztof,

On Fri, Aug 23, 2024 at 2:51 PM 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>

Thanks for your patch!

> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>         const struct rcar_gen4_sysc_info *info;
>         const struct of_device_id *match;
>         struct rcar_gen4_pm_domains *domains;
> -       struct device_node *np;
>         void __iomem *base;
>         unsigned int i;
>         int error;
>
> -       np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
> +       struct device_node *np __free(device_node) =
> +               of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);

This breaks the declarations/blank-line/code structure, so please move
this up.

If you insist on keeping assignment to and validation of np together,
the line should be split in declaration and assignment.

>         if (!np)
>                 return -ENODEV;
>

> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>                 if (error) {
>                         pr_warn("Failed to add PM subdomain %s to parent %u\n",
>                                 area->name, area->parent);
> -                       goto out_put;
> +                       return error;
>                 }
>         }
>
>         error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
>
> -out_put:
> -       of_node_put(np);
>         return error;

return of_genpd_add_provider_onecell(...);

>  }
>  early_initcall(rcar_gen4_sysc_pd_init);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

On 27/08/2024 09:48, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Fri, Aug 23, 2024 at 2:51 PM 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>
> 
> Thanks for your patch!
> 
>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>>         const struct rcar_gen4_sysc_info *info;
>>         const struct of_device_id *match;
>>         struct rcar_gen4_pm_domains *domains;
>> -       struct device_node *np;
>>         void __iomem *base;
>>         unsigned int i;
>>         int error;
>>
>> -       np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
>> +       struct device_node *np __free(device_node) =
>> +               of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
> 
> This breaks the declarations/blank-line/code structure, so please move
> this up.

What do you mean "declaration structure"? That's the way how variables
with constructors are expected to be declared - within the code.

> 
> If you insist on keeping assignment to and validation of np together,
> the line should be split in declaration and assignment.

No, that would be inconsistent with cleanup/constructor coding style.
Maybe this is something new, so let me bring previous discussions:

https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@mail.gmail.com/

https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/

https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com/

and finally it will reach documentation (although it focuses on
unwinding process to be specific - "When the unwind order ..."):
https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/

> 
>>         if (!np)
>>                 return -ENODEV;
>>
> 
>> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>>                 if (error) {
>>                         pr_warn("Failed to add PM subdomain %s to parent %u\n",
>>                                 area->name, area->parent);
>> -                       goto out_put;
>> +                       return error;
>>                 }
>>         }
>>
>>         error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
>>
>> -out_put:
>> -       of_node_put(np);
>>         return error;
> 
> return of_genpd_add_provider_onecell(...);

Ack.

Best regards,
Krzysztof

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

On 27/08/2024 11:33, Krzysztof Kozlowski wrote:
> On 27/08/2024 09:48, Geert Uytterhoeven wrote:
>> Hi Krzysztof,
>>
>> On Fri, Aug 23, 2024 at 2:51 PM 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>
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
>>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
>>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>>>         const struct rcar_gen4_sysc_info *info;
>>>         const struct of_device_id *match;
>>>         struct rcar_gen4_pm_domains *domains;
>>> -       struct device_node *np;
>>>         void __iomem *base;
>>>         unsigned int i;
>>>         int error;
>>>
>>> -       np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
>>> +       struct device_node *np __free(device_node) =
>>> +               of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
>>
>> This breaks the declarations/blank-line/code structure, so please move
>> this up.
> 
> What do you mean "declaration structure"? That's the way how variables
> with constructors are expected to be declared - within the code.

Continuing thoughts, so you prefer:

	struct rcar_gen4_pm_domains *domains;
	void __iomem *base;
	struct device_node *np __free(device_node) =
		of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);

(assuming I will put it at the end of declarations).

Are you sure this is more readable? It's really long line so it
obfuscates a bit the declarations. The point of the scoped assignment is that
you declare it at point of need/first use.

> 
>>
>> If you insist on keeping assignment to and validation of np together,
>> the line should be split in declaration and assignment.
> 
> No, that would be inconsistent with cleanup/constructor coding style.
> Maybe this is something new, so let me bring previous discussions:
> 
> https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@mail.gmail.com/
> 
> https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/
> 
> https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com/
> 
> and finally it will reach documentation (although it focuses on
> unwinding process to be specific - "When the unwind order ..."):
> https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/
> 
>>
>>>         if (!np)
>>>                 return -ENODEV;
>>>
>>
>>> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>>>                 if (error) {
>>>                         pr_warn("Failed to add PM subdomain %s to parent %u\n",
>>>                                 area->name, area->parent);
>>> -                       goto out_put;
>>> +                       return error;
>>>                 }
>>>         }
>>>
>>>         error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
>>>
>>> -out_put:
>>> -       of_node_put(np);
>>>         return error;
>>
>> return of_genpd_add_provider_onecell(...);
> 
> Ack.
> 
> Best regards,
> Krzysztof
> 

Best regards,
Krzysztof

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Geert Uytterhoeven]

Hi Krzysztof,

On Tue, Aug 27, 2024 at 11:39 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 27/08/2024 11:33, Krzysztof Kozlowski wrote:
> > On 27/08/2024 09:48, Geert Uytterhoeven wrote:
> >> On Fri, Aug 23, 2024 at 2:51 PM 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>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> >>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> >>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
> >>>         const struct rcar_gen4_sysc_info *info;
> >>>         const struct of_device_id *match;
> >>>         struct rcar_gen4_pm_domains *domains;
> >>> -       struct device_node *np;
> >>>         void __iomem *base;
> >>>         unsigned int i;
> >>>         int error;
> >>>
> >>> -       np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
> >>> +       struct device_node *np __free(device_node) =
> >>> +               of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
> >>
> >> This breaks the declarations/blank-line/code structure, so please move
> >> this up.
> >
> > What do you mean "declaration structure"? That's the way how variables

First a block with declarations, then a blank line, followed by the actual code
(yeah, the pre-C99 style ;-)

> > with constructors are expected to be declared - within the code.

When it matters.

> Continuing thoughts, so you prefer:
>
>         struct rcar_gen4_pm_domains *domains;
>         void __iomem *base;
>         struct device_node *np __free(device_node) =
>                 of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
>
> (assuming I will put it at the end of declarations).
>
> Are you sure this is more readable? It's really long line so it
> obfuscates a bit the declarations. The point of the scoped assignment is that
> you declare it at point of need/first use.

You're missing reverse Christmas tree order...

> >> If you insist on keeping assignment to and validation of np together,
> >> the line should be split in declaration and assignment.
> >
> > No, that would be inconsistent with cleanup/constructor coding style.
> > Maybe this is something new, so let me bring previous discussions:

[...]

> > and finally it will reach documentation (although it focuses on

Oh, "finally" as in not yet upstream ;-)

> > unwinding process to be specific - "When the unwind order ..."):
> > https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/

"When the unwind order matters..."

So it's perfectly fine to have:

    static int __init rcar_gen4_sysc_pd_init(void)
    {
            struct device_node *np __free(device_node) = NULL;
            struct rcar_gen4_pm_domains *domains;
            const struct rcar_gen4_sysc_info *info;
            const struct of_device_id *match;
            void __iomem *base;
            unsigned int i;
            int error;

            np = of_find_matching_node_and_match(NULL,
rcar_gen4_sysc_matches, &match);
            if (!np)
                    return -ENODEV;

            ...
    }

But my first suggestion:

    static int __init rcar_gen4_sysc_pd_init(void)
    {
            struct device_node *np __free(device_node) =
                    of_find_matching_node_and_match(NULL,
rcar_gen4_sysc_matches, &match);
            struct rcar_gen4_pm_domains *domains;
            const struct rcar_gen4_sysc_info *info;
            const struct of_device_id *match;
            void __iomem *base;
            unsigned int i;
            int error;

            if (!np)
                    return -ENODEV;

            ...
    }

is safer w.r.t. to future modification.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Krzysztof Kozlowski]

On 27/08/2024 12:55, Geert Uytterhoeven wrote:
> 
> So it's perfectly fine to have:
> 
>     static int __init rcar_gen4_sysc_pd_init(void)
>     {
>             struct device_node *np __free(device_node) = NULL;
>             struct rcar_gen4_pm_domains *domains;
>             const struct rcar_gen4_sysc_info *info;
>             const struct of_device_id *match;
>             void __iomem *base;
>             unsigned int i;
>             int error;
> 
>             np = of_find_matching_node_and_match(NULL,
> rcar_gen4_sysc_matches, &match);
>             if (!np)
>                     return -ENODEV;
> 
>             ...
>     }

It is not perfectly fine because it does not match the preference of
having declaration with the constructor. See responses from Linus.

> 
> But my first suggestion:
> 
>     static int __init rcar_gen4_sysc_pd_init(void)
>     {
>             struct device_node *np __free(device_node) =
>                     of_find_matching_node_and_match(NULL,
> rcar_gen4_sysc_matches, &match);
>             struct rcar_gen4_pm_domains *domains;
>             const struct rcar_gen4_sysc_info *info;
>             const struct of_device_id *match;
>             void __iomem *base;
>             unsigned int i;
>             int error;
> 
>             if (!np)
>                     return -ENODEV;
> 
>             ...
>     }
> 
> is safer w.r.t. to future modification.

Indeed, sure, I will re-write it above.



Best regards,
Krzysztof

Re: [PATCH 09/10] pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths

[Geert Uytterhoeven]

Hi Krzysztof,

On Fri, Aug 23, 2024 at 2:51 PM 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>

Revisiting an old patch (the same applies to 10/10)...

> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
> @@ -6,6 +6,7 @@
>   */
>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk/renesas.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>         const struct rcar_gen4_sysc_info *info;
>         const struct of_device_id *match;
>         struct rcar_gen4_pm_domains *domains;
> -       struct device_node *np;
>         void __iomem *base;
>         unsigned int i;
>         int error;
>
> -       np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
> +       struct device_node *np __free(device_node) =
> +               of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match);
>         if (!np)
>                 return -ENODEV;

[...]

> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void)
>                 if (error) {
>                         pr_warn("Failed to add PM subdomain %s to parent %u\n",
>                                 area->name, area->parent);
> -                       goto out_put;
> +                       return error;
>                 }
>         }
>
>         error = of_genpd_add_provider_onecell(np, &domains->onecell_data);

np is passed to of_genpd_add_provider_onecell(), which stores a copy
for later use, so I think it must not be released in case of success?
I.e. both the old and the new code are wrong?

>
> -out_put:
> -       of_node_put(np);
>         return error;
>  }
>  early_initcall(rcar_gen4_sysc_pd_init);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 10/10] pmdomain: renesas: rcar-sysc: 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/pmdomain/renesas/rcar-sysc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/pmdomain/renesas/rcar-sysc.c b/drivers/pmdomain/renesas/rcar-sysc.c
index b99326917330..da169eed638c 100644
--- a/drivers/pmdomain/renesas/rcar-sysc.c
+++ b/drivers/pmdomain/renesas/rcar-sysc.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2015-2017 Glider bvba
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk/renesas.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -348,12 +349,12 @@ static int __init rcar_sysc_pd_init(void)
 	const struct rcar_sysc_info *info;
 	const struct of_device_id *match;
 	struct rcar_pm_domains *domains;
-	struct device_node *np;
 	void __iomem *base;
 	unsigned int i;
 	int error;
 
-	np = of_find_matching_node_and_match(NULL, rcar_sysc_matches, &match);
+	struct device_node *np __free(device_node) =
+		of_find_matching_node_and_match(NULL, rcar_sysc_matches, &match);
 	if (!np)
 		return -ENODEV;
 
@@ -362,7 +363,7 @@ static int __init rcar_sysc_pd_init(void)
 	if (info->init) {
 		error = info->init();
 		if (error)
-			goto out_put;
+			return error;
 	}
 
 	has_cpg_mstp = of_find_compatible_node(NULL, NULL,
@@ -371,8 +372,7 @@ static int __init rcar_sysc_pd_init(void)
 	base = of_iomap(np, 0);
 	if (!base) {
 		pr_warn("%pOF: Cannot map regs\n", np);
-		error = -ENOMEM;
-		goto out_put;
+		return -ENOMEM;
 	}
 
 	rcar_sysc_base = base;
@@ -382,10 +382,8 @@ static int __init rcar_sysc_pd_init(void)
 	rcar_sysc_extmask_val = info->extmask_val;
 
 	domains = kzalloc(sizeof(*domains), GFP_KERNEL);
-	if (!domains) {
-		error = -ENOMEM;
-		goto out_put;
-	}
+	if (!domains)
+		return -ENOMEM;
 
 	domains->onecell_data.domains = domains->domains;
 	domains->onecell_data.num_domains = ARRAY_SIZE(domains->domains);
@@ -403,10 +401,8 @@ static int __init rcar_sysc_pd_init(void)
 
 		n = strlen(area->name) + 1;
 		pd = kzalloc(sizeof(*pd) + n, GFP_KERNEL);
-		if (!pd) {
-			error = -ENOMEM;
-			goto out_put;
-		}
+		if (!pd)
+			return -ENOMEM;
 
 		memcpy(pd->name, area->name, n);
 		pd->genpd.name = pd->name;
@@ -417,7 +413,7 @@ static int __init rcar_sysc_pd_init(void)
 
 		error = rcar_sysc_pd_setup(pd);
 		if (error)
-			goto out_put;
+			return error;
 
 		domains->domains[area->isr_bit] = &pd->genpd;
 
@@ -429,7 +425,7 @@ static int __init rcar_sysc_pd_init(void)
 		if (error) {
 			pr_warn("Failed to add PM subdomain %s to parent %u\n",
 				area->name, area->parent);
-			goto out_put;
+			return error;
 		}
 	}
 
@@ -437,8 +433,6 @@ static int __init rcar_sysc_pd_init(void)
 	if (!error)
 		fwnode_dev_initialized(of_fwnode_handle(np), true);
 
-out_put:
-	of_node_put(np);
 	return error;
 }
 early_initcall(rcar_sysc_pd_init);

-- 
2.43.0

Re: [PATCH 10/10] pmdomain: renesas: rcar-sysc: Use scoped device node handling to simplify error paths

[Geert Uytterhoeven]

Hi Krzysztof,

On Fri, Aug 23, 2024 at 2:51 PM 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>

Thanks for your patch!

> --- a/drivers/pmdomain/renesas/rcar-sysc.c
> +++ b/drivers/pmdomain/renesas/rcar-sysc.c
> @@ -348,12 +349,12 @@ static int __init rcar_sysc_pd_init(void)
>         const struct rcar_sysc_info *info;
>         const struct of_device_id *match;
>         struct rcar_pm_domains *domains;
> -       struct device_node *np;
>         void __iomem *base;
>         unsigned int i;
>         int error;
>
> -       np = of_find_matching_node_and_match(NULL, rcar_sysc_matches, &match);
> +       struct device_node *np __free(device_node) =
> +               of_find_matching_node_and_match(NULL, rcar_sysc_matches, &match);

Same comment as on [PATCH 9/10].

>         if (!np)
>                 return -ENODEV;
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 00/10] pmdomain: Simplify with cleanup.h

[Ulf Hansson]

On Fri, 23 Aug 2024 at 14:51, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Simplify the code with scoped loops, guards and __free().
>
> Best regards,
> Krzysztof
>
> ---
> Krzysztof Kozlowski (10):
>       pmdomain: rockchip: Simplify with scoped for each OF child loop
>       pmdomain: rockchip: Simplify locking with guard()
>       pmdomain: imx: gpc: Simplify with scoped for each OF child loop
>       pmdomain: imx: gpcv2: Simplify with scoped for each OF child loop
>       pmdomain: qcom: cpr: Simplify with dev_err_probe()
>       pmdomain: qcom: cpr: Simplify locking with guard()
>       pmdomain: qcom: rpmhpd: Simplify locking with guard()
>       pmdomain: qcom: rpmpd: Simplify locking with guard()
>       pmdomain: renesas: rcar-gen4-sysc: Use scoped device node handling to simplify error paths
>       pmdomain: renesas: rcar-sysc: Use scoped device node handling to simplify error paths
>
>  drivers/pmdomain/imx/gpc.c                | 14 +++-----
>  drivers/pmdomain/imx/gpcv2.c              |  8 ++---
>  drivers/pmdomain/qcom/cpr.c               | 58 ++++++++++++-------------------
>  drivers/pmdomain/qcom/rpmhpd.c            | 11 +++---
>  drivers/pmdomain/qcom/rpmpd.c             | 20 ++++-------
>  drivers/pmdomain/renesas/rcar-gen4-sysc.c | 26 ++++++--------
>  drivers/pmdomain/renesas/rcar-sysc.c      | 28 ++++++---------
>  drivers/pmdomain/rockchip/pm-domains.c    | 25 ++++---------
>  8 files changed, 68 insertions(+), 122 deletions(-)
> ---
> base-commit: e188fd67a69319f3d105d9b90e424b8d1ff9580c
> change-id: 20240823-cleanup-h-guard-pm-domain-35eb491f35f9
>
> Best regards,
> --
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Patch 1 -> 8 applied for next, thanks!

Kind regards
Uffe