Make sure we don't store the table offsets for min and cur power levels,
store the 0.25dB values instead. This way we don't clamp the tx power level
to max (because now cur_pwr holds the 0.25dB value, not the table offset) after
re-using cur_pwr on reset.
Signed-off-by: Nick Kossifidis <[email protected]>
---
drivers/net/wireless/ath/ath5k/phy.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 8b71a2d..aa1a77d 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -3562,6 +3562,20 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
for (i = 8; i <= 15; i++)
rates[i] -= ah->ah_txpower.txp_cck_ofdm_gainf_delta;
+ /* Save min/max and current tx power for this channel
+ * in 0.25dB units.
+ *
+ * Note: We use rates[0] for current tx power because
+ * it covers most of the rates, in most cases. It's our
+ * tx power limit and what the user expects to see. */
+ ah->ah_txpower.txp_min_pwr = 2 * rates[7];
+ ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
+
+ /* Set max txpower for correct OFDM operation on all rates
+ * -that is the txpower for 54Mbit-, it's used for the PAPD
+ * gain probe and it's in 0.5dB units */
+ ah->ah_txpower.txp_ofdm = rates[7];
+
/* Now that we have all rates setup use table offset to
* match the power range set by user with the power indices
* on PCDAC/PDADC table */
@@ -3571,11 +3585,6 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
if (rates[i] > 63)
rates[i] = 63;
}
-
- /* Min/max in 0.25dB units */
- ah->ah_txpower.txp_min_pwr = 2 * rates[7];
- ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
- ah->ah_txpower.txp_ofdm = rates[7];
}
--
1.7.3.4
rates[i] is unsigned but txp_offset can be negative for newer parts
with PDADC table. We cover the case when rates[i] + txp_offset > 63
but we must also cover the case when its < 0 or else rates[i] will underflow.
Signed-off-by: Nick Kossifidis <[email protected]>
---
drivers/net/wireless/ath/ath5k/phy.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index aa1a77d..84a9aaf 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -3516,6 +3516,7 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
{
unsigned int i;
u16 *rates;
+ s16 rate_idx_scaled = 0;
/* max_pwr is power level we got from driver/user in 0.5dB
* units, switch to 0.25dB units so we can compare */
@@ -3580,10 +3581,13 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
* match the power range set by user with the power indices
* on PCDAC/PDADC table */
for (i = 0; i < 16; i++) {
- rates[i] += ah->ah_txpower.txp_offset;
+ rate_idx_scaled = rates[i] + ah->ah_txpower.txp_offset;
/* Don't get out of bounds */
- if (rates[i] > 63)
- rates[i] = 63;
+ if (rate_idx_scaled > 63)
+ rate_idx_scaled = 63;
+ if (rate_idx_scaled < 0)
+ rate_idx_scaled = 0;
+ rates[i] = rate_idx_scaled;
}
}
--
1.7.3.4
2012/7/29 Thomas Huehn <[email protected]>:
> Hi Nick,
>
>
>> Anyway if readability is the issue we can just do something like
>>
>> int txpower_halfdb = 0;
>>
>> [...]
>>
>> if(ah->power_level) {
>> txpower_halfdb = ah->power_level * 2;
>> } else {
>> txpower_halfdb = AR5K_TUNE_MAX_TXPOWER;
>> }
>>
>> and then it'll look like this
>>
>> ret = ath5k_hw_txpower(ah, channel, txpower_halfdb);
>
>
> I like this way you proposed here. But it is quite personal and compact
> wise the other version is in lead.
>
The thing is it doesn't initialize ah->ah_txpower.txp_requested, read below.
>> >
>> (wouldn't it be weird if someone from above asked us
>> what tx power was requested and we reply with max tx power ?).
>
> you mean in case someone from above did never requested something but
> ask us what was requested and we tell him that nothing was requested
> thats why we use max tx_power ... makes more sense ? :)
>
I mean that ah->ah_txpower.txp_requested or
ah->ah_txpower.txp_user_pwr in your case, both implicate that they are
initialized by the user. We shouldn't initialize them ourselves. Think
of it like this: we don't have permission to write this variable, we
only read it.
If you didn't like my example and you think it doesn't make sense,
here is another one:
How will we distinguish the case when a user asks for the max power
from the case where we have initialized ah->ah_txpower.txp_requested
to max ?
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
Put power_level to ah_txpower struct with the rest tx power infos and
also rename it to txp_requested to make more sense.
Signed-off-by: Nick Kossifidis <[email protected]>
---
drivers/net/wireless/ath/ath5k/ath5k.h | 2 +-
drivers/net/wireless/ath/ath5k/base.c | 5 +++--
drivers/net/wireless/ath/ath5k/mac80211-ops.c | 4 ++--
drivers/net/wireless/ath/ath5k/phy.c | 5 +++--
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 64a453a..3150def 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1331,7 +1331,6 @@ struct ath5k_hw {
unsigned int nexttbtt; /* next beacon time in TU */
struct ath5k_txq *cabq; /* content after beacon */
- int power_level; /* Requested tx power in dBm */
bool assoc; /* associate state */
bool enable_beacon; /* true if beacons are on */
@@ -1425,6 +1424,7 @@ struct ath5k_hw {
/* Value in dB units */
s16 txp_cck_ofdm_pwr_delta;
bool txp_setup;
+ int txp_requested; /* Requested tx power in dBm */
} ah_txpower;
struct ath5k_nfcal_hist ah_nfcal_hist;
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 8c4c040..9d48f9a 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -723,7 +723,7 @@ ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
ret = ah->ah_setup_tx_desc(ah, ds, pktlen,
ieee80211_get_hdrlen_from_skb(skb), padsize,
get_hw_packet_type(skb),
- (ah->power_level * 2),
+ (ah->ah_txpower.txp_requested * 2),
hw_rate,
info->control.rates[0].count, keyidx, ah->ah_tx_ant, flags,
cts_rate, duration);
@@ -1778,7 +1778,8 @@ ath5k_beacon_setup(struct ath5k_hw *ah, struct ath5k_buf *bf)
ds->ds_data = bf->skbaddr;
ret = ah->ah_setup_tx_desc(ah, ds, skb->len,
ieee80211_get_hdrlen_from_skb(skb), padsize,
- AR5K_PKT_TYPE_BEACON, (ah->power_level * 2),
+ AR5K_PKT_TYPE_BEACON,
+ (ah->ah_txpower.txp_requested * 2),
ieee80211_get_tx_rate(ah->hw, info)->hw_value,
1, AR5K_TXKEYIX_INVALID,
antenna, flags, 0, 0);
diff --git a/drivers/net/wireless/ath/ath5k/mac80211-ops.c b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
index 260e7dc..92ee3a0 100644
--- a/drivers/net/wireless/ath/ath5k/mac80211-ops.c
+++ b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
@@ -207,8 +207,8 @@ ath5k_config(struct ieee80211_hw *hw, u32 changed)
}
if ((changed & IEEE80211_CONF_CHANGE_POWER) &&
- (ah->power_level != conf->power_level)) {
- ah->power_level = conf->power_level;
+ (ah->ah_txpower.txp_requested != conf->power_level)) {
+ ah->ah_txpower.txp_requested = conf->power_level;
/* Half dB steps */
ath5k_hw_set_txpower_limit(ah, (conf->power_level * 2));
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 27ca993..b7ea415 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -3802,8 +3802,9 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
* RF buffer settings on 5211/5212+ so that we
* properly set curve indices.
*/
- ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
- ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
+ ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_requested ?
+ ah->ah_txpower.txp_requested * 2 :
+ AR5K_TUNE_MAX_TXPOWER);
if (ret)
return ret;
--
1.7.3.4
By using cur_pwr on phy_init we re-use the power level previously set by the
driver, not the one we got from above.
Signed-off-by: Nick Kossifidis <[email protected]>
---
drivers/net/wireless/ath/ath5k/phy.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 84a9aaf..27ca993 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -3802,8 +3802,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
* RF buffer settings on 5211/5212+ so that we
* properly set curve indices.
*/
- ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
- ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
+ ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
+ ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
if (ret)
return ret;
--
1.7.3.4
Hi Nick
Nick Kossifidis schrieb:
> 2012/7/29 Thomas Huehn <[email protected]>:
>> Hi Nick,
>>
>>
>>> Anyway if readability is the issue we can just do something like
>>>
>>> int txpower_halfdb = 0;
>>>
>>> [...]
>>>
>>> if(ah->power_level) {
>>> txpower_halfdb = ah->power_level * 2;
>>> } else {
>>> txpower_halfdb = AR5K_TUNE_MAX_TXPOWER;
>>> }
>>>
>>> and then it'll look like this
>>>
>>> ret = ath5k_hw_txpower(ah, channel, txpower_halfdb);
>>
> How will we distinguish the case when a user asks for the max power
> from the case where we have initialized ah->ah_txpower.txp_requested
> to max ?
I do not sea any use-case where someone would need this distinction. But
if there is such a need, I would rather store this information in a
variable (e.g. user_set_power=true) explicitly rather than infer it from
fact that our power variable is not initialised. Probably to much of a
personal taste than to be considered further I guess.
Greetings Thomas
Hi Nick,
> Anyway if readability is the issue we can just do something like
>
> int txpower_halfdb = 0;
>
> [...]
>
> if(ah->power_level) {
> txpower_halfdb = ah->power_level * 2;
> } else {
> txpower_halfdb = AR5K_TUNE_MAX_TXPOWER;
> }
>
> and then it'll look like this
>
> ret = ath5k_hw_txpower(ah, channel, txpower_halfdb);
I like this way you proposed here. But it is quite personal and compact
wise the other version is in lead.
> >
> (wouldn't it be weird if someone from above asked us
> what tx power was requested and we reply with max tx power ?).
you mean in case someone from above did never requested something but
ask us what was requested and we tell him that nothing was requested
thats why we use max tx_power ... makes more sense ? :)
I think once the reporting from the driver to mac80211 about the current
tx_power is there, it will be solved anyway.
Greetings Thomas
Let my correct myself
Thomas Huehn schrieb:
> Hi Nick,
>
> Nick Kossifidis schrieb:
>
>> By using cur_pwr on phy_init we re-use the power level previously set by the
>> driver, not the one we got from above.
>>
>> Signed-off-by: Nick Kossifidis <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath5k/phy.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
>> index 84a9aaf..27ca993 100644
>> --- a/drivers/net/wireless/ath/ath5k/phy.c
>> +++ b/drivers/net/wireless/ath/ath5k/phy.c
>> @@ -3802,8 +3802,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
>> * RF buffer settings on 5211/5212+ so that we
>> * properly set curve indices.
>> */
>> - ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
>> - ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
>> + ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
>> + ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
>> if (ret)
>> return ret;
>>
>
>
> I would suggest to initialise the power_level as AR5K_TUNE_MAX_TXPOWER
> in base.c funktion ath5k_init(struct ieee80211_hw *hw)
>
> /* init tx_power setting to maximum */
> ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
>
> that would simplify the readability form above into:
> ret = ath5k_hw_txpower(ah, channel, ah->power_level)
>
> What do you think ?
>
correct version i thought of should be:
/* init tx_power setting to maximum */
ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER / 2;
that would simplify the readability form above into:
ret = ath5k_hw_txpower(ah, channel, ah->power_level * 2)
Greetings Thomas
2012/7/28 Thomas Huehn <[email protected]>:
> Let my correct myself
>
> Thomas Huehn schrieb:
>
>> Hi Nick,
>>
>> Nick Kossifidis schrieb:
>>
>>> By using cur_pwr on phy_init we re-use the power level previously set by the
>>> driver, not the one we got from above.
>>>
>>> Signed-off-by: Nick Kossifidis <[email protected]>
>>> ---
>>> drivers/net/wireless/ath/ath5k/phy.c | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
>>> index 84a9aaf..27ca993 100644
>>> --- a/drivers/net/wireless/ath/ath5k/phy.c
>>> +++ b/drivers/net/wireless/ath/ath5k/phy.c
>>> @@ -3802,8 +3802,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
>>> * RF buffer settings on 5211/5212+ so that we
>>> * properly set curve indices.
>>> */
>>> - ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
>>> - ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
>>> + ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
>>> + ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
>>> if (ret)
>>> return ret;
>>>
>>
>>
>> I would suggest to initialise the power_level as AR5K_TUNE_MAX_TXPOWER
>> in base.c funktion ath5k_init(struct ieee80211_hw *hw)
>>
>> /* init tx_power setting to maximum */
>> ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
>>
>> that would simplify the readability form above into:
>> ret = ath5k_hw_txpower(ah, channel, ah->power_level)
>>
>> What do you think ?
>>
>
>
> correct version i thought of should be:
>
> /* init tx_power setting to maximum */
> ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER / 2;
>
> that would simplify the readability form above into:
> ret = ath5k_hw_txpower(ah, channel, ah->power_level * 2)
>
>
> Greetings Thomas
init functions need cleanup, the idea is to split initialization per
unit and "layer", for example we have ath5k_init, ath5k_init_ah,
ath5k_hw_init etc, and an init function for each unit (phy, pcu,
qcu/dcu, dma etc). I haven't finished this process yet but for example
initializing phy parameters such as tx power (or any unit's
parameters) doesn't belong in the function that initializes driver
state and capabilities, it could belong to ath5k_hw_init but I want to
keep that minimal. Another example is queue initialization, it should
belong on a different function and not ath5k_init, there is also some
mess between ath5k_init_ah and ath5k_init, there is no obvious reason
to someone reading base.c why we have two init functions there and
what's each function purpose.
Anyway if readability is the issue we can just do something like
int txpower_halfdb = 0;
[...]
if(ah->power_level) {
txpower_halfdb = ah->power_level * 2;
} else {
txpower_halfdb = AR5K_TUNE_MAX_TXPOWER;
}
and then it'll look like this
ret = ath5k_hw_txpower(ah, channel, txpower_halfdb);
but really IMHO the compact form used right now is not that bad, it
gets better on the next patch I think:
+ ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_requested ?
+ ah->ah_txpower.txp_requested * 2 :
+ AR5K_TUNE_MAX_TXPOWER);
We don't need to make things more complex, what we want to do is if we
got some tx power setting from above use it, else use the max power.
We don't need to split this across 2 different functions and use more
than a few loc.
>From another point of view there it's also misleading to initialize
the requested tx power value to something when no one ever requested
any tx power setting. This way we don't initialize
ah->ah_txpower.txp_requested and we can use that information in case
we want to do more checks later or in case someone from above needs
that information (wouldn't it be weird if someone from above asked us
what tx power was requested and we reply with max tx power ?).
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
Hi Nick,
Nick Kossifidis schrieb:
> By using cur_pwr on phy_init we re-use the power level previously set by the
> driver, not the one we got from above.
>
> Signed-off-by: Nick Kossifidis <[email protected]>
> ---
> drivers/net/wireless/ath/ath5k/phy.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
> index 84a9aaf..27ca993 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -3802,8 +3802,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> * RF buffer settings on 5211/5212+ so that we
> * properly set curve indices.
> */
> - ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
> - ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
> + ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
> + ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
> if (ret)
> return ret;
>
I would suggest to initialise the power_level as AR5K_TUNE_MAX_TXPOWER
in base.c funktion ath5k_init(struct ieee80211_hw *hw)
/* init tx_power setting to maximum */
ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
that would simplify the readability form above into:
ret = ath5k_hw_txpower(ah, channel, ah->power_level)
What do you think ?
Greetings Thomas