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 <herve.codina@xxxxxxxxxxx> wrote:
> Hi Andy,
>
> On Wed, 5 Jun 2024 23:24:43 +0300
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > 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.
>
> Sure, I will remove unneeded header inclusion.
>
> >
> > ...
> >
> > > +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 :)
>
> >
> > > +}
> >
> > ...
> >
> > > +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.
>
> Will be removed.
>
> >
> > > + 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() ?
>
> Yes indeed.
>
> >
> > > + 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.
>
> 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.
>
> >
> > > + /* 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.
>
> >
> > > + */
> > > + 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?
>
> DMA related.
> Allows the PCI device to be master on the bus and so initiate transactions.
>
> Did I misunderstood ?
>
> >
> > > + ret = of_platform_default_populate(dev->of_node, NULL, dev);
> >
> > dev_of_node()
>
> Yes, sure.
>
> >
> > > + 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()
>
> 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.
>
> >
> > > + 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.
>
> You're right. I will remove this call.
>
> >
> > > +}
> >
> > ...
> >
> > > +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.
>
> >
> > > + { 0, }
> >
> > Unneeded ' 0, ' part
>
> Will be removed.
>
> >
> > > +};
> >
>
> Thanks a lot for your review.
>
> Best regards,
> Hervé
