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