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

[PATCH 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>
---
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 | 49 ++++++++++++---------------------
 1 file changed, 18 insertions(+), 31 deletions(-)
---
base-commit: 412d6f897b7a494b373986e63a14a94d0fbd0fdb
change-id: 20240705-pcie-kirin-dev_err_probe-0c9035188ff9

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

[PATCH 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 | 39 +++++++++++++--------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 0a29136491b8..da8091f6b22b 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;
 
@@ -421,16 +418,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: %d\n", ret);
 
 			slot = PCI_SLOT(ret);
 
@@ -729,16 +724,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.40.1

[PATCH 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 da8091f6b22b..ac0aacb11489 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -447,7 +447,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;
 
@@ -472,17 +472,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 1/2] PCI: kirin: use dev_err_probe() in probe error paths

[Christophe JAILLET]

Le 06/07/2024 à 17:07, Javier Carrasco a écrit :
> 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>
> ---

...

> -			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: %d\n", ret);

Hi,

with dev_err_probe(), there is no more need to add 'ret' explicitly in 
the message.

CJ

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

[Javier Carrasco]

On 06/07/2024 21:47, Christophe JAILLET wrote:
> Le 06/07/2024 à 17:07, Javier Carrasco a écrit :
>> 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>
>> ---
> 
> ...
> 
>> -            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: %d\n", ret);
> 
> Hi,
> 
> with dev_err_probe(), there is no more need to add 'ret' explicitly in
> the message.
> 
> CJ

You are right, thank you. I will remove that from the original message
for v2.

Best regards,
Javier Carrasco

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

[Krzysztof Wilczyński]

Hello,

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

Thank you for this nice refactoring!  Much appreciated.

[...]
> -	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");

Something that would be nice to get consistent: adjust all the errors
capitalisation to make everything consistent, as appropriate, so that it's
either all lower-case or title case.  A mix of both often looks a bit
sloppy.

Do you think this would be something you would be willing to clean up in
this series too?  Especially since we are touching this code now.

> -	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");

Perhaps -ENODEV would be more appropriate here?  Also, the error message is
not the best, as such, I wonder if we could make it better while we are at
it, so to speak.

	Krzysztof

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

[Javier Carrasco]

On 07/07/2024 08:53, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
>> Use dev_err_probe() in all error paths with that construction.
> 
> Thank you for this nice refactoring!  Much appreciated.
> 
> [...]
>> -	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");
> 
> Something that would be nice to get consistent: adjust all the errors
> capitalisation to make everything consistent, as appropriate, so that it's
> either all lower-case or title case.  A mix of both often looks a bit
> sloppy.
> 
> Do you think this would be something you would be willing to clean up in
> this series too?  Especially since we are touching this code now.
> 
>> -	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");
> 
> Perhaps -ENODEV would be more appropriate here?  Also, the error message is
> not the best, as such, I wonder if we could make it better while we are at
> it, so to speak.
> 
> 	Krzysztof

Sure, I will add that to v2. I have seen that typical error messages in
other drivers for this error are "OF node not found", "Device node not
found" and "This driver needs device tree". Given that "OF data missing"
is used in this driver, I will go for the first one of the list, unless
something different is preferred.

Best regards,
Javier Carrasco