On Tue, 2008-09-09 at 16:14 -0700, Anna Neal wrote:
> 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?
Odd. In any case, your suggestion sounds fine. I'll do the V9+ bits
when there's time.
Thanks!
Dan
> >> 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
