[PATCH v3 0/6] Simplify code with _scoped() helper functions

[PATCH v3 0/6] Simplify code with _scoped() helper functions

[Zhang Zekun]

Use _scoped() functions to iterate through the child node, and we don't
need to call of_node_put() manually. This can simplify the code a bit.

v2:
- Use dev_err_probe() to simplify code.
- Fix spelling error in commit message.

v3:
- Fix a spelling error.
- wrap the line when it is too long.

Zhang Zekun (6):
  PCI: kirin: Use helper function
    for_each_available_child_of_node_scoped()
  PCI: kirin: Tidy up _probe() related function with dev_err_probe()
  PCI: mediatek: Use helper function
    for_each_available_child_of_node_scoped()
  PCI: mt7621: Use helper function
    for_each_available_child_of_node_scoped()
  PCI: apple: Use helper function for_each_child_of_node_scoped()
  PCI: tegra: Use helper function for_each_child_of_node_scoped()

 drivers/pci/controller/dwc/pcie-kirin.c | 50 ++++++----------
 drivers/pci/controller/pci-tegra.c      | 80 +++++++++----------------
 drivers/pci/controller/pcie-apple.c     |  4 +-
 drivers/pci/controller/pcie-mediatek.c  | 15 ++---
 drivers/pci/controller/pcie-mt7621.c    | 15 ++---
 5 files changed, 58 insertions(+), 106 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] PCI: kirin: Use helper function for_each_available_child_of_node_scoped()

[Zhang Zekun]

for_each_available_child_of_node_scoped() provides a scope-based cleanup
functionality to put the device_node automatically, and we don't need to
call of_node_put() directly.  Let's simplify the code a bit with the use
of these functions.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2:
Fix spelling error in commit message.

 drivers/pci/controller/dwc/pcie-kirin.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 0a29136491b8..e9bda1746ca5 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -452,7 +452,7 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *child, *node = dev->of_node;
+	struct device_node *node = dev->of_node;
 	void __iomem *apb_base;
 	int ret;
 
@@ -477,17 +477,13 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 		return ret;
 
 	/* Parse OF children */
-	for_each_available_child_of_node(node, child) {
+	for_each_available_child_of_node_scoped(node, child) {
 		ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
 		if (ret)
-			goto put_node;
+			return ret;
 	}
 
 	return 0;
-
-put_node:
-	of_node_put(child);
-	return ret;
 }
 
 static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
-- 
2.17.1

[PATCH v3 2/6] PCI: kirin: Tidy up _probe() related function with dev_err_probe()

[Zhang Zekun]

The combination of dev_err() and the returned error code could be
replaced by dev_err_probe() in driver's probe function. Let's,
converting to dev_err_probe() to make code more simple.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v3: Wrap the line which is too long.

 drivers/pci/controller/dwc/pcie-kirin.c | 40 ++++++++++---------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index e9bda1746ca5..3c9d8da3a241 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -216,10 +216,9 @@ static int hi3660_pcie_phy_start(struct hi3660_pcie_phy *phy)
 
 	usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
 	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0);
-	if (reg_val & PIPE_CLK_STABLE) {
-		dev_err(dev, "PIPE clk is not stable\n");
-		return -EINVAL;
-	}
+	if (reg_val & PIPE_CLK_STABLE)
+		return dev_err_probe(dev, -EINVAL,
+				     "PIPE clk is not stable\n");
 
 	return 0;
 }
@@ -371,10 +370,9 @@ static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie,
 	if (ret < 0)
 		return 0;
 
-	if (ret > MAX_PCI_SLOTS) {
-		dev_err(dev, "Too many GPIO clock requests!\n");
-		return -EINVAL;
-	}
+	if (ret > MAX_PCI_SLOTS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Too many GPIO clock requests!\n");
 
 	pcie->n_gpio_clkreq = ret;
 
@@ -421,16 +419,14 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
 			}
 
 			pcie->num_slots++;
-			if (pcie->num_slots > MAX_PCI_SLOTS) {
-				dev_err(dev, "Too many PCI slots!\n");
-				return -EINVAL;
-			}
+			if (pcie->num_slots > MAX_PCI_SLOTS)
+				return dev_err_probe(dev, -EINVAL,
+						     "Too many PCI slots!\n");
 
 			ret = of_pci_get_devfn(child);
-			if (ret < 0) {
-				dev_err(dev, "failed to parse devfn: %d\n", ret);
-				return ret;
-			}
+			if (ret < 0)
+				return dev_err_probe(dev, ret,
+						     "failed to parse devfn\n");
 
 			slot = PCI_SLOT(ret);
 
@@ -725,16 +721,12 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	int ret;
 
-	if (!dev->of_node) {
-		dev_err(dev, "NULL node\n");
-		return -EINVAL;
-	}
+	if (!dev->of_node)
+		return dev_err_probe(dev, -EINVAL, "NULL node\n");
 
 	data = of_device_get_match_data(dev);
-	if (!data) {
-		dev_err(dev, "OF data missing\n");
-		return -EINVAL;
-	}
+	if (!data)
+		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
 
 	kirin_pcie = devm_kzalloc(dev, sizeof(struct kirin_pcie), GFP_KERNEL);
 	if (!kirin_pcie)
-- 
2.17.1

[PATCH v3 3/6] PCI: mediatek: Use helper function for_each_available_child_of_node_scoped()

[Zhang Zekun]

for_each_available_child_of_node_scoped() provides a scope-based cleanup
functionality to put the device_node automatically, and we don't need to
call of_node_put() directly.  Let's simplify the code a bit with the use
of these functions.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2:
- Use dev_err_probe() to simplify code.
- Fix spelling error in commit message.

 drivers/pci/controller/pcie-mediatek.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 9be3cebd862e..0457b9d0ad8b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -1042,24 +1042,22 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
 static int mtk_pcie_setup(struct mtk_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	struct device_node *node = dev->of_node, *child;
+	struct device_node *node = dev->of_node;
 	struct mtk_pcie_port *port, *tmp;
 	int err, slot;
 
 	slot = of_get_pci_domain_nr(dev->of_node);
 	if (slot < 0) {
-		for_each_available_child_of_node(node, child) {
+		for_each_available_child_of_node_scoped(node, child) {
 			err = of_pci_get_devfn(child);
-			if (err < 0) {
-				dev_err(dev, "failed to get devfn: %d\n", err);
-				goto error_put_node;
-			}
+			if (err < 0)
+				return dev_err_probe(dev, err, "failed to get devfn\n");
 
 			slot = PCI_SLOT(err);
 
 			err = mtk_pcie_parse_port(pcie, child, slot);
 			if (err)
-				goto error_put_node;
+				return err;
 		}
 	} else {
 		err = mtk_pcie_parse_port(pcie, node, slot);
@@ -1080,9 +1078,6 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie)
 		mtk_pcie_subsys_powerdown(pcie);
 
 	return 0;
-error_put_node:
-	of_node_put(child);
-	return err;
 }
 
 static int mtk_pcie_probe(struct platform_device *pdev)
-- 
2.17.1

[PATCH v3 4/6] PCI: mt7621: Use helper function for_each_available_child_of_node_scoped()

[Zhang Zekun]

for_each_available_child_of_node_scoped() provides a scope-based cleanup
functionality to put the device_node automatically, and we don't need to
call of_node_put() directly.  Let's simplify the code a bit with the use
of these functions.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v3: Fix spelling error in commit message.

v2: Use dev_perr_probe to simplify code.

 drivers/pci/controller/pcie-mt7621.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 9b4754a45515..354d401428f0 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -258,30 +258,25 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	struct device_node *node = dev->of_node, *child;
+	struct device_node *node = dev->of_node;
 	int err;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
 
-	for_each_available_child_of_node(node, child) {
+	for_each_available_child_of_node_scoped(node, child) {
 		int slot;
 
 		err = of_pci_get_devfn(child);
-		if (err < 0) {
-			of_node_put(child);
-			dev_err(dev, "failed to parse devfn: %d\n", err);
-			return err;
-		}
+		if (err < 0)
+			return dev_err_probe(dev, err, "failed to parse devfn\n");
 
 		slot = PCI_SLOT(err);
 
 		err = mt7621_pcie_parse_port(pcie, child, slot);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v3 5/6] PCI: apple: Use helper function for_each_child_of_node_scoped()

[Zhang Zekun]

for_each_child_of_node_scoped() provides a scope-based cleanup
functionality to put the device_node automatically, and we don't need to
call of_node_put() directly.  Let's simplify the code a bit with the
use of these functions.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2: Fix spelling error in commit message.

 drivers/pci/controller/pcie-apple.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index fefab2758a06..f1bd20a64b67 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -764,7 +764,6 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
 	struct platform_device *platform = to_platform_device(dev);
-	struct device_node *of_port;
 	struct apple_pcie *pcie;
 	int ret;
 
@@ -787,11 +786,10 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 	if (ret)
 		return ret;
 
-	for_each_child_of_node(dev->of_node, of_port) {
+	for_each_child_of_node_scoped(dev->of_node, of_port) {
 		ret = apple_pcie_setup_port(pcie, of_port);
 		if (ret) {
 			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
-			of_node_put(of_port);
 			return ret;
 		}
 	}
-- 
2.17.1

[PATCH v3 6/6] PCI: tegra: Use helper function for_each_child_of_node_scoped()

[Zhang Zekun]

for_each_child_of_node_scoped() provides a scope-based cleanup
functionality to put the device_node automatically, and we don't need to
call of_node_put() directly.  Let's simplify the code a bit with the use
of these functions.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v2:
- Use dev_err_probe() to simplify code.
- Fix spelling error in commit message.

v3: Wrap the line which is too long.

 drivers/pci/controller/pci-tegra.c | 80 +++++++++++-------------------
 1 file changed, 28 insertions(+), 52 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index d7517c3976e7..d2f29b87ffa5 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2106,47 +2106,39 @@ static int tegra_pcie_get_regulators(struct tegra_pcie *pcie, u32 lane_mask)
 static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	struct device_node *np = dev->of_node, *port;
+	struct device_node *np = dev->of_node;
 	const struct tegra_pcie_soc *soc = pcie->soc;
 	u32 lanes = 0, mask = 0;
 	unsigned int lane = 0;
 	int err;
 
 	/* parse root ports */
-	for_each_child_of_node(np, port) {
+	for_each_child_of_node_scoped(np, port) {
 		struct tegra_pcie_port *rp;
 		unsigned int index;
 		u32 value;
 		char *label;
 
 		err = of_pci_get_devfn(port);
-		if (err < 0) {
-			dev_err(dev, "failed to parse address: %d\n", err);
-			goto err_node_put;
-		}
+		if (err < 0)
+			return dev_err_probe(dev, err, "failed to parse address\n");
 
 		index = PCI_SLOT(err);
 
-		if (index < 1 || index > soc->num_ports) {
-			dev_err(dev, "invalid port number: %d\n", index);
-			err = -EINVAL;
-			goto err_node_put;
-		}
+		if (index < 1 || index > soc->num_ports)
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid port number: %d\n", index);
 
 		index--;
 
 		err = of_property_read_u32(port, "nvidia,num-lanes", &value);
-		if (err < 0) {
-			dev_err(dev, "failed to parse # of lanes: %d\n",
-				err);
-			goto err_node_put;
-		}
+		if (err < 0)
+			return dev_err_probe(dev, err,
+					     "failed to parse # of lanes\n");
 
-		if (value > 16) {
-			dev_err(dev, "invalid # of lanes: %u\n", value);
-			err = -EINVAL;
-			goto err_node_put;
-		}
+		if (value > 16)
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid # of lanes: %u\n", value);
 
 		lanes |= value << (index << 3);
 
@@ -2159,16 +2151,12 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		lane += value;
 
 		rp = devm_kzalloc(dev, sizeof(*rp), GFP_KERNEL);
-		if (!rp) {
-			err = -ENOMEM;
-			goto err_node_put;
-		}
+		if (!rp)
+			return -ENOMEM;
 
 		err = of_address_to_resource(port, 0, &rp->regs);
-		if (err < 0) {
-			dev_err(dev, "failed to parse address: %d\n", err);
-			goto err_node_put;
-		}
+		if (err < 0)
+			return dev_err_probe(dev, err, "failed to parse address\n");
 
 		INIT_LIST_HEAD(&rp->list);
 		rp->index = index;
@@ -2177,16 +2165,12 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		rp->np = port;
 
 		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
-		if (IS_ERR(rp->base)) {
-			err = PTR_ERR(rp->base);
-			goto err_node_put;
-		}
+		if (IS_ERR(rp->base))
+			return PTR_ERR(rp->base);
 
 		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
-		if (!label) {
-			err = -ENOMEM;
-			goto err_node_put;
-		}
+		if (!label)
+			return -ENOMEM;
 
 		/*
 		 * Returns -ENOENT if reset-gpios property is not populated
@@ -2199,34 +2183,26 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 						       GPIOD_OUT_LOW,
 						       label);
 		if (IS_ERR(rp->reset_gpio)) {
-			if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
+			if (PTR_ERR(rp->reset_gpio) == -ENOENT)
 				rp->reset_gpio = NULL;
-			} else {
-				dev_err(dev, "failed to get reset GPIO: %ld\n",
-					PTR_ERR(rp->reset_gpio));
-				err = PTR_ERR(rp->reset_gpio);
-				goto err_node_put;
-			}
+			else
+				return dev_err_probe(dev, PTR_ERR(rp->reset_gpio),
+						     "failed to get reset GPIO\n");
 		}
 
 		list_add_tail(&rp->list, &pcie->ports);
 	}
 
 	err = tegra_pcie_get_xbar_config(pcie, lanes, &pcie->xbar_config);
-	if (err < 0) {
-		dev_err(dev, "invalid lane configuration\n");
-		return err;
-	}
+	if (err < 0)
+		return dev_err_probe(dev, err,
+				     "invalid lane configuration\n");
 
 	err = tegra_pcie_get_regulators(pcie, mask);
 	if (err < 0)
 		return err;
 
 	return 0;
-
-err_node_put:
-	of_node_put(port);
-	return err;
 }
 
 /*
-- 
2.17.1

Re: [PATCH v3 0/6] Simplify code with _scoped() helper functions

[Krzysztof Wilczyński]

Hello,

> Use _scoped() functions to iterate through the child node, and we don't
> need to call of_node_put() manually. This can simplify the code a bit.

Applied to scoped-cleanup, thank you!

	Krzysztof

Re: [PATCH v3 1/6] PCI: kirin: Use helper function for_each_available_child_of_node_scoped()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:08PM +0800, Zhang Zekun wrote:
> for_each_available_child_of_node_scoped() provides a scope-based cleanup
> functionality to put the device_node automatically, and we don't need to
> call of_node_put() directly.  Let's simplify the code a bit with the use
> of these functions.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2:
> Fix spelling error in commit message.
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 0a29136491b8..e9bda1746ca5 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -452,7 +452,7 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
>  				    struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *child, *node = dev->of_node;
> +	struct device_node *node = dev->of_node;
>  	void __iomem *apb_base;
>  	int ret;
>  
> @@ -477,17 +477,13 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
>  		return ret;
>  
>  	/* Parse OF children */
> -	for_each_available_child_of_node(node, child) {
> +	for_each_available_child_of_node_scoped(node, child) {
>  		ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
>  		if (ret)
> -			goto put_node;
> +			return ret;
>  	}
>  
>  	return 0;
> -
> -put_node:
> -	of_node_put(child);
> -	return ret;
>  }
>  
>  static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 2/6] PCI: kirin: Tidy up _probe() related function with dev_err_probe()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:09PM +0800, Zhang Zekun wrote:
> The combination of dev_err() and the returned error code could be
> replaced by dev_err_probe() in driver's probe function. Let's,
> converting to dev_err_probe() to make code more simple.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v3: Wrap the line which is too long.
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 40 ++++++++++---------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index e9bda1746ca5..3c9d8da3a241 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -216,10 +216,9 @@ static int hi3660_pcie_phy_start(struct hi3660_pcie_phy *phy)
>  
>  	usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
>  	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0);
> -	if (reg_val & PIPE_CLK_STABLE) {
> -		dev_err(dev, "PIPE clk is not stable\n");
> -		return -EINVAL;
> -	}
> +	if (reg_val & PIPE_CLK_STABLE)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "PIPE clk is not stable\n");

I guess this is a timeout issue, so -ETIMEDOUT.

>  
>  	return 0;
>  }
> @@ -371,10 +370,9 @@ static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie,
>  	if (ret < 0)
>  		return 0;
>  
> -	if (ret > MAX_PCI_SLOTS) {
> -		dev_err(dev, "Too many GPIO clock requests!\n");
> -		return -EINVAL;
> -	}
> +	if (ret > MAX_PCI_SLOTS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many GPIO clock requests!\n");
>  
>  	pcie->n_gpio_clkreq = ret;
>  
> @@ -421,16 +419,14 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
>  			}
>  
>  			pcie->num_slots++;
> -			if (pcie->num_slots > MAX_PCI_SLOTS) {
> -				dev_err(dev, "Too many PCI slots!\n");
> -				return -EINVAL;
> -			}
> +			if (pcie->num_slots > MAX_PCI_SLOTS)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "Too many PCI slots!\n");
>  
>  			ret = of_pci_get_devfn(child);
> -			if (ret < 0) {
> -				dev_err(dev, "failed to parse devfn: %d\n", ret);
> -				return ret;
> -			}
> +			if (ret < 0)
> +				return dev_err_probe(dev, ret,
> +						     "failed to parse devfn\n");
>  
>  			slot = PCI_SLOT(ret);
>  
> @@ -725,16 +721,12 @@ static int kirin_pcie_probe(struct platform_device *pdev)
>  	struct dw_pcie *pci;
>  	int ret;
>  
> -	if (!dev->of_node) {
> -		dev_err(dev, "NULL node\n");
> -		return -EINVAL;
> -	}
> +	if (!dev->of_node)
> +		return dev_err_probe(dev, -EINVAL, "NULL node\n");

This check is pointless, so you should drop it.

>  
>  	data = of_device_get_match_data(dev);
> -	if (!data) {
> -		dev_err(dev, "OF data missing\n");
> -		return -EINVAL;
> -	}
> +	if (!data)
> +		return dev_err_probe(dev, -EINVAL, "OF data missing\n");

-ENODATA

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 3/6] PCI: mediatek: Use helper function for_each_available_child_of_node_scoped()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:10PM +0800, Zhang Zekun wrote:
> for_each_available_child_of_node_scoped() provides a scope-based cleanup
> functionality to put the device_node automatically, and we don't need to
> call of_node_put() directly.  Let's simplify the code a bit with the use
> of these functions.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2:
> - Use dev_err_probe() to simplify code.
> - Fix spelling error in commit message.
> 
>  drivers/pci/controller/pcie-mediatek.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 9be3cebd862e..0457b9d0ad8b 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -1042,24 +1042,22 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
>  static int mtk_pcie_setup(struct mtk_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> -	struct device_node *node = dev->of_node, *child;
> +	struct device_node *node = dev->of_node;
>  	struct mtk_pcie_port *port, *tmp;
>  	int err, slot;
>  
>  	slot = of_get_pci_domain_nr(dev->of_node);
>  	if (slot < 0) {
> -		for_each_available_child_of_node(node, child) {
> +		for_each_available_child_of_node_scoped(node, child) {
>  			err = of_pci_get_devfn(child);
> -			if (err < 0) {
> -				dev_err(dev, "failed to get devfn: %d\n", err);
> -				goto error_put_node;
> -			}
> +			if (err < 0)
> +				return dev_err_probe(dev, err, "failed to get devfn\n");
>  
>  			slot = PCI_SLOT(err);
>  
>  			err = mtk_pcie_parse_port(pcie, child, slot);
>  			if (err)
> -				goto error_put_node;
> +				return err;
>  		}
>  	} else {
>  		err = mtk_pcie_parse_port(pcie, node, slot);
> @@ -1080,9 +1078,6 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie)
>  		mtk_pcie_subsys_powerdown(pcie);
>  
>  	return 0;
> -error_put_node:
> -	of_node_put(child);
> -	return err;
>  }
>  
>  static int mtk_pcie_probe(struct platform_device *pdev)
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 4/6] PCI: mt7621: Use helper function for_each_available_child_of_node_scoped()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:11PM +0800, Zhang Zekun wrote:
> for_each_available_child_of_node_scoped() provides a scope-based cleanup
> functionality to put the device_node automatically, and we don't need to
> call of_node_put() directly.  Let's simplify the code a bit with the use
> of these functions.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v3: Fix spelling error in commit message.
> 
> v2: Use dev_perr_probe to simplify code.
> 
>  drivers/pci/controller/pcie-mt7621.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 9b4754a45515..354d401428f0 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -258,30 +258,25 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	struct device_node *node = dev->of_node, *child;
> +	struct device_node *node = dev->of_node;
>  	int err;
>  
>  	pcie->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pcie->base))
>  		return PTR_ERR(pcie->base);
>  
> -	for_each_available_child_of_node(node, child) {
> +	for_each_available_child_of_node_scoped(node, child) {
>  		int slot;
>  
>  		err = of_pci_get_devfn(child);
> -		if (err < 0) {
> -			of_node_put(child);
> -			dev_err(dev, "failed to parse devfn: %d\n", err);
> -			return err;
> -		}
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "failed to parse devfn\n");
>  
>  		slot = PCI_SLOT(err);
>  
>  		err = mt7621_pcie_parse_port(pcie, child, slot);
> -		if (err) {
> -			of_node_put(child);
> +		if (err)
>  			return err;
> -		}
>  	}
>  
>  	return 0;
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 5/6] PCI: apple: Use helper function for_each_child_of_node_scoped()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:12PM +0800, Zhang Zekun wrote:
> for_each_child_of_node_scoped() provides a scope-based cleanup
> functionality to put the device_node automatically, and we don't need to
> call of_node_put() directly.  Let's simplify the code a bit with the
> use of these functions.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Fix spelling error in commit message.
> 
>  drivers/pci/controller/pcie-apple.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index fefab2758a06..f1bd20a64b67 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -764,7 +764,6 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
>  	struct platform_device *platform = to_platform_device(dev);
> -	struct device_node *of_port;
>  	struct apple_pcie *pcie;
>  	int ret;
>  
> @@ -787,11 +786,10 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	if (ret)
>  		return ret;
>  
> -	for_each_child_of_node(dev->of_node, of_port) {
> +	for_each_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {
>  			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);
> -			of_node_put(of_port);
>  			return ret;
>  		}
>  	}
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 6/6] PCI: tegra: Use helper function for_each_child_of_node_scoped()

[Manivannan Sadhasivam]

On Sat, Aug 31, 2024 at 12:04:13PM +0800, Zhang Zekun wrote:
> for_each_child_of_node_scoped() provides a scope-based cleanup
> functionality to put the device_node automatically, and we don't need to
> call of_node_put() directly.  Let's simplify the code a bit with the use
> of these functions.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2:
> - Use dev_err_probe() to simplify code.
> - Fix spelling error in commit message.
> 
> v3: Wrap the line which is too long.
> 
>  drivers/pci/controller/pci-tegra.c | 80 +++++++++++-------------------
>  1 file changed, 28 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index d7517c3976e7..d2f29b87ffa5 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2106,47 +2106,39 @@ static int tegra_pcie_get_regulators(struct tegra_pcie *pcie, u32 lane_mask)
>  static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> -	struct device_node *np = dev->of_node, *port;
> +	struct device_node *np = dev->of_node;
>  	const struct tegra_pcie_soc *soc = pcie->soc;
>  	u32 lanes = 0, mask = 0;
>  	unsigned int lane = 0;
>  	int err;
>  
>  	/* parse root ports */
> -	for_each_child_of_node(np, port) {
> +	for_each_child_of_node_scoped(np, port) {
>  		struct tegra_pcie_port *rp;
>  		unsigned int index;
>  		u32 value;
>  		char *label;
>  
>  		err = of_pci_get_devfn(port);
> -		if (err < 0) {
> -			dev_err(dev, "failed to parse address: %d\n", err);
> -			goto err_node_put;
> -		}
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "failed to parse address\n");
>  
>  		index = PCI_SLOT(err);
>  
> -		if (index < 1 || index > soc->num_ports) {
> -			dev_err(dev, "invalid port number: %d\n", index);
> -			err = -EINVAL;
> -			goto err_node_put;
> -		}
> +		if (index < 1 || index > soc->num_ports)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid port number: %d\n", index);
>  
>  		index--;
>  
>  		err = of_property_read_u32(port, "nvidia,num-lanes", &value);
> -		if (err < 0) {
> -			dev_err(dev, "failed to parse # of lanes: %d\n",
> -				err);
> -			goto err_node_put;
> -		}
> +		if (err < 0)
> +			return dev_err_probe(dev, err,
> +					     "failed to parse # of lanes\n");
>  
> -		if (value > 16) {
> -			dev_err(dev, "invalid # of lanes: %u\n", value);
> -			err = -EINVAL;
> -			goto err_node_put;
> -		}
> +		if (value > 16)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid # of lanes: %u\n", value);
>  
>  		lanes |= value << (index << 3);
>  
> @@ -2159,16 +2151,12 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		lane += value;
>  
>  		rp = devm_kzalloc(dev, sizeof(*rp), GFP_KERNEL);
> -		if (!rp) {
> -			err = -ENOMEM;
> -			goto err_node_put;
> -		}
> +		if (!rp)
> +			return -ENOMEM;
>  
>  		err = of_address_to_resource(port, 0, &rp->regs);
> -		if (err < 0) {
> -			dev_err(dev, "failed to parse address: %d\n", err);
> -			goto err_node_put;
> -		}
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "failed to parse address\n");
>  
>  		INIT_LIST_HEAD(&rp->list);
>  		rp->index = index;
> @@ -2177,16 +2165,12 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		rp->np = port;
>  
>  		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
> -		if (IS_ERR(rp->base)) {
> -			err = PTR_ERR(rp->base);
> -			goto err_node_put;
> -		}
> +		if (IS_ERR(rp->base))
> +			return PTR_ERR(rp->base);
>  
>  		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
> -		if (!label) {
> -			err = -ENOMEM;
> -			goto err_node_put;
> -		}
> +		if (!label)
> +			return -ENOMEM;
>  
>  		/*
>  		 * Returns -ENOENT if reset-gpios property is not populated
> @@ -2199,34 +2183,26 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  						       GPIOD_OUT_LOW,
>  						       label);
>  		if (IS_ERR(rp->reset_gpio)) {
> -			if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
> +			if (PTR_ERR(rp->reset_gpio) == -ENOENT)
>  				rp->reset_gpio = NULL;
> -			} else {
> -				dev_err(dev, "failed to get reset GPIO: %ld\n",
> -					PTR_ERR(rp->reset_gpio));
> -				err = PTR_ERR(rp->reset_gpio);
> -				goto err_node_put;
> -			}
> +			else
> +				return dev_err_probe(dev, PTR_ERR(rp->reset_gpio),
> +						     "failed to get reset GPIO\n");
>  		}
>  
>  		list_add_tail(&rp->list, &pcie->ports);
>  	}
>  
>  	err = tegra_pcie_get_xbar_config(pcie, lanes, &pcie->xbar_config);
> -	if (err < 0) {
> -		dev_err(dev, "invalid lane configuration\n");
> -		return err;
> -	}
> +	if (err < 0)
> +		return dev_err_probe(dev, err,
> +				     "invalid lane configuration\n");
>  
>  	err = tegra_pcie_get_regulators(pcie, mask);
>  	if (err < 0)
>  		return err;
>  
>  	return 0;
> -
> -err_node_put:
> -	of_node_put(port);
> -	return err;
>  }
>  
>  /*
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 2/6] PCI: kirin: Tidy up _probe() related function with dev_err_probe()

[Krzysztof Wilczyński]

Hello,

[...]
> >  	usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
> >  	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0);
> > -	if (reg_val & PIPE_CLK_STABLE) {
> > -		dev_err(dev, "PIPE clk is not stable\n");
> > -		return -EINVAL;
> > -	}
> > +	if (reg_val & PIPE_CLK_STABLE)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "PIPE clk is not stable\n");
> 
> I guess this is a timeout issue, so -ETIMEDOUT.

I can update this directly on the branch.

[...]
> > -	if (!dev->of_node) {
> > -		dev_err(dev, "NULL node\n");
> > -		return -EINVAL;
> > -	}
> > +	if (!dev->of_node)
> > +		return dev_err_probe(dev, -EINVAL, "NULL node\n");
> 
> This check is pointless, so you should drop it.

Some other drivers are also doing this, I suppose, as a precaution.

> >  	data = of_device_get_match_data(dev);
> > -	if (!data) {
> > -		dev_err(dev, "OF data missing\n");
> > -		return -EINVAL;
> > -	}
> > +	if (!data)
> > +		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
> 
> -ENODATA

Almost all of the other drivers return -EINVAL in this case.

We can update this here, but I wonder if a follow-up patch to update other
drivers accordingly where it makes sense of course, would be better here?

Thoughts?

	Krzysztof

Re: [PATCH v3 0/6] Simplify code with _scoped() helper functions

[Krzysztof Wilczyński]

> > Use _scoped() functions to iterate through the child node, and we don't
> > need to call of_node_put() manually. This can simplify the code a bit.
> 
> Applied to scoped-cleanup, thank you!

Updated with "Reviewed-by" tags Mani offered recently.

	Krzysztof

Re: [PATCH v3 2/6] PCI: kirin: Tidy up _probe() related function with dev_err_probe()

[Krzysztof Wilczyński]

Hello,

[...]
> > > +	if (reg_val & PIPE_CLK_STABLE)
> > > +		return dev_err_probe(dev, -EINVAL,
> > > +				     "PIPE clk is not stable\n");
> > 
> > I guess this is a timeout issue, so -ETIMEDOUT.
> 
> I can update this directly on the branch.

Done.

[...]
> > > +	if (!dev->of_node)
> > > +		return dev_err_probe(dev, -EINVAL, "NULL node\n");
> > 
> > This check is pointless, so you should drop it.

Done.

[...]
> > > +	if (!data)
> > > +		return dev_err_probe(dev, -EINVAL, "OF data missing\n");
> > 
> > -ENODATA
> 
> Almost all of the other drivers return -EINVAL in this case.
> 
> We can update this here, but I wonder if a follow-up patch to update other
> drivers accordingly where it makes sense of course, would be better here?
> 
> Thoughts?

I left this one as-is for now.

	Krzysztof