2012/7/28 Thomas Huehn <thomas@xxxxxxxxxxxxxxxxxxxxxxx>:
> 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 <mickflemm@xxxxxxxxx>
>>> ---
>>> 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
--
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
