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

[Herve Codina <herve.codina@bootlin.com>]

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@bootlin.com> wrote:

> Hi Andy,
> 
> On Wed, 5 Jun 2024 23:24:43 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> 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é