On Wed, Jun 15, 2011 at 07:49:06PM -0700, Greg Dietsche wrote:
>
>
> On 06/15/2011 08:36 PM, Henry Ptasinski wrote:
> > On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
> >
> >> This code looks wrong to me. I think it meant to return false
> >> if the board's vendor id isn't Broadcom's.
> >>
> >> Compile tested only.
> >>
> >> Signed-off-by: Greg Dietsche<Gregory.Dietsche@xxxxxxx>
> >> ---
> >> drivers/staging/brcm80211/brcmsmac/wlc_bmac.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> index 4534926..ee13238 100644
> >> --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
> >> }
> >>
> >> if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
> >> - return goodboard;
> >> + goodboard = false;
> >>
> >> return goodboard;
> >> }
> >>
> > Actually, the original code is correct.
> >
> > For boards with the Broadcom ID, this function checks that the boardrev is
> > sane. Dev boards that aren't properly configured may have invalid info, so this
> > check is mainly to catch that problem.
> >
> > For boards with any other vendor, we don't have any sanity checks that we know
> > should be done, so the board info is always considered good.
> >
> > The device ID is of course always checked.
> >
> > - Henry
> >
> >
> Hi Henry,
> Thanks for the explanation - I'm still a little confused on this...
> probably more because of how the code reads. Let me explain:
>
> The part that threw me off in this code when I originally read it is
> that it always returns goodboard. It doesn't actually matter what the
> vendor id is. So my first thought was "what did they really mean to code
> here...?" That's why I sent the patch. So if the patch isn't necessary,
> then great! :) But it seems like we could/should just drop the check for
> vendor id?
How's this for a somewhat clearer implementation:
static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
{
bool goodboard = true;
uint boardrev = wlc_hw->boardrev;
if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {
/* validate boardrev */
if (boardrev == 0)
goodboard = false;
else if (boardrev > 0xff) {
/* 4 bits each for board type, major, minor, and tiny
version numbers */
uint brt = (boardrev & 0xf000) >> 12;
uint b0 = (boardrev & 0xf00) >> 8;
uint b1 = (boardrev & 0xf0) >> 4;
uint b2 = boardrev & 0xf;
if ((brt > 2) || (brt == 0) || (b0 > 9) || (b0 == 0)
|| (b1 > 9) || (b2 > 9))
goodboard = false;
}
}
return goodboard;
}
If that's agreeable, I'll add it to a series of cleanup patches that I've got
in progress.
- Henry
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
