Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@bootlin.com> 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