Messages in this thread |  | | From | "Shradha Todi" <> | | Subject | RE: [PATCH 04/10] PCI: exynos: Add platform device private data | | Date | Tue, 27 May 2025 16:13:58 +0530 |
| |
> -----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
|  |