Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-29 Thread Greg KH 
On Sun, Oct 26, 2014 at 04:18:52PM +, Paul McQuade wrote:
 ERROR: spaces required around that ':' (ctx:VxE)
 
 Signed-off-by: Paul McQuade paulmcq...@gmail.com
 ---
  drivers/staging/rtl8723au/core/rtw_ieee80211.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

Your Subject: makes no sense :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-27 Thread Jes Sorensen 
Joe Perches j...@perches.com writes:
 On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
  ERROR: spaces required around that ':' (ctx:VxE)
 []
  diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
  b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 []
  @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
  short_GI_20, u8 short_GI_40,
   
if (rf_type == RF_1T1R) {
if (mcs-rx_mask[0]  BIT(7))
  - max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350):
  + max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350) :
 []
 
  A macro could help intelligibility here - maybe something like:
 
  #define get_max_rate(r1, r2, r3, r4)   
  \
 (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
  and:
 if (mcs-rx_mask[0]  BIT(7))
 max_rate = get_max_rate(1500, 1350, 722, 650);
 
 Hiding a parameter to a macro like that is bad coding practice, so don't
 do that please!

 Yes and no.

 Adding the other 3 arguments to the macro doesn't help legibility.

 Keeping the macro definition local to the place that it's used
 can help avoid typos.

It's wrong, so just don't do it here!

Thank you,
Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-27 Thread Joe Perches 
On Mon, 2014-10-27 at 07:16 +0100, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
  Joe Perches j...@perches.com writes:
   On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
   ERROR: spaces required around that ':' (ctx:VxE)
  []
   diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
   b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
  []
   @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
   short_GI_20, u8 short_GI_40,

   if (rf_type == RF_1T1R) {
   if (mcs-rx_mask[0]  BIT(7))
   -   max_rate = (bw_40MHz) ? 
   ((short_GI_40)?1500:1350):
   +   max_rate = (bw_40MHz) ? 
   ((short_GI_40)?1500:1350) :
  []
  
   A macro could help intelligibility here - maybe something like:
  
   #define get_max_rate(r1, r2, r3, r4) 
   \
(bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
   and:
if (mcs-rx_mask[0]  BIT(7))
max_rate = get_max_rate(1500, 1350, 722, 650);
  
  Hiding a parameter to a macro like that is bad coding practice, so don't
  do that please!
 
  Yes and no.
 
  Adding the other 3 arguments to the macro doesn't help legibility.
 
  Keeping the macro definition local to the place that it's used
  can help avoid typos.
 
 It's wrong, so just don't do it here!

It's not wrong, you just don't like it.



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-27 Thread Jes Sorensen 
Joe Perches j...@perches.com writes:
 On Mon, 2014-10-27 at 07:16 +0100, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
  Joe Perches j...@perches.com writes:
   On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
   ERROR: spaces required around that ':' (ctx:VxE)
  []
   diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
   b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
  []
   @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
   short_GI_20, u8 short_GI_40,

  if (rf_type == RF_1T1R) {
  if (mcs-rx_mask[0]  BIT(7))
   -  max_rate = (bw_40MHz) ? 
   ((short_GI_40)?1500:1350):
   +  max_rate = (bw_40MHz) ? 
   ((short_GI_40)?1500:1350) :
  []
  
   A macro could help intelligibility here - maybe something like:
  
   #define get_max_rate(r1, r2, r3, r4)
   \
   (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
   and:
   if (mcs-rx_mask[0]  BIT(7))
   max_rate = get_max_rate(1500, 1350, 722, 650);
  
  Hiding a parameter to a macro like that is bad coding practice, so don't
  do that please!
 
  Yes and no.
 
  Adding the other 3 arguments to the macro doesn't help legibility.
 
  Keeping the macro definition local to the place that it's used
  can help avoid typos.
 
 It's wrong, so just don't do it here!

 It's not wrong, you just don't like it.

I consider it wrong, and I am the co-maintainer of this driver  and
I NACK it.

There's a gazillion real issues to fix in this driver, adding a bad
macro isn't on that list.

Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-26 Thread Joe Perches 
On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
 ERROR: spaces required around that ':' (ctx:VxE)
 
 Signed-off-by: Paul McQuade paulmcq...@gmail.com
 ---
  drivers/staging/rtl8723au/core/rtw_ieee80211.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c 
 b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 index 6274cb3..41006cf 100644
 --- a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 +++ b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
 short_GI_20, u8 short_GI_40,
  
   if (rf_type == RF_1T1R) {
   if (mcs-rx_mask[0]  BIT(7))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350) :
   ((short_GI_20)?722:650);
   else if (mcs-rx_mask[0]  BIT(6))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?1350:1215):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?1350:1215) :
   ((short_GI_20)?650:585);
   else if (mcs-rx_mask[0]  BIT(5))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?1200:1080):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?1200:1080) :
   ((short_GI_20)?578:520);
   else if (mcs-rx_mask[0]  BIT(4))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?900:810):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?900:810) :
   ((short_GI_20)?433:390);
   else if (mcs-rx_mask[0]  BIT(3))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?600:540):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?600:540) :
   ((short_GI_20)?289:260);
   else if (mcs-rx_mask[0]  BIT(2))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?450:405):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?450:405) :
   ((short_GI_20)?217:195);
   else if (mcs-rx_mask[0]  BIT(1))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?300:270):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?300:270) :
   ((short_GI_20)?144:130);
   else if (mcs-rx_mask[0]  BIT(0))
 - max_rate = (bw_40MHz) ? ((short_GI_40)?150:135):
 + max_rate = (bw_40MHz) ? ((short_GI_40)?150:135) :
   ((short_GI_20)?72:65);

A macro could help intelligibility here - maybe something like:

#define get_max_rate(r1, r2, r3, r4)\
(bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
and:
if (mcs-rx_mask[0]  BIT(7))
max_rate = get_max_rate(1500, 1350, 722, 650);
else if (mcs-rx_mask[0]  BIT(6))
max_rate = get_max_rate(1350, 1215, 650, 585);
else if (mcs-rx_mask[0]  BIT(5))
max_rate = get_max_rate(1200, 1080, 578, 520);
else if (mcs-rx_mask[0]  BIT(4))
max_rate = get_max_rate(900, 810, 433, 390);
else if (mcs-rx_mask[0]  BIT(3))
max_rate = get_max_rate(600, 540, 289, 260);
else if (mcs-rx_mask[0]  BIT(2))
max_rate = get_max_rate(450, 405, 217, 195);
else if (mcs-rx_mask[0]  BIT(1))
max_rate = get_max_rate(300, 270, 144, 130);
else if (mcs-rx_mask[0]  BIT(0))
max_rate = get_max_rate(150, 135, 72, 65);



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-26 Thread Jes Sorensen 
Joe Perches j...@perches.com writes:
 On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
 ERROR: spaces required around that ':' (ctx:VxE)
 
 Signed-off-by: Paul McQuade paulmcq...@gmail.com
 ---
  drivers/staging/rtl8723au/core/rtw_ieee80211.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c 
 b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 index 6274cb3..41006cf 100644
 --- a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 +++ b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
 @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
 short_GI_20, u8 short_GI_40,
  
  if (rf_type == RF_1T1R) {
  if (mcs-rx_mask[0]  BIT(7))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350) :
  ((short_GI_20)?722:650);
  else if (mcs-rx_mask[0]  BIT(6))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?1350:1215):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?1350:1215) :
  ((short_GI_20)?650:585);
  else if (mcs-rx_mask[0]  BIT(5))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?1200:1080):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?1200:1080) :
  ((short_GI_20)?578:520);
  else if (mcs-rx_mask[0]  BIT(4))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?900:810):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?900:810) :
  ((short_GI_20)?433:390);
  else if (mcs-rx_mask[0]  BIT(3))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?600:540):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?600:540) :
  ((short_GI_20)?289:260);
  else if (mcs-rx_mask[0]  BIT(2))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?450:405):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?450:405) :
  ((short_GI_20)?217:195);
  else if (mcs-rx_mask[0]  BIT(1))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?300:270):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?300:270) :
  ((short_GI_20)?144:130);
  else if (mcs-rx_mask[0]  BIT(0))
 -max_rate = (bw_40MHz) ? ((short_GI_40)?150:135):
 +max_rate = (bw_40MHz) ? ((short_GI_40)?150:135) :
  ((short_GI_20)?72:65);

 A macro could help intelligibility here - maybe something like:

 #define get_max_rate(r1, r2, r3, r4)  \
   (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
 and:
   if (mcs-rx_mask[0]  BIT(7))
   max_rate = get_max_rate(1500, 1350, 722, 650);

Hiding a parameter to a macro like that is bad coding practice, so don't
do that please!

Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: rtl8723au:core

2014-10-26 Thread Joe Perches 
On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
  ERROR: spaces required around that ':' (ctx:VxE)
[]
  diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c 
  b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
[]
  @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8 
  short_GI_20, u8 short_GI_40,
   
 if (rf_type == RF_1T1R) {
 if (mcs-rx_mask[0]  BIT(7))
  -  max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350):
  +  max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350) :
[]
 
  A macro could help intelligibility here - maybe something like:
 
  #define get_max_rate(r1, r2, r3, r4)
  \
  (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
  and:
  if (mcs-rx_mask[0]  BIT(7))
  max_rate = get_max_rate(1500, 1350, 722, 650);
 
 Hiding a parameter to a macro like that is bad coding practice, so don't
 do that please!

Yes and no.

Adding the other 3 arguments to the macro doesn't help legibility.

Keeping the macro definition local to the place that it's used
can help avoid typos.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel