Messages in this thread | | | Date | Wed, 12 Apr 2023 11:02:53 -0500 | From | Bjorn Helgaas <> | Subject | Re: [PATCH v1 1/1] PCI: of: Propagate firmware node |
| |
On Wed, Apr 12, 2023 at 04:15:20PM +0300, Andy Shevchenko wrote: > Propagate firmware node by using a specific API call, i.e. device_set_node().
Can you add a line or two about *why* we should do this, e.g., is this headed toward some goal? Is it a simplification that's 100% equivalent (doesn't seem so, see below)?
Seems like there's an underlying long-term effort to unify things from OF and ACPI, which seems like a good thing, but at the moment it's a little confusing to follow. For instance pci_set_of_node() seems like it ought to be sort of analogous to pci_set_acpi_fwnode(), but they look nothing alike.
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pci/of.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 196834ed44fe..4bba00dfbfc5 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -18,19 +18,18 @@ > #ifdef CONFIG_PCI > void pci_set_of_node(struct pci_dev *dev) > { > + struct device_node *node; > + > if (!dev->bus->dev.of_node) > return; > - dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node, > - dev->devfn); > - if (dev->dev.of_node) > - dev->dev.fwnode = &dev->dev.of_node->fwnode; > + node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); > + device_set_node(&dev->dev, of_fwnode_handle(node));
This doesn't seem 100% equivalent. If of_pci_find_child_device() returns NULL, the previous code doesn't set dev->dev.fwnode, but the new code does.
> } > > void pci_release_of_node(struct pci_dev *dev) > { > of_node_put(dev->dev.of_node); > - dev->dev.of_node = NULL; > - dev->dev.fwnode = NULL; > + device_set_node(&dev->dev, NULL); > } > > void pci_set_bus_of_node(struct pci_bus *bus) > @@ -45,17 +44,13 @@ void pci_set_bus_of_node(struct pci_bus *bus) > bus->self->external_facing = true; > } > > - bus->dev.of_node = node; > - > - if (bus->dev.of_node) > - bus->dev.fwnode = &bus->dev.of_node->fwnode; > + device_set_node(&bus->dev, of_fwnode_handle(node)); > } > > void pci_release_bus_of_node(struct pci_bus *bus) > { > of_node_put(bus->dev.of_node); > - bus->dev.of_node = NULL; > - bus->dev.fwnode = NULL; > + device_set_node(&bus->dev, NULL); > } > > struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus) > -- > 2.40.0.1.gaa8946217a0b >
| |