* [PATCH RESEND] HID: New hid-cp2112 driver.
@ 2013-08-27 15:01 David Barksdale
2013-08-28 12:57 ` Jiri Kosina
2013-08-29 16:43 ` David Herrmann
0 siblings, 2 replies; 7+ messages in thread
From: David Barksdale @ 2013-08-27 15:01 UTC (permalink / raw)
To: Jiri Kosina; +Cc: David Barksdale, linux-kernel
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 <dbarksdale@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 <dbarksdale@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 <dbarksdale@uplogix.com>");
+MODULE_LICENSE("GPL");
+
--
tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-08-27 15:01 [PATCH RESEND] HID: New hid-cp2112 driver David Barksdale
@ 2013-08-28 12:57 ` Jiri Kosina
2013-08-28 13:35 ` David Barksdale
2013-08-29 13:51 ` David Barksdale
2013-08-29 16:43 ` David Herrmann
1 sibling, 2 replies; 7+ messages in thread
From: Jiri Kosina @ 2013-08-28 12:57 UTC (permalink / raw)
To: David Barksdale; +Cc: linux-kernel, Benjamin Tissoires
[ adding Benjamin to CC ]
On Tue, 27 Aug 2013, David Barksdale 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.
>
> Signed-off-by: David Barksdale <dbarksdale@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.
> +
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-08-28 12:57 ` Jiri Kosina
@ 2013-08-28 13:35 ` David Barksdale
2013-08-29 13:51 ` David Barksdale
1 sibling, 0 replies; 7+ messages in thread
From: David Barksdale @ 2013-08-28 13:35 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-kernel@vger.kernel.org, Benjamin Tissoires
On 08/28/2013 07:57 AM, Jiri Kosina wrote:
>
> [ adding Benjamin to CC ]
>
> On Tue, 27 Aug 2013, David Barksdale 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.
>>
>> Signed-off-by: David Barksdale <dbarksdale@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. +
>
> 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.
>
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-08-28 12:57 ` Jiri Kosina
2013-08-28 13:35 ` David Barksdale
@ 2013-08-29 13:51 ` David Barksdale
1 sibling, 0 replies; 7+ messages in thread
From: David Barksdale @ 2013-08-29 13:51 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-kernel@vger.kernel.org, Benjamin Tissoires
On 08/28/2013 07:57 AM, Jiri Kosina wrote:
>
> [ adding Benjamin to CC ]
>
> On Tue, 27 Aug 2013, David Barksdale 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.
>>
>> Signed-off-by: David Barksdale <dbarksdale@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.
>> +
>
> 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.
>
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-08-27 15:01 [PATCH RESEND] HID: New hid-cp2112 driver David Barksdale
2013-08-28 12:57 ` Jiri Kosina
@ 2013-08-29 16:43 ` David Herrmann
2013-09-03 21:51 ` David Barksdale
1 sibling, 1 reply; 7+ messages in thread
From: David Herrmann @ 2013-08-29 16:43 UTC (permalink / raw)
To: David Barksdale; +Cc: Jiri Kosina, linux-kernel
Hi
On Tue, Aug 27, 2013 at 5:01 PM, David Barksdale <dbarksdale@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.
>
> Signed-off-by: David Barksdale <dbarksdale@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
Why is that #if needed?
> { 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 <dbarksdale@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,
> +};
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.
> + { }
> +};
> +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;
Isn't there a break; missing?
> + 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);
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);
> +}
> +
> +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);
Use:
module_hid_driver(&cp2112_driver);
to save some lines here.
Cheers
David
> +MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
> +MODULE_AUTHOR("David Barksdale <dbarksdale@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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-08-29 16:43 ` David Herrmann
@ 2013-09-03 21:51 ` David Barksdale
2013-09-05 9:11 ` David Herrmann
0 siblings, 1 reply; 7+ messages in thread
From: David Barksdale @ 2013-09-03 21:51 UTC (permalink / raw)
To: David Herrmann; +Cc: Jiri Kosina, linux-kernel, Benjamin Tissoires
On 08/29/2013 11:43 AM, David Herrmann wrote:
> Hi
>
> On Tue, Aug 27, 2013 at 5:01 PM, David Barksdale <dbarksdale@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.
>>
>> Signed-off-by: David Barksdale <dbarksdale@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
>
> Why is that #if needed?
There are similar #ifs for CONFIG_HID_LENOVO_TPKBD,
CONFIG_HID_LOGITECH_DJ, and CONFIG_HID_ROCCAT. I thought it was a good idea.
>> { 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 <dbarksdale@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,
>> +};
>
> 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)
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.
>> + { }
>> +};
>> +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;
>
> Isn't there a break; missing?
Yes, thanks.
>> + 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);
>
> Use HID_CONNECT_HIDRAW or does your device provide more than just the
> smbus bridge?
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.
>> + }
>> + 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.
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.
>> +}
>> +
>> +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);
>
> Use:
> module_hid_driver(&cp2112_driver);
> to save some lines here.
Done.
> Cheers
> David
>
>> +MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
>> +MODULE_AUTHOR("David Barksdale <dbarksdale@uplogix.com>");
>> +MODULE_LICENSE("GPL");
>> +
>> --
>> tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
>> --
Thank you David for all the helpful comments.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] HID: New hid-cp2112 driver.
2013-09-03 21:51 ` David Barksdale
@ 2013-09-05 9:11 ` David Herrmann
0 siblings, 0 replies; 7+ messages in thread
From: David Herrmann @ 2013-09-05 9:11 UTC (permalink / raw)
To: David Barksdale; +Cc: Jiri Kosina, linux-kernel, Benjamin Tissoires
Hi
On Tue, Sep 3, 2013 at 11:51 PM, David Barksdale <dbarksdale@uplogix.com> wrote:
> On 08/29/2013 11:43 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Tue, Aug 27, 2013 at 5:01 PM, David Barksdale <dbarksdale@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.
>>>
>>> Signed-off-by: David Barksdale <dbarksdale@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
>>
>>
>> Why is that #if needed?
>
>
> There are similar #ifs for CONFIG_HID_LENOVO_TPKBD, CONFIG_HID_LOGITECH_DJ,
> and CONFIG_HID_ROCCAT. I thought it was a good idea.
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.
>
>>> { 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 <dbarksdale@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,
>>> +};
>>
>>
>> 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)
>
>
> Prefixes added. I think the STATUS* ones will be unique enough without
> another prefix.
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.
>
>>> + 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()?
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
>
>>> + 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.
>
>
>>> +}
>>> +
>>> +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);
>>
>>
>> Use:
>> module_hid_driver(&cp2112_driver);
>> to save some lines here.
>
>
> Done.
>
>
>> Cheers
>> David
>>
>>> +MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
>>> +MODULE_AUTHOR("David Barksdale <dbarksdale@uplogix.com>");
>>> +MODULE_LICENSE("GPL");
>>> +
>>> --
>>> tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
>>> --
>
>
> Thank you David for all the helpful comments.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-05 9:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-27 15:01 [PATCH RESEND] HID: New hid-cp2112 driver David Barksdale
2013-08-28 12:57 ` Jiri Kosina
2013-08-28 13:35 ` David Barksdale
2013-08-29 13:51 ` David Barksdale
2013-08-29 16:43 ` David Herrmann
2013-09-03 21:51 ` David Barksdale
2013-09-05 9:11 ` David Herrmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox