Re: [PATCH 16/17] mfd: Add support for LAN966x PCI device

From: Herve Codina <herve.codina@bootlin.com>
To: <Steen.Hegelund@microchip.com>
Cc: <tglx@linutronix.de>, <robh@kernel.org>, <krzk+dt@kernel.org>,
	<conor+dt@kernel.org>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<lee@kernel.org>, <arnd@arndb.de>, <Horatiu.Vultur@microchip.com>,
	<UNGLinuxDriver@microchip.com>, <andrew@lunn.ch>,
	<hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
	<saravanak@google.com>, <bhelgaas@google.com>,
	<p.zabel@pengutronix.de>, <Lars.Povlsen@microchip.com>,
	<Daniel.Machon@microchip.com>, <alexandre.belloni@bootlin.com>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<netdev@vger.kernel.org>, <linux-pci@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<Allan.Nielsen@microchip.com>, <luca.ceresoli@bootlin.com>,
	<thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 16/17] mfd: Add support for LAN966x PCI device
Date: Tue, 14 May 2024 14:55:18 +0200	[thread overview]
Message-ID: <20240514145518.3e989b83@bootlin.com> (raw)
In-Reply-To: <D1447AHUWV6C.13V6FOWZ80GH@microchip.com>

Hi Steen,

On Wed, 8 May 2024 08:20:04 +0000
<Steen.Hegelund@microchip.com> wrote:

...
> > +
> > +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;
> > +}
> > +
> > +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);
> > +
> > +       fwnode = dev_fwnode(&pdev->dev);
> > +       if (!fwnode)
> > +               return ERR_PTR(-ENODEV);
> > +
> > +       intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
> > +       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_LEGACY);
> > +       if (ret < 0) {
> > +               pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
> > +               goto err_remove_domain;
> > +       }
> > +       intr_ctrl->irq = pci_irq_vector(pdev, 0);
> > +       ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> > +                         dev_name(&pdev->dev), intr_ctrl);
> > +       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 pci_dev_remove_intr_ctrl(struct pci_dev_intr_ctrl *intr_ctrl)
> > +{
> > +       free_irq(intr_ctrl->irq, intr_ctrl);
> > +       pci_free_irq_vectors(intr_ctrl->pci_dev);
> > +       irq_dispose_mapping(irq_find_mapping(intr_ctrl->irq_domain, 0));
> > +       irq_domain_remove(intr_ctrl->irq_domain);
> > +       kfree(intr_ctrl);
> > +}
> > +  
> 
> It looks like the two functions below (and their helper functions) are so
> generic that they could be part of the pci driver core support.
> Any plans for that?

Indeed, I tried to write them in a generic way.
Right now, at least for the next iteration of this series, I don't plan to
move them as part of the PCI code.
This piece of code did not get any feedback and I would prefer to keep them
here for the moment.

Of course, they could be move out of the LAN966x PCI driver later.

> 
> > +static void devm_pci_dev_remove_intr_ctrl(void *data)
> > +{
> > +       struct pci_dev_intr_ctrl *intr_ctrl = data;
> > +
> > +       pci_dev_remove_intr_ctrl(intr_ctrl);
> > +}
> > +
> > +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);
> > +
> > +       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);
> > +}
> > +  
> 

Best regards,
Hervé

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel