RE: [PATCH 04/10] PCI: exynos: Add platform device private data

["Shradha Todi" <shradha.t@samsung.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 21 May 2025 15:15
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.or;
> linux-kernel@vger.kernel.org; linux-phy@lists.infradead.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; krzk+dt@kernel.org; conor+dt@kernel.org;
> alim.akhtar@samsung.com; vkoul@kernel.org; kishon@kernel.org; arnd@arndb.de; m.szyprowski@samsung.com;
> jh80.chung@samsung.com; Pankaj Dubey <pankaj.dubey@samsung.com>
> Subject: Re: [PATCH 04/10] PCI: exynos: Add platform device private data
> 
> On Mon, May 19, 2025 at 01:01:46AM GMT, Shradha Todi wrote:
> > -static const struct dw_pcie_ops dw_pcie_ops = {
> > +static const struct dw_pcie_ops exynos_dw_pcie_ops = {
> >  	.read_dbi = exynos_pcie_read_dbi,
> >  	.write_dbi = exynos_pcie_write_dbi,
> >  	.link_up = exynos_pcie_link_up,
> > @@ -279,6 +286,7 @@ static int exynos_pcie_probe(struct
> > platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> >  	struct exynos_pcie *ep;
> > +	const struct samsung_pcie_pdata *pdata;
> >  	struct device_node *np = dev->of_node;
> >  	int ret;
> >
> > @@ -286,8 +294,11 @@ static int exynos_pcie_probe(struct platform_device *pdev)
> >  	if (!ep)
> >  		return -ENOMEM;
> >
> > +	pdata = of_device_get_match_data(dev);
> > +
> > +	ep->pdata = pdata;
> >  	ep->pci.dev = dev;
> > -	ep->pci.ops = &dw_pcie_ops;
> > +	ep->pci.ops = pdata->dwc_ops;
> >
> >  	ep->phy = devm_of_phy_get(dev, np, NULL);
> >  	if (IS_ERR(ep->phy))
> > @@ -363,9 +374,9 @@ static int exynos_pcie_resume_noirq(struct device *dev)
> >  		return ret;
> >
> >  	/* exynos_pcie_host_init controls ep->phy */
> > -	exynos_pcie_host_init(pp);
> > +	ep->pdata->host_ops->init(pp);
> >  	dw_pcie_setup_rc(pp);
> > -	exynos_pcie_start_link(pci);
> > +	ep->pdata->dwc_ops->start_link(pci);
> 
> One more layer of indirection.
> 
> Read:
> https://lore.kernel.org/all/CAL_JsqJgaeOcnUzw+rUF2yO4hQYCdZYssjxHzrDvvHGJimrASA@mail.gmail.com/
> 

I went through this thread and the solution to avoid redirection seems to be:
1. Make the common parts into a library that each driver can call
2. When there is barely anything in common, make a separate driver

From my understanding of these 2 drivers, there is hardly anything that can go into common library
1. host_init, dbi_read, dbi_write these ops have completely different flow
2. link_up, start_link have similar flow but different register offsets
3. write_dbi2 and stop_link is not implemented for exynos but needed for FSD
4. Resources are different - FSD does not have regulator, Exynos5433 does not have syscon, FSD has msi IRQ vs exynos5433 has legacy
5. Exynos is host only whereas FSD is dual mode controller.

I don’t see any other way except redirection, or using lots of if(variant == FSD) which is also discouraged.

And about making it a different driver altogether, I'm completely okay to do so. In fact we had previously tried to post it as a
different driver which was rejected.

If you still feel there is a way to separate out the common parts into a library, please guide me.

> Best regards,
> Krzysztof