On Tue, Jan 11, 2011 at 2:13 AM, Levi, Shahar <shahar_levi@xxxxxx> wrote:
>
> On Mon, Jan 10, 2011 at 5:00 PM, Luciano Coelho <coelho@xxxxxx> wrote:
>>
>> On Mon, 2011-01-03 at 14:42 +0100, Shahar Levi wrote:
>> > Add 80211n BA initiator session support wl1271 driver.
>> > Include BA supported FW version auto detection mechanism.
>> > BA initiator session management included in FW independently.
>> >
>> > Signed-off-by: Shahar Levi <shahar_levi@xxxxxx>
>> > ---
>>
>> Some comments...
>
thanks for your review.
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
>> > index cc4068d..54fd68d 100644
>> > --- a/drivers/net/wireless/wl12xx/acx.c
>> > +++ b/drivers/net/wireless/wl12xx/acx.c
>> > @@ -1309,6 +1309,56 @@ out:
>> > return ret;
>> > }
>> >
>> > +/* Configure BA session initiator\receiver parameters setting in the FW. */
>>
>> Please use forward slash here instead of backslash, ie. use
>> "initiator/receiver".
>
np, will be fix.
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/acx.h b/drivers/net/wireless/wl12xx/acx.h
>> > index 9cbc3f4..df48468 100644
>> > --- a/drivers/net/wireless/wl12xx/acx.h
>> > +++ b/drivers/net/wireless/wl12xx/acx.h
>> > @@ -1051,6 +1051,40 @@ struct wl1271_acx_ht_information {
>> > u8 padding[3];
>> > } __packed;
>> >
>> > +#define BA_WIN_SIZE 8
>>
>> Should this be DEFAULT_BA_WIN_SIZE?
>
No, the FW support win size of 8. it is not configurable.
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/boot.c b/drivers/net/wireless/wl12xx/boot.c
>> > index 4a9f929..cd42e12 100644
>> > --- a/drivers/net/wireless/wl12xx/boot.c
>> > +++ b/drivers/net/wireless/wl12xx/boot.c
>> > @@ -101,6 +101,39 @@ static void wl1271_boot_set_ecpu_ctrl(struct wl1271 *wl, u32 flag)
>> > wl1271_write32(wl, ACX_REG_ECPU_CONTROL, cpu_ctrl);
>> > }
>> >
>> > +static void wl1271_save_fw_ver(struct wl1271 *wl)
>>
>> This function name is not very informative. Why not call it
>> wl1271_parse_fw_ver() instead?
>
np, will be fix
>>
>>
>> > +{
>> > + char fw_ver_str[ETHTOOL_BUSINFO_LEN];
>>
>> This is weird, but it seem to be what is used in cfg80211 (as Shahar
>> pointed out on IRC). IMHO it should be ETHTOOL_FWVERS_LEN instead, both
>> here and in cfg80211.
>>
>> In any case, this is a bit confusing here, because we don't use the
>> fw_version in the wiphy struct (we probably should). Let's keep it like
>> this for now and maybe later we can change.
>>
>> Also, I don't see why you need a local copy here.
>
i use local copy in order to remove '.' (*fw_ver_point = '\0') without
destroyed wl->chip.fw_ver_str.
>>
>> > + char *fw_ver_point;
>> > + int ret, i;
>> > +
>> > + /* copy the fw version to temp str */
>> > + strncpy(fw_ver_str, wl->chip.fw_ver_str, sizeof(fw_ver_str));
>> > +
>> > + for (i = (WL12XX_NUM_FW_VER - 1); i > 0; --i) {
>> > + /* find the last '.' */
>> > + fw_ver_point = strrchr(fw_ver_str, '.');
>> > +
>> > + /* read version number */
>> > + ret = strict_strtoul(fw_ver_point+1, 10, &(wl->chip.fw_ver[i]));
>> > + if (ret < 0) {
>> > + wl1271_warning("fw version incorrect value");
>> > + memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
>> > + return;
>> > + }
>> > +
>> > + /* clean '.' */
>> > + *fw_ver_point = '\0';
>> > + }
>> > +
>> > + ret = strict_strtoul(fw_ver_point-1, 10, &(wl->chip.fw_ver[0]));
>> > + if (ret < 0) {
>> > + wl1271_warning("fw version incorrect value");
>> > + memset(wl->chip.fw_ver, 0, sizeof(wl->chip.fw_ver));
>> > + return;
>> > + }
>> > +}
>>
>> Instead of all this manual parsing, why don't you use sscanf? I think
>> the following could do the work very nicely:
>>
>> ret = sscanf(wl->chip.fw_ver_str + WL12XX_FW_VER_OFFSET,
>> "%lu.%lu.%lu.%lu.%lu", &wl->chip.fw_ver[0],
>> &wl->chip.fw_ver[1], &wl->chip.fw_ver[2]
>> &wl->chip.fw_ver[3], &wl->chip.fw_ver[4]);
>>
>> Wouldn't something like this be much simpler? (or you could use %u if
>> you agree on using unsigned int, see below)
>
good point, i will try and update.
>>
>>
>> > static int wl1271_boot_upload_firmware_chunk(struct wl1271 *wl, void *buf,
>> > diff --git a/drivers/net/wireless/wl12xx/conf.h b/drivers/net/wireless/wl12xx/conf.h
>> > index a16b361..41df771 100644
>> > --- a/drivers/net/wireless/wl12xx/conf.h
>> > +++ b/drivers/net/wireless/wl12xx/conf.h
>> > @@ -1090,6 +1090,12 @@ struct conf_rf_settings {
>> > u8 tx_per_channel_power_compensation_5[CONF_TX_PWR_COMPENSATION_LEN_5];
>> > };
>> >
>> > +#define CONF_BA_INACTIVITY_TIMEOUT 10000
>>
>> If this is a CONFigurable value, it should be in conf.h and in the
>> configuration parameters in main.c, shouldn't it?
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
>> > index 062247e..c44462d 100644
>> > --- a/drivers/net/wireless/wl12xx/main.c
>> > +++ b/drivers/net/wireless/wl12xx/main.c
>> > @@ -233,7 +233,7 @@ static struct conf_drv_settings default_conf = {
>> > .avg_weight_rssi_beacon = 20,
>> > .avg_weight_rssi_data = 10,
>> > .avg_weight_snr_beacon = 20,
>> > - .avg_weight_snr_data = 10
>> > + .avg_weight_snr_data = 10,
>> > },
>> > .scan = {
>> > .min_dwell_time_active = 7500,
>> > @@ -252,6 +252,9 @@ static struct conf_drv_settings default_conf = {
>> > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> > },
>> > },
>> > + .ht = {
>> > + .inactivity_timeout = CONF_BA_INACTIVITY_TIMEOUT,
>> > + },
>>
>> Ah, I see. You are using that macro here, but I guess you could use the
>> value directly, since this is all configuration stuff, so no need to use
>> defines, unless the values mean something specific. Here it is just a
>> measure of time, so a number can be used directly (like in the
>> min_dwell_time_active).
>
np, will be fix
>>
>>
>> > diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
>> > index 01711fe..7b34393 100644
>> > --- a/drivers/net/wireless/wl12xx/wl12xx.h
>> > +++ b/drivers/net/wireless/wl12xx/wl12xx.h
>> > @@ -38,6 +38,11 @@
>> > #define DRIVER_NAME "wl1271"
>> > #define DRIVER_PREFIX DRIVER_NAME ": "
>> >
>> > +#define WL12XX_BA_SUPPORT_FW_SUB_VER 339
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER1 33
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER2_START 50
>> > +#define WL12XX_BA_SUPPORT_FW_COST_VER2_END 60
>>
>> This defines are very confusing. Can you explain a bit?
yes i will add comments.
>>
>> What about
>> "COST" version 0 (like 6.0.0.0.343), will that branch never support BA?Where do the 50 to 60 come from?
>
no, all open source version will be tag from 50 to 60.
>>
>> And what is 33? This kind of magic
>>
>> should be explained.
>>
>>
>> > @@ -161,10 +166,13 @@ struct wl1271_partition_set {
>> >
>> > struct wl1271;
>> >
>> > +#define WL12XX_NUM_FW_VER 5
>> > +
>>
>> WL12XX_FW_VER_OFFSET sounds better to me.
>>
>> And it shouldn't it be 4,
>> which is the "Rev " prefix?
>
the macro represent the number of numbers in the version. it is not offset.
>>
>>
>> > /* FIXME: I'm not sure about this structure name */
>> > struct wl1271_chip {
>> > u32 id;
>> > - char fw_ver[21];
>> > + char fw_ver_str[ETHTOOL_BUSINFO_LEN];
>> > + unsigned long fw_ver[WL12XX_NUM_FW_VER];
>>
>> Why not unsigned int? (and then use %u.%u... as I mentioned earlier).
>
i will try and update.
>>
>> --
>> Cheers,
>> Luca.
>>
Shahar
--
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
