Re: [PATCH v4] HID: New hid-cp2112 driver

[Joe Perches <joe@perches.com>]

On Wed, 2014-01-29 at 17:26 -0600, David Barksdale wrote:
> This patch adds support for the Silicon Labs CP2112
> "Single-Chip HID USB to SMBus Master Bridge."

Just some trivial notes:

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
[]
> +struct __attribute__ ((__packed__)) cp2112_smbus_config_report {

Please use

struct name {
	...
} __packed;

This allows a simpler grep pattern to find the
struct definition.  There are many of these.

[]

> +static int cp2112_hid_get(struct hid_device *hdev, unsigned char report_number,
> +			  u8 *data, size_t count, unsigned char report_type)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmalloc(count, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hdev->hid_get_raw_report(hdev, report_number, buf, count,
> +				       report_type);
> +	memcpy(data, buf, count);
> +	kfree(buf);
> +	return ret;

if the data is going to be copied in data,
why not just use data in hid_get_raw_report
and avoid the malloc?

> +static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
> +			     unsigned char report_type)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmemdup(data, count, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> +	kfree(buf);
> +	return ret;

same question.

> +static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> +		       unsigned short flags, char read_write, u8 command,
> +		       int size, union i2c_smbus_data *data)
> +{
> +	struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
> +	struct hid_device *hdev = dev->hdev;
> +	u8 buf[64];
> +	__be16 word;
> +	size_t count;
> +	size_t read_length = 0;
> +	size_t timeout;

size_t is an odd type for a variable named timeout

[]

> +	for (timeout = 0; timeout < XFER_TIMEOUT; ++timeout) {
> +		ret = cp2112_xfer_status(dev);
> +		if (-EBUSY == ret)
> +			continue;
> +		if (ret < 0)
> +			goto power_normal;
> +		break;
> +	}

and it looks more like an maximum attempt count
than a timeout.

> +
> +	if (XFER_TIMEOUT <= timeout) {
> +		hid_warn(hdev, "Transfer timed out, cancelling.\n");
> +		buf[0] = CP2112_CANCEL_TRANSFER;
> +		buf[1] = 0x01;

[]

> +#define CP2112_CONFIG_ATTR(name, store, format, ...) \
> +static ssize_t name##_store(struct device *kdev, \
> +			    struct device_attribute *attr, const char *buf, \
> +			    size_t count) \
> +{ \
> +	struct hid_device *hdev = container_of(kdev, struct hid_device, dev); \
> +	struct cp2112_usb_config_report cfg; \
> +	int ret = cp2112_get_usb_config(hdev, &cfg); \
> +	if (ret) \
> +		return ret; \
> +	store; \
> +	ret = cp2112_set_usb_config(hdev, &cfg); \
> +	if (ret) \
> +		return ret; \
> +	chmod_sysfs_attrs(hdev); \
> +	return count; \
> +} \
> +static ssize_t name##_show(struct device *kdev, \
> +			   struct device_attribute *attr, char *buf) \
> +{ \
> +	struct hid_device *hdev = container_of(kdev, struct hid_device, dev); \
> +	struct cp2112_usb_config_report cfg; \
> +	int ret = cp2112_get_usb_config(hdev, &cfg); \
> +	if (ret) \
> +		return ret; \
> +	return scnprintf(buf, PAGE_SIZE, format, __VA_ARGS__); \

##__VA_ARGS__ would probably be better


> +} \
> +DEVICE_ATTR_RW(name);
> +
> +CP2112_CONFIG_ATTR(vendor_id, ({
> +	u16 vid;
> +
> +	if (sscanf(buf, "%hi", &vid) != 1)
> +		return -EINVAL;
> +
> +	cfg.vid = cpu_to_le16(vid);
> +	cfg.mask = 0x01;
> +}), "0x%04x\n", le16_to_cpu(cfg.vid));
> +
> +CP2112_CONFIG_ATTR(product_id, ({
> +	u16 pid;
> +
> +	if (sscanf(buf, "%hi", &pid) != 1)
> +		return -EINVAL;
> +
> +	cfg.pid = cpu_to_le16(pid);
> +	cfg.mask = 0x02;
> +}), "0x%04x\n", le16_to_cpu(cfg.pid));

These uses of CP2112_CONFIG_ATTR are kind of ugly.

> +
> +CP2112_CONFIG_ATTR(max_power, ({
> +	int mA;
> +
> +	if (sscanf(buf, "%i", &mA) != 1)
> +		return -EINVAL;
> +
> +	cfg.max_power = (mA + 1) / 2;
> +	cfg.mask = 0x04;
> +}), "%u mA\n", cfg.max_power * 2);
> +
> +CP2112_CONFIG_ATTR(power_mode, ({
> +	if (sscanf(buf, "%hhi", &cfg.power_mode) != 1)
> +		return -EINVAL;
> +
> +	cfg.mask = 0x08;
> +}), "%u\n", cfg.power_mode);
> +
> +CP2112_CONFIG_ATTR(release_version, ({
> +	if (sscanf(buf, "%hhi.%hhi", &cfg.release_major, &cfg.release_minor)
> +	    != 2)
> +		return -EINVAL;
> +
> +	cfg.mask = 0x10;
> +}), "%u.%u\n", cfg.release_major, cfg.release_minor);

[]

> +static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			    u8 *data, int size)
> +{
[]
> +		case STATUS0_ERROR:
> +			switch (xfer->status1) {
> +			case STATUS1_TIMEOUT_NACK:
> +			case STATUS1_TIMEOUT_BUS:
> +				dev->xfer_status = -ETIMEDOUT;
> +				break;
> +			default:
> +				dev->xfer_status = -EIO;

nicer with a break