RE: [EXT] Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode

David Lin
2024-03-19T01:36:05Z

> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Monday, March 18, 2024 7:42 PM
> To: David Lin <yu-hao.lin@nxp.com>
> Cc: Brian Norris <briannorris@chromium.org>; linux-wireless@vger.kernel.org;
> linux-kernel@vger.kernel.org; kvalo@kernel.org; francesco@dolcini.it; Pete
> Hsieh <tsung-hsien.hsieh@nxp.com>; Francesco Dolcini
> <francesco.dolcini@toradex.com>
> Subject: [EXT] Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode
>
> 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
>
>
> Hello David,
>
> On Mon, Mar 18, 2024 at 02:00:35AM +0000, David Lin wrote:
> > > From: Brian Norris <briannorris@chromium.org>
> ...
> > > > .../net/wireless/marvell/mwifiex/sta_ioctl.c | 2 +-
> > > > drivers/net/wireless/marvell/mwifiex/sta_tx.c | 9 +-
> > > > drivers/net/wireless/marvell/mwifiex/util.c | 80 +++++
> > > > 15 files changed, 671 insertions(+), 14 deletions(-)
> > >
> > > (Per the above, I'd normally consider whether ~671 new lines is
> > > worth splitting into multiple patches. But I don't see any great
> > > logical ways to do that.)
> > Francesco suggested to use two patches for this host mlme new feature
> > from previous many patches. I knew it is a lot of changes, but I think
> > it should be the best way to add host mlme with two patches (one for
> > client and one for AP).
>
> What I explicitly asked was to not add code in a patch, and fix the newly added
> code in a following patch. What you are supposed to do is to just amend the
> original code when you get review feedback.
>

Yes. I will do that for patch v10 and keep all 'Review-by:' and 'Tested-by:' tags.

> Splitting a big patch into multiple patches is welcome to easier review, and this
> needs to be done breaking down in logical pieces keeping in mind also
> bisect-ability.
>
> This [1] is an example of the addition of a relatively big new driver, and you
> can see that the series is broken down in smaller patches like "Add skeleton
> PowerVR driver", with intermediate steps that were non-functional, but they
> were building fine, they were correct and they were enabling more effective
> code review.
>
> Unfortunately, as Brian agreed here, there was no easy way to do it for this
> patch.
>
> Francesco
>
> [1]
> https://lore.kern/
> el.org%2Fall%2Fcover.1700668843.git.donald.robson%40imgtec.com%2F&data
> =05%7C02%7Cyu-hao.lin%40nxp.com%7C6ce04f812a5b4dd7e74908dc47405cc
> 7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384635889617189
> 65%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=HPgEuiKfmtEicp
> PfTz57piOdOoT0iQfo5qnPp9p6jlY%3D&reserved=0