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

My bad, I wrongly answered first in private.
-> Resend my answers with people in Cc

Andy, I will also resend your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 17:56:46 +0200
Herve Codina [off-list ref] wrote:

Hi Andy,

On Wed, 5 Jun 2024 23:24:43 +0300
Andy Shevchenko [off-list ref] wrote:

quoted

Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

quoted

Add a PCI driver that handles the LAN966x PCI device using a device-tree
overlay. This overlay is applied to the PCI device DT node and allows to
describe components that are present in the device.

The memory from the device-tree is remapped to the BAR memory thanks to
"ranges" properties computed at runtime by the PCI core during the PCI
enumeration.
The PCI device itself acts as an interrupt controller and is used as the
parent of the internal LAN966x interrupt controller to route the
interrupts to the assigned PCI INTx interrupt.  

...

quoted

+#include <linux/irq.h>
+#include <linux/irqdomain.h>  

quoted

+#include <linux/kernel.h>  

Why do you need this?

quoted

+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/slab.h>  

General comment to the headers (in all your patches), try to follow IWYU
principle, i.e. include what you use explicitly and don't use "proxy" headers
such as kernel.h which basically shouldn't be used at all in the drivers.

Sure, I will remove unneeded header inclusion.

quoted

...

quoted

+static irqreturn_t pci_dev_irq_handler(int irq, void *data)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl = data;
+	int ret;
+
+	ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
+	return ret ? IRQ_NONE : IRQ_HANDLED;  

There is a macro for that IRQ_RETVAL() IIRC.

Didn't known about that. Thanks for pointing out!
I will use it :)

quoted

quoted

+}  

...

quoted

+static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl;
+
+	intr_ctrl = pci_dev_create_intr_ctrl(pdev);  

quoted

+  

Redundant blank line.

Will be removed.

quoted

quoted

+	if (IS_ERR(intr_ctrl))
+		return PTR_ERR(intr_ctrl);
+
+	return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
+}  

...

quoted

+static int lan966x_pci_load_overlay(struct lan966x_pci *data)
+{
+	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
+	void *dtbo_start = __dtbo_lan966x_pci_begin;
+	int ret;
+
+	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);  

dev_of_node() ?

Yes indeed.

quoted

quoted

+	if (ret)
+		return ret;
+
+	return 0;
+}  

...

quoted

+static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data;
+	int ret;  

quoted

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

I will keep the check here and use dev_of_node() instead of dev->of_node.

quoted

quoted

+	/* Need to be done before devm_pci_dev_create_intr_ctrl.
+	 * It allocates an IRQ and so pdev->irq is updated  

Missing period at the end.

Will be added.

quoted

quoted

+	 */
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	ret = devm_pci_dev_create_intr_ctrl(pdev);
+	if (ret)
+		return ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->pci_dev = pdev;
+
+	ret = lan966x_pci_load_overlay(data);
+	if (ret)
+		return ret;  

quoted

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

quoted

quoted

+	ret = of_platform_default_populate(dev->of_node, NULL, dev);  

dev_of_node()

Yes, sure.

quoted

quoted

+	if (ret)
+		goto err_unload_overlay;
+
+	return 0;
+
+err_unload_overlay:
+	lan966x_pci_unload_overlay(data);
+	return ret;
+}  

...

quoted

+static void lan966x_pci_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data = dev_get_drvdata(dev);  

platform_get_drvdata()

platform_get_drvdata() is related to platform_device.
There is no platform_device here but a pci_dev.

I will use pci_get_drvdata() here and update probe() to
use pci_set_drvdata() for consistency.

quoted

quoted

+	of_platform_depopulate(dev);
+
+	lan966x_pci_unload_overlay(data);  

quoted

+	pci_clear_master(pdev);  

No need to call this excplicitly when pcim_enable_device() was called.

You're right. I will remove this call.

quoted

quoted

+}  

...

quoted

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

quoted

quoted

+	{ 0, }  

Unneeded ' 0, ' part

Will be removed.

quoted

quoted

+};  

Thanks a lot for your review.

Best regards,
Hervé