| Message ID | 20210823032800.1660-4-chuanjia.liu@mediatek.com |
|---|---|
| State | Accepted |
| Commit | 436960bb0045854119a61efab20578f55b311a87 |
| Headers | show |
| Series | PCI: mediatek: Spilt PCIe node to comply with hardware design | expand |
On Mon, Aug 23, 2021 at 11:27:57AM +0800, Chuanjia Liu wrote: > Use platform_get_irq_byname() to get the irq number > if the property of "interrupt-names" is defined. From patch 1/6, I have the impression that this patch is part of fixing an MSI issue. If so, this commit log should mention that as well. > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 4296d9e04240..19e35ac62d43 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -654,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > return err; > } > > - port->irq = platform_get_irq(pdev, port->slot); > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > + else > + port->irq = platform_get_irq(pdev, port->slot); This would be the only instance of this pattern, where we look for a property and use the result to decide how to look for the IRQ. dw_pcie_host_init() does something like this: port->irq = platform_get_irq_byname_optional(pdev, "pcie_irq"); if (port->irq < 0) { port->irq = platform_get_irq(pdev, port->slot); if (port->irq < 0) return port->irq; } Would that work for you? If not, the commit log should explain why you can't use the standard pattern. If you do things differently than other drivers, it makes things harder to review and slows things down. If you *have* to do something differently and it adds real value to be different, that's fine. But we should avoid unnecessary differences. > if (port->irq < 0) > return port->irq; > > -- > 2.18.0 >
On Tue, 2021-08-31 at 13:30 -0500, Bjorn Helgaas wrote: > On Mon, Aug 23, 2021 at 11:27:57AM +0800, Chuanjia Liu wrote: > > Use platform_get_irq_byname() to get the irq number > > if the property of "interrupt-names" is defined. > > From patch 1/6, I have the impression that this patch is part of > fixing an MSI issue. If so, this commit log should mention that as > well. Hi ,Bjorn Yes，I will change the commit message as follow In order to parse the new dts format that conforms to the hardware design and fixes the MSI issue，add platform_get_irq_byname_optional to get the irq number. > > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c > > b/drivers/pci/controller/pcie-mediatek.c > > index 4296d9e04240..19e35ac62d43 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -654,7 +654,11 @@ static int mtk_pcie_setup_irq(struct > > mtk_pcie_port *port, > > return err; > > } > > > > - port->irq = platform_get_irq(pdev, port->slot); > > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > > + else > > + port->irq = platform_get_irq(pdev, port->slot); > > This would be the only instance of this pattern, where we look for a > property and use the result to decide how to look for the IRQ. > > dw_pcie_host_init() does something like this: > > port->irq = platform_get_irq_byname_optional(pdev, "pcie_irq"); > if (port->irq < 0) { > port->irq = platform_get_irq(pdev, port->slot); > if (port->irq < 0) > return port->irq; > } > > Would that work for you? If not, the commit log should explain why > you can't use the standard pattern. > > If you do things differently than other drivers, it makes things > harder to review and slows things down. If you *have* to do > something > differently and it adds real value to be different, that's fine. But > we should avoid unnecessary differences. Thanks for your advice，it is very helpful to me, I will use standard pattern to avoid unnecessary differences Thanks again! Chuanjia > > > if (port->irq < 0) > > return port->irq; > > > > -- > > 2.18.0 > >
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index 4296d9e04240..19e35ac62d43 100644 --- a/drivers/pci/controller/pcie-mediatek.c +++ b/drivers/pci/controller/pcie-mediatek.c @@ -654,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, return err; } - port->irq = platform_get_irq(pdev, port->slot); + if (of_find_property(dev->of_node, "interrupt-names", NULL)) + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); + else + port->irq = platform_get_irq(pdev, port->slot); + if (port->irq < 0) return port->irq;