On 11/8/2024 1:42 PM, Johannes Berg wrote:
Hi,
reading through this anyway looking for mac80211 integration, so I'll
nitpick too ... ;-)
...
+ if (wlvif->sta.role_chan_type != NL80211_CHAN_HT40MINUS &&
+ wlvif->sta.role_chan_type != NL80211_CHAN_HT40PLUS)
+ ieee80211_connection_loss(cc33xx_wlvif_to_vif(wlvif));
You seem to support HE (PPE thresholds above and all that), but then you
still use CHAN_HT40MINUS/PLUS?! That seems ... rather wrong? You should
probably track in terms of NL80211_CHAN_WIDTH_*, I'm not even sure how
you get NL80211_CHAN_* values in the first place, mac80211 should never
use them now?
Right, a leftover from wlcore. Since CC33xx is 20 MHz only device I'll
remove it.
...
+static inline void cc33xx_tx_watchdog_work(struct work_struct *work)
+{
+ container_of(to_delayed_work(work), struct cc33xx, tx_watchdog_work);
+}
I don't think that does what you think it does. Well, I don't know what
you think it does, but ...
Also there should be (almost) no inline in C files, and even if it were
correct you'd probably just use it exactly once?
Agree, will get rid of inline in all functions.
...
+static int parse_control_message(struct cc33xx *cc,
+ const u8 *buffer, size_t buffer_length)
+{
+ u8 *const end_of_payload = (u8 *const)buffer + buffer_length;
+ u8 *const start_of_payload = (u8 *const)buffer;
I don't think the "u8 *const" is useful here, and the cast is awkward.
If anything you'd want "const u8 *const" (which should make it not need
the cast), but the const you have adds no value... do you even know what
it means? ;-)
My intent was to express that start and end pointers are fixed and will
not change in the loop below. When reading this again I agree this hurts
more than it helps, I'll drop it.
const u8 *buffer in the prototype illustrates that parse_control_message
will not change the data so I'll keep it if there a re no objections.
+ struct NAB_header *nab_header;
surely checkpatch complained about CamelCase or so with the struct name
like that?
Double-checked, no warnings from checkpatch:
----------------------------------------------
$ scripts/checkpatch.pl --codespell --strict
../../export/upstream/patch/v5-0009-wifi-cc33xx-Add-main.c.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#44:
new file mode 100644
WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module:
Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#5729: FILE: drivers/net/wireless/ti/cc33xx/main.c:5681:
+MODULE_LICENSE("GPL v2");
total: 0 errors, 2 warnings, 0 checks, 5687 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
../../export/upstream/patch/v5-0009-wifi-cc33xx-Add-main.c.patch has
style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
$
----------------------------------------------
Obviously will fix the definition.
Still going over all other items you referred to, will take a minute :)
Thanks and regards,
Michael.
