Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Sedat Dilek 
On Fri, May 15, 2015 at 4:38 PM, Julian Calaby julian.cal...@gmail.com wrote:
 Hi Oleksij,

 On Fri, May 15, 2015 at 10:35 PM, Oleksij Rempel li...@rempel-privat.de 
 wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c

 A better subject might be:

 ath9k: consolidate common phy functions

 phy.c doesn't really mean anything to people outside the developers
 of this particular driver.


Jupp.

Is a consolidation also possible/wise/doable also for phy.h?
( I see this snippet in your new phy.c file... )
...
+#include phy.h
+#include ar9002_phy.h
...

IOW, can ar9002_phy.h move into phy.h?

- sed@ -
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Julian Calaby 
Hi Oleksij,

On Fri, May 15, 2015 at 10:35 PM, Oleksij Rempel li...@rempel-privat.de wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c

A better subject might be:

ath9k: consolidate common phy functions

phy.c doesn't really mean anything to people outside the developers
of this particular driver.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Oleksij Rempel 
Am 16.05.2015 um 07:36 schrieb Joe Perches:
 On Sat, 2015-05-16 at 07:27 +0200, Oleksij Rempel wrote:
 
 Am 15.05.2015 um 20:35 schrieb Joe Perches:
 On Fri, 2015-05-15 at 14:35 +0200, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c
 []
 diff --git a/drivers/net/wireless/ath/ath9k/phy.c 
 b/drivers/net/wireless/ath/ath9k/phy.c

 +void phy_hw_spur_mitigate(struct ath_hw *ah,
 +struct ath9k_channel *chan, int bin)
 +{
 []
 +  static int inc[4] = { 0, 100, 0, 0 };

 static const
 
 This is cleanup patch, no fixes optimisations
 or regressions should be introduced.
 
 Hello Oleksij.
 
 One of the old compilation units (ar9002_phy.c) had const on
 all those arrays, the other did not.

ok, thanks. changed to const version.

 const should be used here.
 
 If you see some problems in this
 code, you are welcome to provide a patch on top of this one :)
 
 I think you should take comments on the patches you submit.

What do you mean?

-- 
Regards,
Oleksij



signature.asc
Description: OpenPGP digital signature
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Joe Perches 
On Sat, 2015-05-16 at 07:27 +0200, Oleksij Rempel wrote:

 Am 15.05.2015 um 20:35 schrieb Joe Perches:
  On Fri, 2015-05-15 at 14:35 +0200, Oleksij Rempel wrote:
  ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c
  []
  diff --git a/drivers/net/wireless/ath/ath9k/phy.c 
  b/drivers/net/wireless/ath/ath9k/phy.c
  
  +void phy_hw_spur_mitigate(struct ath_hw *ah,
  +struct ath9k_channel *chan, int bin)
  +{
[]
  +  static int inc[4] = { 0, 100, 0, 0 };
  
  static const

 This is cleanup patch, no fixes optimisations
 or regressions should be introduced.

Hello Oleksij.

One of the old compilation units (ar9002_phy.c) had const on
all those arrays, the other did not.

const should be used here.

 If you see some problems in this
 code, you are welcome to provide a patch on top of this one :)

I think you should take comments on the patches you submit.


___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Oleksij Rempel 
Am 15.05.2015 um 20:35 schrieb Joe Perches:
 On Fri, 2015-05-15 at 14:35 +0200, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c
 []
 diff --git a/drivers/net/wireless/ath/ath9k/phy.c 
 b/drivers/net/wireless/ath/ath9k/phy.c
 
 +void phy_hw_spur_mitigate(struct ath_hw *ah,
 +  struct ath9k_channel *chan, int bin)
 +{
 +int cur_bin;
 +int upper, lower, cur_vit_mask;
 +int i;
 +int8_t mask_m[123];
 +int8_t mask_p[123];
 
 Looking at this code, I'm not sure if mask_m and mask_p
 are always completely initialized by the loop below.
 
 Perhaps use {} on declaration.
 
 +int8_t mask_amt;
 
 These int8_t could be s8
 
 +int tmp_mask;
 +static int pilot_mask_reg[4] = {
 +AR_PHY_TIMING7, AR_PHY_TIMING8,
 +AR_PHY_PILOT_MASK_01_30, AR_PHY_PILOT_MASK_31_60
 +};
 +static int chan_mask_reg[4] = {
 +AR_PHY_TIMING9, AR_PHY_TIMING10,
 +AR_PHY_CHANNEL_MASK_01_30, AR_PHY_CHANNEL_MASK_31_60
 +};
 +static int inc[4] = { 0, 100, 0, 0 };
 
 static const
 
 +cur_vit_mask = 6100;
 +upper = bin + 120;
 +lower = bin - 120;
 +
 
 Is this loop guaranteed to always initialize all the
 mask_p and mask_m indexes used in the or statements below it?
 
 The 123 for -61 and 0 and +61 array indexes is a bit obscure.
 
 +for (i = 0; i  123; i++) {
 +if ((cur_vit_mask  lower)  (cur_vit_mask  upper)) {
 +/* workaround for gcc bug #37014 */
 +volatile int tmp_v = abs(cur_vit_mask - bin);
 +
 +if (tmp_v  75)
 +mask_amt = 1;
 +else
 +mask_amt = 0;
 +if (cur_vit_mask  0)
 +mask_m[abs(cur_vit_mask / 100)] = mask_amt;
 +else
 +mask_p[cur_vit_mask / 100] = mask_amt;
 +}
 +cur_vit_mask -= 100;
 +}
 +
 +tmp_mask = (mask_m[46]  30) | (mask_m[47]  28)
 +| (mask_m[48]  26) | (mask_m[49]  24)
 +| (mask_m[50]  22) | (mask_m[51]  20)
 +| (mask_m[52]  18) | (mask_m[53]  16)
 +| (mask_m[54]  14) | (mask_m[55]  12)
 +| (mask_m[56]  10) | (mask_m[57]  8)
 +| (mask_m[58]  6) | (mask_m[59]  4)
 +| (mask_m[60]  2) | (mask_m[61]  0);
 +REG_WRITE(ah, AR_PHY_BIN_MASK_1, tmp_mask);
 +REG_WRITE(ah, AR_PHY_VIT_MASK2_M_46_61, tmp_mask);
 +
 +tmp_mask = (mask_m[31]  28)
 +| (mask_m[32]  26) | (mask_m[33]  24)
 +| (mask_m[34]  22) | (mask_m[35]  20)
 +| (mask_m[36]  18) | (mask_m[37]  16)
 +| (mask_m[48]  14) | (mask_m[39]  12)
 +| (mask_m[40]  10) | (mask_m[41]  8)
 +| (mask_m[42]  6) | (mask_m[43]  4)
 +| (mask_m[44]  2) | (mask_m[45]  0);
 +REG_WRITE(ah, AR_PHY_BIN_MASK_2, tmp_mask);
 +REG_WRITE(ah, AR_PHY_MASK2_M_31_45, tmp_mask);
 +
 +tmp_mask = (mask_m[16]  30) | (mask_m[16]  28)
 +| (mask_m[18]  26) | (mask_m[18]  24)
 +| (mask_m[20]  22) | (mask_m[20]  20)
 +| (mask_m[22]  18) | (mask_m[22]  16)
 +| (mask_m[24]  14) | (mask_m[24]  12)
 +| (mask_m[25]  10) | (mask_m[26]  8)
 +| (mask_m[27]  6) | (mask_m[28]  4)
 +| (mask_m[29]  2) | (mask_m[30]  0);
 +REG_WRITE(ah, AR_PHY_BIN_MASK_3, tmp_mask);
 +REG_WRITE(ah, AR_PHY_MASK2_M_16_30, tmp_mask);
 +
 +tmp_mask = (mask_m[0]  30) | (mask_m[1]  28)
 +| (mask_m[2]  26) | (mask_m[3]  24)
 +| (mask_m[4]  22) | (mask_m[5]  20)
 +| (mask_m[6]  18) | (mask_m[7]  16)
 +| (mask_m[8]  14) | (mask_m[9]  12)
 +| (mask_m[10]  10) | (mask_m[11]  8)
 +| (mask_m[12]  6) | (mask_m[13]  4)
 +| (mask_m[14]  2) | (mask_m[15]  0);
 +REG_WRITE(ah, AR_PHY_MASK_CTL, tmp_mask);
 +REG_WRITE(ah, AR_PHY_MASK2_M_00_15, tmp_mask);
 +
 +tmp_mask = (mask_p[15]  28)
 +| (mask_p[14]  26) | (mask_p[13]  24)
 +| (mask_p[12]  22) | (mask_p[11]  20)
 +| (mask_p[10]  18) | (mask_p[9]  16)
 +| (mask_p[8]  14) | (mask_p[7]  12)
 +| (mask_p[6]  10) | (mask_p[5]  8)
 +| (mask_p[4]  6) | (mask_p[3]  4)
 +| (mask_p[2]  2) | (mask_p[1]  0);
 +REG_WRITE(ah, AR_PHY_BIN_MASK2_1, tmp_mask);
 +REG_WRITE(ah, AR_PHY_MASK2_P_15_01, tmp_mask);
 +
 +tmp_mask = (mask_p[30]  28)
 +| (mask_p[29]  26) | (mask_p[28]  24)
 +| (mask_p[27]  22) | (mask_p[26]  20)
 +| (mask_p[25]  18) | (mask_p[24]  16)
 +| (mask_p[23]  14) | (mask_p[22]  12)
 +| (mask_p[21]  10) | (mask_p[20]  8)
 +| (mask_p[19]  6) | (mask_p[18]  4)
 +| (mask_p[17]  2) | (mask_p[16]  0);
 +REG_WRITE(ah, AR_PHY_BIN_MASK2_2,

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Oleksij Rempel 
Am 15.05.2015 um 21:34 schrieb Felix Fietkau:
 On 2015-05-15 14:35, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c

 Signed-off-by: Oleksij Rempel li...@rempel-privat.de
 We already have base functionality for AR5008-AR9002 provided in some
 ar5008_phy.c, and ar5008_hw_attach_phy_ops is called for those chipsets
 as well. Please keep the de-duplicated code there instead of adding a
 new phy.c, because AR9003+ uses a completely different codepath.
 
 - Felix
 

ok, thanks.
Currently i celled this function phy_hw_spur_mitigate(), is it ok or
there is some better name for this code?


-- 
Regards,
Oleksij
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Felix Fietkau 
On 2015-05-16 07:24, Oleksij Rempel wrote:
 Am 15.05.2015 um 21:34 schrieb Felix Fietkau:
 On 2015-05-15 14:35, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c

 Signed-off-by: Oleksij Rempel li...@rempel-privat.de
 We already have base functionality for AR5008-AR9002 provided in some
 ar5008_phy.c, and ar5008_hw_attach_phy_ops is called for those chipsets
 as well. Please keep the de-duplicated code there instead of adding a
 new phy.c, because AR9003+ uses a completely different codepath.
 
 - Felix
 
 
 ok, thanks.
 Currently i celled this function phy_hw_spur_mitigate(), is it ok or
 there is some better name for this code?
Just use the ar5008 prefix like in the other functions.

- Felix
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Felix Fietkau 
On 2015-05-15 14:35, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c
 
 Signed-off-by: Oleksij Rempel li...@rempel-privat.de
We already have base functionality for AR5008-AR9002 provided in some
ar5008_phy.c, and ar5008_hw_attach_phy_ops is called for those chipsets
as well. Please keep the de-duplicated code there instead of adding a
new phy.c, because AR9003+ uses a completely different codepath.

- Felix
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH] ath9k: add phy.c

2015-05-15 Thread Joe Perches 
On Fri, 2015-05-15 at 14:35 +0200, Oleksij Rempel wrote:
 ... and move dup code from ar5008_phy.c and ar9002_phy.c to phy.c
[]
 diff --git a/drivers/net/wireless/ath/ath9k/phy.c 
 b/drivers/net/wireless/ath/ath9k/phy.c

 +void phy_hw_spur_mitigate(struct ath_hw *ah,
 +   struct ath9k_channel *chan, int bin)
 +{
 + int cur_bin;
 + int upper, lower, cur_vit_mask;
 + int i;
 + int8_t mask_m[123];
 + int8_t mask_p[123];

Looking at this code, I'm not sure if mask_m and mask_p
are always completely initialized by the loop below.

Perhaps use {} on declaration.

 + int8_t mask_amt;

These int8_t could be s8

 + int tmp_mask;
 + static int pilot_mask_reg[4] = {
 + AR_PHY_TIMING7, AR_PHY_TIMING8,
 + AR_PHY_PILOT_MASK_01_30, AR_PHY_PILOT_MASK_31_60
 + };
 + static int chan_mask_reg[4] = {
 + AR_PHY_TIMING9, AR_PHY_TIMING10,
 + AR_PHY_CHANNEL_MASK_01_30, AR_PHY_CHANNEL_MASK_31_60
 + };
 + static int inc[4] = { 0, 100, 0, 0 };

static const

 + cur_vit_mask = 6100;
 + upper = bin + 120;
 + lower = bin - 120;
 +

Is this loop guaranteed to always initialize all the
mask_p and mask_m indexes used in the or statements below it?

The 123 for -61 and 0 and +61 array indexes is a bit obscure.

 + for (i = 0; i  123; i++) {
 + if ((cur_vit_mask  lower)  (cur_vit_mask  upper)) {
 + /* workaround for gcc bug #37014 */
 + volatile int tmp_v = abs(cur_vit_mask - bin);
 +
 + if (tmp_v  75)
 + mask_amt = 1;
 + else
 + mask_amt = 0;
 + if (cur_vit_mask  0)
 + mask_m[abs(cur_vit_mask / 100)] = mask_amt;
 + else
 + mask_p[cur_vit_mask / 100] = mask_amt;
 + }
 + cur_vit_mask -= 100;
 + }
 +
 + tmp_mask = (mask_m[46]  30) | (mask_m[47]  28)
 + | (mask_m[48]  26) | (mask_m[49]  24)
 + | (mask_m[50]  22) | (mask_m[51]  20)
 + | (mask_m[52]  18) | (mask_m[53]  16)
 + | (mask_m[54]  14) | (mask_m[55]  12)
 + | (mask_m[56]  10) | (mask_m[57]  8)
 + | (mask_m[58]  6) | (mask_m[59]  4)
 + | (mask_m[60]  2) | (mask_m[61]  0);
 + REG_WRITE(ah, AR_PHY_BIN_MASK_1, tmp_mask);
 + REG_WRITE(ah, AR_PHY_VIT_MASK2_M_46_61, tmp_mask);
 +
 + tmp_mask = (mask_m[31]  28)
 + | (mask_m[32]  26) | (mask_m[33]  24)
 + | (mask_m[34]  22) | (mask_m[35]  20)
 + | (mask_m[36]  18) | (mask_m[37]  16)
 + | (mask_m[48]  14) | (mask_m[39]  12)
 + | (mask_m[40]  10) | (mask_m[41]  8)
 + | (mask_m[42]  6) | (mask_m[43]  4)
 + | (mask_m[44]  2) | (mask_m[45]  0);
 + REG_WRITE(ah, AR_PHY_BIN_MASK_2, tmp_mask);
 + REG_WRITE(ah, AR_PHY_MASK2_M_31_45, tmp_mask);
 +
 + tmp_mask = (mask_m[16]  30) | (mask_m[16]  28)
 + | (mask_m[18]  26) | (mask_m[18]  24)
 + | (mask_m[20]  22) | (mask_m[20]  20)
 + | (mask_m[22]  18) | (mask_m[22]  16)
 + | (mask_m[24]  14) | (mask_m[24]  12)
 + | (mask_m[25]  10) | (mask_m[26]  8)
 + | (mask_m[27]  6) | (mask_m[28]  4)
 + | (mask_m[29]  2) | (mask_m[30]  0);
 + REG_WRITE(ah, AR_PHY_BIN_MASK_3, tmp_mask);
 + REG_WRITE(ah, AR_PHY_MASK2_M_16_30, tmp_mask);
 +
 + tmp_mask = (mask_m[0]  30) | (mask_m[1]  28)
 + | (mask_m[2]  26) | (mask_m[3]  24)
 + | (mask_m[4]  22) | (mask_m[5]  20)
 + | (mask_m[6]  18) | (mask_m[7]  16)
 + | (mask_m[8]  14) | (mask_m[9]  12)
 + | (mask_m[10]  10) | (mask_m[11]  8)
 + | (mask_m[12]  6) | (mask_m[13]  4)
 + | (mask_m[14]  2) | (mask_m[15]  0);
 + REG_WRITE(ah, AR_PHY_MASK_CTL, tmp_mask);
 + REG_WRITE(ah, AR_PHY_MASK2_M_00_15, tmp_mask);
 +
 + tmp_mask = (mask_p[15]  28)
 + | (mask_p[14]  26) | (mask_p[13]  24)
 + | (mask_p[12]  22) | (mask_p[11]  20)
 + | (mask_p[10]  18) | (mask_p[9]  16)
 + | (mask_p[8]  14) | (mask_p[7]  12)
 + | (mask_p[6]  10) | (mask_p[5]  8)
 + | (mask_p[4]  6) | (mask_p[3]  4)
 + | (mask_p[2]  2) | (mask_p[1]  0);
 + REG_WRITE(ah, AR_PHY_BIN_MASK2_1, tmp_mask);
 + REG_WRITE(ah, AR_PHY_MASK2_P_15_01, tmp_mask);
 +
 + tmp_mask = (mask_p[30]  28)
 + | (mask_p[29]  26) | (mask_p[28]  24)
 + | (mask_p[27]  22) | (mask_p[26]  20)
 + | (mask_p[25]  18) | (mask_p[24]  16)
 + | (mask_p[23]  14) | (mask_p[22]  12)
 + | (mask_p[21]  10) | (mask_p[20]  8)
 + | (mask_p[19]  6) | (mask_p[18]  4)
 + | (mask_p[17]  2) | (mask_p[16]  0);
 +