Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Thu, 30 Nov 2023 12:52:20 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
cc:     Shuai Xue <xueshuai@linux.alibaba.com>,
        ilkka@os.amperecomputing.com, kaishen@linux.alibaba.com,
        yangyicong@huawei.com, will@kernel.org,
        Jonathan.Cameron@huawei.com, baolin.wang@linux.alibaba.com,
        robin.murphy@arm.com, chengyou@linux.alibaba.com,
        LKML <linux-kernel@vger.kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
        rdunlap@infradead.org, mark.rutland@arm.com,
        zhuo.song@linux.alibaba.com, renyu.zj@linux.alibaba.com
Subject: Re: [PATCH v11 3/5] PCI: Move pci_clear_and_set_dword() helper to
 PCI header
In-Reply-To: <20231129231555.GA443895@bhelgaas>
Message-ID: <b2aeaf4-8960-8967-ce7b-663f9d77260@linux.intel.com>
References: <20231129231555.GA443895@bhelgaas>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1899874312-1701341547=:1808"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1899874312-1701341547=:1808
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT

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.

--8323329-1899874312-1701341547=:1808--