Re: [PATCH] staging: Initial driver submission for pureLiFi devices

[Dan Carpenter <dan.carpenter@oracle.com>]

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