Messages in this thread | | | Date | Thu, 24 Sep 2020 22:07:58 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH] staging: Initial driver submission for pureLiFi devices |
| |
On Thu, Sep 24, 2020 at 08:48:51PM +0530, Srinivasan Raju wrote: > + > +#include <linux/kernel.h> > +#include <linux/errno.h> > + > +#include "def.h" > +#include "chip.h" > +#include "mac.h" > +#include "usb.h" > +#include "log.h" > + > +void purelifi_chip_init(struct purelifi_chip *chip, > + struct ieee80211_hw *hw, > + struct usb_interface *intf > + )
There is a bunch of really trivial messiness like this. It should look like:
void purelifi_chip_init(struct purelifi_chip *chip, struct ieee80211_hw *hw, struct usb_interface *intf)
> +{ > + memset(chip, 0, sizeof(*chip)); > + mutex_init(&chip->mutex); > + purelifi_usb_init(&chip->usb, hw, intf); > +} > + > +void purelifi_chip_clear(struct purelifi_chip *chip) > +{ > + PURELIFI_ASSERT(!mutex_is_locked(&chip->mutex)); > + purelifi_usb_clear(&chip->usb); > + mutex_destroy(&chip->mutex); > + PURELIFI_MEMCLEAR(chip, sizeof(*chip));
Get rid of the PURELIFI_MEMCLEAR() macro. The weird thing about PURELIFI_MEMCLEAR() is that sometimes it's a no-op. It seems unnecessary to memset() the struct here.
I'm not a fan of all these tiny functions. It feels like I have to jump around a lot to understand the code. What does "clear" mean in this context. Probably "release" is a better name.
> +} > + > +static int scnprint_mac_oui(struct purelifi_chip *chip, char *buffer, > + size_t size) > +{ > + u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip)); > + > + return scnprintf(buffer, size, "%02x-%02x-%02x", > + addr[0], addr[1], addr[2]); > +} > + > +/* Prints an identifier line, which will support debugging. */ > +static int scnprint_id(struct purelifi_chip *chip, char *buffer, size_t size)
This function name is too vague. What ID is it printing?
> +{ > + int i = 0;
The initialization is not required. "i" means "iterator". This should be "cnt" instead.
> + > + i = scnprintf(buffer, size, "purelifi%s chip ", ""); > + i += purelifi_usb_scnprint_id(&chip->usb, buffer + i, size - i); > + i += scnprintf(buffer + i, size - i, " "); > + i += scnprint_mac_oui(chip, buffer + i, size - i); > + i += scnprintf(buffer + i, size - i, " "); > + return i;
This is an example of how tiny functions obfuscate the code. It should be written like this:
static void print_whatever(struct purelifi_chip *chip) { u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip)); struct usb_device *udev = interface_to_usbdev(chip->usb.intf);
pr_info("purelifi chip 04hx:%04hx v%04hx %s %02x-%02x-%02x\n", le16_to_cpu(udev->descriptor.idVendor), le16_to_cpu(udev->descriptor.idProduct), get_bcd_device(udev), speed(udev->speed), addr[0], addr[1], addr[2]); }
> +} > + > +static void print_id(struct purelifi_chip *chip) > +{ > + char buffer[80]; > + > + scnprint_id(chip, buffer, sizeof(buffer)); > + buffer[sizeof(buffer) - 1] = 0;
snprintf() functions alway put a NUL terminator on the end of the string.
> + pl_dev_info(purelifi_chip_dev(chip), "%s\n", buffer); > +} > + > +/* MAC address: if custom mac addresses are to be used CR_MAC_ADDR_P1 and > + * CR_MAC_ADDR_P2 must be overwritten > + */ > +int purelifi_write_mac_addr(struct purelifi_chip *chip, const u8 *mac_addr) > +{ > + int r; > + > + r = usb_write_req(mac_addr, ETH_ALEN, USB_REQ_MAC_WR); > + return r;
Delete the "r" variable.
return usb_write_req(mac_addr, ETH_ALEN, USB_REQ_MAC_WR);
Again, I'm not a huge fan of one line functions for no reason. Actually, the function is never called. Just delete it.
> +} > + > +int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval, > + u8 dtim_period, int type) > +{ > + int r; > + > + if (!interval || (chip->beacon_set && > + chip->beacon_interval == interval)) { > + return 0; > + } > + > + chip->beacon_interval = interval; > + chip->beacon_set = true; > + r = usb_write_req((const u8 *)&chip->beacon_interval, > + sizeof(chip->beacon_interval), > + USB_REQ_BEACON_INTERVAL_WR); > + return r;
Delete the "r" variable.
> +} > + > +static int hw_init(struct purelifi_chip *chip) > +{ > + return purelifi_set_beacon_interval(chip, 100, 0, 0); > +}
This is a oneline function which is only called once. Move it inline.
> + > +int purelifi_chip_init_hw(struct purelifi_chip *chip) > +{ > + int r; > + > + r = hw_init(chip); > + if (r) > + goto out;
Just return directly. The little bunny hop doesn't add anything.
> + > + print_id(chip); > +out: > + return r; > +}
Anyway, those are some ideas.
regards, dan carpenter
| |