RFC: Add module parameter to override regioncode?

[Johan Adolfsson]

----- Original Message ----- 
From: "Holger Schurig" <hs4233 at mail.mn-solutions.de>
To: <libertas-dev at lists.infradead.org>; "Johan Adolfsson" <johana at axis.com>
Sent: Friday, June 06, 2008 11:25 AM
Subject: Re: RFC: Add module parameter to override regioncode?


> Hi Johan !
>
> Thanks for your contribution. I for my part would definitely
> accept a patch for this.
>
> What libertas-flavour are you using, USB, CF or SDIO (that
> question is just out of curiosity, has nothing to do with
> your patch).

I'm evaluating 8686 using SDIO but current products uses
CF and 8385 (but those uses the Marvell driver, not the
libertas driver in the kernel)

>
> I've made some comments inline:
>
>> --- a/drivers/net/wireless/libertas/cmd.c
>> +++ b/drivers/net/wireless/libertas/cmd.c
>> @@ -105,26 +105,45 @@ int lbs_update_hw_spec(struct
>> lbs_private *priv) priv->fwrelease = (priv->fwrelease << 8) |
>>                 (priv->fwrelease >> 24 & 0xff);
>>
>> +       /* Clamp region code to 8-bit since FW spec indicates that it 
>> should
>> +        * only ever be 8-bit, even though the field size is 16-bit. 
>> Some
>> firmw
>
> Please turn off word-wrapping when sending e-mails. I see
> that you're constrained by MS Outlook Express (I've pity with
> you), but surely this e-mail program can also be
> convinced ... :-)
>
> With word wrap, no-one can save your e-mail and incorporate
> the saved file directly into quilt/patch/git.

Sorry, it wasn't meant as a real patch, and I did warn you:-)
It's national holiday here and I just pasted in the dif from home
to get some feedback.

>
>> +        * returns non-zero high 8 bits here.
>
> I see the comment (this line and the lines above), I understand the
> meaning, but now the comment is misplaced. I'd add insert your new
> code, then the comment, and directly after the comment a line like
>
>  priv->regioncode &= 0xff;
>
> line.
>
>
>> -       printk("libertas: %s, fw %u.%u.%up%u, cap 0x%08x\n",
>> -               print_mac(mac, cmd.permanentaddr),
>> -               priv->fwrelease >> 24 & 0xff,
>> -               priv->fwrelease >> 16 & 0xff,
>> -               priv->fwrelease >>  8 & 0xff,
>> -               priv->fwrelease       & 0xff,
>> -               priv->fwcapinfo);
>> +       printk("libertas: %s, fw %u.%u.%up%u, cap 0x%08x regioncode hw 
>> 0x%02x sw
>> +              print_mac(mac, cmd.permanentaddr),
>> +              priv->fwrelease >> 24 & 0xff,
>> +              priv->fwrelease >> 16 & 0xff,
>> +              priv->fwrelease >>  8 & 0xff,
>> +              priv->fwrelease       & 0xff,
>> +              priv->fwcapinfo,
>> +              le16_to_cpu(cmd.regioncode) & 0xFF,
>> +              priv->regioncode);
>
> The removal and addition of those "priv->fwrelease >> xx & 0xff"
> lines looks like a whitespace-changing patch. Does your patch
> survive scripts/checkpatch.pl?

Havent tried, but the indentation was two tabs and not 1 tab
and 7 spaces to align it with the printk( which my emacs wanted,
guess it doesn't quite follow the CodingStyle...


>> --- a/drivers/net/wireless/libertas/main.c
>> +++ b/drivers/net/wireless/libertas/main.c
>> @@ -37,6 +37,10 @@ unsigned int lbs_debug;
>>  EXPORT_SYMBOL_GPL(lbs_debug);
>>  module_param_named(libertas_debug, lbs_debug, int, 0644);
>>
>> +int lbs_wlan_region_code = 0;
>> +EXPORT_SYMBOL_GPL(lbs_wlan_region_code);
>> +module_param_named(wlan_region_code, lbs_wlan_region_code, int, 0644);
>> +
>
> No need to export this. You only need to EXPORT some symbol
> if you want to access it from another module. cmd.c and main.c
> are in the same module, libertas.ko, so exporting is superfluous.

Ok, a little to much copy-paste from our patched Marvell driver,
which possibly didnt need it either.

> Maybe you name your module parameter "libertas_regioncode", so
> that it harmonizes with "libertas_debug". But no strong feeling
> about this ...  :-)

I personally would like to stick to wlan_region_code since thats
what we used already, but no strong feelings about that either...


>> @@ -927,6 +935,9 @@ static int lbs_setup_firmware(struct
>> lbs_private *priv) * Read MAC address from HW
>>          */
>>         memset(priv->current_addr, 0xff, ETH_ALEN);
>> +       if (lbs_wlan_region_code) {
>> +               priv->regioncode = lbs_wlan_region_code;
>> +       }
>
> You don't need curly brackets here.

True, and the CodingStyle says not to use it - just old habit
which I find easier to read and maintain.


>
> I note that this is a RFC, so it's ok that there is no
> Signed-off-by line here, but don't forget to put that
> line in.
>
> Note that I haven't look at the functionality or
> implications of this patch yet. With what region code are
> you using your libertas?

Historically we distinguish between
channels 1-11 using code 0x10 and channels 1-13 using code 0x30,
others fall back to whatever is in the module, so I guess
we have different hardware for japan etc.


> Greetings, Holger

Thanks for the feedback, I'll try to whip up a proper patch
according to CodingStyle etc. against latest git next week.

Best regards
/Johan