Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.

Dismiss

[PATCH RESEND] HID: New hid-cp2112 driver.

501 views

Skip to first unread message

David Barksdale

unread,

Aug 27, 2013, 11:10:02 AM8/27/13

This patch adds support for the Silicon Labs CP2112
"Single-Chip HID USB to SMBus Master Bridge."

I wrote this to support a USB temperature and humidity
sensor I've been working on. It's been tested by using
SMBus byte-read, byte-data-read/write, and word-data-read
transfer modes to talk to an I2C sensor. The other
transfer modes have not been tested.

Signed-off-by: David Barksdale <dbark...@uplogix.com>

---
drivers/hid/Kconfig | 6 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-core.c | 3 +
drivers/hid/hid-cp2112.c | 504 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 514 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 14ef6ab..1833948 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -175,6 +175,12 @@ config HID_PRODIKEYS
multimedia keyboard, but will lack support for the musical keyboard
and some additional multimedia keys.

+config HID_CP2112
+ tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support"
+ depends on USB_HID
+ ---help---
+ Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge.
+
config HID_CYPRESS
tristate "Cypress mouse and barcode readers" if EXPERT
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 6f68728..a88a5c4 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
obj-$(CONFIG_HID_BELKIN) += hid-belkin.o
obj-$(CONFIG_HID_CHERRY) += hid-cherry.o
obj-$(CONFIG_HID_CHICONY) += hid-chicony.o
+obj-$(CONFIG_HID_CP2112) += hid-cp2112.o
obj-$(CONFIG_HID_CYPRESS) += hid-cypress.o
obj-$(CONFIG_HID_DRAGONRISE) += hid-dr.o
obj-$(CONFIG_HID_EMS_FF) += hid-emsff.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 36668d1..b472a0d 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1568,6 +1568,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
+#if IS_ENABLED(CONFIG_HID_CP2112)
+ { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, 0xEA90) },
+#endif
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_1) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
new file mode 100644
index 0000000..8737d18
--- /dev/null
+++ b/drivers/hid/hid-cp2112.c
@@ -0,0 +1,504 @@
+/*
+ hid-cp2112.c - Silicon Labs HID USB to SMBus master bridge
+ Copyright (c) 2013 Uplogix, Inc.
+ David Barksdale <dbark...@uplogix.com>
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/hid.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include "hid-ids.h"
+
+enum {
+ GET_VERSION_INFO = 0x05,
+ SMBUS_CONFIG = 0x06,
+ DATA_READ_REQUEST = 0x10,
+ DATA_WRITE_READ_REQUEST = 0x11,
+ DATA_READ_FORCE_SEND = 0x12,
+ DATA_READ_RESPONSE = 0x13,
+ DATA_WRITE_REQUEST = 0x14,
+ TRANSFER_STATUS_REQUEST = 0x15,
+ TRANSFER_STATUS_RESPONSE = 0x16,
+ CANCEL_TRANSFER = 0x17,
+};
+
+enum {
+ STATUS0_IDLE = 0x00,
+ STATUS0_BUSY = 0x01,
+ STATUS0_COMPLETE = 0x02,
+ STATUS0_ERROR = 0x03,
+};
+
+enum {
+ STATUS1_TIMEOUT_NACK = 0x00,
+ STATUS1_TIMEOUT_BUS = 0x01,
+ STATUS1_ARBITRATION_LOST = 0x02,
+ STATUS1_READ_INCOMPLETE = 0x03,
+ STATUS1_WRITE_INCOMPLETE = 0x04,
+ STATUS1_SUCCESS = 0x05,
+};
+
+/* All values are in big-endian */
+struct __attribute__ ((__packed__)) smbus_config {
+ uint32_t clock_speed; /* Hz */
+ uint8_t device_address; /* Stored in the upper 7 bits */
+ uint8_t auto_send_read; /* 1 = enabled, 0 = disabled */
+ uint16_t write_timeout; /* ms, 0 = no timeout */
+ uint16_t read_timeout; /* ms, 0 = no timeout */
+ uint8_t scl_low_timeout; /* 1 = enabled, 0 = disabled */
+ uint16_t retry_time; /* # of retries, 0 = no limit */
+};
+
+static const int MAX_TIMEOUT = 100;
+
+static const struct hid_device_id cp2112_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, 0xEA90) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, cp2112_devices);
+
+struct cp2112_device {
+ struct i2c_adapter adap;
+ struct hid_device *hdev;
+ wait_queue_head_t wait;
+ uint8_t read_data[61];
+ uint8_t read_length;
+ int xfer_status;
+ atomic_t read_avail;
+ atomic_t xfer_avail;
+};
+
+static int cp2112_wait(struct cp2112_device *dev, atomic_t *avail)
+{
+ int ret = 0;
+
+ ret = wait_event_interruptible_timeout(dev->wait,
+ atomic_read(avail), msecs_to_jiffies(50));
+ if (-ERESTARTSYS == ret)
+ return ret;
+ if (!ret)
+ return -ETIMEDOUT;
+ atomic_set(avail, 0);
+ return 0;
+}
+
+static int cp2112_xfer_status(struct cp2112_device *dev)
+{
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[2];
+ int ret;
+
+ buf[0] = TRANSFER_STATUS_REQUEST;
+ buf[1] = 0x01;
+ atomic_set(&dev->xfer_avail, 0);
+ ret = hdev->hid_output_raw_report(hdev, buf, 2, HID_OUTPUT_REPORT);
+ if (ret < 0) {
+ hid_warn(hdev, "Error requesting status: %d\n", ret);
+ return ret;
+ }
+ ret = cp2112_wait(dev, &dev->xfer_avail);
+ if (ret)
+ return ret;
+ return dev->xfer_status;
+}
+
+static int cp2112_read(struct cp2112_device *dev, uint8_t *data, size_t size)
+{
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[3];
+ int ret;
+
+ buf[0] = DATA_READ_FORCE_SEND;
+ *(uint16_t *)&buf[1] = htons(size);
+ atomic_set(&dev->read_avail, 0);
+ ret = hdev->hid_output_raw_report(hdev, buf, 3, HID_OUTPUT_REPORT);
+ if (ret < 0) {
+ hid_warn(hdev, "Error requesting data: %d\n", ret);
+ return ret;
+ }
+ ret = cp2112_wait(dev, &dev->read_avail);
+ if (ret)
+ return ret;
+ hid_dbg(hdev, "read %d of %d bytes requested\n",
+ dev->read_length, size);
+ if (size > dev->read_length)
+ size = dev->read_length;
+ memcpy(data, dev->read_data, size);
+ return dev->read_length;
+}
+
+static int cp2112_xfer(struct i2c_adapter *adap, uint16_t addr,
+ unsigned short flags, char read_write, uint8_t command,
+ int size, union i2c_smbus_data *data)
+{
+ struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[64];
+ size_t count;
+ size_t read_length = 0;
+ size_t write_length;
+ size_t timeout;
+ int ret;
+
+ hid_dbg(hdev, "%s addr 0x%x flags 0x%x cmd 0x%x size %d\n",
+ read_write == I2C_SMBUS_WRITE ? "write" : "read",
+ addr, flags, command, size);
+ buf[1] = addr << 1;
+ switch (size) {
+ case I2C_SMBUS_BYTE:
+ if (I2C_SMBUS_READ == read_write) {
+ read_length = 1;
+ buf[0] = DATA_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(read_length);
+ count = 4;
+ } else {
+ write_length = 1;
+ buf[0] = DATA_WRITE_REQUEST;
+ buf[2] = write_length;
+ buf[3] = data->byte;
+ count = 4;
+ }
+ break;
+ case I2C_SMBUS_BYTE_DATA:
+ if (I2C_SMBUS_READ == read_write) {
+ read_length = 1;
+ buf[0] = DATA_WRITE_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(read_length);
+ buf[4] = 1;
+ buf[5] = command;
+ count = 6;
+ } else {
+ write_length = 2;
+ buf[0] = DATA_WRITE_REQUEST;
+ buf[2] = write_length;
+ buf[3] = command;
+ buf[4] = data->byte;
+ count = 5;
+ }
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ if (I2C_SMBUS_READ == read_write) {
+ read_length = 2;
+ buf[0] = DATA_WRITE_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(read_length);
+ buf[4] = 1;
+ buf[5] = command;
+ count = 6;
+ } else {
+ write_length = 3;
+ buf[0] = DATA_WRITE_REQUEST;
+ buf[2] = write_length;
+ buf[3] = command;
+ *(uint16_t *)&buf[4] = htons(data->word);
+ count = 6;
+ }
+ break;
+ case I2C_SMBUS_PROC_CALL:
+ size = I2C_SMBUS_WORD_DATA;
+ read_write = I2C_SMBUS_READ;
+ read_length = 2;
+ write_length = 3;
+ buf[0] = DATA_WRITE_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(read_length);
+ buf[4] = write_length;
+ buf[5] = command;
+ *(uint16_t *)&buf[6] = data->word;
+ count = 8;
+ break;
+ case I2C_SMBUS_I2C_BLOCK_DATA:
+ size = I2C_SMBUS_BLOCK_DATA;
+ /* fallthrough */
+ case I2C_SMBUS_BLOCK_DATA:
+ if (I2C_SMBUS_READ == read_write) {
+ buf[0] = DATA_WRITE_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(I2C_SMBUS_BLOCK_MAX);
+ buf[4] = 1;
+ buf[5] = command;
+ count = 6;
+ } else {
+ write_length = data->block[0];
+ if (write_length > 61 - 2)
+ return -EINVAL;
+ buf[0] = DATA_WRITE_REQUEST;
+ buf[2] = write_length + 2;
+ buf[3] = command;
+ memcpy(&buf[4], data->block, write_length + 1);
+ count = 5 + write_length;
+ }
+ break;
+ case I2C_SMBUS_BLOCK_PROC_CALL:
+ size = I2C_SMBUS_BLOCK_DATA;
+ read_write = I2C_SMBUS_READ;
+ write_length = data->block[0];
+ if (write_length > 16 - 2)
+ return -EINVAL;
+ buf[0] = DATA_WRITE_READ_REQUEST;
+ *(uint16_t *)&buf[2] = htons(I2C_SMBUS_BLOCK_MAX);
+ buf[4] = write_length + 2;
+ buf[5] = command;
+ memcpy(&buf[6], data->block, write_length + 1);
+ count = 7 + write_length;
+ default:
+ hid_warn(hdev, "Unsupported transaction %d\n", size);
+ return -EOPNOTSUPP;
+ }
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hw open failed\n");
+ return ret;
+ }
+ ret = hid_hw_power(hdev, PM_HINT_FULLON);
+ if (ret < 0) {
+ hid_err(hdev, "power management error: %d\n", ret);
+ goto hid_close;
+ }
+ ret = hdev->hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
+ if (ret < 0) {
+ hid_warn(hdev, "Error starting transaction: %d\n", ret);
+ goto power_normal;
+ }
+ for (timeout = 0; timeout < MAX_TIMEOUT; ++timeout) {
+ ret = cp2112_xfer_status(dev);
+ if (-EBUSY == ret)
+ continue;
+ if (ret < 0)
+ goto power_normal;
+ break;
+ }
+ if (MAX_TIMEOUT <= timeout) {
+ hid_warn(hdev, "Transfer timed out, cancelling.\n");
+ buf[0] = CANCEL_TRANSFER;
+ buf[1] = 0x01;
+ ret = hdev->hid_output_raw_report(hdev, buf, 2,
+ HID_OUTPUT_REPORT);
+ if (ret < 0) {
+ hid_warn(hdev, "Error cancelling transaction: %d\n",
+ ret);
+ }
+ ret = -ETIMEDOUT;
+ goto power_normal;
+ }
+ if (I2C_SMBUS_WRITE == read_write) {
+ ret = 0;
+ goto power_normal;
+ }
+ if (I2C_SMBUS_BLOCK_DATA == size)
+ read_length = ret;
+ ret = cp2112_read(dev, buf, read_length);
+ if (ret < 0)
+ goto power_normal;
+ if (ret != read_length) {
+ hid_warn(hdev, "short read: %d < %d\n", ret, read_length);
+ ret = -EIO;
+ goto power_normal;
+ }
+ switch (size) {
+ case I2C_SMBUS_BYTE:
+ case I2C_SMBUS_BYTE_DATA:
+ data->byte = buf[0];
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ data->word = ntohs(*(uint16_t *)buf);
+ break;
+ case I2C_SMBUS_BLOCK_DATA:
+ if (read_length > I2C_SMBUS_BLOCK_MAX) {
+ ret = -EPROTO;
+ goto power_normal;
+ }
+ memcpy(data->block, buf, read_length);
+ break;
+ }
+ ret = 0;
+power_normal:
+ hid_hw_power(hdev, PM_HINT_NORMAL);
+hid_close:
+ hid_hw_close(hdev);
+ hid_dbg(hdev, "transfer finished: %d\n", ret);
+ return ret;
+}
+
+static uint32_t cp2122_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA |
+ I2C_FUNC_SMBUS_I2C_BLOCK |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
+}
+
+static const struct i2c_algorithm smbus_algorithm = {
+ .smbus_xfer = cp2112_xfer,
+ .functionality = cp2122_functionality,
+};
+
+static int
+cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ struct cp2112_device *dev;
+ uint8_t buf[64];
+ struct smbus_config *config;
+ int ret;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "parse failed\n");
+ return ret;
+ }
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev, "hw start failed\n");
+ return ret;
+ }
+ ret = hdev->hid_get_raw_report(hdev, GET_VERSION_INFO, buf, 3,
+ HID_FEATURE_REPORT);
+ if (ret != 3) {
+ hid_err(hdev, "error requesting version\n");
+ return ret;
+ }
+ hid_info(hdev, "Part Number: 0x%02X Device Version: 0x%02X\n",
+ buf[1], buf[2]);
+ ret = hdev->hid_get_raw_report(hdev, SMBUS_CONFIG, buf,
+ sizeof(*config) + 1, HID_FEATURE_REPORT);
+ if (ret != sizeof(*config) + 1) {
+ hid_err(hdev, "error requesting SMBus config\n");
+ return ret;
+ }
+ config = (struct smbus_config *)&buf[1];
+ config->retry_time = htons(1);
+ ret = hdev->hid_output_raw_report(hdev, buf,
+ sizeof(*config) + 1, HID_FEATURE_REPORT);
+ if (ret != sizeof(*config) + 1) {
+ hid_err(hdev, "error setting SMBus config\n");
+ return ret;
+ }
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ hid_err(hdev, "out of memory\n");
+ return -ENOMEM;
+ }
+ dev->hdev = hdev;
+ hid_set_drvdata(hdev, (void *)dev);
+ dev->adap.owner = THIS_MODULE;
+ dev->adap.class = I2C_CLASS_HWMON;
+ dev->adap.algo = &smbus_algorithm;
+ dev->adap.algo_data = dev;
+ snprintf(dev->adap.name, sizeof(dev->adap.name),
+ "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
+ init_waitqueue_head(&dev->wait);
+ hid_device_io_start(hdev);
+ ret = i2c_add_adapter(&dev->adap);
+ hid_device_io_stop(hdev);
+ if (ret) {
+ hid_err(hdev, "error registering i2c adapter\n");
+ kfree(dev);
+ hid_set_drvdata(hdev, NULL);
+ }
+ hid_dbg(hdev, "adapter registered\n");
+ return ret;
+}
+
+static void cp2112_remove(struct hid_device *hdev)
+{
+ struct cp2112_device *dev = hid_get_drvdata(hdev);
+
+ if (dev) {
+ i2c_del_adapter(&dev->adap);
+ wake_up_interruptible(&dev->wait);
+ kfree(dev);
+ hid_set_drvdata(hdev, NULL);
+ }
+ hid_hw_stop(hdev);
+}
+
+static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
+ uint8_t *data, int size)
+{
+ struct cp2112_device *dev = hid_get_drvdata(hdev);
+
+ switch (data[0]) {
+ case TRANSFER_STATUS_RESPONSE:
+ hid_dbg(hdev, "xfer status: %02x %02x %04x %04x\n",
+ data[1], data[2], htons(*(uint16_t *)&data[3]),
+ htons(*(uint16_t *)&data[5]));
+ switch (data[1]) {
+ case STATUS0_IDLE:
+ dev->xfer_status = -EAGAIN;
+ break;
+ case STATUS0_BUSY:
+ dev->xfer_status = -EBUSY;
+ break;
+ case STATUS0_COMPLETE:
+ dev->xfer_status = ntohs(*(uint16_t *)&data[5]);
+ break;
+ case STATUS0_ERROR:
+ switch (data[2]) {
+ case STATUS1_TIMEOUT_NACK:
+ case STATUS1_TIMEOUT_BUS:
+ dev->xfer_status = -ETIMEDOUT;
+ break;
+ default:
+ dev->xfer_status = -EIO;
+ }
+ break;
+ default:
+ dev->xfer_status = -EINVAL;
+ break;
+ }
+ atomic_set(&dev->xfer_avail, 1);
+ break;
+ case DATA_READ_RESPONSE:
+ hid_dbg(hdev, "read response: %02x %02x\n", data[1], data[2]);
+ dev->read_length = data[2];
+ if (dev->read_length > sizeof(dev->read_data))
+ dev->read_length = sizeof(dev->read_data);
+ memcpy(dev->read_data, &data[3], dev->read_length);
+ atomic_set(&dev->read_avail, 1);
+ break;
+ default:
+ hid_err(hdev, "unknown report\n");
+ return 0;
+ }
+ wake_up_interruptible(&dev->wait);
+ return 1;
+}
+
+static struct hid_driver cp2112_driver = {
+ .name = "cp2112",
+ .id_table = cp2112_devices,
+ .probe = cp2112_probe,
+ .remove = cp2112_remove,
+ .raw_event = cp2112_raw_event,
+};
+
+static int __init cp2112_init(void)
+{
+ return hid_register_driver(&cp2112_driver);
+}
+
+static void __exit cp2112_exit(void)
+{
+ hid_unregister_driver(&cp2112_driver);
+}
+
+module_init(cp2112_init);
+module_exit(cp2112_exit);
+MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
+MODULE_AUTHOR("David Barksdale <dbark...@uplogix.com>");
+MODULE_LICENSE("GPL");
+
--
tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jiri Kosina

unread,

Aug 28, 2013, 9:00:02 AM8/28/13

[ adding Benjamin to CC ]

Hi David,

this is rather difficult to get by quickly looking at the code, so I'd
like to have a slightly better overview of what the device really is.

Is it talking HID protocol on the SMBus? (if so, there is a hid-i2c
transport driver for it already).
But I guess this is not really the case, as you seem to be using the USB
HID functionality as well.

Thanks in advance for some more background.

--
Jiri Kosina
SUSE Labs

David Barksdale

unread,

Aug 28, 2013, 9:40:03 AM8/28/13

The CP2112 is a USB HID device that provides an SMBus controller.
The datasheet can be found here:
http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf
The interface specification here:
http://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf
The host communicates with the CP2112 via HID reports. Device
configuration uses feature reports and data transfer uses output and
event reports.
I prototyped this in userspace using the /dev/hidraw interface and then
wrote this driver to provide a real i2c adapter device.

David Barksdale

unread,

Aug 29, 2013, 10:00:02 AM8/29/13

On 08/28/2013 07:57 AM, Jiri Kosina wrote:
>

David Herrmann

unread,

Aug 29, 2013, 12:50:02 PM8/29/13

Why is that #if needed?

If you don't add any prefix to these functions (especially
SMBUS_CONFIG) it is quite likely that at some point your driver will
get compile-errors. Any particular reason not to do that? (same
applies to all the function/global-vars below)

> +
> +/* All values are in big-endian */
> +struct __attribute__ ((__packed__)) smbus_config {
> + uint32_t clock_speed; /* Hz */
> + uint8_t device_address; /* Stored in the upper 7 bits */
> + uint8_t auto_send_read; /* 1 = enabled, 0 = disabled */
> + uint16_t write_timeout; /* ms, 0 = no timeout */
> + uint16_t read_timeout; /* ms, 0 = no timeout */
> + uint8_t scl_low_timeout; /* 1 = enabled, 0 = disabled */
> + uint16_t retry_time; /* # of retries, 0 = no limit */
> +};
> +
> +static const int MAX_TIMEOUT = 100;
> +
> +static const struct hid_device_id cp2112_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, 0xEA90) },

You might want to add a
#define USB_PRODUCT_ID_CYGNAL_CP2112 0xEA90
to drivers/hid/hid-ids.h to avoid this magic-number here and in hid-core.c.

Isn't there a break; missing?

Use HID_CONNECT_HIDRAW or does your device provide more than just the
smbus bridge?

> + if (ret) {
> + hid_err(hdev, "hw start failed\n");
> + return ret;
> + }

You must call hid_hw_open() here, otherwise I/O is not guaranteed to
be possible. It might work for USB now, but you shouldn't do that.
Either call hid_hw_close() after setup is done, or call it in
remove().

> + ret = hdev->hid_get_raw_report(hdev, GET_VERSION_INFO, buf, 3,
> + HID_FEATURE_REPORT);
> + if (ret != 3) {
> + hid_err(hdev, "error requesting version\n");
> + return ret;

return (ret < 0) ? ret : -EIO;

And call hid_hw_stop() before returning, please (and hid_hw_close()).

> + }
> + hid_info(hdev, "Part Number: 0x%02X Device Version: 0x%02X\n",
> + buf[1], buf[2]);
> + ret = hdev->hid_get_raw_report(hdev, SMBUS_CONFIG, buf,
> + sizeof(*config) + 1, HID_FEATURE_REPORT);
> + if (ret != sizeof(*config) + 1) {
> + hid_err(hdev, "error requesting SMBus config\n");
> + return ret;

same as above

> + }
> + config = (struct smbus_config *)&buf[1];
> + config->retry_time = htons(1);
> + ret = hdev->hid_output_raw_report(hdev, buf,
> + sizeof(*config) + 1, HID_FEATURE_REPORT);
> + if (ret != sizeof(*config) + 1) {
> + hid_err(hdev, "error setting SMBus config\n");
> + return ret;

same as above

> + }
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + hid_err(hdev, "out of memory\n");
> + return -ENOMEM;

hid_hw_stop() missing (and hid_hw_close())

> + }
> + dev->hdev = hdev;
> + hid_set_drvdata(hdev, (void *)dev);
> + dev->adap.owner = THIS_MODULE;
> + dev->adap.class = I2C_CLASS_HWMON;
> + dev->adap.algo = &smbus_algorithm;
> + dev->adap.algo_data = dev;
> + snprintf(dev->adap.name, sizeof(dev->adap.name),
> + "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
> + init_waitqueue_head(&dev->wait);
> + hid_device_io_start(hdev);
> + ret = i2c_add_adapter(&dev->adap);
> + hid_device_io_stop(hdev);

You should call hid_device_io_stop() only on failure. Otherwise,
hid-core drops any incoming events until you return. If smbus can
handle missing events, this is fine.

In case you don't care for I/O between this and your handler above,
you can call hid_hw_close() here. Note that ->raw_event() is only
called while the device is open.

> + if (ret) {
> + hid_err(hdev, "error registering i2c adapter\n");
> + kfree(dev);
> + hid_set_drvdata(hdev, NULL);

Drop this hid_set_drvdata()
hid_hw_stop() missing (and hid_hw_close())

> + }
> + hid_dbg(hdev, "adapter registered\n");
> + return ret;
> +}
> +
> +static void cp2112_remove(struct hid_device *hdev)
> +{
> + struct cp2112_device *dev = hid_get_drvdata(hdev);
> +
> + if (dev) {

Why if (dev)? Any reason dev might be NULL?

> + i2c_del_adapter(&dev->adap);
> + wake_up_interruptible(&dev->wait);

How can you have waiters on dev->wait if you free() it in the line
below? There ought to be some *_sync() call here which waits for all
possible pending calls to finish.

> + kfree(dev);
> + hid_set_drvdata(hdev, NULL);

Not needed.

> + }
> + hid_hw_stop(hdev);

I recommend calling hid_hw_stop() before destroying your context. Not
strictly needed, but it makes it clear that no I/O is possible during
_remove().

So you can reduce this function to:

hid_hw_stop(hdev);
i2c_del_adapter(&dev->adap);
wake_up_interruptible(&dev->wait);
your_whatever_wait_sync_call()
kfree(dev);

Use:
module_hid_driver(&cp2112_driver);
to save some lines here.

Cheers
David

David Barksdale

unread,

Sep 3, 2013, 6:00:02 PM9/3/13

There are similar #ifs for CONFIG_HID_LENOVO_TPKBD,
CONFIG_HID_LOGITECH_DJ, and CONFIG_HID_ROCCAT. I thought it was a good idea.

Prefixes added. I think the STATUS* ones will be unique enough without
another prefix.

>> +
>> +/* All values are in big-endian */
>> +struct __attribute__ ((__packed__)) smbus_config {
>> + uint32_t clock_speed; /* Hz */
>> + uint8_t device_address; /* Stored in the upper 7 bits */
>> + uint8_t auto_send_read; /* 1 = enabled, 0 = disabled */
>> + uint16_t write_timeout; /* ms, 0 = no timeout */
>> + uint16_t read_timeout; /* ms, 0 = no timeout */
>> + uint8_t scl_low_timeout; /* 1 = enabled, 0 = disabled */
>> + uint16_t retry_time; /* # of retries, 0 = no limit */
>> +};
>> +
>> +static const int MAX_TIMEOUT = 100;
>> +
>> +static const struct hid_device_id cp2112_devices[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, 0xEA90) },
>
> You might want to add a
> #define USB_PRODUCT_ID_CYGNAL_CP2112 0xEA90
> to drivers/hid/hid-ids.h to avoid this magic-number here and in hid-core.c.

Done.

Yes, thanks.

It's just an smbus bridge. I will change to HID_CONNECT_HIDRAW.

>> + if (ret) {
>> + hid_err(hdev, "hw start failed\n");
>> + return ret;
>> + }
>
> You must call hid_hw_open() here, otherwise I/O is not guaranteed to
> be possible. It might work for USB now, but you shouldn't do that.
> Either call hid_hw_close() after setup is done, or call it in
> remove().

Done.

>> + ret = hdev->hid_get_raw_report(hdev, GET_VERSION_INFO, buf, 3,
>> + HID_FEATURE_REPORT);
>> + if (ret != 3) {
>> + hid_err(hdev, "error requesting version\n");
>> + return ret;
>
> return (ret < 0) ? ret : -EIO;
>
> And call hid_hw_stop() before returning, please (and hid_hw_close()).

Done.

I'll call hid_device_io_stop() and hid_hw_close() here since I don't
care for I/O or events until I have a transaction to perform.

>> + if (ret) {
>> + hid_err(hdev, "error registering i2c adapter\n");
>> + kfree(dev);
>> + hid_set_drvdata(hdev, NULL);
>
> Drop this hid_set_drvdata()
> hid_hw_stop() missing (and hid_hw_close())

Done.

>> + }
>> + hid_dbg(hdev, "adapter registered\n");
>> + return ret;
>> +}
>> +
>> +static void cp2112_remove(struct hid_device *hdev)
>> +{
>> + struct cp2112_device *dev = hid_get_drvdata(hdev);
>> +
>> + if (dev) {
>
> Why if (dev)? Any reason dev might be NULL?

This is probably left over from some debugging, just to be safe. Removed.

>> + i2c_del_adapter(&dev->adap);
>> + wake_up_interruptible(&dev->wait);
>
> How can you have waiters on dev->wait if you free() it in the line
> below? There ought to be some *_sync() call here which waits for all
> possible pending calls to finish.

I'm having some trouble with this. I've added some printk's to
understand what happens when I yank the USB cable. For testing I
generate transactions using the i2c-dev driver and i2cdetect utility.
On one occasion it looks like the last cp2112_xfer() completes, then it
is called several more times after the cable is yanked which fail on
hid_output_raw_report() returning -EPIPE. Then cp2112_remove() is
finally called (your version below) and returns. Then cp2112_xfer() is
called yet again and barfs because everything's been freed.
What can I wait on to know that my .smbus_xfer function won't be called
after i2c_del_adapter()?

>> + kfree(dev);
>> + hid_set_drvdata(hdev, NULL);
>
> Not needed.
>
>> + }
>> + hid_hw_stop(hdev);
>
> I recommend calling hid_hw_stop() before destroying your context. Not
> strictly needed, but it makes it clear that no I/O is possible during
> _remove().
>
> So you can reduce this function to:
>
> hid_hw_stop(hdev);
> i2c_del_adapter(&dev->adap);
> wake_up_interruptible(&dev->wait);
> your_whatever_wait_sync_call()
> kfree(dev);

Done.

> Cheers
> David
>
>> +MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
>> +MODULE_AUTHOR("David Barksdale <dbark...@uplogix.com>");
>> +MODULE_LICENSE("GPL");
>> +
>> --
>> tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
>> --

Thank you David for all the helpful comments.

David Herrmann

unread,

Sep 5, 2013, 5:20:02 AM9/5/13

These are used for devices that work with and without special drivers.
If you don't have the special-driver compiled in, you want the generic
HID driver to bind to these devices (Jiri may correct me if I am
wrong).

However, for your device this doesn't make any sense. hid-generic has
no idea of smbus bridges. So yeah, just drop the #if.

It was SMBUS_CONFIG in particular that bothered me. So yeah, if names
look unique enough, no need to add prefixes.

[..snip..]

>>> + }
>>> + dev->hdev = hdev;
>>> + hid_set_drvdata(hdev, (void *)dev);
>>> + dev->adap.owner = THIS_MODULE;
>>> + dev->adap.class = I2C_CLASS_HWMON;
>>> + dev->adap.algo = &smbus_algorithm;
>>> + dev->adap.algo_data = dev;
>>> + snprintf(dev->adap.name, sizeof(dev->adap.name),
>>> + "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
>>> + init_waitqueue_head(&dev->wait);
>>> + hid_device_io_start(hdev);
>>> + ret = i2c_add_adapter(&dev->adap);
>>> + hid_device_io_stop(hdev);
>>
>>
>> You should call hid_device_io_stop() only on failure. Otherwise,
>> hid-core drops any incoming events until you return. If smbus can
>> handle missing events, this is fine.
>>
>> In case you don't care for I/O between this and your handler above,
>> you can call hid_hw_close() here. Note that ->raw_event() is only
>> called while the device is open.
>
>
> I'll call hid_device_io_stop() and hid_hw_close() here since I don't care
> for I/O or events until I have a transaction to perform.

Yepp, sounds good.

Maybe Benjamin can help here as I am not very familiar with i2c core-code.

I did look at it (and other i2c bus-drivers) and i2c_del_adapter
should be enough.

The thing is, your i2c-xfer callback is synchronous. Once
hid->remove() is called, no other HID callback will be called,
anymore. Furthermore, if the device is gone, all HID-I/O calls will
return -EIO immediately. So you should just drop the
wake_up_interruptible() (or move it in front of i2c_del_adapter() in
case you don't care for driver-unload while devices are still active).

Furthermore, the xfer callback shouldn't be called by smbus-core after
i2c_del_adapter() returns. If it is, you should definitely report that
to the i2c mailing-list.

Obviously, for debugging, you should set the i2c-drvdata to NULL after
i2c_del_adapter() and test for that in your xfer callback (may add a
WARN_ON()). This is racy, but it should avoid the use-after-free. Once
you figured it out, you can remove them again. Feel free to CC
i2c-mailing-lists if you are not sure.

And also feel free to send v2 even if there are still bugs. It's often
easier to comment on the recent-state than just an old ML thread.

Thanks
David

David Barksdale

unread,

Sep 11, 2013, 4:20:02 PM9/11/13

Changes since v1:
* Prefix symbols to avoid name collision.
* Define CP2112 device ID.
* Missing break.
* Use module_hid_driver().
* Fix cp2122 typo.
* Fix probe and remove functions.
* Remove #if in hid_have_special_driver[].
* Improve symbolic constants for timeouts.
* Wrap DMA functions to kmalloc buffers.
* Added a short description of the device.
* Add request formatting functions for readability.
* Whitespace changes.
* Fix endianess of word transfers.

However this version doesn't work. Adding the hid_hw_open/hid_hw_close
calls to cp2112_probe() as suggested by David Herrmann seems to have
stopped events being delivered to cp2112_raw_event(). Why is that?

---
drivers/hid/Kconfig | 6 +
drivers/hid/Makefile | 1 +

drivers/hid/hid-core.c | 1 +
drivers/hid/hid-cp2112.c | 559 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-ids.h | 1 +
5 files changed, 568 insertions(+)

index 36668d1..69e5f39 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1568,6 +1568,7 @@ static const struct hid_device_id hid_have_special_driver[] = {

{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },

+ { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },

{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_1) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
new file mode 100644

index 0000000..5c6d294
--- /dev/null
+++ b/drivers/hid/hid-cp2112.c
@@ -0,0 +1,559 @@

+/*
+ hid-cp2112.c - Silicon Labs HID USB to SMBus master bridge
+ Copyright (c) 2013 Uplogix, Inc.
+ David Barksdale <dbark...@uplogix.com>
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+

+/*
+ The Silicon Labs CP2112 chip is a USB HID device which provides an
+ SMBus controller for talking to slave devices. The host communicates
+ with the CP2112 via raw HID reports.

+ */
+
+#include <linux/hid.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include "hid-ids.h"
+
+enum {

+ CP2112_GET_VERSION_INFO = 0x05,
+ CP2112_SMBUS_CONFIG = 0x06,
+ CP2112_DATA_READ_REQUEST = 0x10,
+ CP2112_DATA_WRITE_READ_REQUEST = 0x11,
+ CP2112_DATA_READ_FORCE_SEND = 0x12,
+ CP2112_DATA_READ_RESPONSE = 0x13,
+ CP2112_DATA_WRITE_REQUEST = 0x14,
+ CP2112_TRANSFER_STATUS_REQUEST = 0x15,
+ CP2112_TRANSFER_STATUS_RESPONSE = 0x16,
+ CP2112_CANCEL_TRANSFER = 0x17,

+};
+
+enum {
+ STATUS0_IDLE = 0x00,
+ STATUS0_BUSY = 0x01,
+ STATUS0_COMPLETE = 0x02,
+ STATUS0_ERROR = 0x03,
+};
+
+enum {
+ STATUS1_TIMEOUT_NACK = 0x00,
+ STATUS1_TIMEOUT_BUS = 0x01,
+ STATUS1_ARBITRATION_LOST = 0x02,
+ STATUS1_READ_INCOMPLETE = 0x03,
+ STATUS1_WRITE_INCOMPLETE = 0x04,
+ STATUS1_SUCCESS = 0x05,
+};

+
+/* All values are in big-endian */

+struct __attribute__ ((__packed__)) cp2112_smbus_config {

+ uint32_t clock_speed; /* Hz */
+ uint8_t device_address; /* Stored in the upper 7 bits */
+ uint8_t auto_send_read; /* 1 = enabled, 0 = disabled */
+ uint16_t write_timeout; /* ms, 0 = no timeout */
+ uint16_t read_timeout; /* ms, 0 = no timeout */
+ uint8_t scl_low_timeout; /* 1 = enabled, 0 = disabled */
+ uint16_t retry_time; /* # of retries, 0 = no limit */
+};
+

+/* Number of times to request transfer status before giving up waiting for a
+ transfer to complete. */
+static const int XFER_TIMEOUT = 100;
+
+/* Time in ms to wait for a CP2112_DATA_READ_RESPONSE or
+ CP2112_TRANSFER_STATUS_RESPONSE. */
+static const int RESPONSE_TIMEOUT = 50;

+
+static const struct hid_device_id cp2112_devices[] = {

+ { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },

+ { }
+};
+MODULE_DEVICE_TABLE(hid, cp2112_devices);
+
+struct cp2112_device {
+ struct i2c_adapter adap;
+ struct hid_device *hdev;
+ wait_queue_head_t wait;
+ uint8_t read_data[61];
+ uint8_t read_length;
+ int xfer_status;
+ atomic_t read_avail;
+ atomic_t xfer_avail;
+};
+
+static int cp2112_wait(struct cp2112_device *dev, atomic_t *avail)
+{
+ int ret = 0;
+
+ ret = wait_event_interruptible_timeout(dev->wait,

+ atomic_read(avail), msecs_to_jiffies(RESPONSE_TIMEOUT));

+ if (-ERESTARTSYS == ret)
+ return ret;
+ if (!ret)
+ return -ETIMEDOUT;
+ atomic_set(avail, 0);

+ return 0;
+}
+

+static int cp2112_hid_get(struct hid_device *hdev, unsigned char report_number,
+ uint8_t *data, size_t count,
+ unsigned char report_type)
+{
+ uint8_t *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;
+}
+

+static int cp2112_hid_output(struct hid_device *hdev, uint8_t *data,
+ size_t count, unsigned char report_type)
+{
+ uint8_t *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;
+}
+

+static int cp2112_xfer_status(struct cp2112_device *dev)
+{
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[2];
+ int ret;
+

+ buf[0] = CP2112_TRANSFER_STATUS_REQUEST;

+ buf[1] = 0x01;
+ atomic_set(&dev->xfer_avail, 0);

+ ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);

+ if (ret < 0) {
+ hid_warn(hdev, "Error requesting status: %d\n", ret);

+ return ret;
+ }

+ ret = cp2112_wait(dev, &dev->xfer_avail);
+ if (ret)
+ return ret;
+ return dev->xfer_status;
+}
+
+static int cp2112_read(struct cp2112_device *dev, uint8_t *data, size_t size)
+{
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[3];
+ int ret;
+

+ buf[0] = CP2112_DATA_READ_FORCE_SEND;

+ *(uint16_t *)&buf[1] = htons(size);
+ atomic_set(&dev->read_avail, 0);

+ ret = cp2112_hid_output(hdev, buf, 3, HID_OUTPUT_REPORT);

+ if (ret < 0) {
+ hid_warn(hdev, "Error requesting data: %d\n", ret);

+ return ret;
+ }

+ ret = cp2112_wait(dev, &dev->read_avail);
+ if (ret)
+ return ret;
+ hid_dbg(hdev, "read %d of %d bytes requested\n",
+ dev->read_length, size);
+ if (size > dev->read_length)
+ size = dev->read_length;
+ memcpy(data, dev->read_data, size);
+ return dev->read_length;
+}
+

+static int cp2112_read_req(uint8_t *buf, uint8_t slave_address, uint16_t length)
+{
+ if (length < 1 || length > 512)
+ return -EINVAL;
+ buf[0] = CP2112_DATA_READ_REQUEST;
+ buf[1] = slave_address << 1;
+ *(uint16_t *)&buf[2] = htons(length);
+ return 4;
+}
+
+static int cp2112_write_read_req(uint8_t *buf, uint8_t slave_address,
+ uint16_t length, uint8_t command,
+ uint8_t *data, uint8_t data_length)
+{
+ if (length < 1 || length > 512 || data_length > 15)
+ return -EINVAL;
+ buf[0] = CP2112_DATA_WRITE_READ_REQUEST;
+ buf[1] = slave_address << 1;
+ *(uint16_t *)&buf[2] = htons(length);
+ buf[4] = data_length + 1;

+ buf[5] = command;

+ memcpy(&buf[6], data, data_length);
+ return data_length + 6;
+}
+
+static int cp2112_write_req(uint8_t *buf, uint8_t slave_address,
+ uint8_t command, uint8_t *data, uint8_t data_length)
+{
+ if (data_length > 60)
+ return -EINVAL;
+ buf[0] = CP2112_DATA_WRITE_REQUEST;
+ buf[1] = slave_address << 1;
+ buf[2] = data_length + 1;

+ buf[3] = command;

+ memcpy(&buf[4], data, data_length);
+ return data_length + 4;

+}
+
+static int cp2112_xfer(struct i2c_adapter *adap, uint16_t addr,
+ unsigned short flags, char read_write, uint8_t command,
+ int size, union i2c_smbus_data *data)

+ struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
+ struct hid_device *hdev = dev->hdev;
+ uint8_t buf[64];

+ uint16_t word;

+ size_t count;
+ size_t read_length = 0;

+ size_t timeout;
+ int ret;
+
+ hid_dbg(hdev, "%s addr 0x%x flags 0x%x cmd 0x%x size %d\n",
+ read_write == I2C_SMBUS_WRITE ? "write" : "read",
+ addr, flags, command, size);

+ switch (size) {
+ case I2C_SMBUS_BYTE:

+ read_length = 1;

+ if (I2C_SMBUS_READ == read_write)

+ count = cp2112_read_req(buf, addr, read_length);
+ else
+ count = cp2112_write_req(buf, addr, data->byte, NULL,
+ 0);

+ break;
+ case I2C_SMBUS_BYTE_DATA:

+ read_length = 1;

+ if (I2C_SMBUS_READ == read_write)

+ count = cp2112_write_read_req(buf, addr, read_length,
+ command, NULL, 0);
+ else
+ count = cp2112_write_req(buf, addr, command,
+ &data->byte, 1);
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ read_length = 2;
+ word = htons(data->word);

+ if (I2C_SMBUS_READ == read_write)

+ count = cp2112_write_read_req(buf, addr, read_length,
+ command, NULL, 0);
+ else
+ count = cp2112_write_req(buf, addr, command,
+ (uint8_t *)&word, 2);

+ break;
+ case I2C_SMBUS_PROC_CALL:
+ size = I2C_SMBUS_WORD_DATA;
+ read_write = I2C_SMBUS_READ;
+ read_length = 2;

+ word = htons(data->word);
+ count = cp2112_write_read_req(buf, addr, read_length, command,
+ (uint8_t *)&word, 2);

+ break;
+ case I2C_SMBUS_I2C_BLOCK_DATA:
+ size = I2C_SMBUS_BLOCK_DATA;
+ /* fallthrough */
+ case I2C_SMBUS_BLOCK_DATA:
+ if (I2C_SMBUS_READ == read_write) {

+ count = cp2112_write_read_req(buf, addr,
+ I2C_SMBUS_BLOCK_MAX,
+ command, NULL, 0);
+ } else {
+ count = cp2112_write_req(buf, addr, command,
+ data->block,
+ data->block[0] + 1);

+ }
+ break;
+ case I2C_SMBUS_BLOCK_PROC_CALL:
+ size = I2C_SMBUS_BLOCK_DATA;
+ read_write = I2C_SMBUS_READ;

+ count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
+ command, data->block,
+ data->block[0] + 1);
+ break;
+ default:

+ hid_warn(hdev, "Unsupported transaction %d\n", size);
+ return -EOPNOTSUPP;
+ }

+ if (count < 0)
+ return count;
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hw open failed\n");
+ return ret;
+ }

+ ret = hid_hw_power(hdev, PM_HINT_FULLON);
+ if (ret < 0) {
+ hid_err(hdev, "power management error: %d\n", ret);
+ goto hid_close;
+ }

+ ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);

+ if (ret < 0) {
+ hid_warn(hdev, "Error starting transaction: %d\n", ret);
+ goto power_normal;
+ }

+ for (timeout = 0; timeout < XFER_TIMEOUT; ++timeout) {

+ ret = cp2112_xfer_status(dev);
+ if (-EBUSY == ret)
+ continue;
+ if (ret < 0)
+ goto power_normal;
+ break;
+ }

+ if (XFER_TIMEOUT <= timeout) {

+ hid_warn(hdev, "Transfer timed out, cancelling.\n");

+ buf[0] = CP2112_CANCEL_TRANSFER;

+ buf[1] = 0x01;

+ ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
+ if (ret < 0) {

+ return ret;
+}
+

+static uint32_t cp2112_functionality(struct i2c_adapter *adap)

+{
+ return I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA |
+ I2C_FUNC_SMBUS_I2C_BLOCK |
+ I2C_FUNC_SMBUS_PROC_CALL |
+ I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
+}
+
+static const struct i2c_algorithm smbus_algorithm = {
+ .smbus_xfer = cp2112_xfer,

+ .functionality = cp2112_functionality,
+};
+
+static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{

+ struct cp2112_device *dev;
+ uint8_t buf[64];

+ struct cp2112_smbus_config *config;

+ int ret;
+
+ ret = hid_parse(hdev);

+ if (ret) {
+ hid_err(hdev, "parse failed\n");
+ return ret;
+ }
+ ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+ if (ret) {
+ hid_err(hdev, "hw start failed\n");
+ return ret;
+ }
+ ret = hid_hw_open(hdev);
+ if (ret) {

+ hid_err(hdev, "hw open failed\n");

+ goto hid_stop;

+ }
+ ret = hid_hw_power(hdev, PM_HINT_FULLON);
+ if (ret < 0) {
+ hid_err(hdev, "power management error: %d\n", ret);
+ goto hid_close;
+ }

+ ret = cp2112_hid_get(hdev, CP2112_GET_VERSION_INFO, buf, 3,

+ HID_FEATURE_REPORT);
+ if (ret != 3) {
+ hid_err(hdev, "error requesting version\n");

+ if (ret >= 0)

+ ret = -EIO;
+ goto power_normal;
+ }

+ hid_info(hdev, "Part Number: 0x%02X Device Version: 0x%02X\n",
+ buf[1], buf[2]);

+ ret = cp2112_hid_get(hdev, CP2112_SMBUS_CONFIG, buf,

+ sizeof(*config) + 1, HID_FEATURE_REPORT);
+ if (ret != sizeof(*config) + 1) {
+ hid_err(hdev, "error requesting SMBus config\n");

+ if (ret >= 0)

+ ret = -EIO;
+ goto power_normal;
+ }

+ config = (struct cp2112_smbus_config *)&buf[1];

+ config->retry_time = htons(1);

+ ret = cp2112_hid_output(hdev, buf, sizeof(*config) + 1,
+ HID_FEATURE_REPORT);

+ if (ret != sizeof(*config) + 1) {
+ hid_err(hdev, "error setting SMBus config\n");

+ if (ret >= 0)

+ ret = -EIO;
+ goto power_normal;
+ }

+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ hid_err(hdev, "out of memory\n");

+ ret = -ENOMEM;
+ goto power_normal;

+ }
+ dev->hdev = hdev;
+ hid_set_drvdata(hdev, (void *)dev);
+ dev->adap.owner = THIS_MODULE;
+ dev->adap.class = I2C_CLASS_HWMON;
+ dev->adap.algo = &smbus_algorithm;
+ dev->adap.algo_data = dev;

+ dev->adap.dev.parent = &hdev->dev;

+ snprintf(dev->adap.name, sizeof(dev->adap.name),
+ "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
+ init_waitqueue_head(&dev->wait);
+ hid_device_io_start(hdev);
+ ret = i2c_add_adapter(&dev->adap);
+ hid_device_io_stop(hdev);

+ hid_hw_power(hdev, PM_HINT_NORMAL);
+ hid_hw_close(hdev);

+ if (ret) {
+ hid_err(hdev, "error registering i2c adapter\n");
+ kfree(dev);

+ goto hid_stop;

+ }
+ hid_dbg(hdev, "adapter registered\n");
+ return ret;

+power_normal:
+ hid_hw_power(hdev, PM_HINT_NORMAL);
+hid_close:
+ hid_hw_close(hdev);

+hid_stop:
+ hid_hw_stop(hdev);

+ return ret;
+}
+
+static void cp2112_remove(struct hid_device *hdev)
+{
+ struct cp2112_device *dev = hid_get_drvdata(hdev);
+

+ hid_hw_stop(hdev);
+ i2c_del_adapter(&dev->adap);
+ kfree(dev);

+}
+
+static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
+ uint8_t *data, int size)
+{
+ struct cp2112_device *dev = hid_get_drvdata(hdev);
+
+ switch (data[0]) {

+ case CP2112_TRANSFER_STATUS_RESPONSE:

+ case CP2112_DATA_READ_RESPONSE:

+ hid_dbg(hdev, "read response: %02x %02x\n", data[1], data[2]);
+ dev->read_length = data[2];
+ if (dev->read_length > sizeof(dev->read_data))
+ dev->read_length = sizeof(dev->read_data);
+ memcpy(dev->read_data, &data[3], dev->read_length);
+ atomic_set(&dev->read_avail, 1);
+ break;
+ default:
+ hid_err(hdev, "unknown report\n");
+ return 0;
+ }
+ wake_up_interruptible(&dev->wait);
+ return 1;
+}
+
+static struct hid_driver cp2112_driver = {
+ .name = "cp2112",
+ .id_table = cp2112_devices,
+ .probe = cp2112_probe,
+ .remove = cp2112_remove,
+ .raw_event = cp2112_raw_event,
+};
+

+module_hid_driver(cp2112_driver);

+MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
+MODULE_AUTHOR("David Barksdale <dbark...@uplogix.com>");
+MODULE_LICENSE("GPL");
+

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ffe4c7a..3a9dd5d 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -240,6 +240,7 @@

#define USB_VENDOR_ID_CYGNAL 0x10c4
#define USB_DEVICE_ID_CYGNAL_RADIO_SI470X 0x818a
+#define USB_DEVICE_ID_CYGNAL_CP2112 0xEA90

#define USB_VENDOR_ID_CYPRESS 0x04b4
#define USB_DEVICE_ID_CYPRESS_MOUSE 0x0001

--
tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
--

David Herrmann

unread,

Sep 30, 2013, 3:10:03 PM9/30/13

Hi

Sorry for the delay, but most of us are probably enjoying their
vacations and only available for bugfixes. See below for some
comments:

On Wed, Sep 11, 2013 at 10:13 PM, David Barksdale
<dbark...@uplogix.com> wrote:
> This patch adds support for the Silicon Labs CP2112
> "Single-Chip HID USB to SMBus Master Bridge."
>
> I wrote this to support a USB temperature and humidity
> sensor I've been working on. It's been tested by using
> SMBus byte-read, byte-data-read/write, and word-data-read
> transfer modes to talk to an I2C sensor. The other
> transfer modes have not been tested.

Wouldn't hurt to be a little bit more verbose here. Just explain in
your own words what kind of driver this is, whether there're any
datasheets available, which chipsets supported and what functionality
is working.

Btw., no punctuation in the commit-header please (so drop the final dot)

> Signed-off-by: David Barksdale <dbark...@uplogix.com>
>
> --
> Changes since v1:
> * Prefix symbols to avoid name collision.
> * Define CP2112 device ID.
> * Missing break.
> * Use module_hid_driver().
> * Fix cp2122 typo.
> * Fix probe and remove functions.
> * Remove #if in hid_have_special_driver[].
> * Improve symbolic constants for timeouts.
> * Wrap DMA functions to kmalloc buffers.
> * Added a short description of the device.
> * Add request formatting functions for readability.
> * Whitespace changes.
> * Fix endianess of word transfers.
>
> However this version doesn't work. Adding the hid_hw_open/hid_hw_close
> calls to cp2112_probe() as suggested by David Herrmann seems to have
> stopped events being delivered to cp2112_raw_event(). Why is that?

I suggest calling hid_hw_open() in _probe() and hid_hw_close() in
_remove(). Don't call it anywhere else and remove the hid_hw_power()
calls. Once you have a working driver, feel free to optimize it with
power-management. But my recommendation is to first try to get a
working base-driver in.

> ---
> drivers/hid/Kconfig | 6 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-core.c | 1 +
> drivers/hid/hid-cp2112.c | 559 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/hid/hid-ids.h | 1 +
> 5 files changed, 568 insertions(+)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 14ef6ab..1833948 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -175,6 +175,12 @@ config HID_PRODIKEYS
> multimedia keyboard, but will lack support for the musical keyboard
> and some additional multimedia keys.
>
> +config HID_CP2112
> + tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support"
> + depends on USB_HID
> + ---help---
> + Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge.
> +

I know, most of our config-sections are bad examples, but we should
try to be more verbose in new options at least.

FULLON should be the default, so you can drop this here.

Sorry, not really much time to do a full review now. Could you just
try to get a simple working revision and get it for review on the
list? Any extended functionality can be added later without problems.

Some coding-style hints:
- Don't be afraid of empty lines. After the closing brace of an if()
we usually have an empty line. Have a look at some other core HID code
to get a better feeling. Currently it's hard to read this code without
a proper structure.
- Split big functions. But split them logically. For instance the
cp2112_hid_get()+cp2112_hid_output() below could be split off into a
"cp2112_handshake()" or something like that (not sure what exactly it
does).

Other than that the driver looks really good now. If you resend it
with the small things fixed I will try to do a thorough review.

Thanks for your work!
David

Jiri Kosina

unread,

Oct 25, 2013, 5:30:02 AM10/25/13

On Mon, 30 Sep 2013, David Herrmann wrote:

> Sorry for the delay, but most of us are probably enjoying their
> vacations and only available for bugfixes. See below for some
> comments:

Thanks a lot for the review.

David, what are your plans for re-submitting the driver with feedback
incorporated?

Thanks,

--
Jiri Kosina
SUSE Labs

David Barksdale

unread,

Oct 25, 2013, 10:20:02 AM10/25/13

On 10/25/2013 04:29 AM, Jiri Kosina wrote:
> On Mon, 30 Sep 2013, David Herrmann wrote:
>
>> Sorry for the delay, but most of us are probably enjoying their
>> vacations and only available for bugfixes. See below for some
>> comments:
>
> Thanks a lot for the review.
>
> David, what are your plans for re-submitting the driver with feedback
> incorporated?
>
> Thanks,
>

I plan on re-submitting it. I just haven't had time to work on it.

0 new messages