Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver

[Bjorn Helgaas]

On Tue, Jan 06, 2026 at 06:49:58PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@xxxxxxxxxxxxxxxxxx wrote:
> > > > From: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx>
> > > > 
> > > > 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.

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