Re: [PATCH] libertas: Enable/disable automatic tx power control via SIOCSIWTXPOW.

["Anna Neal" <anna@cozybit.com>]

Hello,

Thanks for the great feedback.

On Tue, Sep 9, 2008 at 2:39 PM, Dan Williams <dcbw@redhat.com> 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