On Mon, Aug 5, 2024 at 12:19 PM Herve Codina [off-list ref] wrote:

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.

...

+ device.h

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

+#include <linux/pci.h>

+#include <linux/pci_ids.h>

AFAIU pci_ids..h is guaranteed to be included by pci.h, but having it
here explicitly doesn't make it worse, so up to you.

+#include <linux/slab.h>

...

+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 IRQ_RETVAL(!ret);

Hmm... I dunno if it was me who suggested IRQ_RETVAL() here, but it
usually makes sense for the cases where ret is not inverted.

Perhaps

  if (ret)
    return NONE;
  return HANDLED;

is slightly better in this case?

+}

...

+static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+       struct pci_dev_intr_ctrl *intr_ctrl;
+       struct fwnode_handle *fwnode;
+       int ret;

+       if (!pdev->irq)
+               return ERR_PTR(-EOPNOTSUPP);

Before even trying to get it via APIs? (see below as well)
Also, when is it possible to have 0 here?

+       fwnode = dev_fwnode(&pdev->dev);
+       if (!fwnode)
+               return ERR_PTR(-ENODEV);
+
+       intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);

Hmm... Why not use __free()?

+       if (!intr_ctrl)
+               return ERR_PTR(-ENOMEM);
+
+       intr_ctrl->pci_dev = pdev;
+
+       intr_ctrl->irq_domain = irq_domain_create_linear(fwnode, 1, &pci_dev_irq_domain_ops,
+                                                        intr_ctrl);
+       if (!intr_ctrl->irq_domain) {
+               pci_err(pdev, "Failed to create irqdomain\n");
+               ret = -ENOMEM;
+               goto err_free_intr_ctrl;
+       }

+       ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
+       if (ret < 0) {
+               pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
+               goto err_remove_domain;
+       }

I am wondering if you even need this in case you want solely INTx.

+       intr_ctrl->irq = pci_irq_vector(pdev, 0);

Don't remember documentation by heart for this, but the implementation
suggests that it can be called without the above for retrieving INTx.

+       ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
+                         dev_name(&pdev->dev), intr_ctrl);

pci_name() ? (IIRC the macro name)

+       if (ret) {
+               pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
+               goto err_free_irq_vector;
+       }
+
+       return intr_ctrl;
+
+err_free_irq_vector:
+       pci_free_irq_vectors(pdev);
+err_remove_domain:
+       irq_domain_remove(intr_ctrl->irq_domain);
+err_free_intr_ctrl:
+       kfree(intr_ctrl);
+       return ERR_PTR(ret);
+}

...

+static void devm_pci_dev_remove_intr_ctrl(void *data)
+{

+       struct pci_dev_intr_ctrl *intr_ctrl = data;

It can be eliminated

static void devm_pci_...(void *intr_ctrl)

+       pci_dev_remove_intr_ctrl(intr_ctrl);
+}

...

+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, dev_of_node(data->dev));
+       if (ret)
+               return ret;
+
+       return 0;

return of_overlay_fdt_apply() ?

+}

...

+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;
+
+       /*
+        * On ACPI system, fwnode can point to the ACPI node.
+        * This driver needs an of_node to be used as the device-tree overlay
+        * target. This of_node should be set by the PCI core if it succeeds in
+        * creating it (CONFIG_PCI_DYNAMIC_OF_NODES feature).
+        * Check here for the validity of this of_node.
+        */
+       if (!dev_of_node(dev)) {

+               dev_err(dev, "Missing of_node for device\n");
+               return -EINVAL;

return dev_err_probe() ?

+       }
+
+       /* Need to be done before devm_pci_dev_create_intr_ctrl.
+        * It allocates an IRQ and so pdev->irq is updated.
+        */
+       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;
+
+       pci_set_drvdata(pdev, data);
+       data->dev = dev;
+
+       ret = lan966x_pci_load_overlay(data);
+       if (ret)
+               return ret;
+
+       pci_set_master(pdev);
+
+       ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
+       if (ret)
+               goto err_unload_overlay;
+
+       return 0;
+
+err_unload_overlay:
+       lan966x_pci_unload_overlay(data);
+       return ret;
+}

...

+#include <dt-bindings/clock/microchip,lan966x.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/phy/phy-lan966x-serdes.h>

+#include <dt-bindings/gpio/gpio.h>

Alphabetical order?

-- 
With Best Regards,
Andy Shevchenko