lkml.org 
[lkml]   [2014]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4] HID: New hid-cp2112 driver
From
Date
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




\
 
 \ /
  Last update: 2014-01-30 01:41    [W:0.074 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site