Thanks for the review. most will be fix on v2.
Two comments inline.
regards,
Shahar
Luciano Coelho wrote:
On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
New ops support ampdu_action as initiator returns EINVAL due to
the fact that BA initiator session manage in the FW independently.
acx function set BA policies.
Please rephrase this as it is hard to understand what the patch really
is about.
Signed-off-by: Shahar Levi <shahar_levi@xxxxxx>
---
Some more, mostly style comments. Having mostly style-related comments
means that your implementation is really good and I appreciate it! It
just need a bit of polishing and parfumerie ;)
This patch can also be merged with the next one. This doesn't add any
real functionality and there are things added here (for example the
mac_addr = FFFFFFFFFFFF is not used at all) that are changed in the next
patch. This just confuses things.
diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.c b/drivers/net/wireless/wl12xx/wl1271_acx.c
index cc6b7d8..f82656e 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.c
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.c
@@ -1317,6 +1317,61 @@ out:
return ret;
}
+/*
+ * wl1271_acx_set_ba_session
Remove this line. And then the comment can be in one line.
+ * configure BA session initiator\receiver parameters setting in the FW.
+ */
+int wl1271_acx_set_ba_session(struct wl1271 *wl,
+ u16 id,
+ u8 tid_index,
+ u8 policy)
Fix the indentation and maybe it all fits in one line?
one character is go beyond.
+{
+ struct wl1271_acx_ba_session_policy *acx;
+ int ret = 0;
+ /*
+ * Note, currently this value will be set to FFFFFFFFFFFF to indicate
+ * it is relevant for all peers.
+ */
+ u8 mac_address[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
No need to do this here, since this code is not called and, when you
call it (ie. in patch 03/03), you remove this FF'ing anyway. ;)
+ if (ACX_BA_SESSION_INITIATOR_POLICY == id)
Please use natural order (ie. id == ACX_BA_SESSION_INITIATOR_POLICY).
+ acx->inactivity_timeout = BA_INACTIVITY_TIMEOUT;
+ else
+ if (ACX_BA_SESSION_RESPONDER_POLICY == id)
+ acx->inactivity_timeout = 0;
+ else {
+ wl1271_error("Incorrect acx command id=%x\n", id);
+ ret = -EINVAL;
+ goto out;
+ }
Use this structure for the if else statements:
if (...) {
...
} else if (...) {
...
} else {
...
}
Or a switch-case.
+
+ ret = wl1271_cmd_configure(wl,
+ id,
+ acx,
+ sizeof(*acx));
Fits in one line.
diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index 6c3c7c0..f417624 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -1205,6 +1205,10 @@ int wl1271_acx_set_ht_capabilities(struct wl1271 *wl,
bool allow_ht_operation);
int wl1271_acx_set_ht_information(struct wl1271 *wl,
u16 ht_operation_mode);
+int wl1271_acx_set_ba_session(struct wl1271 *wl,
+ u16 id,
+ u8 tid_index,
+ u8 policy);
Indentation.
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index af092f4..53c03e5 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1128,6 +1128,22 @@ static void wl1271_configure_filters(struct wl1271 *wl, unsigned int filters)
}
}
+/*
+ * wl1271_set_ba_policies
Same as earlier: remove this line. And then the comment can be in one
line.
+ * Set the BA session policies to the FW.
+ */
+static void wl1271_set_ba_policies(struct wl1271 *wl)
+{
+ u8 tid_index;
No need for the TAB between u8 and tid_index.
@@ -2056,6 +2072,32 @@ static int wl1271_op_get_survey(struct ieee80211_hw *hw, int idx,
return 0;
}
+int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ enum ieee80211_ampdu_mlme_action action,
+ struct ieee80211_sta *sta, u16 tid, u16 *ssn)
+{
+ int ret;
+
+ switch (action) {
+ case IEEE80211_AMPDU_RX_START:
+ case IEEE80211_AMPDU_RX_STOP:
+
+ /* The BA initiator session management in FW independently */
+ case IEEE80211_AMPDU_TX_START:
+ case IEEE80211_AMPDU_TX_STOP:
+ case IEEE80211_AMPDU_TX_OPERATIONAL:
+ ret = -EINVAL;
+ break;
+
+ default:
+ wl1271_error("Incorrect ampdu action id=%x\n", action);
+ ret = -ENODEV;
+ }
+
+ return ret;
+}
This function doesn't do anything, really. Except for returning -EINVAL
in TX cases. Is it really needed?
you right, for TX we didn't need it. i will remove it.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
