On Fri, 30 Aug 2024 11:58:19 +0800
Zhang Zekun <zhangzekun11@xxxxxxxxxx> 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@xxxxxxxxxx>
One coding style thing + some long lines that could do with wrapping.
With those tidied up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> 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");
> }
