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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 30 Aug 2024 11:58:19 +0800
Zhang Zekun <zhangzekun11@huawei.com> 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>
One coding style thing + some long lines that could do with wrapping.
With those tidied up.
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/pci-tegra.c | 74 ++++++++++--------------------
>  1 file changed, 23 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index d7517c3976e7..94a768a4616d 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2106,47 +2106,36 @@ 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);
Long line that I'd wrap.  Same for other similar cases.

>  
>  		lanes |= value << (index << 3);
>  

> @@ -2201,32 +2182,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		if (IS_ERR(rp->reset_gpio)) {
>  			if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
This { is only here because of the coding style rule about being
consistent.  You aren't any more as

>  				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
this is never valid under the kernel coding style.
However, you don't need the {} at all any more as all branches are if
the if / else are single lines.


> +				return dev_err_probe(dev, PTR_ERR(rp->reset_gpio),
> +						     "failed to get reset GPIO\n");
>  		}