RE: [PATCH 15/16] PCI: samsung: Add structure to hold resource operations

[Shradha Todi]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxx]
> Sent: 16 February 2023 16:42
> To: Shradha Todi <shradha.t@xxxxxxxxxxx>; lpieralisi@xxxxxxxxxx;
> kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; alim.akhtar@xxxxxxxxxxx;
> jingoohan1@xxxxxxxxx; Sergey.Semin@xxxxxxxxxxxxxxxxxxxx;
> lukas.bulwahn@xxxxxxxxx; hongxing.zhu@xxxxxxx; tglx@xxxxxxxxxxxxx;
> m.szyprowski@xxxxxxxxxxx; jh80.chung@xxxxxxxxxx;
> pankaj.dubey@xxxxxxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 15/16] PCI: samsung: Add structure to hold resource
> operations
> 
> On 14/02/2023 13:13, Shradha Todi wrote:
> > Some resources might differ based on platforms and we
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://protect2.fireeye.com/v1/url?k=66656d8a-07ee78a5-6664e6c5-
> 74fe485cbfe7-a61191c61bcf38f7&q=1&e=80994c2d-d0ca-4b83-a7ca-
> 5242c4bb701f&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18-
> rc4%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting-
> patches.rst%23L586
> 
> Wrapping looks a bit short...

Ack

> 
> > need platform specific functions to initialize or alter them. For
> > better code reusibility, making a separate
> 
> typo, I think it is: re-usability

Ack

> 
> > res_ops which will hold all such function pointers or other resource
> > specific data.
> 
> Are you saying that interrupts differ in different devices?
> 

Yes, the interrupts are routed and integrated differently for the different platforms

> >
> > This patch includes adding function pointer for IRQ
> 
> Do not use "This commit/patch".
> https://protect2.fireeye.com/v1/url?k=ffdc2502-9e57302d-ffddae4d-
> 74fe485cbfe7-49aeaacd1141660f&q=1&e=80994c2d-d0ca-4b83-a7ca-
> 5242c4bb701f&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.17.1%2
> Fsource%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%23L95
> 

Ack

> > initialization which will help to move common operations for host init
> > into the probe sequence.
> >
> > Suggested-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pci-samsung.c | 26
> > ++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-samsung.c
> > b/drivers/pci/controller/dwc/pci-samsung.c
> > index 47ca2a6a545d..01882f2d06c7 100644
> > --- a/drivers/pci/controller/dwc/pci-samsung.c
> > +++ b/drivers/pci/controller/dwc/pci-samsung.c
> > @@ -55,6 +55,7 @@ struct samsung_pcie_pdata {
> >  	struct pci_ops				*pci_ops;
> >  	const struct dw_pcie_ops		*dwc_ops;
> >  	const struct dw_pcie_host_ops		*host_ops;
> > +	const struct samsung_res_ops		*res_ops;
> >  };
> >
> >  /*
> > @@ -77,6 +78,10 @@ struct samsung_pcie {
> >  	struct regulator_bulk_data	supplies[2];
> >  };
> >
> > +struct samsung_res_ops {
> > +	int (*irq_init)(struct samsung_pcie *sp, struct platform_device
> > +*pdev); };
> > +
> >  static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp)
> > {
> >  	struct device *dev = sp->pci.dev;
> > @@ -276,7 +281,7 @@ static const struct dw_pcie_host_ops
> exynos_pcie_host_ops = {
> >  	.host_init = exynos_pcie_host_init,
> >  };
> >
> > -static int exynos_add_pcie_port(struct samsung_pcie *sp,
> > +static int exynos_irq_init(struct samsung_pcie *sp,
> >  				       struct platform_device *pdev)  {
> >  	struct dw_pcie *pci = &sp->pci;
> > @@ -295,15 +300,8 @@ static int exynos_add_pcie_port(struct
> samsung_pcie *sp,
> >  		return ret;
> >  	}
> >
> > -	pp->ops = &exynos_pcie_host_ops;
> >  	pp->msi_irq[0] = -ENODEV;
> >
> > -	ret = dw_pcie_host_init(pp);
> > -	if (ret) {
> > -		dev_err(dev, "failed to initialize host\n");
> > -		return ret;
> > -	}
> > -
> >  	return 0;
> >  }
> >
> > @@ -314,6 +312,10 @@ static const struct dw_pcie_ops
> exynos_dw_pcie_ops = {
> >  	.start_link = exynos_pcie_start_link,  };
> >
> > +static const struct samsung_res_ops exynos_res_ops_data = {
> > +	.irq_init		= exynos_irq_init,
> > +};
> > +
> >  static int samsung_pcie_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > @@ -357,7 +359,12 @@ static int samsung_pcie_probe(struct
> > platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, sp);
> >
> > -	ret = exynos_add_pcie_port(sp, pdev);
> > +	if (pdata->res_ops->irq_init)
> > +		pdata->res_ops->irq_init(sp, pdev);
> 
> Check return value and handle errors.
> 

Ack

> > +
> > +	sp->pci.pp.ops = pdata->host_ops;
> > +
> > +	ret = dw_pcie_host_init(&sp->pci.pp);
> >  	if (ret < 0)
> >  		goto fail_probe;
> >
> > @@ -428,6 +435,7 @@ static const struct samsung_pcie_pdata
> exynos_5433_pcie_rc_pdata = {
> >  	.dwc_ops		= &exynos_dw_pcie_ops,
> >  	.pci_ops		= &exynos_pci_ops,
> >  	.host_ops		= &exynos_pcie_host_ops,
> > +	.res_ops		= &exynos_res_ops_data,
> >  };
> >
> >  static const struct of_device_id samsung_pcie_of_match[] = {
> 
> Best regards,
> Krzysztof