[OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip

[Ian Kent]

Hi David,

Please forgive the top posting.

I don't think that the kernel programming style guidelines recommend
against it and I also don't know if checkpatch.pl will complain about it
but personally I find the additional blank lines in things like the
below actually make the code harder to read:

struct hostcmd_header {

        u16 cmd;
        u16 len;
        u8 seq_num;
        u8 macid;
        u16 result;

} __packed;

and 
        if ((conf->chandef.width == NL80211_CHAN_WIDTH_20_NOHT) ||
            (conf->chandef.width == NL80211_CHAN_WIDTH_20)) {

                width = CH_20_MHz_WIDTH;
                sub_ch = NO_EXT_CHANNEL;

        } else if (conf->chandef.width == NL80211_CHAN_WIDTH_40) {

Certainly we all have own own preferences about what is more readable
for ourselves but please re-consider whether this really does aid
readability.

One other thing to consider about using checkpatch.pl is that the
changes it reports are strongly recommended to be made, based on the
kernel coding style guide, but if you feel strongly about some of the
needed changes you can try and make a case for what you believe aids
readability more so than the recommended changes.

It might not be my place to ask but I'm having some problems with the
driver. I've started to become familiar with the code and, after a
while, may have some questions. This is likely to be a little tedious
for you since mac80211 and wireless is not my area in the kernel. 

Would you mind discussing them here on the list or would you rather me
wait for the next update (don't feel obligated, I'll survive)?

Ian

On Sun, 2015-01-11 at 19:46 -0800, David Lin wrote:
> Hi Jiri,
> 
> 	It is always nice to include all needed headers in all .c files. For example when you use skb_queue_head_init please include <linux/skbuff.h>
> 	Spotted a comment typo in mwl_fwdl.c:
> 	"This file implements frimware downloa related functions"
> 	"MODULE_" macros are usually placed by the end of file.
> 
> 	--> Modifications should be included in next release.
> 
> 	Drop comments like:
> 	/* PRIVATE FUNCTION DEFINITION
> 	*/
> 	Not needed.
> 
> 	--> Unless it is really forbidden, I will keep this comment.
> 
> 	I would suggest you to develop your driver inside net-next git:
> 	http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/
> 	That would save you time during potential forward port need.
> 	Also, if you use scripts/checkpatch.pl script to check your patches, it will reveal many problems to you. The obvious one is lines longer than 80 chars.
> 	Module parameters should be always avoided if possible.
> 	I suggest you make patch against net-next and send it on linux-wireless at vger.kernel.org and netdev at vger.kernel.org mailing lists.
> 	You will get much more reviewers there.
> 
> 	--> After major problems reported from community are fixed, I will try to apply patch against net-next as suggested by you. Lines longer than 80 chars and avoiding module parameters would be checked at that time.
> 
> 	Note: Once if a version is ready for release, I will add patch to https://github.com/kaloz/mwlwifi.
> 
> Regards
> David
> 
> -----Original Message-----
> From: Jiri Pirko [mailto:jiri at resnulli.us] 
> Sent: Thursday, January 01, 2015 2:55 AM
> To: David Lin
> Cc: openwrt-devel at lists.openwrt.org; Imre Kaloz
> Subject: Re: [OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip
> 
> Wed, Dec 24, 2014 at 05:04:29PM CET, dlin at marvell.com wrote:
> >Hi all,
> >
> >
> >
> >Marvell is pleased to announce this open source driver for the 88W8864 chip as a joint effort with Linksys.  The 88W8864 features 4x4 MIMO 3-spatial Stream Dual-band 802.11ac enabling 1.3Gbps WLAN PHY rate and support for 80/40/20 MHz channel bandwidths. The 88W8864 is used in the Linksys WRT1900AC and other products.
> >
> >
> >
> >Please note that this is an initial release and we plan to send the driver upstream after cleanup.
> >
> >
> >
> >We do appreciate your feedback and look forward to serving the community with exciting new offerings in the future!
> 
> 
> Hi.
> 
> I took a very quick peek at the sources you sent.
> 
> First I would like to thank you for taking the upstream way with this.
> Much appreciated.
> 
> I would suggest you to develop your driver inside net-next git:
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/
> 
> That would save you time during potential forward port need.
> 
> Also, if you use scripts/checkpatch.pl script to check your patches, it will reveal many problems to you. The obvious one is lines longer than 80 chars.
> 
> It is always nice to include all needed headers in all .c files. For example when you use skb_queue_head_init please include <linux/skbuff.h>
> 
> Spotted a comment typo in mwl_fwdl.c:
> "This file implements frimware downloa related functions"
> 
> Drop comments like:
> /* PRIVATE FUNCTION DEFINITION
> */
> Not needed.
> 
> "MODULE_" macros are usually placed by the end of file.
> 
> Module parameters should be always avoided if possible.
> 
> 
> I suggest you make patch against net-next and send it on linux-wireless at vger.kernel.org and netdev at vger.kernel.org mailing lists.
> You will get much more reviewers there.
> 
> 
> Please do not hesitate to email me in case I can be of any more help.
> 
> Thanks!
> 
> Jiri
> 
> >
> >
> >
> >Thanks, David
> >
> 
> 
> >_______________________________________________
> >openwrt-devel mailing list
> >openwrt-devel at lists.openwrt.org
> >https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel