On Wed, 29 Nov 2023, Bjorn Helgaas wrote: > On Mon, Nov 27, 2023 at 09:34:05AM +0800, Shuai Xue wrote: > > On 2023/11/22 21:14, Ilpo Järvinen wrote: > > > On Tue, 21 Nov 2023, Shuai Xue wrote: > > > > > >> The clear and set pattern is commonly used for accessing PCI config, > > >> move the helper pci_clear_and_set_dword() from aspm.c into PCI header. > > >> In addition, rename to pci_clear_and_set_config_dword() to retain the > > >> "config" information and match the other accessors. > > >> > > >> No functional change intended. > > >> + > > >> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos, > > >> + u32 clear, u32 set) > > > > > > Just noting that annoyingly the ordering within the name is inconsistent > > > between: > > > pci_clear_and_set_config_dword() > > > and > > > pcie_capability_clear_and_set_dword() > > > > > > And if changed, it would be again annoyingly inconsistent with > > > pci_read/write_config_*(), oh well... And renaming pci_read/write_config_* > > > into the hierarchical pci_config_read/write_*() form for would touch only > > > ~6k lines... ;-D > > > > I think it is a good question, but I don't have a clear answer. I don't > > know much about the name history. As you mentioned, the above two > > accessors are the foundation operation, may it comes to @Bjorn decision. > > > > The pci_clear_and_set_config_dword() is a variant of below pci accessors: > > > > pci_read_config_dword() > > pci_write_config_dword() > > > > At last, they are consistent :) > > "pcie_capability_clear_and_set_dword" is specific to the PCIe > Capability, doesn't work for arbitrary config space, and doesn't > include the word "config". > > "pci_clear_and_set_config_dword" seems consistent with the arbitrary > config space accessor pattern. > > At least "clear_and_set" is consistent across both. > > I'm not too bothered by the difference between "clear_and_set_dword" > (for the PCIe capability) and "clear_and_set_config_dword" (for > arbitrary things). > > Yes, "pcie_capability_clear_and_set_config_dword" would be a little > more consistent, but seems excessively wordy (no pun intended). > > But maybe I'm missing your point, Ilpo. If so, what would you > propose? What I was hoping for a way to (eventually) have consistency in naming like this (that is, the place where "config" or "capabilitity" appears in the name): pci_config_read_dword() pci_config_clear_and_set_dword() pcie_capability_read_dword() pcie_capability_clear_and_set_dword() (+ the omitted clear/set/write & size variants) But thanks to pci_read_config_dword() & friends being there since dawn of time and with 6k+ instances, I guess I'm just dreaming of impossible things. -- i.
