| Message ID | 1343485971-31360-3-git-send-email-mickflemm@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@gmail.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/7/28 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>: > 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@gmail.com> >>> --- >>> 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 ?).
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/7/29 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>: > 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 ?
Hi Nick Nick Kossifidis schrieb: > 2012/7/29 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>: >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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@gmail.com> --- drivers/net/wireless/ath/ath5k/phy.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)