On Thu, 2013-01-31 at 10:33 -0600, Seth Forshee wrote: > On Thu, Jan 31, 2013 at 04:20:48PM +0100, Johannes Berg wrote: > > On Tue, 2013-01-29 at 17:47 -0600, Seth Forshee wrote: > > > > > +static inline bool ieee80211_is_ps_disabled(struct ieee80211_conf *conf) > > > > > +static inline bool ieee80211_is_ps_enabled(struct ieee80211_conf *conf) > > > > Huh, is that worth the confusion? It seems !enabled should be the same > > as disabled, but it's not quite the same, which might be confusing. > > In this patch there's no distinction, but after adding the off-channel > powersave state there is -- disabled == !enabled && !offchannel. I thought it was something like that, yeah. > Actually one of the last bugs I fixed before sending these was a place > where I had used disabled instead of !enabled, and the frames ended up > with PM set when it shouldn't have been. > > I agree though that the distinction is confusing. Maybe some better > state names are needed. Perhaps awake, offchannel, and doze? I think what you really want is to distinguish between "HW can go to powersave" and "PM bit should be set"? That's pretty much what your CONF_PS_ENABLED and CONF_PS_OFFCHANNEL means, respectively, but maybe putting it in different terms would make it less confusing? > > > +/** > > > + * ieee80211_set_ps_state - set device powersave state > > > + * > > > + * Sets the powersave state in the supplied device configuration to the > > > + * specified state. > > > + * > > > + * @conf: device configuration > > > + * @state: new powersave state. Must be one of the IEEE80211_CONF_PS_* > > > + * flags from enum ieee80211_conf_flags. > > > + */ > > > +static inline void ieee80211_set_ps_state(struct ieee80211_conf *conf, > > > + u32 state) > > > +{ > > > + conf->flags = (conf->flags & ~IEEE80211_CONF_PS_MASK) | > > > + (state & IEEE80211_CONF_PS_MASK); > > > +} > > > > I don't think the driver should do this, so the inline shouldn't be > > here? > > That's true. Would moving it to ieee80211_i.h be appropriate, or is > there somewhere better? ieee80211_i.h is good johannes -- 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