> -----Original Message-----
> From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Sent: Saturday, June 22, 2024 2:20 AM
> To: David Lin <yu-hao.lin@xxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; briannorris@xxxxxxxxxxxx;
> kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh
> <tsung-hsien.hsieh@xxxxxxx>
> Subject: [EXT] Re: [PATCH 00/43] wifi: nxpwifi: create nxpwifi to support iw61x
>
>
> On Fri, 2024-06-21 at 15:51 +0800, David Lin wrote:
> >
> > wifi: nxpwifi: add ioctl.h
>
> even the name here sounds questionable :)
Renamed ioctl.h and sta_ioctl.h to cfg.h and sta_cfg.h
> > 48 files changed, 34928 insertions(+)
> >
>
> This is ... huge. I don't know who could possibly review it at all.
Since this is a new driver, any suggestions on how we can make it easier for review?
> A quick look suggests that it's got a bunch of things we probably really don't
> want to do that way any more, like
>
> using semaphores in a wifi driver:
>
> > +#include <linux/semaphore.h>
Removed in V3
> having a bunch of (sometimes wrong!) element definitions in a driver:
>
> > +struct ieee_types_aid {
> ...
> > + u16 aid;
Fixed in V3
> embedding a (default?) wireless_dev when clearly the driver supports more
> than one netdev/wdev:
>
> > + struct wireless_dev wdev;
>
> Having multiple own workqueues is probably also unreasonable:
>
> > + struct workqueue_struct *dfs_cac_workqueue;
> > + struct workqueue_struct *dfs_chan_sw_workqueue;
> > + struct workqueue_struct *workqueue;
> > + struct workqueue_struct *rx_workqueue;
> > + struct workqueue_struct *host_mlme_workqueue;
>
> as is a misnamed mutex, but really you could use wiphy work and likely not
> have a mutex at all:
>
> > + /* mutex for scan */
> > + struct mutex async_mutex;
Consolidated to two workqueues and one Rx tasklet in V3.
> but also simple things like not wanting to use ERR_PTR()?
Addressed in V3
> > +static int nxpwifi_register(void *card, struct device *dev,
> > + struct nxpwifi_if_ops *if_ops, void
> > +**padapter)
>
> (padapter is an out parameter)
Fixed in V3
> Why random numbers for cookies instead of just assigning from a static
> variable:
>
> > + *cookie = get_random_u32() | 1;
Use static variable in V3
> Open-coding -EPERM?
>
> > + if (nxpwifi_deinit_priv_params(priv))
> > + return -1;
Addressed in V3
> Using -EFAULT for FW errors seems like a really bad idea:
>
> > + if (nxpwifi_drv_get_data_rate(priv, &rate)) {
> > + nxpwifi_dbg(priv->adapter, ERROR,
> > + "getting data rate error\n");
> > + return -EFAULT;
Addressed in V3
Hi Johannes,
We have addressed the comments you sent on June 22, 2024 in patches V3. Please review and let us know if you have any additional feedback or suggestions.
Thanks.
Jeff
