Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 26 Jun 2023 13:11:55 -0500 | From | Bjorn Helgaas <> | Subject | Re: [PATCH V9 2/5] PCI: Create device tree node for selected devices |
| |
On Mon, Jun 26, 2023 at 10:34:05AM -0700, Lizhi Hou wrote: > On 6/21/23 13:22, Bjorn Helgaas wrote:
> Added an of_pci_make_dev_node() interface that can be used to create > device tree node for PCI devices. > > Added a PCI_DYNAMIC_OF_NODES config option. When the option is turned > on, > the kernel will generate device tree nodes for PCI bridges > unconditionally. > > Initially, the basic properties are added for the dynamically generated > device tree nodes which include #address-cells, #size-cells, > device_type, > compatible, ranges, reg.
s/Added/Add/ (twice, mentioned before).
The commit log should say what the *patch* does, not what *you* did.
> > > @@ -501,8 +501,10 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args * > > > * to rely on this function (you ship a firmware that doesn't > > > * create device nodes for all PCI devices). > > > */ > > > - if (ppnode) > > > + if (ppnode && of_property_present(ppnode, "interrupt-map")) > > > > Maybe this deserves a comment? The connection between "interrupt-map" > > and the rest of this patch isn't obvious to me. > > > > Also, it looks like this happens for *everybody*, regardless of > > PCI_DYNAMIC_OF_NODES, which seems a little suspect. If it's an > > unrelated bug fix it should be a different patch. > > This is not a bug fix. The check will distinguish between device tree nodes > automatically created for pci bridges by this patch with those created by a > DT based system. With this patch, device tree nodes are created for pci > bridges, thus ppnode here will be non-zero and we will break out of the > loop. In order to still use pci_swizzle_interrupt_pin(), checking > “interrupt-map” for ppnode is added here. > > After thinking about this more, using “interrupt-map” property may not be > correct for the cases where ppnode is not dynamically generated and it does > not have “interrupt-map”. So, I would introduce a new property “dynamic” for > pci bridge nodes generated dynamically. And change the code to: if (ppnode > && of_property_present(ppnode, "dynamic")). > > Does this make sense?
Makes a lot more sense to me than relying on some unrelated and undocumented property. Probably still would benefit from an #ifdef.
Rob might have an opinion on whether "dynamic" makes sense from a DT perspective.
Bjorn
| ![\](/images/icornerr.gif) |