On Sat, Mar 15, 2025 at 08:06:52AM +0800, Hans Zhang wrote: > On 2025/3/15 04:31, Bjorn Helgaas wrote: > > On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote: > > > ... > > > > > Even though this patch is mostly for an out of tree controller > > > driver which is not going to be upstreamed, the patch itself is > > > serving some purpose. I really like to avoid the hardcoded offsets > > > wherever possible. So I'm in favor of this patch. > > > > > > However, these newly introduced functions are a duplicated version > > > of DWC functions. So we will end up with duplicated functions in > > > multiple places. I'd like them to be moved (both this and DWC) to > > > drivers/pci/pci.c if possible. The generic function > > > *_find_capability() can accept the controller specific readl/ readw > > > APIs and the controller specific private data. > > > > I agree, it would be really nice to share this code. > > > > It looks a little messy to deal with passing around pointers to > > controller read ops, and we'll still end up with a lot of duplicated > > code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(), > > etc. > > > > Maybe someday we'll make a generic way to access non-PCI "config" > > space like this host controller space and PCIe RCRBs. > > > > Or if you add interfaces that accept read/write ops, maybe the > > existing pci_find_capability() etc could be refactored on top of them > > by passing in pci_bus_read_config_word() as the accessor. > > I have already replied to an email, please help review whether it is > appropriate. URL to the email on lore.kernel.org? If you mean this: https://lore.kernel.org/r/3cadf8d5-c4d8-4941-ae2e-8b00ceb83a8f@xxxxxxx, just post it as a v3 patch with the usual commit log and motivation for the change, and it will automatically get picked up in patchwork so it doesn't get forgotten. Right now the urgency seems fairly low since (IIUC) it doesn't fix an existing bug in the upstream code. Bjorn
