[PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby at gmail.com> wrote:
> On 09/11/2009 08:16 AM, Nick Kossifidis wrote:
>>>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>>  {
>>> -       return ioread32(ah->ah_iobase + reg);
>>> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>>>  }
>>>
>>> -/*
>>> - * Write to a register
>>> - */
>>>  static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>>>  {
>>> -       iowrite32(val, ah->ah_iobase + reg);
>>> +       ath5k_hw_common(ah)->ops->write(ah, reg, val);
> ...
>> Since each driver will use it's own reg read/write functions (ath5k hw
>> code still uses ath5k_hw_reg_read/write), why do we need to have
>> common reg read/write functions like that used in the driver code ?
>> This makes the code more complex that it needs to be and i don't see a
>> reason why we need it. I understand why we need a way to handle
>> read/write functions from common ath code but i don't see a reason to
>> use these functions on ath5k, we can just fill ath_ops struct so that
>> common code can use them and leave ath5k_hw_read/write untouched.
>
> I definitely agree with Nick here. Althought whole ath_ops will be hot
> cache after the first operation, there is no need to prolong hot paths
> by computing the op address and a call. Ok, read/write on PCI is pretty
> slow, but still...

That is the way I had it originally before submission, and I
completely agree its reasonable to not incur additional cost at the
expense of having two separate read/write paths, and perhaps we should
only incur the extra cost on routines shared between
ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
penalty?

This is why I asked if someone can test and give measurable
differences over this. If there really isn't then that's not strong
point against it.

For example, long ago I had argued over the cost incurred over the
unnecessary branching on ioread()/iowrite() when you know you have
MMIO devices [1] -- the defense then, and IMHO reasonable now, was
that the benefits of allowing cleaner drivers through the new
interfaces outweigh the theoretical penalties imposed by them.

Granted you can argue these new interfaces between
ath5k/ath9k/ath9k_htc would make things a little more complex, but I
would expect sharing the code will help in the end. And if these
interfaces are not acceptable I'm completely open to better suggested
alternatives.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0709.2/1068.html

  Luis