lkml.org 
[lkml]   [2025]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 04/10] PCI: exynos: Add platform device private data
Date


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




\
 
 \ /
  Last update: 2025-05-28 06:54    [from the cache]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog