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

On Thu, 07 Nov 2024, Robert Joslyn wrote:

quoted

-----Original Message-----
From: Lee Jones <lee@kernel.org>
Sent: Tuesday, November 5, 2024 9:31 AM
To: Robert Joslyn <redacted>
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.

quoted

support

[Caution - External]

On Mon, 28 Oct 2024, Robert Joslyn wrote:

quoted

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 <redacted>
---
 MAINTAINERS                |   8 +
 drivers/mfd/Kconfig        |  16 ++
 drivers/mfd/Makefile       |   3 +
 drivers/mfd/selpvmf-core.c | 482
+++++++++++++++++++++++++++++++++++++
 drivers/mfd/selpvmf-cvp.c  | 431

+++++++++++++++++++++++++++++++++

quoted

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

[...]

quoted

quoted

+/**
+ * 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.

quoted

quoted

+                        const char *name,

This never changes.  Why supply it as a variable?

quoted

+                        unsigned int start, +
unsigned int length,

This never changes.  Why supply it as a variable?

quoted

+                        unsigned int msix_start,

This never changes.  Why supply it as a variable?

quoted

+                        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.

[...]

quoted

quoted

+     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.

[...]

quoted

quoted

+             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.

[...] 

quoted

quoted

+                     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.

[...]

quoted

quoted

+ +             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.

quoted

quoted

+     }, +     { +             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 [李琼斯]