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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
