diff mbox series

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

Message ID 20230214121333.1837-16-shradha.t@samsung.com (mailing list archive)
State New
Headers show
Series [01/16] dt-bindings: PCI: Rename Exynos PCIe binding to Samsung PCIe | expand

Commit Message

Shradha Todi Feb. 14, 2023, 12:13 p.m. UTC 
Some resources might differ based on platforms and we
need platform specific functions to initialize or alter
them. For better code reusibility, making a separate
res_ops which will hold all such function pointers or
other resource specific data.

This patch includes adding function pointer for IRQ
initialization which will help to move common operations for
host init into the probe sequence.

Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pci-samsung.c | 26 ++++++++++++++++--------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Krzysztof Kozlowski Feb. 16, 2023, 11:11 a.m. UTC | #1 
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://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

Wrapping looks a bit short...

> need platform specific functions to initialize or alter
> them. For better code reusibility, making a separate

typo, I think it is: re-usability

> res_ops which will hold all such function pointers or
> other resource specific data.

Are you saying that interrupts differ in different devices?

> 
> This patch includes adding function pointer for IRQ

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> initialization which will help to move common operations for
> host init into the probe sequence.
> 
> Suggested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  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.

> +
> +	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
Shradha Todi March 2, 2023, 1:10 p.m. UTC | #2 
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 February 2023 16:42
> To: Shradha Todi <shradha.t@samsung.com>; 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
> 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@samsung.com>
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  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
diff mbox series

Patch

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);
+
+	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[] = {