[PATCH v2 0/2] PCI: kirin: cleanup (dev_err_probe() and scoped loop)

[PATCH v2 0/2] PCI: kirin: cleanup (dev_err_probe() and scoped loop)

[Javier Carrasco]

This series removes some patterns that require multiple steps to achieve
what single calls can achieve.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Remove error value from the message "failed to parse devfn: %d\n", as
  it is already handled by dev_err_probe().
- Fix capitalization in error messages.
- Return -ENODEV and provide more appropriate error message if no
  dev->of_node is available.
- Link to v1: https://lore.kernel.org/r/20240706-pcie-kirin-dev_err_probe-v1-0-56df797fb8ee@gmail.com

---
Javier Carrasco (2):
      PCI: kirin: use dev_err_probe() in probe error paths
      PCI: kirin: use for_each_available_child_of_node_scoped()

 drivers/pci/controller/dwc/pcie-kirin.c | 54 ++++++++++++---------------------
 1 file changed, 20 insertions(+), 34 deletions(-)
---
base-commit: 412d6f897b7a494b373986e63a14a94d0fbd0fdb
change-id: 20240705-pcie-kirin-dev_err_probe-0c9035188ff9

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v2 1/2] PCI: kirin: use dev_err_probe() in probe error paths

[Javier Carrasco]

dev_err_probe() is used in some probe error paths, yet the
"dev_err() + return" pattern is used in some others.

Use dev_err_probe() in all error paths with that construction.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/pci/controller/dwc/pcie-kirin.c | 44 +++++++++++++--------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 0a29136491b8..e00152b1ee99 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -216,10 +216,8 @@ 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 +369,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;
 
@@ -384,7 +381,7 @@ static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie,
 							GPIOD_OUT_LOW);
 		if (IS_ERR(pcie->id_clkreq_gpio[i]))
 			return dev_err_probe(dev, PTR_ERR(pcie->id_clkreq_gpio[i]),
-					     "unable to get a valid clken gpio\n");
+					     "Unable to get a valid clken gpio\n");
 
 		pcie->clkreq_names[i] = devm_kasprintf(dev, GFP_KERNEL,
 						       "pcie_clkreq_%d", i);
@@ -417,20 +414,17 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
 				if (PTR_ERR(pcie->id_reset_gpio[i]) == -ENOENT)
 					continue;
 				return dev_err_probe(dev, PTR_ERR(pcie->id_reset_gpio[i]),
-						     "unable to get a valid reset gpio\n");
+						     "Unable to get a valid reset gpio\n");
 			}
 
 			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);
 
@@ -469,7 +463,7 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 	kirin_pcie->id_dwc_perst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(kirin_pcie->id_dwc_perst_gpio))
 		return dev_err_probe(dev, PTR_ERR(kirin_pcie->id_dwc_perst_gpio),
-				     "unable to get a valid gpio pin\n");
+				     "Unable to get a valid gpio pin\n");
 	gpiod_set_consumer_name(kirin_pcie->id_dwc_perst_gpio, "pcie_perst_bridge");
 
 	ret = kirin_pcie_get_gpio_enable(kirin_pcie, pdev);
@@ -729,16 +723,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, -ENODEV, "OF node not found\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.40.1

[PATCH v2 2/2] PCI: kirin: use for_each_available_child_of_node_scoped()

[Javier Carrasco]

The scoped version of the macro automatically decrements the child node
refcount on early exits, removing the need for the `goto` and
`of_node_put()`.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 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 e00152b1ee99..7c591f50d0b2 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -446,7 +446,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;
 
@@ -471,17 +471,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.40.1

Re: [PATCH v2 0/2] PCI: kirin: cleanup (dev_err_probe() and scoped loop)

[Krzysztof Wilczyński]

Hello,

> This series removes some patterns that require multiple steps to achieve
> what single calls can achieve.

Thank you!

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

... for botch patches.

	Krzysztof

Re: [PATCH v2 1/2] PCI: kirin: use dev_err_probe() in probe error paths

[Jonathan Cameron]

On Sun, 07 Jul 2024 15:54:01 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> dev_err_probe() is used in some probe error paths, yet the
> "dev_err() + return" pattern is used in some others.
> 
> Use dev_err_probe() in all error paths with that construction.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 2/2] PCI: kirin: use for_each_available_child_of_node_scoped()

[Jonathan Cameron]

On Sun, 07 Jul 2024 15:54:02 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The scoped version of the macro automatically decrements the child node
> refcount on early exits, removing the need for the `goto` and
> `of_node_put()`.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  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 e00152b1ee99..7c591f50d0b2 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -446,7 +446,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;
>  
> @@ -471,17 +471,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,
>