From:   "Shradha Todi" <shradha.t@samsung.com>
To:     "'Krzysztof Kozlowski'" <krzysztof.kozlowski@linaro.org>,
        <lpieralisi@kernel.org>, <kw@linux.com>, <robh@kernel.org>,
        <bhelgaas@google.com>, <krzysztof.kozlowski+dt@linaro.org>,
        <alim.akhtar@samsung.com>, <jingoohan1@gmail.com>,
        <Sergey.Semin@baikalelectronics.ru>, <lukas.bulwahn@gmail.com>,
        <hongxing.zhu@nxp.com>, <tglx@linutronix.de>,
        <m.szyprowski@samsung.com>, <jh80.chung@samsung.co>,
        <pankaj.dubey@samsung.com>
Cc:     <linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-samsung-soc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
In-Reply-To: <a0c79665-adb9-a846-5a84-d85e0684c25f@linaro.org>
Subject: RE: [PATCH 15/16] PCI: samsung: Add structure to hold resource
 operations
Date:   Thu, 2 Mar 2023 18:40:12 +0530
Message-ID: <139901d94d08$54fb9030$fef2b090$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQHB3D0FgV9/Br8Y35oSVRdB1anKHwH9qfdGAn+u9fwBvPnmbq7khR1w
Content-Language: en-in
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <20230214121333.1837-1-shradha.t@samsung.com>
        <CGME20230214121503epcas5p291dce2b37ec4cdabcfecbf8fbdfcca51@epcas5p2.samsung.com>
        <20230214121333.1837-16-shradha.t@samsung.com>
        <a0c79665-adb9-a846-5a84-d85e0684c25f@linaro.org>
Precedence: bulk


> -----Original Message-----
> From: Krzysztof Kozlowski =5Bmailto:krzysztof.kozlowski=40linaro.org=5D
> Sent: 16 February 2023 16:42
> To: Shradha Todi <shradha.t=40samsung.com>; lpieralisi=40kernel.org;
> kw=40linux.com; robh=40kernel.org; bhelgaas=40google.com;
> krzysztof.kozlowski+dt=40linaro.org; alim.akhtar=40samsung.com;
> jingoohan1=40gmail.com; Sergey.Semin=40baikalelectronics.ru;
> lukas.bulwahn=40gmail.com; hongxing.zhu=40nxp.com; tglx=40linutronix.de;
> m.szyprowski=40samsung.com; jh80.chung=40samsung.co;
> pankaj.dubey=40samsung.com
> Cc: linux-pci=40vger.kernel.org; devicetree=40vger.kernel.org; linux-arm-
> kernel=40lists.infradead.org; linux-samsung-soc=40vger.kernel.org; linux-
> kernel=40vger.kernel.org
> Subject: Re: =5BPATCH 15/16=5D PCI: samsung: Add structure to hold resour=
ce
> operations
>=20
> On 14/02/2023 13:13, Shradha Todi wrote:
> > Some resources might differ based on platforms and we
>=20
> 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=3D66656d8a-07ee78a5-6664e6c5-
> 74fe485cbfe7-a61191c61bcf38f7&q=3D1&e=3D80994c2d-d0ca-4b83-a7ca-
> 5242c4bb701f&u=3Dhttps%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18-
> rc4%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting-
> patches.rst%23L586
>=20
> Wrapping looks a bit short...

Ack

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

Ack

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

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

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

Ack

> > initialization which will help to move common operations for host init
> > into the probe sequence.
> >
> > Suggested-by: Pankaj Dubey <pankaj.dubey=40samsung.com>
> > Signed-off-by: Shradha Todi <shradha.t=40samsung.com>
> > ---
> >  drivers/pci/controller/dwc/pci-samsung.c =7C 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
> > =40=40 -55,6 +55,7 =40=40 struct samsung_pcie_pdata =7B
> >  	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;
> >  =7D;
> >
> >  /*
> > =40=40 -77,6 +78,10 =40=40 struct samsung_pcie =7B
> >  	struct regulator_bulk_data	supplies=5B2=5D;
> >  =7D;
> >
> > +struct samsung_res_ops =7B
> > +	int (*irq_init)(struct samsung_pcie *sp, struct platform_device
> > +*pdev); =7D;
> > +
> >  static int samsung_pcie_init_clk_resources(struct samsung_pcie *sp)
> > =7B
> >  	struct device *dev =3D sp->pci.dev;
> > =40=40 -276,7 +281,7 =40=40 static const struct dw_pcie_host_ops
> exynos_pcie_host_ops =3D =7B
> >  	.host_init =3D exynos_pcie_host_init,
> >  =7D;
> >
> > -static int exynos_add_pcie_port(struct samsung_pcie *sp,
> > +static int exynos_irq_init(struct samsung_pcie *sp,
> >  				       struct platform_device *pdev)  =7B
> >  	struct dw_pcie *pci =3D &sp->pci;
> > =40=40 -295,15 +300,8 =40=40 static int exynos_add_pcie_port(struct
> samsung_pcie *sp,
> >  		return ret;
> >  	=7D
> >
> > -	pp->ops =3D &exynos_pcie_host_ops;
> >  	pp->msi_irq=5B0=5D =3D -ENODEV;
> >
> > -	ret =3D dw_pcie_host_init(pp);
> > -	if (ret) =7B
> > -		dev_err(dev, =22failed to initialize host=5Cn=22);
> > -		return ret;
> > -	=7D
> > -
> >  	return 0;
> >  =7D
> >
> > =40=40 -314,6 +312,10 =40=40 static const struct dw_pcie_ops
> exynos_dw_pcie_ops =3D =7B
> >  	.start_link =3D exynos_pcie_start_link,  =7D;
> >
> > +static const struct samsung_res_ops exynos_res_ops_data =3D =7B
> > +	.irq_init		=3D exynos_irq_init,
> > +=7D;
> > +
> >  static int samsung_pcie_probe(struct platform_device *pdev)  =7B
> >  	struct device *dev =3D &pdev->dev;
> > =40=40 -357,7 +359,12 =40=40 static int samsung_pcie_probe(struct
> > platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, sp);
> >
> > -	ret =3D exynos_add_pcie_port(sp, pdev);
> > +	if (pdata->res_ops->irq_init)
> > +		pdata->res_ops->irq_init(sp, pdev);
>=20
> Check return value and handle errors.
>=20

Ack

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