On Mon, Aug 20, 2012 at 9:47 AM, Jiang Liu [off-list ref] wrote:

On 08/20/2012 11:35 PM, Bjorn Helgaas wrote:

quoted

On Mon, Aug 20, 2012 at 9:26 AM, Jiang Liu [off-list ref] wrote:

quoted

On 08/14/2012 12:25 PM, Bjorn Helgaas wrote:

quoted

On Wed, Aug 1, 2012 at 8:54 AM, Jiang Liu [off-list ref] wrote:

quoted

From: Jiang Liu <redacted>

As suggested by Bjorn Helgaas and Don Dutile in threads
http://www.spinics.net/lists/linux-pci/msg15663.html, we could improve access
to PCIe capabilities register in to way:
1) cache content of PCIe Capabilities Register into struct pce_dev to avoid
   repeatedly reading this register because it's read only.
2) provide access functions for PCIe Capabilities registers to hide differences
   among PCIe base specifications, so the caller don't need to handle those
   differences.

This patch set applies to
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci-next

Would you mind rebasing this to v3.6-rc1?  I think you posted this
when my branch was still 3.5-based, and there are some upstream
changes that cause minor conflicts here.

You currently have:

    int pci_pcie_capability_change_word(struct pci_dev *dev, int pos,
u16 set_bits, u16 clear_bits)

I think this is a bit awkward because the function name doesn't
suggest *how* the word will be changed, and the clearing happens
before the setting (opposite the parameter order).  Something like:

    int pci_pcie_capability_mask_and_set_word(..., u16 mask, u16 set) or
    int pci_pcie_capability_clear_and_set_word(..., u16 clear, u16 set)

would be more obvious.  If you use "mask_and_set", I think the
function should do "(val & mask) | set" with the complement being at
the call site.  If you use "clear_and_set", I think it's OK to do
"(val & ~mask) | set" as in your current patch.

I know I suggested the "pci_pcie_capability_*" names, but they're
getting a bit unwieldy, especially if we do "mask_and_set" or similar.
 There are already several "pcie_*" functions, so maybe we should
drop the leading "pci_" from these and just have:

    pcie_capability_read_word
    pcie_capability_write_word
    pcie_capability_mask_and_set_word

Bjorn

Hi Bjorn,
        I have made following changes according to your suggestions,
        1) get rid of the "pci_" prefix for access functions.
        2) rename pci_pcie_capability_change_{word|dword}() to
        pcie_capability_clear_and_set_{word|dword}.
        3) add pcie_capability_{set|clear}_{word|dword}().

Are 2) and 3) really the same?  If they're really different, we'll end up with:

    pcie_capability_clear_and_set_word()
    pcie_capability_clear_and_set_dword()
    pcie_capability_set_word()
    pcie_capability_set_dword()
    pcie_capability_clear_word()
    pcie_capability_clear_dword()

It seems a little excessive to have all six interfaces, since the
first two are sufficient to provide all the functionality.

Hi Bjorn,
        pcie_capability_{set|clear}_{word|dword}() are implemented as inline functions
which just call pcie_capability_clear_and_set_{word|dword}(). If it seems a little redundant,
I could help to remove them.

I looked through your v3 patches and found

  12 clear_word callers
  9 set_word callers
  11 clear_and_set_word callers
  0 clear_dword callers
  0 set_dword callers
  2 clear_and_set_dword callers that should be clear_and_set_word
callers (in tsi721.c)
  1 clear_and_set_dword caller (PCI_EXP_RTSTA, Root Status)

I had expected that almost all would be clear_and_set users, but that
wasn't the case.  Bottom line, it does seem like there are enough
callers of clear, set, and clear_and_set to keep all of them.

There's only one legitimate user of the dword interfaces, but I'm OK
with keeping all of them for symmetry with the word interfaces.

So I'll try pulling your branch (I'll do something about the tsi721.c
stuff myself).

Thanks!

Bjorn