On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
