[PATCH 0/4] atheros: implement common read/write ops

[PATCH 0/4] atheros: implement common read/write ops

[Luis R. Rodriguez]

This is my fourth series, as I had noted earlier another
thing we needed to do was make read/write driver core
specific so that ath9k_htc can define its own and yet
share common hw code. As it turns out this means the work
here allows us to also start sharing hw code between ath9k
and ath5k. The last patch is an example of this.

Luis R. Rodriguez (4):
  atheros/ath9k: add common read/write ops and port ath9k to use it
  ath5k: allocate ath5k_hw prior to initializing hw
  ath5k: define ath_common ops
  atheros: define shared bssidmask setting

 drivers/net/wireless/ath/Makefile        |    5 +-
 drivers/net/wireless/ath/ath.h           |    9 ++
 drivers/net/wireless/ath/ath5k/ath5k.h   |   24 ++++---
 drivers/net/wireless/ath/ath5k/attach.c  |   25 +-----
 drivers/net/wireless/ath/ath5k/base.c    |   38 ++++++++-
 drivers/net/wireless/ath/ath5k/base.h    |   11 ---
 drivers/net/wireless/ath/ath5k/pcu.c     |  113 +--------------------------
 drivers/net/wireless/ath/ath9k/ath9k.h   |    3 -
 drivers/net/wireless/ath/ath9k/hw.c      |   42 +----------
 drivers/net/wireless/ath/ath9k/hw.h      |    4 +-
 drivers/net/wireless/ath/ath9k/main.c    |   43 ++++++++++
 drivers/net/wireless/ath/ath9k/recv.c    |    2 +-
 drivers/net/wireless/ath/ath9k/virtual.c |    2 +-
 drivers/net/wireless/ath/hw.c            |  126 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/reg.h           |    2 +
 15 files changed, 245 insertions(+), 204 deletions(-)
 create mode 100644 drivers/net/wireless/ath/hw.c
 create mode 100644 drivers/net/wireless/ath/reg.h

[ath9k-devel] [PATCH 0/4] atheros: implement common read/write ops

[Luis R. Rodriguez]

This is my fourth series, as I had noted earlier another
thing we needed to do was make read/write driver core
specific so that ath9k_htc can define its own and yet
share common hw code. As it turns out this means the work
here allows us to also start sharing hw code between ath9k
and ath5k. The last patch is an example of this.

Luis R. Rodriguez (4):
  atheros/ath9k: add common read/write ops and port ath9k to use it
  ath5k: allocate ath5k_hw prior to initializing hw
  ath5k: define ath_common ops
  atheros: define shared bssidmask setting

 drivers/net/wireless/ath/Makefile        |    5 +-
 drivers/net/wireless/ath/ath.h           |    9 ++
 drivers/net/wireless/ath/ath5k/ath5k.h   |   24 ++++---
 drivers/net/wireless/ath/ath5k/attach.c  |   25 +-----
 drivers/net/wireless/ath/ath5k/base.c    |   38 ++++++++-
 drivers/net/wireless/ath/ath5k/base.h    |   11 ---
 drivers/net/wireless/ath/ath5k/pcu.c     |  113 +--------------------------
 drivers/net/wireless/ath/ath9k/ath9k.h   |    3 -
 drivers/net/wireless/ath/ath9k/hw.c      |   42 +----------
 drivers/net/wireless/ath/ath9k/hw.h      |    4 +-
 drivers/net/wireless/ath/ath9k/main.c    |   43 ++++++++++
 drivers/net/wireless/ath/ath9k/recv.c    |    2 +-
 drivers/net/wireless/ath/ath9k/virtual.c |    2 +-
 drivers/net/wireless/ath/hw.c            |  126 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/reg.h           |    2 +
 15 files changed, 245 insertions(+), 204 deletions(-)
 create mode 100644 drivers/net/wireless/ath/hw.c
 create mode 100644 drivers/net/wireless/ath/reg.h

[PATCH 1/4] atheros/ath9k: add common read/write ops and port ath9k to use it

[Luis R. Rodriguez]

In an effort to make hw code driver core agnostic read
and write operations are defined on the ath_common structure.
This patch adds that and makes ath9k use it. This allows
drivers like ath9k_htc to define its own read/write ops and
still rely on the same hw code. This also paves the way for
sharing code between ath9k/ath5k/ath9k_htc.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath.h         |    6 ++++
 drivers/net/wireless/ath/ath9k/ath9k.h |    3 --
 drivers/net/wireless/ath/ath9k/hw.c    |   32 ------------------------
 drivers/net/wireless/ath/ath9k/hw.h    |    4 +-
 drivers/net/wireless/ath/ath9k/main.c  |   42 ++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 7589b2a..91cb43c 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -39,6 +39,11 @@ struct ath_regulatory {
 	struct reg_dmn_pair_mapping *regpair;
 };
 
+struct ath_ops {
+	unsigned int (*read)(void *, u32 reg_offset);
+        void (*write)(void *, u32 reg_offset, u32 val);
+};
+
 struct ath_common {
 	u16 cachelsz;
 	u16 curaid;
@@ -46,6 +51,7 @@ struct ath_common {
 	u8 curbssid[ETH_ALEN];
 	u8 bssidmask[ETH_ALEN];
 	struct ath_regulatory regulatory;
+	struct ath_ops *ops;
 };
 
 struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 0962505..affc3e5 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -718,8 +718,5 @@ bool ath9k_wiphy_scanning(struct ath_softc *sc);
 void ath9k_wiphy_work(struct work_struct *work);
 bool ath9k_all_wiphys_idle(struct ath_softc *sc);
 
-void ath9k_iowrite32(struct ath_hw *ah, u32 reg_offset, u32 val);
-unsigned int ath9k_ioread32(struct ath_hw *ah, u32 reg_offset);
-
 int ath_tx_get_qnum(struct ath_softc *sc, int qtype, int haltype);
 #endif /* ATH9K_H */
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 5436244..dabff52 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -81,38 +81,6 @@ static u32 ath9k_hw_mac_to_clks(struct ath_hw *ah, u32 usecs)
 		return ath9k_hw_mac_clks(ah, usecs);
 }
 
-/*
- * Read and write, they both share the same lock. We do this to serialize
- * reads and writes on Atheros 802.11n PCI devices only. This is required
- * as the FIFO on these devices can only accept sanely 2 requests. After
- * that the device goes bananas. Serializing the reads/writes prevents this
- * from happening.
- */
-
-void ath9k_iowrite32(struct ath_hw *ah, u32 reg_offset, u32 val)
-{
-	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
-		unsigned long flags;
-		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
-		iowrite32(val, ah->ah_sc->mem + reg_offset);
-		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
-	} else
-		iowrite32(val, ah->ah_sc->mem + reg_offset);
-}
-
-unsigned int ath9k_ioread32(struct ath_hw *ah, u32 reg_offset)
-{
-	u32 val;
-	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
-		unsigned long flags;
-		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
-		val = ioread32(ah->ah_sc->mem + reg_offset);
-		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
-	} else
-		val = ioread32(ah->ah_sc->mem + reg_offset);
-	return val;
-}
-
 bool ath9k_hw_wait(struct ath_hw *ah, u32 reg, u32 mask, u32 val, u32 timeout)
 {
 	int i;
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 84deae4..19b1da8 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -51,8 +51,8 @@
 #define AT9285_COEX3WIRE_DA_SUBSYSID	0x30ab
 
 /* Register read/write primitives */
-#define REG_WRITE(_ah, _reg, _val) ath9k_iowrite32((_ah), (_reg), (_val))
-#define REG_READ(_ah, _reg) ath9k_ioread32((_ah), (_reg))
+#define REG_WRITE(_ah, _reg, _val) _ah->common.ops->write((_ah), (_reg), (_val))
+#define REG_READ(_ah, _reg) _ah->common.ops->read((_ah), (_reg))
 
 #define SM(_v, _f)  (((_v) << _f##_S) & _f)
 #define MS(_v, _f)  (((_v) & _f) >> _f##_S)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b530e47..5f8a18a 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1494,6 +1494,47 @@ static int ath_init_btcoex_timer(struct ath_softc *sc)
 }
 
 /*
+ * Read and write, they both share the same lock. We do this to serialize
+ * reads and writes on Atheros 802.11n PCI devices only. This is required
+ * as the FIFO on these devices can only accept sanely 2 requests. After
+ * that the device goes bananas. Serializing the reads/writes prevents this
+ * from happening.
+ */
+
+static void ath9k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	struct ath_hw *ah = (struct ath_hw *) hw_priv;
+
+	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+		unsigned long flags;
+		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+		iowrite32(val, ah->ah_sc->mem + reg_offset);
+		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+	} else
+		iowrite32(val, ah->ah_sc->mem + reg_offset);
+}
+
+static unsigned int ath9k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	struct ath_hw *ah = (struct ath_hw *) hw_priv;
+	u32 val;
+
+	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+		unsigned long flags;
+		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+		val = ioread32(ah->ah_sc->mem + reg_offset);
+		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+	} else
+		val = ioread32(ah->ah_sc->mem + reg_offset);
+	return val;
+}
+
+static struct ath_ops ath9k_common_ops = {
+	.read = ath9k_ioread32,
+	.write = ath9k_iowrite32,
+};
+
+/*
  * Initialize and fill ath_softc, ath_sofct is the
  * "Software Carrier" struct. Historically it has existed
  * to allow the separation between hardware specific
@@ -1532,6 +1573,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid)
 	sc->sc_ah = ah;
 
 	common = ath9k_hw_common(ah);
+	common->ops = &ath9k_common_ops;
 
 	/*
 	 * Cache line size is used to size and align various
-- 
1.6.3.3

[ath9k-devel] [PATCH 1/4] atheros/ath9k: add common read/write ops and port ath9k to use it

[Luis R. Rodriguez]

In an effort to make hw code driver core agnostic read
and write operations are defined on the ath_common structure.
This patch adds that and makes ath9k use it. This allows
drivers like ath9k_htc to define its own read/write ops and
still rely on the same hw code. This also paves the way for
sharing code between ath9k/ath5k/ath9k_htc.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath.h         |    6 ++++
 drivers/net/wireless/ath/ath9k/ath9k.h |    3 --
 drivers/net/wireless/ath/ath9k/hw.c    |   32 ------------------------
 drivers/net/wireless/ath/ath9k/hw.h    |    4 +-
 drivers/net/wireless/ath/ath9k/main.c  |   42 ++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 7589b2a..91cb43c 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -39,6 +39,11 @@ struct ath_regulatory {
 	struct reg_dmn_pair_mapping *regpair;
 };
 
+struct ath_ops {
+	unsigned int (*read)(void *, u32 reg_offset);
+        void (*write)(void *, u32 reg_offset, u32 val);
+};
+
 struct ath_common {
 	u16 cachelsz;
 	u16 curaid;
@@ -46,6 +51,7 @@ struct ath_common {
 	u8 curbssid[ETH_ALEN];
 	u8 bssidmask[ETH_ALEN];
 	struct ath_regulatory regulatory;
+	struct ath_ops *ops;
 };
 
 struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 0962505..affc3e5 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -718,8 +718,5 @@ bool ath9k_wiphy_scanning(struct ath_softc *sc);
 void ath9k_wiphy_work(struct work_struct *work);
 bool ath9k_all_wiphys_idle(struct ath_softc *sc);
 
-void ath9k_iowrite32(struct ath_hw *ah, u32 reg_offset, u32 val);
-unsigned int ath9k_ioread32(struct ath_hw *ah, u32 reg_offset);
-
 int ath_tx_get_qnum(struct ath_softc *sc, int qtype, int haltype);
 #endif /* ATH9K_H */
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 5436244..dabff52 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -81,38 +81,6 @@ static u32 ath9k_hw_mac_to_clks(struct ath_hw *ah, u32 usecs)
 		return ath9k_hw_mac_clks(ah, usecs);
 }
 
-/*
- * Read and write, they both share the same lock. We do this to serialize
- * reads and writes on Atheros 802.11n PCI devices only. This is required
- * as the FIFO on these devices can only accept sanely 2 requests. After
- * that the device goes bananas. Serializing the reads/writes prevents this
- * from happening.
- */
-
-void ath9k_iowrite32(struct ath_hw *ah, u32 reg_offset, u32 val)
-{
-	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
-		unsigned long flags;
-		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
-		iowrite32(val, ah->ah_sc->mem + reg_offset);
-		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
-	} else
-		iowrite32(val, ah->ah_sc->mem + reg_offset);
-}
-
-unsigned int ath9k_ioread32(struct ath_hw *ah, u32 reg_offset)
-{
-	u32 val;
-	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
-		unsigned long flags;
-		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
-		val = ioread32(ah->ah_sc->mem + reg_offset);
-		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
-	} else
-		val = ioread32(ah->ah_sc->mem + reg_offset);
-	return val;
-}
-
 bool ath9k_hw_wait(struct ath_hw *ah, u32 reg, u32 mask, u32 val, u32 timeout)
 {
 	int i;
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 84deae4..19b1da8 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -51,8 +51,8 @@
 #define AT9285_COEX3WIRE_DA_SUBSYSID	0x30ab
 
 /* Register read/write primitives */
-#define REG_WRITE(_ah, _reg, _val) ath9k_iowrite32((_ah), (_reg), (_val))
-#define REG_READ(_ah, _reg) ath9k_ioread32((_ah), (_reg))
+#define REG_WRITE(_ah, _reg, _val) _ah->common.ops->write((_ah), (_reg), (_val))
+#define REG_READ(_ah, _reg) _ah->common.ops->read((_ah), (_reg))
 
 #define SM(_v, _f)  (((_v) << _f##_S) & _f)
 #define MS(_v, _f)  (((_v) & _f) >> _f##_S)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b530e47..5f8a18a 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1494,6 +1494,47 @@ static int ath_init_btcoex_timer(struct ath_softc *sc)
 }
 
 /*
+ * Read and write, they both share the same lock. We do this to serialize
+ * reads and writes on Atheros 802.11n PCI devices only. This is required
+ * as the FIFO on these devices can only accept sanely 2 requests. After
+ * that the device goes bananas. Serializing the reads/writes prevents this
+ * from happening.
+ */
+
+static void ath9k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	struct ath_hw *ah = (struct ath_hw *) hw_priv;
+
+	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+		unsigned long flags;
+		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+		iowrite32(val, ah->ah_sc->mem + reg_offset);
+		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+	} else
+		iowrite32(val, ah->ah_sc->mem + reg_offset);
+}
+
+static unsigned int ath9k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	struct ath_hw *ah = (struct ath_hw *) hw_priv;
+	u32 val;
+
+	if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+		unsigned long flags;
+		spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+		val = ioread32(ah->ah_sc->mem + reg_offset);
+		spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+	} else
+		val = ioread32(ah->ah_sc->mem + reg_offset);
+	return val;
+}
+
+static struct ath_ops ath9k_common_ops = {
+	.read = ath9k_ioread32,
+	.write = ath9k_iowrite32,
+};
+
+/*
  * Initialize and fill ath_softc, ath_sofct is the
  * "Software Carrier" struct. Historically it has existed
  * to allow the separation between hardware specific
@@ -1532,6 +1573,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid)
 	sc->sc_ah = ah;
 
 	common = ath9k_hw_common(ah);
+	common->ops = &ath9k_common_ops;
 
 	/*
 	 * Cache line size is used to size and align various
-- 
1.6.3.3

[PATCH 2/4] ath5k: allocate ath5k_hw prior to initializing hw

[Luis R. Rodriguez]

We can propagate better errors upon failed hw initialization,
and set up the ath_common structure for attach purposes. This
will become important once we start using the ath_common
for read/write ops.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/ath5k.h  |    2 +-
 drivers/net/wireless/ath/ath5k/attach.c |   23 ++++-------------------
 drivers/net/wireless/ath/ath5k/base.c   |   20 ++++++++++++++++----
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index fee16fd..29ce868 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1147,7 +1147,7 @@ struct ath5k_hw {
  */
 
 /* Attach/Detach Functions */
-extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc);
+extern int ath5k_hw_attach(struct ath5k_softc *sc);
 extern void ath5k_hw_detach(struct ath5k_hw *ah);
 
 /* LED functions */
diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index 123612a..c0840ab 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -101,28 +101,15 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
  * -ENODEV if the device is not supported or prints an error msg if something
  * else went wrong.
  */
-struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
+int ath5k_hw_attach(struct ath5k_softc *sc)
 {
-	struct ath5k_hw *ah;
+	struct ath5k_hw *ah = sc->ah;
 	struct ath_common *common;
 	struct pci_dev *pdev = sc->pdev;
 	struct ath5k_eeprom_info *ee;
 	int ret;
 	u32 srev;
 
-	/*If we passed the test malloc a ath5k_hw struct*/
-	ah = kzalloc(sizeof(struct ath5k_hw), GFP_KERNEL);
-	if (ah == NULL) {
-		ret = -ENOMEM;
-		ATH5K_ERR(sc, "out of memory\n");
-		goto err;
-	}
-
-	ah->ah_sc = sc;
-	ah->ah_sc->ah = ah;
-	ah->ah_iobase = sc->iobase;
-	common = ath5k_hw_common(ah);
-
 	/*
 	 * HW information
 	 */
@@ -347,11 +334,10 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
 	/* turn on HW LEDs */
 	ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);
 
-	return ah;
+	return 0;
 err_free:
 	kfree(ah);
-err:
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /**
@@ -371,5 +357,4 @@ void ath5k_hw_detach(struct ath5k_hw *ah)
 	ath5k_eeprom_detach(ah);
 
 	/* assume interrupts are down */
-	kfree(ah);
 }
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 06fc893..3cb0752 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -565,16 +565,25 @@ ath5k_pci_probe(struct pci_dev *pdev,
 		goto err_free;
 	}
 
-	/* Initialize device */
-	sc->ah = ath5k_hw_attach(sc);
-	if (IS_ERR(sc->ah)) {
-		ret = PTR_ERR(sc->ah);
+	/*If we passed the test malloc a ath5k_hw struct*/
+	sc->ah = kzalloc(sizeof(struct ath5k_hw), GFP_KERNEL);
+	if (!sc->ah) {
+		ret = -ENOMEM;
+		ATH5K_ERR(sc, "out of memory\n");
 		goto err_irq;
 	}
 
+	sc->ah->ah_sc = sc;
+	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
 	common->cachelsz = csz << 2; /* convert to bytes */
 
+	/* Initialize device */
+	ret = ath5k_hw_attach(sc);
+	if (ret) {
+		goto err_free_ah;
+	}
+
 	/* set up multi-rate retry capabilities */
 	if (sc->ah->ah_version == AR5K_AR5212) {
 		hw->max_rates = 4;
@@ -643,6 +652,8 @@ err_ah:
 	ath5k_hw_detach(sc->ah);
 err_irq:
 	free_irq(pdev->irq, sc);
+err_free_ah:
+	kfree(sc->ah);
 err_free:
 	ieee80211_free_hw(hw);
 err_map:
@@ -664,6 +675,7 @@ ath5k_pci_remove(struct pci_dev *pdev)
 	ath5k_debug_finish_device(sc);
 	ath5k_detach(pdev, hw);
 	ath5k_hw_detach(sc->ah);
+	kfree(sc->ah);
 	free_irq(pdev->irq, sc);
 	pci_iounmap(pdev, sc->iobase);
 	pci_release_region(pdev, 0);
-- 
1.6.3.3

[ath9k-devel] [PATCH 2/4] ath5k: allocate ath5k_hw prior to initializing hw

[Luis R. Rodriguez]

We can propagate better errors upon failed hw initialization,
and set up the ath_common structure for attach purposes. This
will become important once we start using the ath_common
for read/write ops.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/ath5k.h  |    2 +-
 drivers/net/wireless/ath/ath5k/attach.c |   23 ++++-------------------
 drivers/net/wireless/ath/ath5k/base.c   |   20 ++++++++++++++++----
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index fee16fd..29ce868 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1147,7 +1147,7 @@ struct ath5k_hw {
  */
 
 /* Attach/Detach Functions */
-extern struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc);
+extern int ath5k_hw_attach(struct ath5k_softc *sc);
 extern void ath5k_hw_detach(struct ath5k_hw *ah);
 
 /* LED functions */
diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index 123612a..c0840ab 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -101,28 +101,15 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
  * -ENODEV if the device is not supported or prints an error msg if something
  * else went wrong.
  */
-struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
+int ath5k_hw_attach(struct ath5k_softc *sc)
 {
-	struct ath5k_hw *ah;
+	struct ath5k_hw *ah = sc->ah;
 	struct ath_common *common;
 	struct pci_dev *pdev = sc->pdev;
 	struct ath5k_eeprom_info *ee;
 	int ret;
 	u32 srev;
 
-	/*If we passed the test malloc a ath5k_hw struct*/
-	ah = kzalloc(sizeof(struct ath5k_hw), GFP_KERNEL);
-	if (ah == NULL) {
-		ret = -ENOMEM;
-		ATH5K_ERR(sc, "out of memory\n");
-		goto err;
-	}
-
-	ah->ah_sc = sc;
-	ah->ah_sc->ah = ah;
-	ah->ah_iobase = sc->iobase;
-	common = ath5k_hw_common(ah);
-
 	/*
 	 * HW information
 	 */
@@ -347,11 +334,10 @@ struct ath5k_hw *ath5k_hw_attach(struct ath5k_softc *sc)
 	/* turn on HW LEDs */
 	ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);
 
-	return ah;
+	return 0;
 err_free:
 	kfree(ah);
-err:
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /**
@@ -371,5 +357,4 @@ void ath5k_hw_detach(struct ath5k_hw *ah)
 	ath5k_eeprom_detach(ah);
 
 	/* assume interrupts are down */
-	kfree(ah);
 }
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 06fc893..3cb0752 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -565,16 +565,25 @@ ath5k_pci_probe(struct pci_dev *pdev,
 		goto err_free;
 	}
 
-	/* Initialize device */
-	sc->ah = ath5k_hw_attach(sc);
-	if (IS_ERR(sc->ah)) {
-		ret = PTR_ERR(sc->ah);
+	/*If we passed the test malloc a ath5k_hw struct*/
+	sc->ah = kzalloc(sizeof(struct ath5k_hw), GFP_KERNEL);
+	if (!sc->ah) {
+		ret = -ENOMEM;
+		ATH5K_ERR(sc, "out of memory\n");
 		goto err_irq;
 	}
 
+	sc->ah->ah_sc = sc;
+	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
 	common->cachelsz = csz << 2; /* convert to bytes */
 
+	/* Initialize device */
+	ret = ath5k_hw_attach(sc);
+	if (ret) {
+		goto err_free_ah;
+	}
+
 	/* set up multi-rate retry capabilities */
 	if (sc->ah->ah_version == AR5K_AR5212) {
 		hw->max_rates = 4;
@@ -643,6 +652,8 @@ err_ah:
 	ath5k_hw_detach(sc->ah);
 err_irq:
 	free_irq(pdev->irq, sc);
+err_free_ah:
+	kfree(sc->ah);
 err_free:
 	ieee80211_free_hw(hw);
 err_map:
@@ -664,6 +675,7 @@ ath5k_pci_remove(struct pci_dev *pdev)
 	ath5k_debug_finish_device(sc);
 	ath5k_detach(pdev, hw);
 	ath5k_hw_detach(sc->ah);
+	kfree(sc->ah);
 	free_irq(pdev->irq, sc);
 	pci_iounmap(pdev, sc->iobase);
 	pci_release_region(pdev, 0);
-- 
1.6.3.3

[PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

All read/write ops now go through the common ops.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/ath5k.h |   20 ++++++++++++--------
 drivers/net/wireless/ath/ath5k/base.c  |   17 +++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.h  |   11 -----------
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 29ce868..997101b 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1315,20 +1315,24 @@ static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
 	return turbo ? (clock / 80) : (clock / 40);
 }
 
-/*
- * Read from a register
- */
+static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
+{
+        return &ah->common;
+}
+
+static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
+{
+        return &(ath5k_hw_common(ah)->regulatory);
+}
+
 static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
 {
-	return ioread32(ah->ah_iobase + reg);
+	return ath5k_hw_common(ah)->ops->read(ah, reg);
 }
 
-/*
- * Write to a register
- */
 static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
 {
-	iowrite32(val, ah->ah_iobase + reg);
+	ath5k_hw_common(ah)->ops->write(ah, reg, val);
 }
 
 #if defined(_ATH5K_RESET) || defined(_ATH5K_PHY)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 3cb0752..535ea72 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -437,6 +437,22 @@ ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
 
 	return name;
 }
+static unsigned int ath5k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
+	return ioread32(ah->ah_iobase + reg_offset);
+}
+
+static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
+	iowrite32(val, ah->ah_iobase + reg_offset);
+}
+
+static struct ath_ops ath5k_common_ops = {
+	.read = ath5k_ioread32,
+	.write = ath5k_iowrite32,
+};
 
 static int __devinit
 ath5k_pci_probe(struct pci_dev *pdev,
@@ -576,6 +592,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	sc->ah->ah_sc = sc;
 	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
+	common->ops = &ath5k_common_ops;
 	common->cachelsz = csz << 2; /* convert to bytes */
 
 	/* Initialize device */
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 005d25f..b14ba07 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -201,15 +201,4 @@ struct ath5k_softc {
 #define ath5k_hw_hasveol(_ah) \
 	(ath5k_hw_get_capability(_ah, AR5K_CAP_VEOL, 0, NULL) == 0)
 
-static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
-{
-	return &ah->common;
-}
-
-static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
-{
-	return &(ath5k_hw_common(ah)->regulatory);
-
-}
-
 #endif
-- 
1.6.3.3

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

All read/write ops now go through the common ops.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/ath5k.h |   20 ++++++++++++--------
 drivers/net/wireless/ath/ath5k/base.c  |   17 +++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.h  |   11 -----------
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 29ce868..997101b 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1315,20 +1315,24 @@ static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
 	return turbo ? (clock / 80) : (clock / 40);
 }
 
-/*
- * Read from a register
- */
+static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
+{
+        return &ah->common;
+}
+
+static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
+{
+        return &(ath5k_hw_common(ah)->regulatory);
+}
+
 static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
 {
-	return ioread32(ah->ah_iobase + reg);
+	return ath5k_hw_common(ah)->ops->read(ah, reg);
 }
 
-/*
- * Write to a register
- */
 static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
 {
-	iowrite32(val, ah->ah_iobase + reg);
+	ath5k_hw_common(ah)->ops->write(ah, reg, val);
 }
 
 #if defined(_ATH5K_RESET) || defined(_ATH5K_PHY)
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 3cb0752..535ea72 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -437,6 +437,22 @@ ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
 
 	return name;
 }
+static unsigned int ath5k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
+	return ioread32(ah->ah_iobase + reg_offset);
+}
+
+static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
+	iowrite32(val, ah->ah_iobase + reg_offset);
+}
+
+static struct ath_ops ath5k_common_ops = {
+	.read = ath5k_ioread32,
+	.write = ath5k_iowrite32,
+};
 
 static int __devinit
 ath5k_pci_probe(struct pci_dev *pdev,
@@ -576,6 +592,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	sc->ah->ah_sc = sc;
 	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
+	common->ops = &ath5k_common_ops;
 	common->cachelsz = csz << 2; /* convert to bytes */
 
 	/* Initialize device */
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 005d25f..b14ba07 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -201,15 +201,4 @@ struct ath5k_softc {
 #define ath5k_hw_hasveol(_ah) \
 	(ath5k_hw_get_capability(_ah, AR5K_CAP_VEOL, 0, NULL) == 0)
 
-static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
-{
-	return &ah->common;
-}
-
-static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
-{
-	return &(ath5k_hw_common(ah)->regulatory);
-
-}
-
 #endif
-- 
1.6.3.3

[PATCH 4/4] atheros: define shared bssidmask setting

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/Makefile        |    5 +-
 drivers/net/wireless/ath/ath.h           |    3 +
 drivers/net/wireless/ath/ath5k/ath5k.h   |    2 +-
 drivers/net/wireless/ath/ath5k/attach.c  |    2 +-
 drivers/net/wireless/ath/ath5k/base.c    |    1 +
 drivers/net/wireless/ath/ath5k/pcu.c     |  113 +--------------------------
 drivers/net/wireless/ath/ath9k/hw.c      |   10 +--
 drivers/net/wireless/ath/ath9k/main.c    |    1 +
 drivers/net/wireless/ath/ath9k/recv.c    |    2 +-
 drivers/net/wireless/ath/ath9k/virtual.c |    2 +-
 drivers/net/wireless/ath/hw.c            |  126 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/reg.h           |    2 +
 12 files changed, 145 insertions(+), 124 deletions(-)
 create mode 100644 drivers/net/wireless/ath/hw.c
 create mode 100644 drivers/net/wireless/ath/reg.h

diff --git a/drivers/net/wireless/ath/Makefile b/drivers/net/wireless/ath/Makefile
index 4bb0132..6ebf214 100644
--- a/drivers/net/wireless/ath/Makefile
+++ b/drivers/net/wireless/ath/Makefile
@@ -3,4 +3,7 @@ obj-$(CONFIG_ATH9K)		+= ath9k/
 obj-$(CONFIG_AR9170_USB)        += ar9170/
 
 obj-$(CONFIG_ATH_COMMON)	+= ath.o
-ath-objs 		:= main.o regd.o
+
+ath-objs :=	main.o \
+		regd.o \
+		hw.o
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 91cb43c..1d022a3 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -45,6 +45,7 @@ struct ath_ops {
 };
 
 struct ath_common {
+	void *ah;
 	u16 cachelsz;
 	u16 curaid;
 	u8 macaddr[ETH_ALEN];
@@ -58,4 +59,6 @@ struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
 				u32 len,
 				gfp_t gfp_mask);
 
+void ath_hw_setbssidmask(struct ath_common *common);
+
 #endif /* ATH_H */
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 997101b..cc116c7 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1192,7 +1192,7 @@ extern int ath5k_hw_set_opmode(struct ath5k_hw *ah);
 /* BSSID Functions */
 extern int ath5k_hw_set_lladdr(struct ath5k_hw *ah, const u8 *mac);
 extern void ath5k_hw_set_associd(struct ath5k_hw *ah, const u8 *bssid, u16 assoc_id);
-extern int ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask);
+extern void ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask);
 /* Receive start/stop functions */
 extern void ath5k_hw_start_rx_pcu(struct ath5k_hw *ah);
 extern void ath5k_hw_stop_rx_pcu(struct ath5k_hw *ah);
diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index c0840ab..e230de8 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -104,7 +104,7 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
 int ath5k_hw_attach(struct ath5k_softc *sc)
 {
 	struct ath5k_hw *ah = sc->ah;
-	struct ath_common *common;
+	struct ath_common *common = ath5k_hw_common(ah);
 	struct pci_dev *pdev = sc->pdev;
 	struct ath5k_eeprom_info *ee;
 	int ret;
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 535ea72..02e1ce6 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -593,6 +593,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
 	common->ops = &ath5k_common_ops;
+	common->ah = sc->ah;
 	common->cachelsz = csz << 2; /* convert to bytes */
 
 	/* Initialize device */
diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
index f03c06d..6329148 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -316,125 +316,18 @@ void ath5k_hw_set_associd(struct ath5k_hw *ah, const u8 *bssid, u16 assoc_id)
 	ath5k_hw_enable_pspoll(ah, NULL, 0);
 }
 
-/**
- * ath5k_hw_set_bssid_mask - filter out bssids we listen
- *
- * @ah: the &struct ath5k_hw
- * @mask: the bssid_mask, a u8 array of size ETH_ALEN
- *
- * BSSID masking is a method used by AR5212 and newer hardware to inform PCU
- * which bits of the interface's MAC address should be looked at when trying
- * to decide which packets to ACK. In station mode and AP mode with a single
- * BSS every bit matters since we lock to only one BSS. In AP mode with
- * multiple BSSes (virtual interfaces) not every bit matters because hw must
- * accept frames for all BSSes and so we tweak some bits of our mac address
- * in order to have multiple BSSes.
- *
- * NOTE: This is a simple filter and does *not* filter out all
- * relevant frames. Some frames that are not for us might get ACKed from us
- * by PCU because they just match the mask.
- *
- * When handling multiple BSSes you can get the BSSID mask by computing the
- * set of  ~ ( MAC XOR BSSID ) for all bssids we handle.
- *
- * When you do this you are essentially computing the common bits of all your
- * BSSes. Later it is assumed the harware will "and" (&) the BSSID mask with
- * the MAC address to obtain the relevant bits and compare the result with
- * (frame's BSSID & mask) to see if they match.
- */
-/*
- * Simple example: on your card you have have two BSSes you have created with
- * BSSID-01 and BSSID-02. Lets assume BSSID-01 will not use the MAC address.
- * There is another BSSID-03 but you are not part of it. For simplicity's sake,
- * assuming only 4 bits for a mac address and for BSSIDs you can then have:
- *
- *                  \
- * MAC:        0001 |
- * BSSID-01:   0100 | --> Belongs to us
- * BSSID-02:   1001 |
- *                  /
- * -------------------
- * BSSID-03:   0110  | --> External
- * -------------------
- *
- * Our bssid_mask would then be:
- *
- *             On loop iteration for BSSID-01:
- *             ~(0001 ^ 0100)  -> ~(0101)
- *                             ->   1010
- *             bssid_mask      =    1010
- *
- *             On loop iteration for BSSID-02:
- *             bssid_mask &= ~(0001   ^   1001)
- *             bssid_mask =   (1010)  & ~(0001 ^ 1001)
- *             bssid_mask =   (1010)  & ~(1001)
- *             bssid_mask =   (1010)  &  (0110)
- *             bssid_mask =   0010
- *
- * A bssid_mask of 0010 means "only pay attention to the second least
- * significant bit". This is because its the only bit common
- * amongst the MAC and all BSSIDs we support. To findout what the real
- * common bit is we can simply "&" the bssid_mask now with any BSSID we have
- * or our MAC address (we assume the hardware uses the MAC address).
- *
- * Now, suppose there's an incoming frame for BSSID-03:
- *
- * IFRAME-01:  0110
- *
- * An easy eye-inspeciton of this already should tell you that this frame
- * will not pass our check. This is beacuse the bssid_mask tells the
- * hardware to only look at the second least significant bit and the
- * common bit amongst the MAC and BSSIDs is 0, this frame has the 2nd LSB
- * as 1, which does not match 0.
- *
- * So with IFRAME-01 we *assume* the hardware will do:
- *
- *     allow = (IFRAME-01 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
- *  --> allow = (0110 & 0010) == (0010 & 0001) ? 1 : 0;
- *  --> allow = (0010) == 0000 ? 1 : 0;
- *  --> allow = 0
- *
- *  Lets now test a frame that should work:
- *
- * IFRAME-02:  0001 (we should allow)
- *
- *     allow = (0001 & 1010) == 1010
- *
- *     allow = (IFRAME-02 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
- *  --> allow = (0001 & 0010) ==  (0010 & 0001) ? 1 :0;
- *  --> allow = (0010) == (0010)
- *  --> allow = 1
- *
- * Other examples:
- *
- * IFRAME-03:  0100 --> allowed
- * IFRAME-04:  1001 --> allowed
- * IFRAME-05:  1101 --> allowed but its not for us!!!
- *
- */
-int ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask)
+void ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask)
 {
 	struct ath_common *common = ath5k_hw_common(ah);
-	u32 low_id, high_id;
 	ATH5K_TRACE(ah->ah_sc);
 
 	/* Cache bssid mask so that we can restore it
 	 * on reset */
 	memcpy(common->bssidmask, mask, ETH_ALEN);
-	if (ah->ah_version == AR5K_AR5212) {
-		low_id = get_unaligned_le32(mask);
-		high_id = get_unaligned_le16(mask + 4);
-
-		ath5k_hw_reg_write(ah, low_id, AR5K_BSS_IDM0);
-		ath5k_hw_reg_write(ah, high_id, AR5K_BSS_IDM1);
-
-		return 0;
-	}
-
-	return -EIO;
+	if (ah->ah_version == AR5K_AR5212)
+		ath_hw_setbssidmask(common);
 }
 
-
 /************\
 * RX Control *
 \************/
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index dabff52..80bfa18 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -2423,7 +2423,7 @@ int ath9k_hw_reset(struct ath_hw *ah, struct ath9k_channel *chan,
 		  | ah->sta_id1_defaults);
 	ath9k_hw_set_operating_mode(ah, ah->opmode);
 
-	ath9k_hw_setbssidmask(ah);
+	ath_hw_setbssidmask(common);
 
 	REG_WRITE(ah, AR_DEF_ANTENNA, saveDefAntenna);
 
@@ -3950,14 +3950,6 @@ void ath9k_hw_setmcastfilter(struct ath_hw *ah, u32 filter0, u32 filter1)
 	REG_WRITE(ah, AR_MCAST_FIL1, filter1);
 }
 
-void ath9k_hw_setbssidmask(struct ath_hw *ah)
-{
-	struct ath_common *common = ath9k_hw_common(ah);
-
-	REG_WRITE(ah, AR_BSSMSKL, get_unaligned_le32(common->bssidmask));
-	REG_WRITE(ah, AR_BSSMSKU, get_unaligned_le16(common->bssidmask + 4));
-}
-
 void ath9k_hw_write_associd(struct ath_hw *ah)
 {
 	struct ath_common *common = ath9k_hw_common(ah);
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 5f8a18a..9a8c9ce 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1574,6 +1574,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid)
 
 	common = ath9k_hw_common(ah);
 	common->ops = &ath9k_common_ops;
+	common->ah = ah;
 
 	/*
 	 * Cache line size is used to size and align various
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 97a5efe..fb635a0 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -282,7 +282,7 @@ static void ath_opmode_init(struct ath_softc *sc)
 
 	/* configure bssid mask */
 	if (ah->caps.hw_caps & ATH9K_HW_CAP_BSSIDMASK)
-		ath9k_hw_setbssidmask(ah);
+		ath_hw_setbssidmask(common);
 
 	/* configure operational mode */
 	ath9k_hw_setopmode(ah);
diff --git a/drivers/net/wireless/ath/ath9k/virtual.c b/drivers/net/wireless/ath/ath9k/virtual.c
index 7b763b6..bc7d173 100644
--- a/drivers/net/wireless/ath/ath9k/virtual.c
+++ b/drivers/net/wireless/ath/ath9k/virtual.c
@@ -94,7 +94,7 @@ void ath9k_set_bssid_mask(struct ieee80211_hw *hw)
 	common->bssidmask[4] = ~mask[4];
 	common->bssidmask[5] = ~mask[5];
 
-	ath9k_hw_setbssidmask(sc->sc_ah);
+	ath_hw_setbssidmask(common);
 }
 
 int ath9k_wiphy_add(struct ath_softc *sc)
diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
new file mode 100644
index 0000000..5f7598e
--- /dev/null
+++ b/drivers/net/wireless/ath/hw.c
@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) 2009 Atheros Communications Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <asm/unaligned.h>
+
+#include "ath.h"
+#include "reg.h"
+
+#define REG_READ	common->ops->read
+#define REG_WRITE	common->ops->write
+
+/**
+ * ath_hw_set_bssid_mask - filter out bssids we listen
+ *
+ * @common: the ath_common struct for the device.
+ *
+ * BSSID masking is a method used by AR5212 and newer hardware to inform PCU
+ * which bits of the interface's MAC address should be looked at when trying
+ * to decide which packets to ACK. In station mode and AP mode with a single
+ * BSS every bit matters since we lock to only one BSS. In AP mode with
+ * multiple BSSes (virtual interfaces) not every bit matters because hw must
+ * accept frames for all BSSes and so we tweak some bits of our mac address
+ * in order to have multiple BSSes.
+ *
+ * NOTE: This is a simple filter and does *not* filter out all
+ * relevant frames. Some frames that are not for us might get ACKed from us
+ * by PCU because they just match the mask.
+ *
+ * When handling multiple BSSes you can get the BSSID mask by computing the
+ * set of  ~ ( MAC XOR BSSID ) for all bssids we handle.
+ *
+ * When you do this you are essentially computing the common bits of all your
+ * BSSes. Later it is assumed the harware will "and" (&) the BSSID mask with
+ * the MAC address to obtain the relevant bits and compare the result with
+ * (frame's BSSID & mask) to see if they match.
+ *
+ * Simple example: on your card you have have two BSSes you have created with
+ * BSSID-01 and BSSID-02. Lets assume BSSID-01 will not use the MAC address.
+ * There is another BSSID-03 but you are not part of it. For simplicity's sake,
+ * assuming only 4 bits for a mac address and for BSSIDs you can then have:
+ *
+ *                  \
+ * MAC:        0001 |
+ * BSSID-01:   0100 | --> Belongs to us
+ * BSSID-02:   1001 |
+ *                  /
+ * -------------------
+ * BSSID-03:   0110  | --> External
+ * -------------------
+ *
+ * Our bssid_mask would then be:
+ *
+ *             On loop iteration for BSSID-01:
+ *             ~(0001 ^ 0100)  -> ~(0101)
+ *                             ->   1010
+ *             bssid_mask      =    1010
+ *
+ *             On loop iteration for BSSID-02:
+ *             bssid_mask &= ~(0001   ^   1001)
+ *             bssid_mask =   (1010)  & ~(0001 ^ 1001)
+ *             bssid_mask =   (1010)  & ~(1001)
+ *             bssid_mask =   (1010)  &  (0110)
+ *             bssid_mask =   0010
+ *
+ * A bssid_mask of 0010 means "only pay attention to the second least
+ * significant bit". This is because its the only bit common
+ * amongst the MAC and all BSSIDs we support. To findout what the real
+ * common bit is we can simply "&" the bssid_mask now with any BSSID we have
+ * or our MAC address (we assume the hardware uses the MAC address).
+ *
+ * Now, suppose there's an incoming frame for BSSID-03:
+ *
+ * IFRAME-01:  0110
+ *
+ * An easy eye-inspeciton of this already should tell you that this frame
+ * will not pass our check. This is beacuse the bssid_mask tells the
+ * hardware to only look at the second least significant bit and the
+ * common bit amongst the MAC and BSSIDs is 0, this frame has the 2nd LSB
+ * as 1, which does not match 0.
+ *
+ * So with IFRAME-01 we *assume* the hardware will do:
+ *
+ *     allow = (IFRAME-01 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
+ *  --> allow = (0110 & 0010) == (0010 & 0001) ? 1 : 0;
+ *  --> allow = (0010) == 0000 ? 1 : 0;
+ *  --> allow = 0
+ *
+ *  Lets now test a frame that should work:
+ *
+ * IFRAME-02:  0001 (we should allow)
+ *
+ *     allow = (0001 & 1010) == 1010
+ *
+ *     allow = (IFRAME-02 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
+ *  --> allow = (0001 & 0010) ==  (0010 & 0001) ? 1 :0;
+ *  --> allow = (0010) == (0010)
+ *  --> allow = 1
+ *
+ * Other examples:
+ *
+ * IFRAME-03:  0100 --> allowed
+ * IFRAME-04:  1001 --> allowed
+ * IFRAME-05:  1101 --> allowed but its not for us!!!
+ *
+ */
+void ath_hw_setbssidmask(struct ath_common *common)
+{
+	void *ah = common->ah;
+
+	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);
diff --git a/drivers/net/wireless/ath/reg.h b/drivers/net/wireless/ath/reg.h
new file mode 100644
index 0000000..70453f6
--- /dev/null
+++ b/drivers/net/wireless/ath/reg.h
@@ -0,0 +1,2 @@
+#define AR_BSSMSKL		0x80e0
+#define AR_BSSMSKU		0x80e4
-- 
1.6.3.3

[ath9k-devel] [PATCH 4/4] atheros: define shared bssidmask setting

[Luis R. Rodriguez]

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/Makefile        |    5 +-
 drivers/net/wireless/ath/ath.h           |    3 +
 drivers/net/wireless/ath/ath5k/ath5k.h   |    2 +-
 drivers/net/wireless/ath/ath5k/attach.c  |    2 +-
 drivers/net/wireless/ath/ath5k/base.c    |    1 +
 drivers/net/wireless/ath/ath5k/pcu.c     |  113 +--------------------------
 drivers/net/wireless/ath/ath9k/hw.c      |   10 +--
 drivers/net/wireless/ath/ath9k/main.c    |    1 +
 drivers/net/wireless/ath/ath9k/recv.c    |    2 +-
 drivers/net/wireless/ath/ath9k/virtual.c |    2 +-
 drivers/net/wireless/ath/hw.c            |  126 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/reg.h           |    2 +
 12 files changed, 145 insertions(+), 124 deletions(-)
 create mode 100644 drivers/net/wireless/ath/hw.c
 create mode 100644 drivers/net/wireless/ath/reg.h

diff --git a/drivers/net/wireless/ath/Makefile b/drivers/net/wireless/ath/Makefile
index 4bb0132..6ebf214 100644
--- a/drivers/net/wireless/ath/Makefile
+++ b/drivers/net/wireless/ath/Makefile
@@ -3,4 +3,7 @@ obj-$(CONFIG_ATH9K)		+= ath9k/
 obj-$(CONFIG_AR9170_USB)        += ar9170/
 
 obj-$(CONFIG_ATH_COMMON)	+= ath.o
-ath-objs 		:= main.o regd.o
+
+ath-objs :=	main.o \
+		regd.o \
+		hw.o
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 91cb43c..1d022a3 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -45,6 +45,7 @@ struct ath_ops {
 };
 
 struct ath_common {
+	void *ah;
 	u16 cachelsz;
 	u16 curaid;
 	u8 macaddr[ETH_ALEN];
@@ -58,4 +59,6 @@ struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
 				u32 len,
 				gfp_t gfp_mask);
 
+void ath_hw_setbssidmask(struct ath_common *common);
+
 #endif /* ATH_H */
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 997101b..cc116c7 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -1192,7 +1192,7 @@ extern int ath5k_hw_set_opmode(struct ath5k_hw *ah);
 /* BSSID Functions */
 extern int ath5k_hw_set_lladdr(struct ath5k_hw *ah, const u8 *mac);
 extern void ath5k_hw_set_associd(struct ath5k_hw *ah, const u8 *bssid, u16 assoc_id);
-extern int ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask);
+extern void ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask);
 /* Receive start/stop functions */
 extern void ath5k_hw_start_rx_pcu(struct ath5k_hw *ah);
 extern void ath5k_hw_stop_rx_pcu(struct ath5k_hw *ah);
diff --git a/drivers/net/wireless/ath/ath5k/attach.c b/drivers/net/wireless/ath/ath5k/attach.c
index c0840ab..e230de8 100644
--- a/drivers/net/wireless/ath/ath5k/attach.c
+++ b/drivers/net/wireless/ath/ath5k/attach.c
@@ -104,7 +104,7 @@ static int ath5k_hw_post(struct ath5k_hw *ah)
 int ath5k_hw_attach(struct ath5k_softc *sc)
 {
 	struct ath5k_hw *ah = sc->ah;
-	struct ath_common *common;
+	struct ath_common *common = ath5k_hw_common(ah);
 	struct pci_dev *pdev = sc->pdev;
 	struct ath5k_eeprom_info *ee;
 	int ret;
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 535ea72..02e1ce6 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -593,6 +593,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	sc->ah->ah_iobase = sc->iobase;
 	common = ath5k_hw_common(sc->ah);
 	common->ops = &ath5k_common_ops;
+	common->ah = sc->ah;
 	common->cachelsz = csz << 2; /* convert to bytes */
 
 	/* Initialize device */
diff --git a/drivers/net/wireless/ath/ath5k/pcu.c b/drivers/net/wireless/ath/ath5k/pcu.c
index f03c06d..6329148 100644
--- a/drivers/net/wireless/ath/ath5k/pcu.c
+++ b/drivers/net/wireless/ath/ath5k/pcu.c
@@ -316,125 +316,18 @@ void ath5k_hw_set_associd(struct ath5k_hw *ah, const u8 *bssid, u16 assoc_id)
 	ath5k_hw_enable_pspoll(ah, NULL, 0);
 }
 
-/**
- * ath5k_hw_set_bssid_mask - filter out bssids we listen
- *
- * @ah: the &struct ath5k_hw
- * @mask: the bssid_mask, a u8 array of size ETH_ALEN
- *
- * BSSID masking is a method used by AR5212 and newer hardware to inform PCU
- * which bits of the interface's MAC address should be looked@when trying
- * to decide which packets to ACK. In station mode and AP mode with a single
- * BSS every bit matters since we lock to only one BSS. In AP mode with
- * multiple BSSes (virtual interfaces) not every bit matters because hw must
- * accept frames for all BSSes and so we tweak some bits of our mac address
- * in order to have multiple BSSes.
- *
- * NOTE: This is a simple filter and does *not* filter out all
- * relevant frames. Some frames that are not for us might get ACKed from us
- * by PCU because they just match the mask.
- *
- * When handling multiple BSSes you can get the BSSID mask by computing the
- * set of  ~ ( MAC XOR BSSID ) for all bssids we handle.
- *
- * When you do this you are essentially computing the common bits of all your
- * BSSes. Later it is assumed the harware will "and" (&) the BSSID mask with
- * the MAC address to obtain the relevant bits and compare the result with
- * (frame's BSSID & mask) to see if they match.
- */
-/*
- * Simple example: on your card you have have two BSSes you have created with
- * BSSID-01 and BSSID-02. Lets assume BSSID-01 will not use the MAC address.
- * There is another BSSID-03 but you are not part of it. For simplicity's sake,
- * assuming only 4 bits for a mac address and for BSSIDs you can then have:
- *
- *                  \
- * MAC:        0001 |
- * BSSID-01:   0100 | --> Belongs to us
- * BSSID-02:   1001 |
- *                  /
- * -------------------
- * BSSID-03:   0110  | --> External
- * -------------------
- *
- * Our bssid_mask would then be:
- *
- *             On loop iteration for BSSID-01:
- *             ~(0001 ^ 0100)  -> ~(0101)
- *                             ->   1010
- *             bssid_mask      =    1010
- *
- *             On loop iteration for BSSID-02:
- *             bssid_mask &= ~(0001   ^   1001)
- *             bssid_mask =   (1010)  & ~(0001 ^ 1001)
- *             bssid_mask =   (1010)  & ~(1001)
- *             bssid_mask =   (1010)  &  (0110)
- *             bssid_mask =   0010
- *
- * A bssid_mask of 0010 means "only pay attention to the second least
- * significant bit". This is because its the only bit common
- * amongst the MAC and all BSSIDs we support. To findout what the real
- * common bit is we can simply "&" the bssid_mask now with any BSSID we have
- * or our MAC address (we assume the hardware uses the MAC address).
- *
- * Now, suppose there's an incoming frame for BSSID-03:
- *
- * IFRAME-01:  0110
- *
- * An easy eye-inspeciton of this already should tell you that this frame
- * will not pass our check. This is beacuse the bssid_mask tells the
- * hardware to only look at the second least significant bit and the
- * common bit amongst the MAC and BSSIDs is 0, this frame has the 2nd LSB
- * as 1, which does not match 0.
- *
- * So with IFRAME-01 we *assume* the hardware will do:
- *
- *     allow = (IFRAME-01 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
- *  --> allow = (0110 & 0010) == (0010 & 0001) ? 1 : 0;
- *  --> allow = (0010) == 0000 ? 1 : 0;
- *  --> allow = 0
- *
- *  Lets now test a frame that should work:
- *
- * IFRAME-02:  0001 (we should allow)
- *
- *     allow = (0001 & 1010) == 1010
- *
- *     allow = (IFRAME-02 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
- *  --> allow = (0001 & 0010) ==  (0010 & 0001) ? 1 :0;
- *  --> allow = (0010) == (0010)
- *  --> allow = 1
- *
- * Other examples:
- *
- * IFRAME-03:  0100 --> allowed
- * IFRAME-04:  1001 --> allowed
- * IFRAME-05:  1101 --> allowed but its not for us!!!
- *
- */
-int ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask)
+void ath5k_hw_set_bssid_mask(struct ath5k_hw *ah, const u8 *mask)
 {
 	struct ath_common *common = ath5k_hw_common(ah);
-	u32 low_id, high_id;
 	ATH5K_TRACE(ah->ah_sc);
 
 	/* Cache bssid mask so that we can restore it
 	 * on reset */
 	memcpy(common->bssidmask, mask, ETH_ALEN);
-	if (ah->ah_version == AR5K_AR5212) {
-		low_id = get_unaligned_le32(mask);
-		high_id = get_unaligned_le16(mask + 4);
-
-		ath5k_hw_reg_write(ah, low_id, AR5K_BSS_IDM0);
-		ath5k_hw_reg_write(ah, high_id, AR5K_BSS_IDM1);
-
-		return 0;
-	}
-
-	return -EIO;
+	if (ah->ah_version == AR5K_AR5212)
+		ath_hw_setbssidmask(common);
 }
 
-
 /************\
 * RX Control *
 \************/
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index dabff52..80bfa18 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -2423,7 +2423,7 @@ int ath9k_hw_reset(struct ath_hw *ah, struct ath9k_channel *chan,
 		  | ah->sta_id1_defaults);
 	ath9k_hw_set_operating_mode(ah, ah->opmode);
 
-	ath9k_hw_setbssidmask(ah);
+	ath_hw_setbssidmask(common);
 
 	REG_WRITE(ah, AR_DEF_ANTENNA, saveDefAntenna);
 
@@ -3950,14 +3950,6 @@ void ath9k_hw_setmcastfilter(struct ath_hw *ah, u32 filter0, u32 filter1)
 	REG_WRITE(ah, AR_MCAST_FIL1, filter1);
 }
 
-void ath9k_hw_setbssidmask(struct ath_hw *ah)
-{
-	struct ath_common *common = ath9k_hw_common(ah);
-
-	REG_WRITE(ah, AR_BSSMSKL, get_unaligned_le32(common->bssidmask));
-	REG_WRITE(ah, AR_BSSMSKU, get_unaligned_le16(common->bssidmask + 4));
-}
-
 void ath9k_hw_write_associd(struct ath_hw *ah)
 {
 	struct ath_common *common = ath9k_hw_common(ah);
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 5f8a18a..9a8c9ce 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1574,6 +1574,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid)
 
 	common = ath9k_hw_common(ah);
 	common->ops = &ath9k_common_ops;
+	common->ah = ah;
 
 	/*
 	 * Cache line size is used to size and align various
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 97a5efe..fb635a0 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -282,7 +282,7 @@ static void ath_opmode_init(struct ath_softc *sc)
 
 	/* configure bssid mask */
 	if (ah->caps.hw_caps & ATH9K_HW_CAP_BSSIDMASK)
-		ath9k_hw_setbssidmask(ah);
+		ath_hw_setbssidmask(common);
 
 	/* configure operational mode */
 	ath9k_hw_setopmode(ah);
diff --git a/drivers/net/wireless/ath/ath9k/virtual.c b/drivers/net/wireless/ath/ath9k/virtual.c
index 7b763b6..bc7d173 100644
--- a/drivers/net/wireless/ath/ath9k/virtual.c
+++ b/drivers/net/wireless/ath/ath9k/virtual.c
@@ -94,7 +94,7 @@ void ath9k_set_bssid_mask(struct ieee80211_hw *hw)
 	common->bssidmask[4] = ~mask[4];
 	common->bssidmask[5] = ~mask[5];
 
-	ath9k_hw_setbssidmask(sc->sc_ah);
+	ath_hw_setbssidmask(common);
 }
 
 int ath9k_wiphy_add(struct ath_softc *sc)
diff --git a/drivers/net/wireless/ath/hw.c b/drivers/net/wireless/ath/hw.c
new file mode 100644
index 0000000..5f7598e
--- /dev/null
+++ b/drivers/net/wireless/ath/hw.c
@@ -0,0 +1,126 @@
+/*
+ * Copyright (c) 2009 Atheros Communications Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <asm/unaligned.h>
+
+#include "ath.h"
+#include "reg.h"
+
+#define REG_READ	common->ops->read
+#define REG_WRITE	common->ops->write
+
+/**
+ * ath_hw_set_bssid_mask - filter out bssids we listen
+ *
+ * @common: the ath_common struct for the device.
+ *
+ * BSSID masking is a method used by AR5212 and newer hardware to inform PCU
+ * which bits of the interface's MAC address should be looked at when trying
+ * to decide which packets to ACK. In station mode and AP mode with a single
+ * BSS every bit matters since we lock to only one BSS. In AP mode with
+ * multiple BSSes (virtual interfaces) not every bit matters because hw must
+ * accept frames for all BSSes and so we tweak some bits of our mac address
+ * in order to have multiple BSSes.
+ *
+ * NOTE: This is a simple filter and does *not* filter out all
+ * relevant frames. Some frames that are not for us might get ACKed from us
+ * by PCU because they just match the mask.
+ *
+ * When handling multiple BSSes you can get the BSSID mask by computing the
+ * set of  ~ ( MAC XOR BSSID ) for all bssids we handle.
+ *
+ * When you do this you are essentially computing the common bits of all your
+ * BSSes. Later it is assumed the harware will "and" (&) the BSSID mask with
+ * the MAC address to obtain the relevant bits and compare the result with
+ * (frame's BSSID & mask) to see if they match.
+ *
+ * Simple example: on your card you have have two BSSes you have created with
+ * BSSID-01 and BSSID-02. Lets assume BSSID-01 will not use the MAC address.
+ * There is another BSSID-03 but you are not part of it. For simplicity's sake,
+ * assuming only 4 bits for a mac address and for BSSIDs you can then have:
+ *
+ *                  \
+ * MAC:        0001 |
+ * BSSID-01:   0100 | --> Belongs to us
+ * BSSID-02:   1001 |
+ *                  /
+ * -------------------
+ * BSSID-03:   0110  | --> External
+ * -------------------
+ *
+ * Our bssid_mask would then be:
+ *
+ *             On loop iteration for BSSID-01:
+ *             ~(0001 ^ 0100)  -> ~(0101)
+ *                             ->   1010
+ *             bssid_mask      =    1010
+ *
+ *             On loop iteration for BSSID-02:
+ *             bssid_mask &= ~(0001   ^   1001)
+ *             bssid_mask =   (1010)  & ~(0001 ^ 1001)
+ *             bssid_mask =   (1010)  & ~(1001)
+ *             bssid_mask =   (1010)  &  (0110)
+ *             bssid_mask =   0010
+ *
+ * A bssid_mask of 0010 means "only pay attention to the second least
+ * significant bit". This is because its the only bit common
+ * amongst the MAC and all BSSIDs we support. To findout what the real
+ * common bit is we can simply "&" the bssid_mask now with any BSSID we have
+ * or our MAC address (we assume the hardware uses the MAC address).
+ *
+ * Now, suppose there's an incoming frame for BSSID-03:
+ *
+ * IFRAME-01:  0110
+ *
+ * An easy eye-inspeciton of this already should tell you that this frame
+ * will not pass our check. This is beacuse the bssid_mask tells the
+ * hardware to only look at the second least significant bit and the
+ * common bit amongst the MAC and BSSIDs is 0, this frame has the 2nd LSB
+ * as 1, which does not match 0.
+ *
+ * So with IFRAME-01 we *assume* the hardware will do:
+ *
+ *     allow = (IFRAME-01 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
+ *  --> allow = (0110 & 0010) == (0010 & 0001) ? 1 : 0;
+ *  --> allow = (0010) == 0000 ? 1 : 0;
+ *  --> allow = 0
+ *
+ *  Lets now test a frame that should work:
+ *
+ * IFRAME-02:  0001 (we should allow)
+ *
+ *     allow = (0001 & 1010) == 1010
+ *
+ *     allow = (IFRAME-02 & bssid_mask) == (bssid_mask & MAC) ? 1 : 0;
+ *  --> allow = (0001 & 0010) ==  (0010 & 0001) ? 1 :0;
+ *  --> allow = (0010) == (0010)
+ *  --> allow = 1
+ *
+ * Other examples:
+ *
+ * IFRAME-03:  0100 --> allowed
+ * IFRAME-04:  1001 --> allowed
+ * IFRAME-05:  1101 --> allowed but its not for us!!!
+ *
+ */
+void ath_hw_setbssidmask(struct ath_common *common)
+{
+	void *ah = common->ah;
+
+	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);
diff --git a/drivers/net/wireless/ath/reg.h b/drivers/net/wireless/ath/reg.h
new file mode 100644
index 0000000..70453f6
--- /dev/null
+++ b/drivers/net/wireless/ath/reg.h
@@ -0,0 +1,2 @@
+#define AR_BSSMSKL		0x80e0
+#define AR_BSSMSKU		0x80e4
-- 
1.6.3.3

Re: [PATCH 3/4] ath5k: define ath_common ops

[Bob Copeland]

On Thu, Sep 10, 2009 at 9:34 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>  {
> -       return ioread32(ah->ah_iobase + reg);
> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>  }

Is there any way we can do this without two pointer dereferences for every
read and write?  I have a feeling this is going to make certain operations
(e.g. loading initvals during reset) really suck.

-- 
Bob Copeland %% www.bobcopeland.com

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Bob Copeland]

On Thu, Sep 10, 2009 at 9:34 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> ?static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
> ?{
> - ? ? ? return ioread32(ah->ah_iobase + reg);
> + ? ? ? return ath5k_hw_common(ah)->ops->read(ah, reg);
> ?}

Is there any way we can do this without two pointer dereferences for every
read and write?  I have a feeling this is going to make certain operations
(e.g. loading initvals during reset) really suck.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Thu, Sep 10, 2009 at 6:42 PM, Bob Copeland <bcopeland@gmail.com> wrote:
> On Thu, Sep 10, 2009 at 9:34 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>  {
>> -       return ioread32(ah->ah_iobase + reg);
>> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>>  }
>
> Is there any way we can do this without two pointer dereferences for every
> read and write?  I have a feeling this is going to make certain operations
> (e.g. loading initvals during reset) really suck.

It seems to work fine on my box, perhaps a single CPU (real single
CPU) user can test to see if there are any differences noted. I don't
think there should be really.

In any case if its deemed too nested for ath5k purposes you can just
leave an ioread/iowrite on the  ath5k_hw_reg_read() and later just
move all read/write ops to use the common ops calls. If someone can
think of better alternatives I'm all ears.

  Luis

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Thu, Sep 10, 2009 at 6:42 PM, Bob Copeland <bcopeland@gmail.com> wrote:
> On Thu, Sep 10, 2009 at 9:34 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> ?static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>> ?{
>> - ? ? ? return ioread32(ah->ah_iobase + reg);
>> + ? ? ? return ath5k_hw_common(ah)->ops->read(ah, reg);
>> ?}
>
> Is there any way we can do this without two pointer dereferences for every
> read and write? ?I have a feeling this is going to make certain operations
> (e.g. loading initvals during reset) really suck.

It seems to work fine on my box, perhaps a single CPU (real single
CPU) user can test to see if there are any differences noted. I don't
think there should be really.

In any case if its deemed too nested for ath5k purposes you can just
leave an ioread/iowrite on the  ath5k_hw_reg_read() and later just
move all read/write ops to use the common ops calls. If someone can
think of better alternatives I'm all ears.

  Luis

Re: [PATCH 3/4] ath5k: define ath_common ops

[Nick Kossifidis]

2009/9/11 Luis R. Rodriguez <lrodriguez@atheros.com>:
> All read/write ops now go through the common ops.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  drivers/net/wireless/ath/ath5k/ath5k.h |   20 ++++++++++++--------
>  drivers/net/wireless/ath/ath5k/base.c  |   17 +++++++++++++++++
>  drivers/net/wireless/ath/ath5k/base.h  |   11 -----------
>  3 files changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
> index 29ce868..997101b 100644
> --- a/drivers/net/wireless/ath/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath/ath5k/ath5k.h
> @@ -1315,20 +1315,24 @@ static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
>        return turbo ? (clock / 80) : (clock / 40);
>  }
>
> -/*
> - * Read from a register
> - */
> +static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
> +{
> +        return &ah->common;
> +}
> +
> +static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
> +{
> +        return &(ath5k_hw_common(ah)->regulatory);
> +}
> +
>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>  {
> -       return ioread32(ah->ah_iobase + reg);
> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>  }
>
> -/*
> - * Write to a register
> - */
>  static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>  {
> -       iowrite32(val, ah->ah_iobase + reg);
> +       ath5k_hw_common(ah)->ops->write(ah, reg, val);
>  }
>
>  #if defined(_ATH5K_RESET) || defined(_ATH5K_PHY)
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 3cb0752..535ea72 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -437,6 +437,22 @@ ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
>
>        return name;
>  }
> +static unsigned int ath5k_ioread32(void *hw_priv, u32 reg_offset)
> +{
> +       struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
> +       return ioread32(ah->ah_iobase + reg_offset);
> +}
> +
> +static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
> +{
> +       struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
> +       iowrite32(val, ah->ah_iobase + reg_offset);
> +}
> +
> +static struct ath_ops ath5k_common_ops = {
> +       .read = ath5k_ioread32,
> +       .write = ath5k_iowrite32,
> +};
>
>  static int __devinit
>  ath5k_pci_probe(struct pci_dev *pdev,
> @@ -576,6 +592,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
>        sc->ah->ah_sc = sc;
>        sc->ah->ah_iobase = sc->iobase;
>        common = ath5k_hw_common(sc->ah);
> +       common->ops = &ath5k_common_ops;
>        common->cachelsz = csz << 2; /* convert to bytes */
>
>        /* Initialize device */
> diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
> index 005d25f..b14ba07 100644
> --- a/drivers/net/wireless/ath/ath5k/base.h
> +++ b/drivers/net/wireless/ath/ath5k/base.h
> @@ -201,15 +201,4 @@ struct ath5k_softc {
>  #define ath5k_hw_hasveol(_ah) \
>        (ath5k_hw_get_capability(_ah, AR5K_CAP_VEOL, 0, NULL) == 0)
>
> -static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
> -{
> -       return &ah->common;
> -}
> -
> -static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
> -{
> -       return &(ath5k_hw_common(ah)->regulatory);
> -
> -}
> -
>  #endif

Since each driver will use it's own reg read/write functions (ath5k hw
code still uses ath5k_hw_reg_read/write), why do we need to have
common reg read/write functions like that used in the driver code ?
This makes the code more complex that it needs to be and i don't see a
reason why we need it. I understand why we need a way to handle
read/write functions from common ath code but i don't see a reason to
use these functions on ath5k, we can just fill ath_ops struct so that
common code can use them and leave ath5k_hw_read/write untouched.

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Nick Kossifidis]

2009/9/11 Luis R. Rodriguez <lrodriguez@atheros.com>:
> All read/write ops now go through the common ops.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
> ?drivers/net/wireless/ath/ath5k/ath5k.h | ? 20 ++++++++++++--------
> ?drivers/net/wireless/ath/ath5k/base.c ?| ? 17 +++++++++++++++++
> ?drivers/net/wireless/ath/ath5k/base.h ?| ? 11 -----------
> ?3 files changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
> index 29ce868..997101b 100644
> --- a/drivers/net/wireless/ath/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath/ath5k/ath5k.h
> @@ -1315,20 +1315,24 @@ static inline unsigned int ath5k_hw_clocktoh(unsigned int clock, bool turbo)
> ? ? ? ?return turbo ? (clock / 80) : (clock / 40);
> ?}
>
> -/*
> - * Read from a register
> - */
> +static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
> +{
> + ? ? ? ?return &ah->common;
> +}
> +
> +static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
> +{
> + ? ? ? ?return &(ath5k_hw_common(ah)->regulatory);
> +}
> +
> ?static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
> ?{
> - ? ? ? return ioread32(ah->ah_iobase + reg);
> + ? ? ? return ath5k_hw_common(ah)->ops->read(ah, reg);
> ?}
>
> -/*
> - * Write to a register
> - */
> ?static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
> ?{
> - ? ? ? iowrite32(val, ah->ah_iobase + reg);
> + ? ? ? ath5k_hw_common(ah)->ops->write(ah, reg, val);
> ?}
>
> ?#if defined(_ATH5K_RESET) || defined(_ATH5K_PHY)
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 3cb0752..535ea72 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -437,6 +437,22 @@ ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
>
> ? ? ? ?return name;
> ?}
> +static unsigned int ath5k_ioread32(void *hw_priv, u32 reg_offset)
> +{
> + ? ? ? struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
> + ? ? ? return ioread32(ah->ah_iobase + reg_offset);
> +}
> +
> +static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
> +{
> + ? ? ? struct ath5k_hw *ah = (struct ath5k_hw *) hw_priv;
> + ? ? ? iowrite32(val, ah->ah_iobase + reg_offset);
> +}
> +
> +static struct ath_ops ath5k_common_ops = {
> + ? ? ? .read = ath5k_ioread32,
> + ? ? ? .write = ath5k_iowrite32,
> +};
>
> ?static int __devinit
> ?ath5k_pci_probe(struct pci_dev *pdev,
> @@ -576,6 +592,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
> ? ? ? ?sc->ah->ah_sc = sc;
> ? ? ? ?sc->ah->ah_iobase = sc->iobase;
> ? ? ? ?common = ath5k_hw_common(sc->ah);
> + ? ? ? common->ops = &ath5k_common_ops;
> ? ? ? ?common->cachelsz = csz << 2; /* convert to bytes */
>
> ? ? ? ?/* Initialize device */
> diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
> index 005d25f..b14ba07 100644
> --- a/drivers/net/wireless/ath/ath5k/base.h
> +++ b/drivers/net/wireless/ath/ath5k/base.h
> @@ -201,15 +201,4 @@ struct ath5k_softc {
> ?#define ath5k_hw_hasveol(_ah) \
> ? ? ? ?(ath5k_hw_get_capability(_ah, AR5K_CAP_VEOL, 0, NULL) == 0)
>
> -static inline struct ath_common *ath5k_hw_common(struct ath5k_hw *ah)
> -{
> - ? ? ? return &ah->common;
> -}
> -
> -static inline struct ath_regulatory *ath5k_hw_regulatory(struct ath5k_hw *ah)
> -{
> - ? ? ? return &(ath5k_hw_common(ah)->regulatory);
> -
> -}
> -
> ?#endif

Since each driver will use it's own reg read/write functions (ath5k hw
code still uses ath5k_hw_reg_read/write), why do we need to have
common reg read/write functions like that used in the driver code ?
This makes the code more complex that it needs to be and i don't see a
reason why we need it. I understand why we need a way to handle
read/write functions from common ath code but i don't see a reason to
use these functions on ath5k, we can just fill ath_ops struct so that
common code can use them and leave ath5k_hw_read/write untouched.

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 3/4] ath5k: define ath_common ops

[Jiri Slaby]

On 09/11/2009 08:16 AM, Nick Kossifidis wrote:
>>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>  {
>> -       return ioread32(ah->ah_iobase + reg);
>> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>>  }
>>
>> -/*
>> - * Write to a register
>> - */
>>  static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>>  {
>> -       iowrite32(val, ah->ah_iobase + reg);
>> +       ath5k_hw_common(ah)->ops->write(ah, reg, val);
...
> Since each driver will use it's own reg read/write functions (ath5k hw
> code still uses ath5k_hw_reg_read/write), why do we need to have
> common reg read/write functions like that used in the driver code ?
> This makes the code more complex that it needs to be and i don't see a
> reason why we need it. I understand why we need a way to handle
> read/write functions from common ath code but i don't see a reason to
> use these functions on ath5k, we can just fill ath_ops struct so that
> common code can use them and leave ath5k_hw_read/write untouched.

I definitely agree with Nick here. Althought whole ath_ops will be hot
cache after the first operation, there is no need to prolong hot paths
by computing the op address and a call. Ok, read/write on PCI is pretty
slow, but still...

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Jiri Slaby]

On 09/11/2009 08:16 AM, Nick Kossifidis wrote:
>>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>  {
>> -       return ioread32(ah->ah_iobase + reg);
>> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>>  }
>>
>> -/*
>> - * Write to a register
>> - */
>>  static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>>  {
>> -       iowrite32(val, ah->ah_iobase + reg);
>> +       ath5k_hw_common(ah)->ops->write(ah, reg, val);
...
> Since each driver will use it's own reg read/write functions (ath5k hw
> code still uses ath5k_hw_reg_read/write), why do we need to have
> common reg read/write functions like that used in the driver code ?
> This makes the code more complex that it needs to be and i don't see a
> reason why we need it. I understand why we need a way to handle
> read/write functions from common ath code but i don't see a reason to
> use these functions on ath5k, we can just fill ath_ops struct so that
> common code can use them and leave ath5k_hw_read/write untouched.

I definitely agree with Nick here. Althought whole ath_ops will be hot
cache after the first operation, there is no need to prolong hot paths
by computing the op address and a call. Ok, read/write on PCI is pretty
slow, but still...

Re: [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/11/2009 08:16 AM, Nick Kossifidis wrote:
>>>  static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>>  {
>>> -       return ioread32(ah->ah_iobase + reg);
>>> +       return ath5k_hw_common(ah)->ops->read(ah, reg);
>>>  }
>>>
>>> -/*
>>> - * Write to a register
>>> - */
>>>  static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>>>  {
>>> -       iowrite32(val, ah->ah_iobase + reg);
>>> +       ath5k_hw_common(ah)->ops->write(ah, reg, val);
> ...
>> Since each driver will use it's own reg read/write functions (ath5k hw
>> code still uses ath5k_hw_reg_read/write), why do we need to have
>> common reg read/write functions like that used in the driver code ?
>> This makes the code more complex that it needs to be and i don't see a
>> reason why we need it. I understand why we need a way to handle
>> read/write functions from common ath code but i don't see a reason to
>> use these functions on ath5k, we can just fill ath_ops struct so that
>> common code can use them and leave ath5k_hw_read/write untouched.
>
> I definitely agree with Nick here. Althought whole ath_ops will be hot
> cache after the first operation, there is no need to prolong hot paths
> by computing the op address and a call. Ok, read/write on PCI is pretty
> slow, but still...

That is the way I had it originally before submission, and I
completely agree its reasonable to not incur additional cost at the
expense of having two separate read/write paths, and perhaps we should
only incur the extra cost on routines shared between
ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
penalty?

This is why I asked if someone can test and give measurable
differences over this. If there really isn't then that's not strong
point against it.

For example, long ago I had argued over the cost incurred over the
unnecessary branching on ioread()/iowrite() when you know you have
MMIO devices [1] -- the defense then, and IMHO reasonable now, was
that the benefits of allowing cleaner drivers through the new
interfaces outweigh the theoretical penalties imposed by them.

Granted you can argue these new interfaces between
ath5k/ath9k/ath9k_htc would make things a little more complex, but I
would expect sharing the code will help in the end. And if these
interfaces are not acceptable I'm completely open to better suggested
alternatives.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0709.2/1068.html

  Luis

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/11/2009 08:16 AM, Nick Kossifidis wrote:
>>> ?static inline u32 ath5k_hw_reg_read(struct ath5k_hw *ah, u16 reg)
>>> ?{
>>> - ? ? ? return ioread32(ah->ah_iobase + reg);
>>> + ? ? ? return ath5k_hw_common(ah)->ops->read(ah, reg);
>>> ?}
>>>
>>> -/*
>>> - * Write to a register
>>> - */
>>> ?static inline void ath5k_hw_reg_write(struct ath5k_hw *ah, u32 val, u16 reg)
>>> ?{
>>> - ? ? ? iowrite32(val, ah->ah_iobase + reg);
>>> + ? ? ? ath5k_hw_common(ah)->ops->write(ah, reg, val);
> ...
>> Since each driver will use it's own reg read/write functions (ath5k hw
>> code still uses ath5k_hw_reg_read/write), why do we need to have
>> common reg read/write functions like that used in the driver code ?
>> This makes the code more complex that it needs to be and i don't see a
>> reason why we need it. I understand why we need a way to handle
>> read/write functions from common ath code but i don't see a reason to
>> use these functions on ath5k, we can just fill ath_ops struct so that
>> common code can use them and leave ath5k_hw_read/write untouched.
>
> I definitely agree with Nick here. Althought whole ath_ops will be hot
> cache after the first operation, there is no need to prolong hot paths
> by computing the op address and a call. Ok, read/write on PCI is pretty
> slow, but still...

That is the way I had it originally before submission, and I
completely agree its reasonable to not incur additional cost at the
expense of having two separate read/write paths, and perhaps we should
only incur the extra cost on routines shared between
ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
penalty?

This is why I asked if someone can test and give measurable
differences over this. If there really isn't then that's not strong
point against it.

For example, long ago I had argued over the cost incurred over the
unnecessary branching on ioread()/iowrite() when you know you have
MMIO devices [1] -- the defense then, and IMHO reasonable now, was
that the benefits of allowing cleaner drivers through the new
interfaces outweigh the theoretical penalties imposed by them.

Granted you can argue these new interfaces between
ath5k/ath9k/ath9k_htc would make things a little more complex, but I
would expect sharing the code will help in the end. And if these
interfaces are not acceptable I'm completely open to better suggested
alternatives.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0709.2/1068.html

  Luis

Re: [PATCH 3/4] ath5k: define ath_common ops

[Bob Copeland]

On Fri, Sep 11, 2009 at 3:23 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?
>
> This is why I asked if someone can test and give measurable
> differences over this. If there really isn't then that's not strong
> point against it.

Honestly, it probably won't matter in the grand scheme of things, but I
think if you are proposing a patch that touches every hotpath in two
drivers, then you need to do the work to say "by the way, this has benefit
X which outweighs the very small (or absent) performance regression Y,
and here are the numbers.

-- 
Bob Copeland %% www.bobcopeland.com

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Bob Copeland]

On Fri, Sep 11, 2009 at 3:23 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?
>
> This is why I asked if someone can test and give measurable
> differences over this. If there really isn't then that's not strong
> point against it.

Honestly, it probably won't matter in the grand scheme of things, but I
think if you are proposing a patch that touches every hotpath in two
drivers, then you need to do the work to say "by the way, this has benefit
X which outweighs the very small (or absent) performance regression Y,
and here are the numbers.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 3/4] ath5k: define ath_common ops

[Linus Torvalds]

On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
> 
> That is the way I had it originally before submission, and I
> completely agree its reasonable to not incur additional cost at the
> expense of having two separate read/write paths, and perhaps we should
> only incur the extra cost on routines shared between
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?

There's a measurable size penalty, at least.

In fact, if you know what kind of IO op it is (ie "it's always MMIO"), 
you'd be even better using "writel()" directly, in which case it turns 
into just a single store on most architectures, and doesn't cause all the 
register save/restore of a function call.

			Linus

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Linus Torvalds]

On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
> 
> That is the way I had it originally before submission, and I
> completely agree its reasonable to not incur additional cost at the
> expense of having two separate read/write paths, and perhaps we should
> only incur the extra cost on routines shared between
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?

There's a measurable size penalty, at least.

In fact, if you know what kind of IO op it is (ie "it's always MMIO"), 
you'd be even better using "writel()" directly, in which case it turns 
into just a single store on most architectures, and doesn't cause all the 
register save/restore of a function call.

			Linus

Re: [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Fri, Sep 11, 2009 at 7:24 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
>>
>> That is the way I had it originally before submission, and I
>> completely agree its reasonable to not incur additional cost at the
>> expense of having two separate read/write paths, and perhaps we should
>> only incur the extra cost on routines shared between
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>
> There's a measurable size penalty, at least.

My tests so far yield no performance difference but I'm sure there is
some, maybe as Jouni noted, more visible on embedded systems.

> In fact, if you know what kind of IO op it is (ie "it's always MMIO"),
> you'd be even better using "writel()" directly,

Heh.. you realize I tried to document such a thing a while ago and it
seems you opposed it [1]?

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0709.2/0593.html

  Luis

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Fri, Sep 11, 2009 at 7:24 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
>>
>> That is the way I had it originally before submission, and I
>> completely agree its reasonable to not incur additional cost at the
>> expense of having two separate read/write paths, and perhaps we should
>> only incur the extra cost on routines shared between
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>
> There's a measurable size penalty, at least.

My tests so far yield no performance difference but I'm sure there is
some, maybe as Jouni noted, more visible on embedded systems.

> In fact, if you know what kind of IO op it is (ie "it's always MMIO"),
> you'd be even better using "writel()" directly,

Heh.. you realize I tried to document such a thing a while ago and it
seems you opposed it [1]?

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0709.2/0593.html

  Luis

Re: [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Fri, Sep 11, 2009 at 4:35 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Fri, Sep 11, 2009 at 3:23 AM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>>
>> This is why I asked if someone can test and give measurable
>> differences over this. If there really isn't then that's not strong
>> point against it.
>
> Honestly, it probably won't matter in the grand scheme of things, but I
> think if you are proposing a patch that touches every hotpath in two
> drivers, then you need to do the work to say "by the way, this has benefit
> X which outweighs the very small (or absent) performance regression Y,
> and here are the numbers.

You're completely right, sorry about that. I thought the advantages
would have been obvious but let me clarify them them:

So far I've tested this with:

time iw list dev wlan0 scan > /dev/null

Both with and without the patches and the time it takes to scan, when
not associated, remains the same. Granted I do have an Intel Core Duo
1.8 GHz, so if some others could test this on some embedded platforms
that would be appreciated.

The main added advantage to these changes is the possibility to now
share hw access code between ath5k/ath9k. With the patches as-is you
get one hot path on the driver, whether or not you use common hw code
through ath.ko or through the driver's own hw code. It is unclear to
me whether this has any measurable benefits so an alternative is to
only use the common read/write ops on the common ath.ko.

Although I don't see any measurable differences at the moment I
suspect most people are inclined to leave hw access directly on the
driver and only use common hw read/write ops for the common code. I'll
respin these patches to do just that.

  Luis

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Fri, Sep 11, 2009 at 4:35 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Fri, Sep 11, 2009 at 3:23 AM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>>
>> This is why I asked if someone can test and give measurable
>> differences over this. If there really isn't then that's not strong
>> point against it.
>
> Honestly, it probably won't matter in the grand scheme of things, but I
> think if you are proposing a patch that touches every hotpath in two
> drivers, then you need to do the work to say "by the way, this has benefit
> X which outweighs the very small (or absent) performance regression Y,
> and here are the numbers.

You're completely right, sorry about that. I thought the advantages
would have been obvious but let me clarify them them:

So far I've tested this with:

time iw list dev wlan0 scan > /dev/null

Both with and without the patches and the time it takes to scan, when
not associated, remains the same. Granted I do have an Intel Core Duo
1.8 GHz, so if some others could test this on some embedded platforms
that would be appreciated.

The main added advantage to these changes is the possibility to now
share hw access code between ath5k/ath9k. With the patches as-is you
get one hot path on the driver, whether or not you use common hw code
through ath.ko or through the driver's own hw code. It is unclear to
me whether this has any measurable benefits so an alternative is to
only use the common read/write ops on the common ath.ko.

Although I don't see any measurable differences at the moment I
suspect most people are inclined to leave hw access directly on the
driver and only use common hw read/write ops for the common code. I'll
respin these patches to do just that.

  Luis

Re: [PATCH 3/4] ath5k: define ath_common ops

[Linus Torvalds]

On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
> 
> > In fact, if you know what kind of IO op it is (ie "it's always MMIO"),
> > you'd be even better using "writel()" directly,
> 
> Heh.. you realize I tried to document such a thing a while ago and it
> seems you opposed it [1]?

If it's mapped with "pci_iomap()" you should use ioread*/iowrite*. 

But if you know it's always MMIO, and you use just ioremap() to map it, 
and then readl/writel.

It's the _mixing_ of the two that I object to:
 - pci_iomap() - can do either MMIO of PIO
 - readl/writel - always just MMIO
should preferably not be mixed.

(Sure it will _work_, but I wouldn't encourage it)

			Linus

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Linus Torvalds]

On Fri, 11 Sep 2009, Luis R. Rodriguez wrote:
> 
> > In fact, if you know what kind of IO op it is (ie "it's always MMIO"),
> > you'd be even better using "writel()" directly,
> 
> Heh.. you realize I tried to document such a thing a while ago and it
> seems you opposed it [1]?

If it's mapped with "pci_iomap()" you should use ioread*/iowrite*. 

But if you know it's always MMIO, and you use just ioremap() to map it, 
and then readl/writel.

It's the _mixing_ of the two that I object to:
 - pci_iomap() - can do either MMIO of PIO
 - readl/writel - always just MMIO
should preferably not be mixed.

(Sure it will _work_, but I wouldn't encourage it)

			Linus

Re: [PATCH 3/4] ath5k: define ath_common ops

[Jiri Slaby]

On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> I definitely agree with Nick here. Althought whole ath_ops will be hot
>> cache after the first operation, there is no need to prolong hot paths
>> by computing the op address and a call. Ok, read/write on PCI is pretty
>> slow, but still...
> 
> That is the way I had it originally before submission, and I
> completely agree its reasonable to not incur additional cost at the
> expense of having two separate read/write paths, and perhaps we should
> only incur the extra cost on routines shared between
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?

Hardly there is a measurable one. As I wrote earlier one will wait ages
for PCI in comparison to few load+call cycles.

> This is why I asked if someone can test and give measurable
> differences over this. If there really isn't then that's not strong
> point against it.

Well, honestly I see no strong point for it. It rather looks like an
obfuscation, not improvement.

> For example, long ago I had argued over the cost incurred over the
> unnecessary branching on ioread()/iowrite() when you know you have
> MMIO devices [1] -- the defense then, and IMHO reasonable now, was
> that the benefits of allowing cleaner drivers through the new
> interfaces outweigh the theoretical penalties imposed by them.

Ok, that one has benefits. You just needn't care about what is behind
that mapping. It will choose a PIO or MMIO op on its own.

When it's always MMIO I personally prefer simple ioremap though. (I
didn't at the time of merging the driver.)

> Granted you can argue these new interfaces between
> ath5k/ath9k/ath9k_htc would make things a little more complex, but I
> would expect sharing the code will help in the end. And if these
> interfaces are not acceptable I'm completely open to better suggested
> alternatives.

Ok, I think nothing more than this is needed:
+static u32 ath5k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	return ath5k_hw_reg_read(hw_priv, reg_offset)
+}
+
+static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	ath5k_hw_reg_write(hw_priv, val, reg_offset);
+}
+
+static struct ath_ops ath5k_common_ops = {
+	.read = ath5k_ioread32,
+	.write = ath5k_iowrite32,
+};

What I wonder is why ath_ops has reg+val parameters in the opposite
manner than the rest of kernel? It is error-prone.

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Jiri Slaby]

On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> I definitely agree with Nick here. Althought whole ath_ops will be hot
>> cache after the first operation, there is no need to prolong hot paths
>> by computing the op address and a call. Ok, read/write on PCI is pretty
>> slow, but still...
> 
> That is the way I had it originally before submission, and I
> completely agree its reasonable to not incur additional cost at the
> expense of having two separate read/write paths, and perhaps we should
> only incur the extra cost on routines shared between
> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
> penalty?

Hardly there is a measurable one. As I wrote earlier one will wait ages
for PCI in comparison to few load+call cycles.

> This is why I asked if someone can test and give measurable
> differences over this. If there really isn't then that's not strong
> point against it.

Well, honestly I see no strong point for it. It rather looks like an
obfuscation, not improvement.

> For example, long ago I had argued over the cost incurred over the
> unnecessary branching on ioread()/iowrite() when you know you have
> MMIO devices [1] -- the defense then, and IMHO reasonable now, was
> that the benefits of allowing cleaner drivers through the new
> interfaces outweigh the theoretical penalties imposed by them.

Ok, that one has benefits. You just needn't care about what is behind
that mapping. It will choose a PIO or MMIO op on its own.

When it's always MMIO I personally prefer simple ioremap though. (I
didn't at the time of merging the driver.)

> Granted you can argue these new interfaces between
> ath5k/ath9k/ath9k_htc would make things a little more complex, but I
> would expect sharing the code will help in the end. And if these
> interfaces are not acceptable I'm completely open to better suggested
> alternatives.

Ok, I think nothing more than this is needed:
+static u32 ath5k_ioread32(void *hw_priv, u32 reg_offset)
+{
+	return ath5k_hw_reg_read(hw_priv, reg_offset)
+}
+
+static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
+{
+	ath5k_hw_reg_write(hw_priv, val, reg_offset);
+}
+
+static struct ath_ops ath5k_common_ops = {
+	.read = ath5k_ioread32,
+	.write = ath5k_iowrite32,
+};

What I wonder is why ath_ops has reg+val parameters in the opposite
manner than the rest of kernel? It is error-prone.

Re: [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Sat, Sep 12, 2009 at 4:53 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
>> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>>> I definitely agree with Nick here. Althought whole ath_ops will be hot
>>> cache after the first operation, there is no need to prolong hot paths
>>> by computing the op address and a call. Ok, read/write on PCI is pretty
>>> slow, but still...
>>
>> That is the way I had it originally before submission, and I
>> completely agree its reasonable to not incur additional cost at the
>> expense of having two separate read/write paths, and perhaps we should
>> only incur the extra cost on routines shared between
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>
> Hardly there is a measurable one. As I wrote earlier one will wait ages
> for PCI in comparison to few load+call cycles.

Alright, this helps.

>> This is why I asked if someone can test and give measurable
>> differences over this. If there really isn't then that's not strong
>> point against it.
>
> Well, honestly I see no strong point for it. It rather looks like an
> obfuscation, not improvement.

It may be and for ath5k this is not a requirement, for ath9k however
we do need a way to share hw code and therefore we cannot keep
read/write ops as part of hw.c but more importantly we need something
to map READ_READ/REG_WRITE macros on hw.h which will work for both
ath9k and ath9k_htc.

>> Granted you can argue these new interfaces between
>> ath5k/ath9k/ath9k_htc would make things a little more complex, but I
>> would expect sharing the code will help in the end. And if these
>> interfaces are not acceptable I'm completely open to better suggested
>> alternatives.
>
> Ok, I think nothing more than this is needed:
> +static u32 ath5k_ioread32(void *hw_priv, u32 reg_offset)
> +{
> +       return ath5k_hw_reg_read(hw_priv, reg_offset)
> +}
> +
> +static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
> +{
> +       ath5k_hw_reg_write(hw_priv, val, reg_offset);
> +}
> +
> +static struct ath_ops ath5k_common_ops = {
> +       .read = ath5k_ioread32,
> +       .write = ath5k_iowrite32,
> +};

That's fine for ath5k.

> What I wonder is why ath_ops has reg+val parameters in the opposite
> manner than the rest of kernel? It is error-prone.

Heh, yeah, that's just how the Atheros hw code was written, so I'll
change the common ops to match standard practice but don't want to
change the all the REG_READ/REG_WRITE macros, so ath9k stuff will just
do the flip.

  Luis

[ath9k-devel] [PATCH 3/4] ath5k: define ath_common ops

[Luis R. Rodriguez]

On Sat, Sep 12, 2009 at 4:53 AM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 09/11/2009 09:23 AM, Luis R. Rodriguez wrote:
>> On Thu, Sep 10, 2009 at 11:46 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>>> I definitely agree with Nick here. Althought whole ath_ops will be hot
>>> cache after the first operation, there is no need to prolong hot paths
>>> by computing the op address and a call. Ok, read/write on PCI is pretty
>>> slow, but still...
>>
>> That is the way I had it originally before submission, and I
>> completely agree its reasonable to not incur additional cost at the
>> expense of having two separate read/write paths, and perhaps we should
>> only incur the extra cost on routines shared between
>> ath9k/ath9k/ath9k_htc. But -- is there really is a measurable cost
>> penalty?
>
> Hardly there is a measurable one. As I wrote earlier one will wait ages
> for PCI in comparison to few load+call cycles.

Alright, this helps.

>> This is why I asked if someone can test and give measurable
>> differences over this. If there really isn't then that's not strong
>> point against it.
>
> Well, honestly I see no strong point for it. It rather looks like an
> obfuscation, not improvement.

It may be and for ath5k this is not a requirement, for ath9k however
we do need a way to share hw code and therefore we cannot keep
read/write ops as part of hw.c but more importantly we need something
to map READ_READ/REG_WRITE macros on hw.h which will work for both
ath9k and ath9k_htc.

>> Granted you can argue these new interfaces between
>> ath5k/ath9k/ath9k_htc would make things a little more complex, but I
>> would expect sharing the code will help in the end. And if these
>> interfaces are not acceptable I'm completely open to better suggested
>> alternatives.
>
> Ok, I think nothing more than this is needed:
> +static u32 ath5k_ioread32(void *hw_priv, u32 reg_offset)
> +{
> + ? ? ? return ath5k_hw_reg_read(hw_priv, reg_offset)
> +}
> +
> +static void ath5k_iowrite32(void *hw_priv, u32 reg_offset, u32 val)
> +{
> + ? ? ? ath5k_hw_reg_write(hw_priv, val, reg_offset);
> +}
> +
> +static struct ath_ops ath5k_common_ops = {
> + ? ? ? .read = ath5k_ioread32,
> + ? ? ? .write = ath5k_iowrite32,
> +};

That's fine for ath5k.

> What I wonder is why ath_ops has reg+val parameters in the opposite
> manner than the rest of kernel? It is error-prone.

Heh, yeah, that's just how the Atheros hw code was written, so I'll
change the common ops to match standard practice but don't want to
change the all the REG_READ/REG_WRITE macros, so ath9k stuff will just
do the flip.

  Luis