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 however
we do need a way to share hw code and therefore we cannot keep
read/write ops as part of hw.c but more importantly we need something
to map READ_READ/REG_WRITE macros on hw.h which will work for both
ath9k 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'll
change the common ops to match standard practice but don't want to
change the all the REG_READ/REG_WRITE macros, so ath9k stuff will just
do the flip.
Luis
--
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
