Re: [PATCH v2 18/19] mfd: Add support for LAN966x PCI device

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Thu, 20 Jun 2024 18:43:09 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> > On Thu, 20 Jun 2024 18:07:16 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

...

> > > > > > +   if (!dev->of_node) {
> > > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > > +           return -EINVAL;
> > > > > > +   }
> > > > >
> > > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > > already for this case.
> > > >
> > > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > > >
> > > > The check here is to ensure that an of_node is available as it will be use
> > > > for DT overlay loading.
> > >
> > > So, what exactly do you want to check? fwnode check covers this.
> > >
> > > > I will keep the check here and use dev_of_node() instead of dev->of_node.
> > >
> > > It needs to be well justified as from a coding point of view this is a
> > > duplication.
>
> On DT based system, if a fwnode is set it is an of_node.
> On ACPI, if a fwnode is set is is an acpi_node.
>
> The core PCI, when it successfully creates the DT node for a device
> (CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
> So we can have a device with:
>  - fwnode from ACPI
>  - of_node from core PCI creation

Does PCI device creation not set fwnode?

> This driver needs the of_node to load the overlay.
> Even if the core PCI cannot create a DT node for the PCI device right
> now, I don't expect this LAN855x PCI driver updated when the core PCI
> is able to create this PCI device DT node.

If it's really needed, I think the correct call here is is_of_node()
to show exactly why it's not a duplication. It also needs a comment on
top of this call.

...

> > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > >
> > > > > Don't you have VENDOR_ID defined somewhere?
> > > >
> > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > >
> > > > I will patch pci-ids.h to create:
> > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > > >
> > > > And use PCI_VENDOR_ID_MCHP in this series.
> > >
> > > Okay, but I don't think (but I haven't checked) we have something like
> > > this ever done there. In any case it's up to Bjorn how to implement
> > > this.
>
> Right, I wait for Bjorn reply before changing anything.

But we already have the vendor ID with the same value. Even if the
company was acquired, the old ID still may be used. In that case an
update on PCI IDs can go in a separate change justifying it. In any
case, I would really want to hear from Bjorn on this and if nothing
happens, to use the existing vendor ID for now to speed up the series
to be reviewed/processed.


-- 
With Best Regards,
Andy Shevchenko