[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

["Bjørn Mork" <bjorn@mork.no>]

Igor Russkikh <Igor.Russkikh@aquantia.com> writes:

> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			     u16 index, u16 size, void *data, int nopm)
> +{
> +	int ret;
> +	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> +		  u16 index, void *data, u16 size);
> +
> +	if (nopm)
> +		fn = usbnet_read_cmd_nopm;
> +	else
> +		fn = usbnet_read_cmd;
> +
> +	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +		 value, index, data, size);
> +	if (size == 2)
> +		le16_to_cpus(data);
> +
> +	if (unlikely(ret < 0))
> +		netdev_warn(dev->net,
> +			    "Failed to read(0x%x) reg index 0x%04x: %d\n",
> +			    cmd, index, ret);
> +	return ret;
> +}
> +
> +static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> +				u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
> +}
> +
> +static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			   u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
> +}
> +

Why would you want to do something like this instead of simply
implementing aqc111_read_cmd_nopm() and aqc111_read_cmd() as separate
functions?  The function pointer stuff is incredibly ugly, as Oliver
pointed out.  It wasn't done like that in usbnet.c, so why should we do
it like that here?

And the "if (size == 2) le16_to_cpus(data)" looks like something that
will come back and haunt you.  Will this code never read larger
integers?  Maybe add some sanity checks then, just in case...

Or simply add more helpers.  An additional pair of helpers for reading
16bit integers might simplify your code quite a bit.


Bjørn