Message-ID: <mpfjjsrihmauddt5qwayarmylctowgme6izi6mslc7mhfild4q@ngipeevjuaxv>
Date: Wed, 21 Jan 2026 18:42:11 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: zhangsenchuan <zhangsenchuan@...incomputing.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, bhelgaas@...gle.com, 
	krzk+dt@...nel.org, conor+dt@...nel.org, lpieralisi@...nel.org, 
	kwilczynski@...nel.org, robh@...nel.org, p.zabel@...gutronix.de, jingoohan1@...il.com, 
	gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, christian.bruel@...s.st.com, mayank.rana@....qualcomm.com, 
	shradha.t@...sung.com, krishna.chundru@....qualcomm.com, thippeswamy.havalige@....com, 
	inochiama@...il.com, Frank.li@....com, ningyu@...incomputing.com, 
	linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com, 
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
 driver

On Wed, Jan 21, 2026 at 08:00:26PM +0800, zhangsenchuan wrote:
> > Subject: Re: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > 
> > > > On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > > > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@...incomputing.com wrote:
> > > > > > > From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> > > > > > > 
> > > > > > > Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> > > > > > > the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> > > > > > > supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> > > > > > > interrupts.
> > > > > > 
> > > > > > > +config PCIE_EIC7700
> > > > > > > +	tristate "Eswin EIC7700 PCIe controller"
> > > > > > 
> > > > > > > +/* Vendor and device ID value */
> > > > > > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > > > > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > > > > > 
> > > > > > Usually the device name is a little more than just the vendor.  What
> > > > > > if Eswin ever makes a second device?
> > > > > 
> > > > > Okey, thanks.
> > > > > Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> > > 
> > > Check pci_ids.h and follow the style used there.  Device ID macros
> > > typically include both the vendor and the device.
> > 
> > Okey, thanks.
> > 
> > > 
> > > > > > > +static struct platform_driver eic7700_pcie_driver = {
> > > > > > > +	.probe = eic7700_pcie_probe,
> > > > > > 
> > > > > > This driver is tristate but has no .remove() callback.  Seems like it
> > > > > > should have one?
> > > > > 
> > > > > In v2 patch, I referred to Mani's comments and removed the .remove()
> > > > > callback, as follows:
> > > > > "Since this controller implements irqchip using the DWC core driver,
> > > > > it is not safe to remove it during runtime."
> > > > > https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> > > > > 
> > > > > In addition, remove .remove() callback, because this driver has been 
> > > > > modified to builtin_platform_driver and does not support HotPlug, 
> > > > > therefore, the .remove() callback is not needed. Do you have any
> > > > > better suggestions?
> > > > 
> > > > Yes, builtin_platform_driver() wouldn't allow the users to remove
> > > > the module. So remove() callback will become useless. The reason why
> > > > this driver is tristate is that it could be loaded from rootfs and
> > > > not always statically built to the kernel image.
> > > 
> > > This .remove() vs IRQ thing is a perennial issue and it's hard to know
> > > what style new drivers should copy.
> > > 
> > > There are lots of DWC-based drivers that are tristate, implement
> > > .remove(), and use module_platform_driver() (e.g., bt1, kirin,
> > > tegra194, rcar-gen4, exynos, k1, stm32).  Is there something different
> > > about the way they implement irqchip that makes .remove() safe?
> > 
> > Hi Bjorn, Mani,
> > 
> > The comments are as follows:
> > "You can make it tristate as you've used builtin_platform_driver() which
> > guarantees that this driver won't be removed once loaded."
> > https://lore.kernel.org/linux-pci/uijg47suvluvamftyxwc65kl34eo2eu2af2o5aia4nu45hanqc@grcr2bjgph2i/
> > 
> > Do not add the remove callback. It needs to be set to a bool：
> > In v6 patch, it was mentioned to set tristate. Now, after careful 
> > consideration, setting tristate can allow loading as a module, but the 
> > driver implementation does not have a remove function. If it exists in 
> > the form of a module, after testing, When insmod driver is followed by 
> > rmmod driver, the resources cannot be released, and problems will occur
> > when insmod driver is used again. So I think that if the remove callback
> > function is not provided in the form of builtin, it can only be set to 
> > bool.
> > 
> > Add the remove callback. It can make it tristate:
> > Questions about removing it during runtime. I don't have a very good idea.
> > I still don't quite understand why it's not safe. Could you explain it to 
> > me?
> > 
> > At present, refer to other manufacturers, i think there are two ways to 
> > achieve it.
> > 1.Set a bool. Do not add the remove function, module loading is not 
> > allowed, and the driver currently does not support HotPlug.
> > 2.Set a tristate, add .remove callback.
> > 
> > I think the first one might be better for me, because there is no need 
> > to add the remove function, my understanding might also be incorrect. 
> > Please review it for me. Thanks！
> > 
> 
> Hi Bjorn, Mani,
> 
> Regarding the issue of whether to add the.remove callback, could you 
> please help me review it again? Thanks very much!
> 
> By the way, for the patch that parses multiple Root Ports, will it be 
> updated later? Do I need to wait for it to send the next version?
> 

If you are referring to the patch from Sumit [1], then no need to wait for it as
it is not a dependency for this driver.

- Mani

[1] https://lore.kernel.org/all/20260105-dt-parser-v1-0-b11c63cb5e2c@oss.qualcomm.com/

> Kind regards,
> Senchuan
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists