On 21.12.2009 11:41 海藻敬之 wrote:
> In ath5k_hw_write_ofdm_timings(), comment says
> "ALGO: coef = (5 * clock * carrier_freq) / 2) ",
> but current code is
> "coef_scaled = ((5 * (clock << 24)) / 2) / channel->center_freq;"
>
> Did they match each other ?
> I am wondering the the comment is wrong, but I am not sure that either
> is wrong.
Good point, it seems the comment is wrong. The calculation would overflow
32-bit integer if there was a multiplication instead of a division.
Lukas Turek
On 21.12.2009 16:08 Bob Copeland wrote:
> I reviewed the patch, looks fine to me. The ATH hal uses a lookup
> table to keep it inline but I don't think we have a convenient
> index available to do the same.
Yes, HAL uses a lookup table for mode -> clocks coversion, but before that can
be used it has to convert channel flags to mode, and that's a sequence of ifs
too (see function ath_hal_chan2wmode).
If performance mattered, we could store the mode index somwhere. But it
doesn't, the conversion is only needed when setting ACK timeout and slot
time - which is, for most users, never done and defaults from initvals.c are
used instead.
Lukas Turek
On Mon, Dec 21, 2009 at 04:28:35PM +0100, Luk???? Turek wrote:
> On 21.12.2009 16:08 Bob Copeland wrote:
> If performance mattered, we could store the mode index somwhere. But it
> doesn't, the conversion is only needed when setting ACK timeout and slot
> time - which is, for most users, never done and defaults from initvals.c are
> used instead.
Agreed. Feel free to add my
Acked-by: Bob Copeland <[email protected]>
to that patch.
--
Bob Copeland %% http://www.bobcopeland.com
2009/12/21 Lukáš Turek <[email protected]>:
> On 21.12.2009 11:41 海藻敬之 wrote:
>> In ath5k_hw_write_ofdm_timings(), comment says
>> "ALGO: coef = (5 * clock * carrier_freq) / 2) ",
>> but current code is
>> "coef_scaled = ((5 * (clock << 24)) / 2) / channel->center_freq;"
>>
>> Did they match each other ?
>> I am wondering the the comment is wrong, but I am not sure that either
>> is wrong.
> Good point, it seems the comment is wrong. The calculation would overflow
> 32-bit integer if there was a multiplication instead of a division.
>
> Lukas Turek
The original comment was:
/*
* ALGO -> coef = 1e8/fcarrier*fclock/40;
* scaled coef to provide precision for this floating calculation
*/
coef_scaled = clockMhzScaled / chan->channel;
So dividing by the carrier frequency sounds like the right thing,
I guess the comment is wrong.
I reviewed the patch, looks fine to me. The ATH hal uses a lookup
table to keep it inline but I don't think we have a convenient
index available to do the same.
I'll see what I can find about the pilot tracking to see if that
makes sense here.
--
Bob Copeland %% http://www.bobcopeland.com