On 12/10/18 11:44, Bjørn Mork wrote:
Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> writes:
+static inline struct qmi_wwan_state *usbnet_to_qmi(struct usbnet *usb)
+{
+ return (void *) &usb->data;
+}
checkpatch doesn't like this, and it is also inconsistent with the
coding style elsewhere in the driver.
Thank you for pointing this out, will fix in v2.
CHECK: No space is necessary after a cast
#30: FILE: drivers/net/usb/qmi_wwan.c:81:
+ return (void *) &usb->data;
total: 0 errors, 0 warnings, 1 checks, 115 lines checked
And as for consistency: I realize that "dev" is a very generic name,
but it's used consistendly for all struct usbnet pointers in the driver:
Ok, I'll change it.
bjorn@miraculix:/usr/local/src/git/linux$ git grep 'struct usbnet' drivers/net/usb/qmi_wwan.c
drivers/net/usb/qmi_wwan.c:static struct net_device *qmimux_find_dev(struct usbnet *dev, u8 mux_id)
drivers/net/usb/qmi_wwan.c:static bool qmimux_has_slaves(struct usbnet *dev)
drivers/net/usb/qmi_wwan.c:static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(net);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = netdev_priv(to_net_dev(d));
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_manage_power(struct usbnet *dev, int on)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_register_subdriver(struct usbnet *dev)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_change_dtr(struct usbnet *dev, bool on)
drivers/net/usb/qmi_wwan.c:static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
drivers/net/usb/qmi_wwan.c: BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) <
drivers/net/usb/qmi_wwan.c:static void qmi_wwan_unbind(struct usbnet *dev, struct usb_interface *intf)
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
drivers/net/usb/qmi_wwan.c: struct usbnet *dev = usb_get_intfdata(intf);
The name was chosen to be consistent with the cdc_ether usage. I'd
prefer it if this function could use the "dev" name, to avoid confusing
things unnecessarly with yet another generic name like "usb". Which
isn't used anywhere else in the driver, and could just as easily refer
to a struct usb_device or struct usb_interface.
Ok, should be fairly easy to change this.
(yes, I know there are a couple of "struct usb_device *dev" cases. But
I'd rather see those renamed TBH)
I'd rather not touch that at the moment, this is already gaining complexity.
I also don't see why this function should be qmi_wwan specific. No need
to duplicate the same function in every minidriver, when you can do with
two generic helpers (maybe with more meaningful names):
I personally prefer the return type to be specifically the private
data type for the driver. It makes it a bit easier to spot when
you don't get the cast right.
The functions below are the idea I am working towards for the
usbnet driver, I was trying to avoid doing everything in one
go. Making the accessor functions means the next round of changes
can be much smaller, touching 1-2 areas per driver.
static inline void *usbnet_priv(const struct usbnet *dev)
{
return (void *)&dev->data;
}
static inline void *usbnet_priv0(const struct usbnet * *dev)
{
return (void *)dev->data[0];
}
This is probably the end-game of the patch series, just need to
look at a couple of issues and see if there's anything better
than can be done.
I might also add a usbnet_set_privdata(dev, data) or something
like usbnet_alloc_privdata(dev, sizeof(data).
Note, there's also the fun here of the usbnet having private data
for itself, and these sub-drivers also having their own private
data... this probably needs to be named carefully.
Please fix the checkpatch warning and consider the other comments.
Personally I don't really think it makes the code any easier to read.
But if you want it, then I'm fine this. I guess I've grown too used to
seeing those casts ;-)
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
