On Sat, Sep 12, 2009 at 4:53 AM, Jiri Slaby <jirislaby@xxxxxxxxx> wrote:> 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.
Alright, this helps.
>> 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.
It may be and for ath5k this is not a requirement, for ath9k howeverwe do need a way to share hw code and therefore we cannot keepread/write ops as part of hw.c but more importantly we need somethingto map READ_READ/REG_WRITE macros on hw.h which will work for bothath9k and ath9k_htc.
>> 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,> +};
That's fine for ath5k.
> What I wonder is why ath_ops has reg+val parameters in the opposite> manner than the rest of kernel? It is error-prone.
Heh, yeah, that's just how the Atheros hw code was written, so I'llchange the common ops to match standard practice but don't want tochange the all the REG_READ/REG_WRITE macros, so ath9k stuff will justdo the flip.
Luis_______________________________________________devel mailing listdevel@xxxxxxxxxxxxxxxxxxxxxxxxxx://driverdev.linuxdriverproject.org/mailman/listinfo/devel
