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

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

Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:
> 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.

...

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

> +#include <linux/kernel.h>

Why do you need this?

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

...

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

> +}

...

> +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);

> +

Redundant blank line.

> +	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);
> +}

...

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

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

...

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

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

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

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

> +	pci_set_master(pdev);

You don't use MSI, what is this for?

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

dev_of_node()

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

...

> +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()

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

> +	pci_clear_master(pdev);

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

> +}

...

> +static struct pci_device_id lan966x_pci_ids[] = {
> +	{ PCI_DEVICE(0x1055, 0x9660) },

Don't you have VENDOR_ID defined somewhere?

> +	{ 0, }

Unneeded ' 0, ' part

> +};

-- 
With Best Regards,
Andy Shevchenko