i2c: add support for Cypress CYUSBS234 USB-I2C adapter
Subsystems (2)
PATCH v4 2/3 i2c: add support for Cypress CYUSBS234 USB-I2C adapter
Adds support for USB-I2C interface of Cypress Semiconductor CYUSBS234 USB-Serial Bridge controller.
The read/write operation is setup using vendor command through control endpoint and actual data transfer happens through bulk in/out endpoints.
Details about the device can be found at:
http://www.cypress.com/?rID=84126
---
Changes since v3:
* corrected typo
Changes since v2:
* Retrieved the i2c transfer status using interrupt in endpoint
* Used kstrtol instead of manually parsing and scnprintf instead of sprintf
* Given i2c adapter device for dev_xxx
* cleaned up the code
Changes since v1:
* allocated memory on heap for usb transfer data
* Used DEVICE_ATTR_xx and friends and attribute groups
* Added the device files under i2c-adapter rather than platform device
drivers/i2c/busses/Kconfig | 12 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-cyusbs23x.c | 584 +++++++++++++++++++++++++++++++++++++ 3 files changed, 597 insertions(+) create mode 100644 drivers/i2c/busses/i2c-cyusbs23x.c
drivers/i2c/busses/Kconfig +12 -0
@@ -871,6 +871,18 @@ config I2C_RCAR comment "External I2C/SMBus adapter drivers" +config I2C_CYUSBS23X + tristate "CYUSBS23x I2C adapter" + depends on MFD_CYUSBS23X && USB + help + Say yes if you would like to access Cypress CYUSBS23x I2C device. + + This driver enables the I2C interface of CYUSBS23x USB Serial Bridge + controller. + + This driver can also be built as a module. If so, the module will be + called i2c-cyusbs23x. + config I2C_DIOLAN_U2C tristate "Diolan U2C-12 USB adapter" depends on USB
drivers/i2c/busses/Makefile +1 -0
@@ -86,6 +86,7 @@ obj-$(CONFIG_I2C_XLR) += i2c-xlr.o obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o # External I2C/SMBus adapter drivers +obj-$(CONFIG_I2C_CYUSBS23X) += i2c-cyusbs23x.o obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o obj-$(CONFIG_I2C_PARPORT_LIGHT) += i2c-parport-light.o
drivers/i2c/busses/i2c-cyusbs23x.c new file +584 -0
584 lines not on this page · raw message
--
1.8.3.2
This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.
Hi,
thanks for this submission. Looks like an interesting device. However, some very high level questions first:
Quoted 4 lines
+/*
+ * It exposes sysfs entries under the i2c adapter for getting the i2c transfer
+ * status, reset i2c read/write module, get/set nak and stop bits.
+ */
Yes, I see that. Yet, I don't know why they are needed? The driver should know when to send NAK/STOP. Why expose that to the user? And why do we need a reset? Are there stability problems? What about unloading the module? And what is the use case for reading the status?
drivers/i2c/busses/i2c-cyusbs23x.c line 438
Quoted 4 lines
drivers/i2c/busses/i2c-cyusbs23x.c new file +4
-0+if (num >1 ) { +dev_err (&adapter->dev,"i2c_msg number is > 1\n" ); +return -EIO ; + }
Ouch! Don't you have any repeated start option somewhere?
Thanks,
Wolfram
Hi,
Thanks for reviewing the code.
Quoted 10 lines
drivers/i2c/busses/i2c-cyusbs23x.c line 22
Quoted 5 lines
drivers/i2c/busses/i2c-cyusbs23x.c new file +5
-0+/* +* It exposes sysfs entries under the i2c adapter for getting the i2c +transfer +* status, reset i2c read/write module, get/set nak and stop bits. +*/ Yes, I see that. Yet, I don't know why they are needed? The driver should know when to send NAK/STOP. Why expose that to the user? And why do we need a reset? Are there stability problems? What about unloading the module? And what is the use case for reading the status?
For repeated start (Sr) scenario, the STOP bit will not be set.
For dummy write scenario (writing EEPROM address from I2C EEPROM slave), the STOP bit should not be set. But, for normal I2C write, the STOP bit should be set. We provide control to user to control when to STOP/NAK to handle different scenarios.
The reset is an additional provision given to user to reset the I2C read or write engines. It does not require unloading I2C kernel module. The status indicates the information about the last i2c read/write operation.
Quoted 7 lines
drivers/i2c/busses/i2c-cyusbs23x.c line 438
Quoted 5 lines
drivers/i2c/busses/i2c-cyusbs23x.c new file +4
-0+if (num >1 ) { +dev_err (&adapter->dev,"i2c_msg number is > 1\n" ); +return -EIO ; + }Ouch! Don't you have any repeated start option somewhere?
In this version of the driver, we handle only single i2c message. In future iterations, we will add support for multiple i2c messages.
Thanks,
Muthu
This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.
Quoted 6 lines
For repeated start (Sr) scenario, the STOP bit will not be set. For dummy write scenario (writing EEPROM address from I2C EEPROM slave), the STOP bit should not be set. But, for normal I2C write, the STOP bit should be set. We provide control to user to control when to STOP/NAK to handle different scenarios.
No, your driver needs to do all of that.
Quoted 4 lines
The reset is an additional provision given to user to reset the I2C read or write engines. It does not require unloading I2C kernel module. The status indicates the information about the last i2c read/write operation.
I don't see the need for the user to do this. Better skip it, there should be a real need for userspace API.
Quoted 2 lines
In this version of the driver, we handle only single i2c message. In future iterations, we will add support for multiple i2c messages.
Then please resubmit when all this is done. It is OK to add features incrementally, but hey, the essential things should be there right from the beginning. Also, it would be nice if you told me your plans about incrementally working on it right from the beginning. That would have saved me a bit of reviewing time, a resource which is too scarce already.