RFC: Add module parameter to override regioncode?

[Dan Williams]

On Fri, 2008-06-06 at 11:25 +0200, Holger Schurig wrote:
> 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'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.
> 
> 
> > +        * 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?

In general whitespace shouldn't be changed unless it was wrong to begin
with.

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

I'd rather have "region", or whatever mac80211 drivers might use.  I
also would rather have "debug" instead of "libertas_debug".  Having the
libertas_ is just redundant.

Dan

> > @@ -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.
> 
> 
> 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?
> 
> 
> Greetings, Holger
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev