[PATCH] staging: brcm80211: Make CFLAGS explicit

[PATCH] staging: brcm80211: Make CFLAGS explicit

[Tim Gardner]

Its too easy to confuse the obscurely named WLC_LOW/WLC_HIGH CFLAG
macros.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Cc: Brett Rudley <brudley@broadcom.com>
Cc: Henry Ptasinski <henryp@broadcom.com>
Cc: Nohee Ko <noheek@broadcom.com>
---
 drivers/staging/brcm80211/Makefile |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/brcm80211/Makefile b/drivers/staging/brcm80211/Makefile
index 05a4103..7a77e7f 100644
--- a/drivers/staging/brcm80211/Makefile
+++ b/drivers/staging/brcm80211/Makefile
@@ -15,8 +15,9 @@
 # OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
 # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
-EXTRA_CFLAGS := -DBCMDBG -DWLC_HIGH -DSTA -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
+EXTRA_CFLAGS := -DBCMDBG -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
 
+USB_CFLAGS := -DWLC_HIGH
 PCI_CFLAGS := -DWLC_LOW
 
 BRCM80211_OFILES := \
-- 
1.7.0.4

Re: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Greg KH]

On Fri, Sep 17, 2010 at 09:20:10AM -0600, Tim Gardner wrote:
> Its too easy to confuse the obscurely named WLC_LOW/WLC_HIGH CFLAG
> macros.

True, but doesn't this change how the code is being built?  Why make
this change?

> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> Cc: Brett Rudley <brudley@broadcom.com>
> Cc: Henry Ptasinski <henryp@broadcom.com>
> Cc: Nohee Ko <noheek@broadcom.com>
> ---
>  drivers/staging/brcm80211/Makefile |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/brcm80211/Makefile b/drivers/staging/brcm80211/Makefile
> index 05a4103..7a77e7f 100644
> --- a/drivers/staging/brcm80211/Makefile
> +++ b/drivers/staging/brcm80211/Makefile
> @@ -15,8 +15,9 @@
>  # OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
>  # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>  
> -EXTRA_CFLAGS := -DBCMDBG -DWLC_HIGH -DSTA -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
> +EXTRA_CFLAGS := -DBCMDBG -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes

how about 2 patches here, one to break it out into one flag per line so
we can see what is really happening here, and then one patch to make the
change you are asking for.

>  
> +USB_CFLAGS := -DWLC_HIGH

Are you sure about this?  Why set this when I don't see anything ever
using it?

What problem are you trying to fix here.

totally confused,

greg k-h

Re: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Tim Gardner]

On 09/17/2010 10:21 AM, Greg KH wrote:
> On Fri, Sep 17, 2010 at 09:20:10AM -0600, Tim Gardner wrote:
>> Its too easy to confuse the obscurely named WLC_LOW/WLC_HIGH CFLAG
>> macros.
>
> True, but doesn't this change how the code is being built?  Why make
> this change?
>
>>
>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>> Cc: Brett Rudley<brudley@broadcom.com>
>> Cc: Henry Ptasinski<henryp@broadcom.com>
>> Cc: Nohee Ko<noheek@broadcom.com>
>> ---
>>   drivers/staging/brcm80211/Makefile |    3 ++-
>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/brcm80211/Makefile b/drivers/staging/brcm80211/Makefile
>> index 05a4103..7a77e7f 100644
>> --- a/drivers/staging/brcm80211/Makefile
>> +++ b/drivers/staging/brcm80211/Makefile
>> @@ -15,8 +15,9 @@
>>   # OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
>>   # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>>
>> -EXTRA_CFLAGS := -DBCMDBG -DWLC_HIGH -DSTA -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
>> +EXTRA_CFLAGS := -DBCMDBG -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
>
> how about 2 patches here, one to break it out into one flag per line so
> we can see what is really happening here, and then one patch to make the
> change you are asking for.
>
>>
>> +USB_CFLAGS := -DWLC_HIGH
>
> Are you sure about this?  Why set this when I don't see anything ever
> using it?
>
> What problem are you trying to fix here.
>
> totally confused,
>
> greg k-h
>

Well, when groveling through the code you can see a bunch of '#ifdef 
WLC_HIGH_ONLY' which I initially assumed was defined because of 
'-DWLC_HIGH' in EXTRA_CFLAGS. This is quite misleading if you don't 
notice 'PCI_CFLAGS := -DWLC_LOW' a bit later in the Makefile (which I 
didn't for quite awhile). Specifying both macros in EXTRA_CFLAGS seems 
unnecessary.

The macro magic happens in brcm80211/sys/wlc_cfg.h:

/* Keep WLC_HIGH_ONLY, WLC_SPLIT for USB extension later on */
#if !defined(WLC_LOW)
#define WLC_HIGH_ONLY
#endif
#if !defined(WLC_LOW)
#define WLC_SPLIT
#endif

What does WLC_LOW and WLC_HIGH mean anyway?

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Greg KH]

On Fri, Sep 17, 2010 at 11:44:25AM -0600, Tim Gardner wrote:
> On 09/17/2010 10:21 AM, Greg KH wrote:
> >On Fri, Sep 17, 2010 at 09:20:10AM -0600, Tim Gardner wrote:
> >>Its too easy to confuse the obscurely named WLC_LOW/WLC_HIGH CFLAG
> >>macros.
> >
> >True, but doesn't this change how the code is being built?  Why make
> >this change?
> >
> >>
> >>Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
> >>Cc: Brett Rudley<brudley@broadcom.com>
> >>Cc: Henry Ptasinski<henryp@broadcom.com>
> >>Cc: Nohee Ko<noheek@broadcom.com>
> >>---
> >>  drivers/staging/brcm80211/Makefile |    3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/drivers/staging/brcm80211/Makefile b/drivers/staging/brcm80211/Makefile
> >>index 05a4103..7a77e7f 100644
> >>--- a/drivers/staging/brcm80211/Makefile
> >>+++ b/drivers/staging/brcm80211/Makefile
> >>@@ -15,8 +15,9 @@
> >>  # OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> >>  # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> >>
> >>-EXTRA_CFLAGS := -DBCMDBG -DWLC_HIGH -DSTA -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
> >>+EXTRA_CFLAGS := -DBCMDBG -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
> >
> >how about 2 patches here, one to break it out into one flag per line so
> >we can see what is really happening here, and then one patch to make the
> >change you are asking for.
> >
> >>
> >>+USB_CFLAGS := -DWLC_HIGH
> >
> >Are you sure about this?  Why set this when I don't see anything ever
> >using it?
> >
> >What problem are you trying to fix here.
> >
> >totally confused,
> >
> >greg k-h
> >
> 
> Well, when groveling through the code you can see a bunch of '#ifdef
> WLC_HIGH_ONLY' which I initially assumed was defined because of
> '-DWLC_HIGH' in EXTRA_CFLAGS. This is quite misleading if you don't
> notice 'PCI_CFLAGS := -DWLC_LOW' a bit later in the Makefile (which
> I didn't for quite awhile). Specifying both macros in EXTRA_CFLAGS
> seems unnecessary.
> 
> The macro magic happens in brcm80211/sys/wlc_cfg.h:
> 
> /* Keep WLC_HIGH_ONLY, WLC_SPLIT for USB extension later on */
> #if !defined(WLC_LOW)
> #define WLC_HIGH_ONLY
> #endif
> #if !defined(WLC_LOW)
> #define WLC_SPLIT
> #endif
> 
> What does WLC_LOW and WLC_HIGH mean anyway?

Who knows, but why create USB_CFLAGS, as that's not used by anything.

Henry, Brett, Nohee?

RE: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Brett Rudley]

In a split mac dongle, the HIGH code runs on the host processor while the LOW code runs on the dongle and the two talk across an RPC API over SDIO or USB.

On a NIC both HIGH and LOW run on the host and the RPC API remains identical but the APIs become function straight calls.
 
This lets us use the same code and API in both models.

Getting some documentation out on this is in TODO list.  

Brett

> What does WLC_LOW and WLC_HIGH mean anyway?
> 
> rtg
> --
> Tim Gardner tim.gardner@canonical.com

Re: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Tim Gardner]

On 09/17/2010 12:03 PM, Brett Rudley wrote:
>
> In a split mac dongle, the HIGH code runs on the host processor while
> the LOW code runs on the dongle and the two talk across an RPC API
> over SDIO or USB.
>

Don't you have the orientation inverted? WLC_LOW is defined for a PCI NIC.

> On a NIC both HIGH and LOW run on the host and the RPC API remains
> identical but the APIs become function straight calls.
>

I'm a bit confused by this. For PCI NICs WLC_HIGH_ONLY is definitely 
_not_ enabled, which means to me that HIGH is not running on the host.

> This lets us use the same code and API in both models.
>
> Getting some documentation out on this is in TODO list.
>

Thanks. Perhaps that'll clear up some confusion.

> Brett

-- 
Tim Gardner tim.gardner@canonical.com

RE: [PATCH] staging: brcm80211: Make CFLAGS explicit

[Brett Rudley]

 
> Don't you have the orientation inverted? WLC_LOW is defined for a
> PCI NIC.

Both LOW and HIGH are used for NIC.  That is to say everything runs on the host.

On splitmac dongle, only the HIGH runs on the host while the LOW runs on the dongle.

Both models run the HIGH code on the host.

Brett

Re: [PATCH] staging: brcm80211: add fullmac driver

[Greg KH]

On Mon, Sep 20, 2010 at 07:43:22PM -0700, Nohee Ko wrote:
> This patch to the existing bcm80211 directory in the staging tree adds fullmac driver support for the BCM4329 SDIO chip from Broadcom. Configuration of the mac80211 driver or the fullmac driver can be done through menuconfig.

Please wrap your lines at 72 columns.

Your patch does not apply to the latest linux-next tree.  Your patch is
also mime-encoded which probably doesn't help things.

Care to fix this up and resend it so I can apply it?

> --- a/drivers/staging/brcm80211/util/siutils.c
> +++ b/drivers/staging/brcm80211/util/siutils.c
> @@ -59,12 +59,16 @@ static bool si_buscore_prep(si_info_t *sii, uint bustype, uint devid,
>                             void *sdh);
>  static bool si_buscore_setup(si_info_t *sii, chipcregs_t *cc, uint bustype,
>                              uint32 savewin, uint *origidx, void *regs);
> +#ifndef BRCM_FULLMAC
>  static void si_nvram_process(si_info_t *sii, char *pvars);
> +#endif
> 
>  /* dev path concatenation util */
> +#ifndef BRCM_FULLMAC

<snip>

Why all the #ifdef mess added to the siutils.c file?  Is it really
needed?  Why not just leave it alone and have the functions always
build.

thanks,

greg k-h

Re: [PATCH] staging: brcm80211: Make CFLAGS explicit

[matt mooney]

On 09:20 Fri 17 Sep     , Tim Gardner wrote:
> -EXTRA_CFLAGS := -DBCMDBG -DWLC_HIGH -DSTA -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes
> +EXTRA_CFLAGS := -DBCMDBG -DWME -DWL11N -DDBAND -DBCMDMA32 -DBCMNVRAMR -Idrivers/staging/brcm80211/sys -Idrivers/staging/brcm80211/phy -Idrivers/staging/brcm80211/util -Idrivers/staging/brcm80211/include -Wall -Wstrict-prototypes -Werror -Wmissing-prototypes

Also, since you are changing this file, you should use ccflags-y instead of the
deprecated EXTRA_CFLAGS.

Thanks,
mfm