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

From: Herve Codina <[email-masked]>
Date: Thu, 20 Jun 2024 18:43:09 +0200

My bad, I wrongly answered first in private.
I already eesend my answers with people in Cc

Now, this is the Andy's your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 18:07:16 +0200
Andy Shevchenko <[email-masked]> wrote:

> On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[email-masked]> wrote:
> > On Wed, 5 Jun 2024 23:24:43 +0300
> > Andy Shevchenko <[email-masked]> 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.
> 
> ...
> 
> > > > +   pci_set_master(pdev);  
> > >
> > > You don't use MSI, what is this for?  
> >
> > DMA related.
> > Allows the PCI device to be master on the bus and so initiate transactions.
> >
> > Did I misunderstood ?  
> 
> So, you mean that the PCI device may initiate DMA transactions and
> they are not related to MSI, correct?
> 
> ...
> 
> > > > +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.
> 
> > > > +   { 0, }  
> > >
> > > Unneeded ' 0, ' part  
> >
> > Will be removed.
> >  
> > > > +};  
>