Re: [OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip
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 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-wirel...@vger.kernel.org and net...@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:j...@resnulli.us] Sent: Thursday, January 01, 2015 2:55 AM To: David Lin Cc: openwrt-devel@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, d...@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 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-wirel...@vger.kernel.org and net...@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@lists.openwrt.org >https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip
Hi Ian,
1. I will take off these additional blank lines.
2. I work for problems reported from community. If you do find problems
for the driver, please let me know.
Thanks
David
-Original Message-
From: Ian Kent [mailto:ra...@themaw.net]
Sent: Tuesday, January 13, 2015 10:41 AM
To: David Lin
Cc: Jiri Pirko; openwrt-devel@lists.openwrt.org; Imre Kaloz
Subject: Re: [OpenWrt-Devel] Announcement of mac80211 driver support for
Marvell 88W8864 chip
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
> 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-wirel...@vger.kernel.org and net...@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:j...@resnulli.us]
> Sent: Thursday, January 01, 2015 2:55 AM
> To: David Lin
> Cc: openwrt-devel@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, d...@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 th
Re: [OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip
Hi Ian, Thanks for the feedback. 1. Client mode support is already in the work. We will plan to release it as soon as available. It should be in time for coming WRT1200AC. 2. We will add description to the rate related info. Thanks, David -Original Message- From: Ian Kent [mailto:ra...@themaw.net] Sent: Saturday, January 24, 2015 5:37 PM To: David Lin Cc: Jiri Pirko; openwrt-devel@lists.openwrt.org; Imre Kaloz; Chor Teck Law Subject: Re: [OpenWrt-Devel] Announcement of mac80211 driver support for Marvell 88W8864 chip On Mon, 2015-01-12 at 19:03 -0800, David Lin wrote: > Hi Ian, > > 1. I will take off these additional blank lines. > 2. I work for problems reported from community. If you do find problems > for the driver, please let me know. Hi David, I was disappointed to find that client mode wireless in the driver isn't supported yet. I logged an issue for it at https://github.com/kaloz/mwlwifi/issues/13 along with the kernel WARN() info. As I say in the issue I believe that client mode is quite important for the coming WRT1200AC, assuming the wireless on that device is to be supported by this driver. I've been looking at the driver source and I'm a bit puzzled about the rate and ht_sig2 fields in the receive descriptor structure. There's no documentation (that I can locate) on the interface to the wireless device so I can't refer to it. It appears the rate field isn't just a rate or an index into the rate table or just an MCS rate either. So can you describe what the encoding is please? The ht_sig2 field has a describing comment of "as the name suggests" but HT SIG 2 is 24 bits not 16 and even if the CRC and tail fields were discarded, I see there's more than the remaining 10 bits being used in the field. So can you describe the encoding in this field too please? The other puzzling thing is the handling of VHT modes. I don't see anything in the receive descriptor for that case. It looks like the wireless device handles the rate (along with a lot more) but how does the driver find out the VHT information that it may need for informational and reporting purposes (up to the mac80211 layer). Could you help with this please. These questions may sound like I don't know low level wireless concepts and that would be correct, but I'm slowly working on that, so sorry for the possibly silly questions. Ian ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
