Re: [PATCH 04/19] wilc: add host_interface.c

From: Johannes Berg <johannes@sipsolutions.net>
To: Adham.Abozaeid@microchip.com, Ajay.Kathat@microchip.com,
	linux-wireless@vger.kernel.org
Cc: kvalo@codeaurora.org, gregkh@linuxfoundation.org,
	Ganesh.Krishna@microchip.com, Aditya.Shankar@microchip.com,
	Venkateswara.Kaja@microchip.com, Claudiu.Beznea@microchip.com
Subject: Re: [PATCH 04/19] wilc: add host_interface.c
Date: Thu, 18 Oct 2018 10:23:17 +0200	[thread overview]
Message-ID: <627fbc583ac6679cc9986959d84e15467eaea897.camel@sipsolutions.net> (raw)
In-Reply-To: <5BC11842.1050700@microchip.com>

On Fri, 2018-10-12 at 21:55 +0000, Adham.Abozaeid@microchip.com wrote:
> 
> Here the driver is configuring parameters in the device by sending a
> WID command for each parameters.
> The val pointer points to the value of the parameter to be set, and
> here all parameters being set to 0 were sharing the dummyval variable.

Ok.

> Looking again at this, these constant parameters can be omitted from
> the driver and done on the device instead.

I think it's fine, just the pointers look strange. Would probably be
different once you resolve the middle layer though.

> > > +	if (conn_attr->bssid)
> > > +		memcpy(cur_byte, conn_attr->bssid, 6);
> > > +	cur_byte += 6;
> > 
> > u8 bssid[ETH_ALEN];
> > 
> > > +	if (conn_attr->bssid)
> > > +		memcpy(cur_byte, conn_attr->bssid, 6);
> > > +	cur_byte += 6;
> > 
> > again?
> 
> Agree. Can be changed to avoid duplication. Requires a matching change on the device.

Again, like above, don't worry too much about changing the
device/firmware. I was just pointing out it's duplicate, if that's the
way it is then too bad, but it doesn't really matter.

The more interesting thing here is to change it to use a struct and not
pack it manually.

johannes