| Message ID | 1525854012-24228-2-git-send-email-chenyao11@huawei.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Add MSI support | expand |
[+cc Tejun, Wolfram] On Wed, May 09, 2018 at 04:20:11PM +0800, Yao Chen wrote: > Add support for MSI. > ... > @@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port *pp) > static int __init kirin_add_pcie_port(struct dw_pcie *pci, > struct platform_device *pdev) > { > + int ret; > + > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + pci->pp.msi_irq = platform_get_irq(pdev, 0); > + if (!pci->pp.msi_irq) { I think this test is incorrect. platform_get_irq() returns a negative errno value when it fails. Most calls test "irq < 0" to check for failure. There's a lot of duplicated code like this, so maybe we should consider putting that check into devm_request_irq(), similar to what devm_ioremap_resource() does, so the driver code could look like this: pci->pp.msi_irq = platform_get_irq(pdev, 0); ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, ...); if (ret) { dev_err(&pdev->dev, "failed to request MSI IRQ\n"); return ret; } The basic devm_ioremap_resource() motivation is here: 72f8c0bfa0de ("lib: devres: add convenience function to remap a resource") and the same considerations seem to apply here. But that's more than you need to do for *this* series. So for now, I would simply fix the test to check for "irq < 0" and update the messages as I mention below. > + dev_err(&pdev->dev, "failed to get msi irq[%d]\n", > + pci->pp.msi_irq); > + return -ENODEV; > + } > + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, > + kirin_pcie_msi_irq_handler, > + IRQF_SHARED | IRQF_NO_THREAD, > + "kirin_pcie_msi", &pci->pp); > + if (ret) { > + dev_err(&pdev->dev, "failed to request msi irq[%d]\n", s/msi irq/MSI IRQ/ in both dev_err() messages above. This is because the message is English text (not code), and the convention is that non-words like these initialisms written in all caps. I would style the first one as "failed to get MSI IRQ (%d)" because the %d there is a return code, probably -ENXIO. The second one should be "failed to request MSI IRQ %d" because here the %d is the actual IRQ. > + pci->pp.msi_irq); > + return ret; > + } > + } > + > pci->pp.ops = &kirin_pcie_host_ops; > > return dw_pcie_host_init(&pci->pp); > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn, > > [+cc Tejun, Wolfram] > > On Wed, May 09, 2018 at 04:20:11PM +0800, Yao Chen wrote: > > Add support for MSI. > > ... > > > @@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port > > *pp) static int __init kirin_add_pcie_port(struct dw_pcie *pci, > > struct platform_device *pdev) { > > + int ret; > > + > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > + pci->pp.msi_irq = platform_get_irq(pdev, 0); > > + if (!pci->pp.msi_irq) { > > I think this test is incorrect. platform_get_irq() returns a negative errno > value when it fails. Most calls test "irq < 0" to check for failure. > > There's a lot of duplicated code like this, so maybe we should consider > putting that check into devm_request_irq(), similar to what > devm_ioremap_resource() does, so the driver code could look like this: > > pci->pp.msi_irq = platform_get_irq(pdev, 0); > ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, ...); > if (ret) { > dev_err(&pdev->dev, "failed to request MSI IRQ\n"); > return ret; > } > > The basic devm_ioremap_resource() motivation is here: 72f8c0bfa0de ("lib: > devres: add convenience function to remap a resource") and the same > considerations seem to apply here. > > But that's more than you need to do for *this* series. So for now, I would > simply fix the test to check for "irq < 0" and update the messages as I > mention below. Thank you for pointing out my mistake. I'll fix it. > > > + dev_err(&pdev->dev, "failed to get msi irq[%d]\n", > > + pci->pp.msi_irq); > > + return -ENODEV; > > + } > > + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, > > + kirin_pcie_msi_irq_handler, > > + IRQF_SHARED | IRQF_NO_THREAD, > > + "kirin_pcie_msi", &pci->pp); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to request msi > irq[%d]\n", > > s/msi irq/MSI IRQ/ in both dev_err() messages above. This is because the > message is English text (not code), and the convention is that non-words like > these initialisms written in all caps. > > I would style the first one as "failed to get MSI IRQ (%d)" because the %d > there is a return code, probably -ENXIO. > > The second one should be "failed to request MSI IRQ %d" because here > the %d is the actual IRQ. > I'll fix it. Thanks again. > > + pci->pp.msi_irq); > > + return ret; > > + } > > + } > > + > > pci->pp.ops = &kirin_pcie_host_ops; > > > > return dw_pcie_host_init(&pci->pp); > > -- > > 1.9.1 > > Best regards, Yao
diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index d2970a0..4328efe 100644 --- a/drivers/pci/dwc/pcie-kirin.c +++ b/drivers/pci/dwc/pcie-kirin.c @@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port *pp) return 0; } +static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg) +{ + struct pcie_port *pp = arg; + + return dw_handle_msi_irq(pp); +} + +static void kirin_pcie_msi_init(struct pcie_port *pp) +{ + dw_pcie_msi_init(pp); +} + +static void kirin_pcie_enable_interrupts(struct pcie_port *pp) +{ + if (IS_ENABLED(CONFIG_PCI_MSI)) + kirin_pcie_msi_init(pp); +} + static int kirin_pcie_host_init(struct pcie_port *pp) { kirin_pcie_establish_link(pp); + kirin_pcie_enable_interrupts(pp); return 0; } @@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port *pp) static int __init kirin_add_pcie_port(struct dw_pcie *pci, struct platform_device *pdev) { + int ret; + + if (IS_ENABLED(CONFIG_PCI_MSI)) { + pci->pp.msi_irq = platform_get_irq(pdev, 0); + if (!pci->pp.msi_irq) { + dev_err(&pdev->dev, "failed to get msi irq[%d]\n", + pci->pp.msi_irq); + return -ENODEV; + } + ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, + kirin_pcie_msi_irq_handler, + IRQF_SHARED | IRQF_NO_THREAD, + "kirin_pcie_msi", &pci->pp); + if (ret) { + dev_err(&pdev->dev, "failed to request msi irq[%d]\n", + pci->pp.msi_irq); + return ret; + } + } + pci->pp.ops = &kirin_pcie_host_ops; return dw_pcie_host_init(&pci->pp);
Add support for MSI. Signed-off-by: Yao Chen <chenyao11@huawei.com> --- drivers/pci/dwc/pcie-kirin.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)