Message-ID:
 <PA4PR04MB9638D0C97ED064776855E2ABD18B2@PA4PR04MB9638.eurprd04.prod.outlook.com>
Date: Mon, 26 Aug 2024 05:49:01 +0000
From: David Lin <yu-hao.lin@....com>
To: Calvin Owens <calvin@...nvd.org>
CC: Sascha Hauer <s.hauer@...gutronix.de>, "linux-wireless@...r.kernel.org"
	<linux-wireless@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "kvalo@...nel.org" <kvalo@...nel.org>,
	"johannes@...solutions.net" <johannes@...solutions.net>,
	"briannorris@...omium.org" <briannorris@...omium.org>, "francesco@...cini.it"
	<francesco@...cini.it>, Pete Hsieh <tsung-hsien.hsieh@....com>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: RE: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create nxpwifi to
 support iw61x

> From: Calvin Owens <calvin@...nvd.org>
> Sent: Monday, August 26, 2024 1:31 PM
> To: David Lin <yu-hao.lin@....com>
> Cc: Sascha Hauer <s.hauer@...gutronix.de>; linux-wireless@...r.kernel.org;
> linux-kernel@...r.kernel.org; kvalo@...nel.org; johannes@...solutions.net;
> briannorris@...omium.org; francesco@...cini.it; Pete Hsieh
> <tsung-hsien.hsieh@....com>; kernel@...gutronix.de; calvin@...nvd.org
> Subject: Re: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create nxpwifi to support
> iw61x
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> On Monday 08/26 at 02:56 +0000, David Lin wrote:
> > > From: Calvin Owens <calvin@...nvd.org>
> > > Sent: Monday, August 26, 2024 10:50 AM
> > > To: David Lin <yu-hao.lin@....com>
> > > Cc: Sascha Hauer <s.hauer@...gutronix.de>;
> > > linux-wireless@...r.kernel.org; linux-kernel@...r.kernel.org;
> > > kvalo@...nel.org; johannes@...solutions.net;
> > > briannorris@...omium.org; francesco@...cini.it; Pete Hsieh
> > > <tsung-hsien.hsieh@....com>; kernel@...gutronix.de
> > > Subject: Re: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create
> > > nxpwifi to support iw61x
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments. When in doubt, report the message
> > > using the 'Report this email' button
> > >
> > >
> > > On Monday 08/26 at 02:33 +0000, David Lin wrote:
> > > > > From: Calvin Owens <calvin@...nvd.org>
> > > > > Sent: Monday, August 26, 2024 4:38 AM
> > > > > To: Sascha Hauer <s.hauer@...gutronix.de>
> > > > > Cc: David Lin <yu-hao.lin@....com>;
> > > > > linux-wireless@...r.kernel.org; linux-kernel@...r.kernel.org;
> > > > > kvalo@...nel.org; johannes@...solutions.net;
> > > > > briannorris@...omium.org; francesco@...cini.it; Pete Hsieh
> > > > > <tsung-hsien.hsieh@....com>; kernel@...gutronix.de;
> > > > > calvin@...nvd.org
> > > > > Subject: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create
> > > > > nxpwifi to support iw61x
> > > > >
> > > > > Caution: This is an external email. Please take care when
> > > > > clicking links or opening attachments. When in doubt, report the
> > > > > message using the 'Report this email' button
> > > > >
> > > > >
> > > > > On Thursday 08/22 at 14:56 +0200, Sascha Hauer wrote:
> > > > > > On Fri, Aug 09, 2024 at 05:44:50PM +0800, David Lin wrote:
> > > > > > > This series adds support for IW61x which is a new family of
> > > > > > > 2.4/5 GHz dual-band 1x1 Wi-Fi 6, Bluetooth/Bluetooth Low
> > > > > > > Energy
> > > > > > > 5.2 and
> > > > > > > 15.4 tri-radio single chip by NXP. These devices support
> > > > > > > 20/40/80MHz single spatial stream in both STA and AP mode.
> > > > > > > Communication to the IW61x is done via SDIO interface
> > > > > > >
> > > > > > > This driver is a derivative of existing Mwifiex [1] and
> > > > > > > based on similar full-MAC architecture [2]. It has been
> > > > > > > tested with i.MX8M Mini evaluation kits in both AP and STA mode.
> > > > > > >
> > > > > > > All code passes sparse and checkpatch
> > > > > > >
> > > > > > > Data sheet (require registration):
> > > > > > > https://ww/
> > > > > > > w.nxp.com%2Fproducts%2Fwireless-connectivity%2Fwi-fi-plus-bl
> > > > > > > ueto
> > > > > > > oth-
> > > > > > >
> > > > >
> > >
> &data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cff25728795724a618a5208dcc5
> > > > > 45c
> > > > > > >
> > > > >
> > >
> 5fd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63860215067862
> > > > > 3224%
> > > > > > >
> > > > >
> > >
> 7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> > > > > TiI6
> > > > > > >
> > > > >
> > >
> Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=U0Cej8ysBD%2Fg1Sa4Ia
> > > > > Ph63Ot
> > > > > > > iTcemadiCfMINYM%2BRL4%3D&reserved=0
> > > > > > > plus-802-15-4/2-4-5-ghz-dual-band-1x1-wi-fi-6-802-11ax-plus-
> > > > > > > blue
> > > > > > > toot
> > > > > > > h-5-
> > > > > > > 4-plus-802-15-4-tri-radio-solution:IW612
> > > > > > >
> > > > > > > Known gaps to be addressed in the following patches,
> > > > > > >   - Enable 11ax capabilities. This initial patch support up to 11ac.
> > > > > > >   - Support DFS channel. This initial patch doesn't support
> > > > > > > DFS channel
> > > in
> > > > > > >     both AP/STA mode.
> > > > > > >
> > > > > > > This patch is presented as a request for comment with the
> > > > > > > intention of being made into a patch after initial feedbacks
> > > > > > > are addressed
> > > > > > >
> > > > > > > [1] We had considered adding IW61x to mwifiex driver, however
> due to
> > > > > > >     FW architecture, host command interface and supported
> > > > > > > features
> > > are
> > > > > > >     significantly different, we have to create the new nxpwifi
> driver.
> > > > > > >     Subsequent NXP chipsets will be added and sustained in
> > > > > > > this new
> > > > > driver.
> > > > > >
> > > > > > I added IW61x support to the mwifiex driver and besides the
> > > > > > VDLL handling which must be added I didn't notice any differences.
> > > > > > There might be other differences, but I doubt that these can't
> > > > > > be integrated into the mwifiex driver.
> > > > >
> > > > > Hi Sascha,
> > > > >
> > > > > I'd also love to see this patchset, if you're able to share it.
> > > > > I can test on an
> > > > > IW612 if that's helpful at all.
> > > > >
> > > > > > Honestly I don't think adding a new driver is a good ideai,
> > > > > > given how big wifi drivers are and how limited the review bandwidth
> is.
> > > > > >
> > > > > > What we'll end up with is that we'll receive the same patches
> > > > > > for both drivers, or worse, only for one driver while the
> > > > > > other stays
> > > unpatched.
> > > > >
> > > > > I have some concrete experience with "in-tree driver forks" like this:
> > > > > a pair of SCSI drivers named mpt2sas and mpt3sas.
> > > > >
> > > > > The latter was created as a near copy of the former:
> > > > >
> > > > >
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%25
> > > > >
> 2Fgi%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cfeaa265eadca4cef88
> > > > >
> 3608dcc5903c0d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 6
> > > > >
> 02470501904923%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QI
> > > > >
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=7uf
> > > > > Rl6fwgw1oxLa4DBQxpwrm9D6ojbdFYNAuHRCA5vk%3D&reserved=0
> > > > >
> > >
> t.kernel%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C582c3c0573b74f83
> > > 42
> > > > >
> > >
> 3408dcc579bc4a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > > 60237
> > > > >
> > >
> 3871805816%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > > V2luM
> > > > >
> > >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=bLjsRTRsR%2
> > > BTtA
> > > > > jUIVDY396ZF%2BIkwwUFhAubTCin3IVk%3D&reserved=0
> > > >
> > >
> > .org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fc
> > > > o
> > > m
> > > > >
> > >
> mit%2F%3Fid%3Df92363d12359&data=05%7C02%7Cyu-hao.lin%40nxp.com%7
> > > > >
> > >
> Cff25728795724a618a5208dcc545c5fd%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > > > >
> > >
> 35%7C0%7C0%7C638602150678637352%7CUnknown%7CTWFpbGZsb3d8eyJW
> > > > >
> > >
> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0
> > > > >
> > >
> %7C%7C%7C&sdata=mzrLLqJNee7vIdV47j8xVSU%2FByjh%2FnNKnRsx1nw3yNo
> > > > > %3D&reserved=0
> > > > >
> > > > > The result was *exactly* what you forsee happening here: both
> > > > > drivers were constantly missing fixes from the other, and they
> > > > > were just subtly different enough that it wasn't simple to
> > > > > "port" patches from one to the other. It was a frustrating
> > > > > experience for everybody involved. I think their git histories
> > > > > prove your point, I'd encourage everyone with a horse in this race to
> take a look at them.
> > > > >
> > > > > It took three years to finally unify them:
> > > > >
> > > > >
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%25
> > > > >
> 2Fgi%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cfeaa265eadca4cef88
> > > > >
> 3608dcc5903c0d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 6
> > > > >
> 02470501916954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QI
> > > > >
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=wbG
> > > > > SKlaMVBnzXon1ZEEAmctWCBCkVDFqWld9d6zLvOQ%3D&reserved=0
> > > > >
> > >
> t.kernel%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C582c3c0573b74f83
> > > 42
> > > > >
> > >
> 3408dcc579bc4a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > > 60237
> > > > >
> > >
> 3871815005%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > > V2luM
> > > > >
> > >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=RfY6N6WWXI
> > > n0gZP
> > > > > SBoRySz5eeU8WkFH2HvFHLVNgu3Q%3D&reserved=0
> > > >
> > >
> > .org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fc
> > > > o
> > > m
> > > > >
> > >
> mit%2F%3Fid%3Dc84b06a48c4d&data=05%7C02%7Cyu-hao.lin%40nxp.com%7
> > > > >
> > >
> Cff25728795724a618a5208dcc545c5fd%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > > > >
> > >
> 35%7C0%7C0%7C638602150678649431%7CUnknown%7CTWFpbGZsb3d8eyJW
> > > > >
> > >
> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0
> > > > >
> > >
> %7C%7C%7C&sdata=UGjDfngO1POWuydIfmOL%2BR%2BqJ1BoDQW6NboQUV
> > > > > q2Xh8%3D&reserved=0
> > > > >
> > > > > I doubt anyone would disagree that wifi drivers are much more
> > > > > complex than SCSI drivers. It would be strictly *worse* here,
> > > > > and the path to unifying them strictly longer.
> > > > >
> > > > > Thanks,
> > > > > Calvin
> > > > >
> > > >
> > > > I think Nxpwifi will support NXP new WiFi chips and Mwifiex will
> > > > support
> > > existed NXP WiFi chips.
> > >
> > > Hi David,
> > >
> > > I understand that. I don't think that really changes anything: there
> > > will still be many future patches which need to be applied to both,
> > > because the bug being fixed existed before the fork. As the forked
> > > driver diverges, that will only become more difficult and error prone.
> > >
> > > Thanks,
> > > Calvin
> > >
> >
> > Nxpwifi is not only a fork from Mwifiex. Especially after we modified
> > Nxpwifi based on the comments from Johannes.
>
> I understand you've done real work here. But at the same time, nearly 1/3rd of
> the changeset against the original driver is renaming things:
>
>     {0}[calvin ~/linux/drivers/net/wireless/nxp/nxpwifi] tar cf
> ~/nxpwifi-v2.tar .
>     {0}[calvin ~/linux/drivers/net/wireless/nxp/nxpwifi]
> cd ../../marvell/mwifiex
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] tar xf
> ~/nxpwifi-v2.tar
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git add *
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git diff --cached
> --shortstat
>      42 files changed, 13878 insertions(+), 14274 deletions(-)
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] sed -i
> 's/nxpwifi/mwifiex/g' *
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] sed -i
> 's/NXPWIFI/MWIFIEX/g' *
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git add *
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git diff --cached
> --shortstat
>      42 files changed, 9940 insertions(+), 10336 deletions(-)
>     {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] cat * | wc -l
>     45591
>
> I'd consider that a fork. For those following at home, I pushed a branch to
> github with nxpwifi v2 applied as a single patchbomb to mwifiex, with the
> renames backed out:
>
>
> https://github.co/
> m%2Fjcalvinowens%2Flinux%2Ftree%2Fwork%2Fnxpwifi-on-mwifiex-no-renam
> es&data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cfeaa265eadca4cef883608dcc
> 5903c0d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386024705
> 01925106%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=D%2BO4x
> pCU1OMwPYkMq7T6X9Cf95Gdt7bNhnfqNzaOyQA%3D&reserved=0
>
> For my own selfish part, I agree with Sascha: I'd prefer to see that changeset as
> patches against the existing driver, because I know from experience that
> having a community built around one driver will enable me to deliver better
> results for my users, and will make it easier for me to contribute when I find
> bugs to fix.
>
> But this is just one random opinion, I'm not who you need to convince :)
>
> Thanks,
> Calvin
>

Yes. From your commit, it is very clear that Nxpwifi has a lot of changes which are used for:

Cleanup original issues of Mwifiex for checkpatch.
Remove command response file due to command table is used (command response handler
can be hooked to command node when command is searched at first time).
Only updated API/interface, host command and SDIO new mode are supported.
Only support separate auth/assoc.
Modifications based on comments from Johannes (please check changelog for patch v2).

Nxpwifi will be a good base line to continue to add new features and new NXP WiFi new chips.
It is also clean and easy to be maintained.

Thanks,
David

Powered by blists - more mailing lists