Re: [PATCH v10 2/4] PCI: mediatek: Add new method to get shared pcie-cfg base address and parse node

[Chuanjia Liu <chuanjia.liu@mediatek.com>]

On Tue, 2021-07-13 at 13:32 +0200, Matthias Brugger wrote:
> 
> On 11/06/2021 08:09, Chuanjia Liu wrote:
> > For the new dts format, add a new method to get
> > shared pcie-cfg base address and parse node.
> > 
> > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> 
> You missed the
> Reviewed-by: Rob Herring <robh@kernel.org>
> given in v8. Or were there any substantial changes in this patch?
> 
Thanks for your review!

Only a small change，as shown below

 		if (err)
-			goto error_put_node;
+			return err;
 	}
I have a description in the V9 version:
fix kernel-ci bot warning，In the scene of using new dts format,
when mtk_pcie_parse_port fails, of_node_put don't need to be called.

So I didn't add reviewed-by rob because I changed the patch。
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++-------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 62a042e75d9a..950f577a2f44 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/msi.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> >  #include <linux/reset.h>
> >  
> >  #include "../pci.h"
> > @@ -207,6 +209,7 @@ struct mtk_pcie_port {
> >   * struct mtk_pcie - PCIe host information
> >   * @dev: pointer to PCIe device
> >   * @base: IO mapped register base
> > + * @cfg: IO mapped register map for PCIe config
> >   * @free_ck: free-run reference clock
> >   * @mem: non-prefetchable memory resource
> >   * @ports: pointer to PCIe port information
> > @@ -215,6 +218,7 @@ struct mtk_pcie_port {
> >  struct mtk_pcie {
> >  	struct device *dev;
> >  	void __iomem *base;
> > +	struct regmap *cfg;
> >  	struct clk *free_ck;
> >  
> >  	struct list_head ports;
> > @@ -650,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);
> > +
> 
> Do I understand that this is used for backwards compatibility with older DTS? I
> just wonder why we don't need to mandate
> interrupt-names = "pcie_irq"
> in the binding description.
Yes，this is used for backwards compatibility with older DTS。
If necessary, I will add the following in binding description.
- interrupt-names：Must include the following entries：
    - "pcie_irq"： The interrupt that is asserted when an MSI/INTX is
received

Best regards
Chuanjia
> 
> Regards,
> Matthias