Re: [PATCH 3/4] ath5k: define ath_common ops

[Jiri Slaby <jirislaby@gmail.com>]

On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> 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?

Hardly there is a measurable one. As I wrote earlier one will wait ages
for PCI in comparison to few load+call cycles.

> 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.

Well, honestly I see no strong point for it. It rather looks like an
obfuscation, not improvement.

> 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.

Ok, that one has benefits. You just needn't care about what is behind
that mapping. It will choose a PIO or MMIO op on its own.

When it's always MMIO I personally prefer simple ioremap though. (I
didn't at the time of merging the driver.)

> 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.

Ok, I think nothing more than this is needed:
+static u32 ath5k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	return ath5k_hw_reg_read(hw_priv, reg_offset)
+}
+
+static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	ath5k_hw_reg_write(hw_priv, val, reg_offset);
+}
+
+static struct ath_ops ath5k_common_ops = {
+	.read = ath5k_ioread32,
+	.write = ath5k_iowrite32,
+};

What I wonder is why ath_ops has reg+val parameters in the opposite
manner than the rest of kernel? It is error-prone.