* [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