Re: [PATCH 10/41] rt2800pci: add rt2800_register_[read,write]() wrappers

[Gertjan van Wingerde <gwingerde@gmail.com>]

On 11/06/09 17:13, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 04 November 2009 20:16:26 Gertjan van Wingerde wrote:
>> On Wed, Nov 4, 2009 at 6:33 PM, Bartlomiej Zolnierkiewicz
>> <bzolnier@gmail.com> wrote:
>>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>> Subject: [PATCH] rt2800pci: add rt2800_register_[read,write]() wrappers
>>>
>>> Part of preparations for later code unification.
>>>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>> ---
>>>  drivers/net/wireless/rt2x00/rt2800pci.c |  479 ++++++++++++++++----------------
>>>  drivers/net/wireless/rt2x00/rt2800pci.h |   21 +
>>>  2 files changed, 261 insertions(+), 239 deletions(-)
>>>
>>> Index: b/drivers/net/wireless/rt2x00/rt2800pci.c
>>> ===================================================================
>>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>>> @@ -57,7 +57,7 @@ MODULE_PARM_DESC(nohwcrypt, "Disable har
>>>  /*
>>>  * Register access.
>>>  * All access to the CSR registers will go through the methods
>>> - * rt2x00pci_register_read and rt2x00pci_register_write.
>>> + * rt2800_register_read and rt2800_register_write.
>>>  * BBP and RF register require indirect register access,
>>>  * and use the CSR registers BBPCSR and RFCSR to achieve this.
>>>  * These indirect registers work with busy bits,
>>> @@ -66,6 +66,7 @@ MODULE_PARM_DESC(nohwcrypt, "Disable har
>>>  * between each attampt. When the busy bit is still set at that time,
>>>  * the access attempt is considered to have failed,
>>>  * and we will print an error.
>>> + * The _lock versions must be used if you already hold the csr_mutex
>>>  */
>>>  #define WAIT_FOR_BBP(__dev, __reg) \
>>>        rt2x00pci_regbusy_read((__dev), BBP_CSR_CFG, BBP_CSR_CFG_BUSY, (__reg))
>>
>> The change to the _lock variant seems a bit odd. See below.
>>
>> <snip>
>>
>>> Index: b/drivers/net/wireless/rt2x00/rt2800pci.h
>>> ===================================================================
>>> --- a/drivers/net/wireless/rt2x00/rt2800pci.h
>>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.h
>>> @@ -27,6 +27,27 @@
>>>  #ifndef RT2800PCI_H
>>>  #define RT2800PCI_H
>>>
>>> +static inline void rt2800_register_read(struct rt2x00_dev *rt2x00dev,
>>> +                                       const unsigned int offset,
>>> +                                       u32 *value)
>>> +{
>>> +       rt2x00pci_register_read(rt2x00dev, offset, value);
>>> +}
>>> +
>>> +static inline void rt2800_register_write(struct rt2x00_dev *rt2x00dev,
>>> +                                        const unsigned int offset,
>>> +                                        u32 value)
>>> +{
>>> +       rt2x00pci_register_write(rt2x00dev, offset, value);
>>> +}
>>> +
>>> +static inline void rt2800_register_write_lock(struct rt2x00_dev *rt2x00dev,
>>> +                                             const unsigned int offset,
>>> +                                             u32 value)
>>> +{
>>> +       rt2x00pci_register_write(rt2x00dev, offset, value);
>>> +}
>>> +
>>>  /*
>>>  * RF chip defines.
>>>  *
>>
>> Can we add a comment to the _lock variant explaining that this one
>> technically isn't
>> needed, but is present for alignment purposes with rt2800usb?
> 
> I couldn't come with the good comment for it so I just went for
> the minimal one in patch #25 (which removed all quoted above inlines):
> 
> +static const struct rt2800_ops rt2800pci_rt2800_ops = {
> +       .register_read          = rt2x00pci_register_read,
> +       .register_write         = rt2x00pci_register_write,
> +       .register_write_lock    = rt2x00pci_register_write, /* same for PCI */
> +
> +       .register_multiread     = rt2x00pci_register_multiread,
> +       .register_multiwrite    = rt2x00pci_register_multiwrite,
> +
> +       .regbusy_read           = rt2x00pci_regbusy_read,
> +};
> 
> but it certainly can be expanded if somebody has a better idea how
> the comment should look like.
> 

OK. Looks good enough for the moment. At least now there is some recognition
that it is not a bug / typo that the _write and _write_lock are the same on PCI.

With this change:

Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

---
Gertjan.