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

[Krzysztof Kozlowski]

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@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.

> +
> +	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