Hello,
Thanks for the great feedback.
On Tue, Sep 9, 2008 at 2:39 PM, Dan Williams <dcbw@xxxxxxxxxx> wrote:
> On Fri, 2008-09-05 at 15:56 -0700, Anna Neal wrote:
>> diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c
>> index 802547e..6cc4858 100644
>> --- a/drivers/net/wireless/libertas/cmd.c
>> +++ b/drivers/net/wireless/libertas/cmd.c
>> +int lbs_tpc_cfg(struct lbs_private *priv, int enable, int8_t p0, int8_t p1,
>> + int8_t p2, int usesnr)
>
> Can we rename this to lbs_set_tpc_cfg()? It's a setter, and there could
> be a getter later, best to be clear about this.
Definitely.
>> +int lbs_power_adapt_cfg(struct lbs_private *priv, int enable, int8_t p0,
>> + int8_t p1, int8_t p2)
>
> Same here, can we change this to lbs_set_pa_cfg() or
> lbs_set_power_adapt_cfg() ?
Makes sense.
>> +{
>> + struct cmd_ds_802_11_pa_cfg cmd;
>> + int ret;
>> +
>> + memset(&cmd, 0, sizeof(cmd));
>> + cmd.hdr.size = cpu_to_le16(sizeof(cmd));
>> + cmd.action = cpu_to_le16(CMD_ACT_SET);
>> + cmd.enable = !!enable;
>> + cmd.P0 = p0;
>> + cmd.P1 = p1;
>> + cmd.P2 = p2;
>
> And this isn't compatible with the V9 and later firmware, which uses the
> PowerAdapt_Group_t Marvell IE. I'd suggest defining two different
> commands, one for V8 and below which uses the format you have here, and
> one for V9 and above which uses the new format. I have no idea what the
> fields in PA_Group_t are, but I'm sure you can find out since you're
> from CozyBit :)
We don't have V{9,10} hardware nor access to a final published spec.
How about if we wrap our changes around version checks and leave
things unchanged for other versions?
>> diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
>> index da618fc..a916bb9 100644
>> --- a/drivers/net/wireless/libertas/host.h
>> +++ b/drivers/net/wireless/libertas/host.h
>> @@ -83,6 +83,7 @@
>> #define CMD_802_11_INACTIVITY_TIMEOUT 0x0067
>> #define CMD_802_11_SLEEP_PERIOD 0x0068
>> #define CMD_802_11_TPC_CFG 0x0072
>> +#define CMD_802_11_PA_CFG 0x0073
>
> Should also:
>
> #define CMD_802_11_POWER_ADAPT_CFG_EXT 0x0073 /* v9 firmware and above */
>
> and use that in the v9+ lbs_set_power_adapt_cfg_ext() function.
Again, without hardware or the final spec implementation of this may
need to wait on my end.
>> +struct cmd_ds_802_11_pa_cfg {
>> + struct cmd_header hdr;
>> +
>> + __le16 action;
>> + uint8_t enable;
>> + int8_t P0;
>> + int8_t P1;
>> + int8_t P2;
>> +} __attribute__ ((packed));
>> +
>> +
>
> And for v9:
>
> struct pa_group {
> u16 level;
> u16 rate_map;
> u32 reserved;
> }
>
> struct mrvl_ie_pa_group {
> mrvlietypes_header hdr;
> struct pa_group groups[5];
> }
>
> struct cmd_ds_802_11_pa_cfg_ext {
> struct cmd_header hdr;
>
> __le16 action;
> u16 enable;
> struct mrvl_ie_pa_group pa_groups;
> }
>
> note that there isn't an existing GPL implementation of
> POWER_ADAPT_CFG_EXT either at moblin or anywhere else, so I can't
> suggest what to fill in for the rate_map and level of struct pa_group in
> the implementation. Ask Luis or Javier I guess since they probably have
> access to that information.
Javier just confirmed that we don't have access to this.
>> struct cmd_ds_802_11_led_ctrl {
>> __le16 action;
>> __le16 numled;
>> diff --git a/drivers/net/wireless/libertas/wext.c b/drivers/net/wireless/libertas/wext.c
>> index 426f1fe..b08bad8 100644
>> --- a/drivers/net/wireless/libertas/wext.c
>> +++ b/drivers/net/wireless/libertas/wext.c
>> @@ -1820,7 +1820,17 @@ static int lbs_set_txpow(struct net_device *dev, struct iw_request_info *info,
>> }
>>
>> if (vwrq->fixed == 0) {
>> - /* Auto power control */
>> + /* User requests automatic tx power control, however there are
>> + * many auto tx settings. For now use firmware defaults until
>> + * we come up with a good way to expose these to the user. */
>> + ret = lbs_power_adapt_cfg(priv, 1, POW_ADAPT_DEFAULT_P0,
>> + POW_ADAPT_DEFAULT_P1, POW_ADAPT_DEFAULT_P2);
>
> and do the firmware check right here and call lbs_set_power_adapt_cfg()
> for V8 and lower, and lbs_set_power_adapt_cfg_ext() for v9 and higher
> like so:
>
> if (priv->fwrelease >= 0x09000000) {
> ret = lbs_power_adapt_cfg_ext(...);
> } else {
> ret = lbs_power_adapt_cfg(...);
> }
So, to summarize, what I can do is...
> if (priv->fwrelease < 0x09000000) {
> ret = lbs_power_adapt_cfg(...);
> }
...would this be acceptable? After all, it is still an improvement
over what's currently in the driver.
Thanks again!
Anna
--
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
