On Monday 08/26 at 02:56 +0000, David Lin wrote: > > From: Calvin Owens <calvin@xxxxxxxxxx> > > Sent: Monday, August 26, 2024 10:50 AM > > To: David Lin <yu-hao.lin@xxxxxxx> > > Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx; kvalo@xxxxxxxxxx; johannes@xxxxxxxxxxxxxxxx; > > briannorris@xxxxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh > > <tsung-hsien.hsieh@xxxxxxx>; kernel@xxxxxxxxxxxxxx > > 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@xxxxxxxxxx> > > > > Sent: Monday, August 26, 2024 4:38 AM > > > > To: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > > > Cc: David Lin <yu-hao.lin@xxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx; > > > > linux-kernel@xxxxxxxxxxxxxxx; kvalo@xxxxxxxxxx; > > > > johannes@xxxxxxxxxxxxxxxx; briannorris@xxxxxxxxxxxx; > > > > francesco@xxxxxxxxxx; Pete Hsieh <tsung-hsien.hsieh@xxxxxxx>; > > > > kernel@xxxxxxxxxxxxxx; calvin@xxxxxxxxxx > > > > 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-blueto > > > > > > 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%2Fgi > > > > > > 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%2Fco > > 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%2Fgi > > > > > > 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%2Fco > > 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.com/jcalvinowens/linux/tree/work/nxpwifi-on-mwifiex-no-renames 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 > I think the real bugs fixes of Mwifiex will become less frequently. > We can monitor these patches and apply them from Mwifiex to Nxpwifi. > > If we fix issues of Nxpwifi which is also related to Mwifiex, we will > submit patches back to Mwifiex. > > Thanks, > David >
