[PATCH] ath_hw: Use common REG_WRITE parameter order

[PATCH] ath_hw: Use common REG_WRITE parameter order

[Sven Eckelmann]

All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
"register" and "value". hw.c is the only file using the order "ah", "value" and
"register". This inconsistent definition can easily lead to implementation
errors.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/net/wireless/ath/hw.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
index 19befb3..39e8a59 100644
--- a/drivers/net/wireless/ath/hw.c
+++ b/drivers/net/wireless/ath/hw.c
@@ -20,8 +20,8 @@
 #include "ath.h"
 #include "reg.h"
 
-#define REG_READ	(common->ops->read)
-#define REG_WRITE	(common->ops->write)
+#define REG_READ			(common->ops->read)
+#define REG_WRITE(_ah, _reg, _val)	(common->ops->write)(_ah, _val, _reg)
 
 /**
  * ath_hw_set_bssid_mask - filter out bssids we listen
@@ -119,8 +119,8 @@ void ath_hw_setbssidmask(struct ath_common *common)
 {
 	void *ah = common->ah;
 
-	REG_WRITE(ah, get_unaligned_le32(common->bssidmask), AR_BSSMSKL);
-	REG_WRITE(ah, get_unaligned_le16(common->bssidmask + 4), AR_BSSMSKU);
+	REG_WRITE(ah, AR_BSSMSKL, get_unaligned_le32(common->bssidmask));
+	REG_WRITE(ah, AR_BSSMSKU, get_unaligned_le16(common->bssidmask + 4));
 }
 EXPORT_SYMBOL(ath_hw_setbssidmask);
 
@@ -139,7 +139,7 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
 	void *ah = common->ah;
 
 	/* freeze */
-	REG_WRITE(ah, AR_MIBC_FMC, AR_MIBC);
+	REG_WRITE(ah, AR_MIBC, AR_MIBC_FMC);
 
 	/* read */
 	cycles = REG_READ(ah, AR_CCCNT);
@@ -148,13 +148,13 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
 	tx = REG_READ(ah, AR_TFCNT);
 
 	/* clear */
-	REG_WRITE(ah, 0, AR_CCCNT);
-	REG_WRITE(ah, 0, AR_RFCNT);
-	REG_WRITE(ah, 0, AR_RCCNT);
-	REG_WRITE(ah, 0, AR_TFCNT);
+	REG_WRITE(ah, AR_CCCNT, 0);
+	REG_WRITE(ah, AR_RFCNT, 0);
+	REG_WRITE(ah, AR_RCCNT, 0);
+	REG_WRITE(ah, AR_TFCNT, 0);
 
 	/* unfreeze */
-	REG_WRITE(ah, 0, AR_MIBC);
+	REG_WRITE(ah, AR_MIBC, 0);
 
 	/* update all cycle counters here */
 	common->cc_ani.cycles += cycles;
-- 
1.7.10.4

Re: [PATCH] ath_hw: Use common REG_WRITE parameter order

[Luis R. Rodriguez]

On Thu, Oct 4, 2012 at 9:05 AM, Sven Eckelmann <sven@narfation.org> wrote:
> All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
> "register" and "value". hw.c is the only file using the order "ah", "value" and
> "register". This inconsistent definition can easily lead to implementation
> errors.
>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Good catch! But can you resend specifying in your commit log that this
change is a no-op? Also it may help the reader for you to git grep and
show the other REG_WRITE() definitions on the other c files within the
commit log.

  Luis

Re: [PATCH] ath_hw: Use common REG_WRITE parameter order

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Thursday 04 October 2012 10:29:59 Luis R. Rodriguez wrote:
> On Thu, Oct 4, 2012 at 9:05 AM, Sven Eckelmann <sven@narfation.org> wrote:
> > All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
> > "register" and "value". hw.c is the only file using the order "ah",
> > "value" and "register". This inconsistent definition can easily lead to
> > implementation errors.
> > 
> > Signed-off-by: Sven Eckelmann <sven@narfation.org>
> 
> Good catch! But can you resend specifying in your commit log that this
> change is a no-op?

I really don't know what you want from me here.

> Also it may help the reader for you to git grep and
> show the other REG_WRITE() definitions on the other c files within the
> commit log.

Ehrm, ok.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv2] ath_hw: Use common REG_WRITE parameter order

[Sven Eckelmann]

All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
"register" and "value". hw.c is the only file using the order "ah", "value" and
"register".

drivers/net/wireless/ath/ath9k/hw.h:#define REG_WRITE(_ah, _reg, _val) \
drivers/net/wireless/ath/key.c:#define REG_WRITE(_ah, _reg, _val) (common->ops->write)(_ah, _val, _reg)

This inconsistent definition can easily lead to implementation errors. The
modification doesn't change the behavior of the driver or the generated code.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
Changed commit message

 drivers/net/wireless/ath/hw.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
index 19befb3..39e8a59 100644
--- a/drivers/net/wireless/ath/hw.c
+++ b/drivers/net/wireless/ath/hw.c
@@ -20,8 +20,8 @@
 #include "ath.h"
 #include "reg.h"
 
-#define REG_READ	(common->ops->read)
-#define REG_WRITE	(common->ops->write)
+#define REG_READ			(common->ops->read)
+#define REG_WRITE(_ah, _reg, _val)	(common->ops->write)(_ah, _val, _reg)
 
 /**
  * ath_hw_set_bssid_mask - filter out bssids we listen
@@ -119,8 +119,8 @@ void ath_hw_setbssidmask(struct ath_common *common)
 {
 	void *ah = common->ah;
 
-	REG_WRITE(ah, get_unaligned_le32(common->bssidmask), AR_BSSMSKL);
-	REG_WRITE(ah, get_unaligned_le16(common->bssidmask + 4), AR_BSSMSKU);
+	REG_WRITE(ah, AR_BSSMSKL, get_unaligned_le32(common->bssidmask));
+	REG_WRITE(ah, AR_BSSMSKU, get_unaligned_le16(common->bssidmask + 4));
 }
 EXPORT_SYMBOL(ath_hw_setbssidmask);
 
@@ -139,7 +139,7 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
 	void *ah = common->ah;
 
 	/* freeze */
-	REG_WRITE(ah, AR_MIBC_FMC, AR_MIBC);
+	REG_WRITE(ah, AR_MIBC, AR_MIBC_FMC);
 
 	/* read */
 	cycles = REG_READ(ah, AR_CCCNT);
@@ -148,13 +148,13 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
 	tx = REG_READ(ah, AR_TFCNT);
 
 	/* clear */
-	REG_WRITE(ah, 0, AR_CCCNT);
-	REG_WRITE(ah, 0, AR_RFCNT);
-	REG_WRITE(ah, 0, AR_RCCNT);
-	REG_WRITE(ah, 0, AR_TFCNT);
+	REG_WRITE(ah, AR_CCCNT, 0);
+	REG_WRITE(ah, AR_RFCNT, 0);
+	REG_WRITE(ah, AR_RCCNT, 0);
+	REG_WRITE(ah, AR_TFCNT, 0);
 
 	/* unfreeze */
-	REG_WRITE(ah, 0, AR_MIBC);
+	REG_WRITE(ah, AR_MIBC, 0);
 
 	/* update all cycle counters here */
 	common->cc_ani.cycles += cycles;
-- 
1.7.10.4

Re: [PATCH] ath_hw: Use common REG_WRITE parameter order

[Luis R. Rodriguez]

On Thu, Oct 4, 2012 at 10:42 AM, Sven Eckelmann <sven@narfation.org> wrote:
> On Thursday 04 October 2012 10:29:59 Luis R. Rodriguez wrote:
>> On Thu, Oct 4, 2012 at 9:05 AM, Sven Eckelmann <sven@narfation.org> wrote:
>> > All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
>> > "register" and "value". hw.c is the only file using the order "ah",
>> > "value" and "register". This inconsistent definition can easily lead to
>> > implementation errors.
>> >
>> > Signed-off-by: Sven Eckelmann <sven@narfation.org>
>>
>> Good catch! But can you resend specifying in your commit log that this
>> change is a no-op?
>
> I really don't know what you want from me here.

Just specify in the commit log that this change causes no functional changes.

  Luis

Re: [PATCHv2] ath_hw: Use common REG_WRITE parameter order

[Luis R. Rodriguez]

On Thu, Oct 04, 2012 at 07:43:15PM +0200, Sven Eckelmann wrote:
> All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
> "register" and "value". hw.c is the only file using the order "ah", "value" and
> "register".
> 
> drivers/net/wireless/ath/ath9k/hw.h:#define REG_WRITE(_ah, _reg, _val) \
> drivers/net/wireless/ath/key.c:#define REG_WRITE(_ah, _reg, _val) (common->ops->write)(_ah, _val, _reg)
> 
> This inconsistent definition can easily lead to implementation errors. The
> modification doesn't change the behavior of the driver or the generated code.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

Acked-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

  Luis

Re: [PATCH] ath_hw: Use common REG_WRITE parameter order

[Mohammed Shafi]

On Thu, Oct 4, 2012 at 9:35 PM, Sven Eckelmann <sven@narfation.org> wrote:
> All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
> "register" and "value". hw.c is the only file using the order "ah", "value" and
> "register". This inconsistent definition can easily lead to implementation
> errors.

oh yes, once Raj and myself were reviewing cycle counters stuff and
was intially confused
with the slight variance in the REG_WRITE definition in ath9k/hw.h,
and ath/hw.c.
thanks to you now, as  it was gone and make our life easier.

>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  drivers/net/wireless/ath/hw.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
> index 19befb3..39e8a59 100644
> --- a/drivers/net/wireless/ath/hw.c
> +++ b/drivers/net/wireless/ath/hw.c
> @@ -20,8 +20,8 @@
>  #include "ath.h"
>  #include "reg.h"
>
> -#define REG_READ       (common->ops->read)
> -#define REG_WRITE      (common->ops->write)
> +#define REG_READ                       (common->ops->read)
> +#define REG_WRITE(_ah, _reg, _val)     (common->ops->write)(_ah, _val, _reg)
>
>  /**
>   * ath_hw_set_bssid_mask - filter out bssids we listen
> @@ -119,8 +119,8 @@ void ath_hw_setbssidmask(struct ath_common *common)
>  {
>         void *ah = common->ah;
>
> -       REG_WRITE(ah, get_unaligned_le32(common->bssidmask), AR_BSSMSKL);
> -       REG_WRITE(ah, get_unaligned_le16(common->bssidmask + 4), AR_BSSMSKU);
> +       REG_WRITE(ah, AR_BSSMSKL, get_unaligned_le32(common->bssidmask));
> +       REG_WRITE(ah, AR_BSSMSKU, get_unaligned_le16(common->bssidmask + 4));
>  }
>  EXPORT_SYMBOL(ath_hw_setbssidmask);
>
> @@ -139,7 +139,7 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
>         void *ah = common->ah;
>
>         /* freeze */
> -       REG_WRITE(ah, AR_MIBC_FMC, AR_MIBC);
> +       REG_WRITE(ah, AR_MIBC, AR_MIBC_FMC);
>
>         /* read */
>         cycles = REG_READ(ah, AR_CCCNT);
> @@ -148,13 +148,13 @@ void ath_hw_cycle_counters_update(struct ath_common *common)
>         tx = REG_READ(ah, AR_TFCNT);
>
>         /* clear */
> -       REG_WRITE(ah, 0, AR_CCCNT);
> -       REG_WRITE(ah, 0, AR_RFCNT);
> -       REG_WRITE(ah, 0, AR_RCCNT);
> -       REG_WRITE(ah, 0, AR_TFCNT);
> +       REG_WRITE(ah, AR_CCCNT, 0);
> +       REG_WRITE(ah, AR_RFCNT, 0);
> +       REG_WRITE(ah, AR_RCCNT, 0);
> +       REG_WRITE(ah, AR_TFCNT, 0);
>
>         /* unfreeze */
> -       REG_WRITE(ah, 0, AR_MIBC);
> +       REG_WRITE(ah, AR_MIBC, 0);
>
>         /* update all cycle counters here */
>         common->cc_ani.cycles += cycles;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
thanks,
shafi

Re: [PATCHv2] ath_hw: Use common REG_WRITE parameter order

[Arend van Spriel]

On 10/04/2012 07:43 PM, Sven Eckelmann wrote:
> All defines for REG_WRITE in Atheros wireless drivers use the order "ah",
> "register" and "value". hw.c is the only file using the order "ah", "value" and
> "register".
>
> drivers/net/wireless/ath/ath9k/hw.h:#define REG_WRITE(_ah, _reg, _val) \

Just to bad you did not catch the next line in hw.h.

Gr. AvS

> drivers/net/wireless/ath/key.c:#define REG_WRITE(_ah, _reg, _val) (common->ops->write)(_ah, _val, _reg)
>
> This inconsistent definition can easily lead to implementation errors. The
> modification doesn't change the behavior of the driver or the generated code.
>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> ---
> Changed commit message
>
>   drivers/net/wireless/ath/hw.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)