Re: [RFC PATCH 1/2] mfd: Add SEL PCI Virtual Multifunction (PVMF) support

[Lee Jones <lee@kernel.org>]

On Thu, 07 Nov 2024, Robert Joslyn wrote:

> > -----Original Message-----
> > From: Lee Jones <lee@kernel.org>
> > Sent: Tuesday, November 5, 2024 9:31 AM
> > To: Robert Joslyn <robert_joslyn@selinc.com>
> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> > Subject: Re: [RFC PATCH 1/2] mfd: Add SEL PCI Virtual Multifunction (PVMF)

These should not end up in the mail's body.

> > support
> > 
> > [Caution - External]
> > 
> > On Mon, 28 Oct 2024, Robert Joslyn wrote:
> > 
> > > Add support for SEL FPGA based PCIe devices. These expose a single PCI
> > > BAR with multiple IP cores for various functions, such as serial
> > > ports, ethernet, and time (PTP/IRIG). This initial driver supports
> > > Ethernet on the SEL-3350 mainboard, SEL-3390E4 ethernet card, and
> > > SEL-3390T ethernet and time card.
> > >
> > > Signed-off-by: Robert Joslyn <robert_joslyn@selinc.com>
> > > ---
> > >  MAINTAINERS                |   8 +
> > >  drivers/mfd/Kconfig        |  16 ++
> > >  drivers/mfd/Makefile       |   3 +
> > >  drivers/mfd/selpvmf-core.c | 482
> > > +++++++++++++++++++++++++++++++++++++
> > >  drivers/mfd/selpvmf-cvp.c  | 431
> > +++++++++++++++++++++++++++++++++
> > > drivers/mfd/selpvmf-cvp.h  |  18 ++
> > >  6 files changed, 958 insertions(+)
> > >  create mode 100644 drivers/mfd/selpvmf-core.c  create mode 100644
> > > drivers/mfd/selpvmf-cvp.c  create mode 100644
> > > drivers/mfd/selpvmf-cvp.h

[...]

> > > +/**
> > > + * sel_create_cell() - Create an MFD cell and add it to the device.
> > > + * @pdev: The PCI device to operate on
> > > + * @name: MFD cell name
> > > + * @start: Start address of memory resource
> > > + * @length: Length of memory resource
> > > + * @msix_start: MSIX vector number to start from.
> > > + * @resources: Pointer to resources to add to the cell.
> > > + * @num_resources: Number of resources. The first resource is assumed to
> > > + *                 be memory, all other resources are IRQs.
> > > + */
> > > +static int sel_create_cell(struct pci_dev *pdev,
> > 
> > This is misleading.  It should be sel_register_device.
> 
> I'll rename the function.

Please refrain from responding to comments that you agree with, then
trim the rest.  It will save multiple people a lot of time.

> 
> > 
> > > +                        const char *name,
> > 
> > This never changes.  Why supply it as a variable?
> > 
> > > +                        unsigned int start, +
> > > unsigned int length,
> > 
> > This never changes.  Why supply it as a variable?
> > 
> > > +                        unsigned int msix_start,
> > 
> > This never changes.  Why supply it as a variable?
> > 
> > > +                        struct resource *resources, +
> > > unsigned int num_resources)
> > 
> > This never changes.  Why supply it as a variable?
> 
> This patch only enables ethernet, but we have additional functionality
> that we will enable in subsequent patches, such as serial ports, a
> time component, and a firmware upgrade mechanism. Those components
> have different names, lengths, interrupt counts, etc. If you prefer, I
> can remove this abstraction until we have another cell type to add.

I can only review what I see before me.

Add the extra pieces when/if you upstream the additional functionality.

[...]

> > > +     if (rc != 0) +             return rc; + +     /* Perform CvP
> > > program if necessary */ +     cvp_capable =
> > > pci_find_ext_capability(pdev, CVP_CAPABILITY_ID); +     if
> > > (!skipcvp && cvp_capable) {
> > 
> > The '_' inconsistency is bothering me.
> > 
> > As is the placement of "cvp".
> 
> Would you prefer to see skipcvp called skip_cvp? Or cvp_skip? I'm
> happy to change it.

The latter seems more consistent.

[...]

> > > +             rc = cvp_start(pdev); +             if (rc != 0) { +
> > > dev_err(&pdev->dev, "Error: CvP failed: 0x%x\n", + rc);
> > 
> > This is a user facing error message and I still don't know what CvP
> > is.
> > 
> > Please be more forthcoming.
> 
> CvP is Configuration via Protocol. It's a method for loading firmware
> into Altera FPGAs. If CvP fails, it means we're unable to load
> firmware into the FPGA and the card is basically dead. I can improve
> the error message. Maybe something like, "Loading FPGA firmware
> failed"?

Sounds good.

[...] 

> > 
> > > +                     goto out_disable; +             } +     } +
> > > +     rc = alloc_fn(pdev);
> > 
> > These call-backs are horrible.  Please don't do this.
> 
> Ok, we'll do something else. Would you prefer to pass in a constant
> and use a switch statement to select which function to call?

That sort of thing, yes.

[...]

> > > + +             pci_restore_state(pdev); +     } + +
> > > pci_set_master(pdev); + +     return 0; +} + +static const struct
> > > dev_pm_ops selpvmf_pm_ops = { +
> > > SYSTEM_SLEEP_PM_OPS(selpvmf_suspend, selpvmf_resume) }; + +static
> > > struct pci_device_id selpvmf_pci_tbl[] = { +     { +
> > > PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B1190), +
> > > .driver_data = (kernel_ulong_t)b1190_alloc_mfd_cells,
> > 
> > Pass an identifier here instead of call-back functions.
> 
> Do you have a preference for the type of construct we use? Just a
> #define integer with a switch statement in probe? Something else?

Probably an enum.  Take a look at some other drivers.

> > > +     }, +     { +             PCI_DEVICE(PCI_VENDOR_ID_SEL,
> > > PCI_DEVICE_ID_B2077), +             .driver_data =
> > > (kernel_ulong_t)b2077_alloc_mfd_cells, +     }, +     { +
> > > PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B2091), +
> > > .driver_data = (kernel_ulong_t)b2091_alloc_mfd_cells, +     }, +
> > > { +             PCI_DEVICE(PCI_VENDOR_ID_SEL,
> > > PCI_DEVICE_ID_B2093), +             .driver_data =
> > > (kernel_ulong_t)b2093_alloc_mfd_cells, +     }, +     {0,}
> > 
> > What is this?  Where have you seen that before?
> 
> The {0,} is how to signal the end of this array. I don't think the
> comma is required though. Random example:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/lpc_sch.c?h=v6.12-rc6#n66

{} is fine.

-- 
Lee Jones [李琼斯]