LinuxLists.cc - [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

2021-03-17 10:32:55

Subject: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

Signed-off-by: Bence Csókás <[email protected]>
---
drivers/i2c/busses/Kconfig | 10 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-cp2615.c | 282 ++++++++++++++++++++++++++++++++
3 files changed, 293 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-cp2615.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a49e0ed4a599..7a0dd18140d3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1227,6 +1227,16 @@ config I2C_DLN2
This driver can also be built as a module. If so, the module
will be called i2c-dln2.

+config I2C_CP2615
+ tristate "Silicon Labs CP2615 USB sound card and I2C adapter"
+ depends on USB
+ help
+ If you say yes to this option, support will be included for Silicon
+ Labs CP2615's I2C interface.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-cp2615.
+
config I2C_PARPORT
tristate "Parallel port adapter"
depends on PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 683c49faca05..adb71d9c9d42 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -127,6 +127,7 @@ obj-$(CONFIG_I2C_ZX2967) += i2c-zx2967.o
# External I2C/SMBus adapter drivers
obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
obj-$(CONFIG_I2C_DLN2) += i2c-dln2.o
+obj-$(CONFIG_I2C_CP2615) += i2c-cp2615.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_ROBOTFUZZ_OSIF) += i2c-robotfuzz-osif.o
obj-$(CONFIG_I2C_TAOS_EVM) += i2c-taos-evm.o
diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
new file mode 100644
index 000000000000..75f76201fbb3
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cp2615.c
@@ -0,0 +1,282 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * i2c support for Silicon Labs' CP2615 Digital Audio Bridge
+ *
+ * (c) 2021, Bence Csókás <[email protected]>
+ */
+
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/usb.h>
+
+/** CP2615 I/O Protocol implementation */
+
+#define CP2615_VID 0x10c4
+#define CP2615_PID 0xeac1
+
+#define IOP_EP_IN 0x82
+#define IOP_EP_OUT 0x02
+#define IOP_IFN 1
+#define IOP_ALTSETTING 2
+
+#define MAX_IOP_SIZE 64
+#define MAX_IOP_PAYLOAD_SIZE MAX_IOP_SIZE-6
+#define MAX_I2C_SIZE MAX_IOP_PAYLOAD_SIZE-4
+
+enum cp2615_iop_msg_type {
+ iop_GetAccessoryInfo = 0xD100,
+ iop_AccessoryInfo = 0xA100,
+ iop_GetPortConfiguration = 0xD203,
+ iop_PortConfiguration = 0xA203,
+ // ...
+ iop_DoI2cTransfer = 0xD400,
+ iop_I2cTransferResult = 0xA400,
+ iop_GetSerialState = 0xD501,
+ iop_SerialState = 0xA501
+};
+
+struct cp2615_iop_msg {
+ uint16_t preamble, length, msg;
+ uint8_t data[MAX_IOP_PAYLOAD_SIZE];
+};
+
+#define PART_ID_A01 0x1400
+#define PART_ID_A02 0x1500
+
+struct cp2615_iop_accessory_info {
+ uint16_t part_id, option_id, proto_ver;
+};
+
+struct cp2615_i2c_transfer {
+ uint8_t tag, i2caddr, read_len, write_len;
+ uint8_t data[MAX_I2C_SIZE];
+};
+
+/** Possible values for struct cp2615_i2c_transfer_result.status
+ *
+ * Values extracted from the USBXpress(r) SDK
+ */
+enum cp2615_i2c_status {
+ /* Writing to the internal EEPROM failed, because it is locked */
+ CP2615_CFG_LOCKED = -6,
+ /* read_len or write_len out of range */
+ CP2615_INVALID_PARAM = -4,
+ /* I2C slave did not ACK in time */
+ CP2615_TIMEOUT,
+ /* I2C bus busy */
+ CP2615_BUS_BUSY,
+ /* I2C bus error (ie. device NAK'd the request) */
+ CP2615_BUS_ERROR,
+ CP2615_SUCCESS
+};
+
+struct cp2615_i2c_transfer_result {
+ uint8_t tag, i2caddr;
+ int8_t status;
+ uint8_t read_len;
+ uint8_t data[MAX_I2C_SIZE];
+};
+
+int cp2615_init_iop_msg(struct cp2615_iop_msg *ret, enum cp2615_iop_msg_type msg, const void *data, size_t data_len)
+{
+ if (data_len > MAX_IOP_PAYLOAD_SIZE)
+ return -EFBIG;
+
+ if (ret) {
+ ret->preamble = 0x2A2A;
+ ret->length = htons(data_len+6);
+ ret->msg = htons(msg);
+ if(data && data_len)
+ memcpy(&ret->data, data, data_len);
+ return 0;
+ } else {
+ return -EINVAL;
+ }
+}
+
+int cp2615_init_i2c_msg(struct cp2615_iop_msg *ret, const struct cp2615_i2c_transfer *data)
+{
+ return cp2615_init_iop_msg(ret, iop_DoI2cTransfer, data, 4 + data->write_len);
+}
+
+/* Translates status codes to Linux errno's */
+int cp2615_check_status(enum cp2615_i2c_status status)
+{
+ switch (status) {
+ case CP2615_SUCCESS:
+ return 0;
+ case CP2615_BUS_ERROR:
+ return -ECOMM;
+ case CP2615_BUS_BUSY:
+ return -EAGAIN;
+ case CP2615_TIMEOUT:
+ return -ETIMEDOUT;
+ case CP2615_INVALID_PARAM:
+ return -EINVAL;
+ case CP2615_CFG_LOCKED:
+ return -EPERM;
+ }
+ /* Unknown error code */
+ return -EPROTO;
+}
+
+
+static int
+cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
+{
+ struct cp2615_iop_msg *msg = kzalloc(sizeof(struct cp2615_iop_msg), GFP_KERNEL);
+ struct usb_device *usbdev = interface_to_usbdev(usbif);
+ int res = cp2615_init_i2c_msg(msg, i2c_w);
+ if (!res)
+ res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT), msg, ntohs(msg->length), NULL, 0);
+ kfree(msg);
+ return res;
+}
+
+static int
+cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
+{
+ struct cp2615_iop_msg *msg = kzalloc(sizeof(struct cp2615_iop_msg), GFP_KERNEL);
+ struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result*) &msg->data;
+ struct usb_device *usbdev = interface_to_usbdev(usbif);
+ int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN), msg, sizeof(struct cp2615_iop_msg), NULL, 0);
+ if (res < 0)
+ return res;
+
+ if (msg->msg != htons(iop_I2cTransferResult) || i2c_r->tag != tag)
+ return -EIO;
+
+ res = cp2615_check_status(i2c_r->status);
+ if (res < 0)
+ return res;
+
+ memcpy(buf, &i2c_r->data, i2c_r->read_len);
+ kfree(msg);
+ return 0;
+}
+
+static int
+cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct usb_interface *usbif = adap->algo_data;
+ int i = 0, ret = 0;
+ struct i2c_msg *msg;
+ struct cp2615_i2c_transfer i2c_w = {0};
+ dev_dbg(&usbif->dev, "Doing %d I2C transactions\n", num);
+
+ for(; !ret && i < num; i++) {
+ msg = &msgs[i];
+
+ i2c_w.tag = 0xdd;
+ i2c_w.i2caddr = i2c_8bit_addr_from_msg(msg);
+ if (msg->flags & I2C_M_RD) {
+ i2c_w.read_len = msg->len;
+ i2c_w.write_len = 0;
+ } else {
+ i2c_w.read_len = 0;
+ i2c_w.write_len = msg->len;
+ memcpy(&i2c_w.data, msg->buf, i2c_w.write_len);
+ }
+ ret = cp2615_i2c_send(usbif, &i2c_w);
+ if (ret)
+ break;
+ ret = cp2615_i2c_recv(usbif, i2c_w.tag, msg->buf);
+ }
+ if (ret < 0)
+ return ret;
+ return i;
+}
+
+static u32
+cp2615_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm cp2615_i2c_algo = {
+ .master_xfer = cp2615_i2c_master_xfer,
+ .functionality = cp2615_i2c_func,
+};
+
+/*
+ * This chip has some limitations: one is that the USB endpoint
+ * can only receive 64 bytes/transfer, that leaves 54 bytes for
+ * the I2C transfer. On top of that, EITHER read_len OR write_len
+ * may be zero, but not both. If both are non-zero, the adapter
+ * issues a write followed by a read. And the chip does not
+ * support repeated START between the write and read phases.
+ *
+ * FIXME: There in no quirk flag for specifying that the adapter
+ * does not support empty transfers, or that it cannot emit a
+ * START condition between the combined phases.
+ */
+struct i2c_adapter_quirks cp2615_i2c_quirks = {
+ .max_write_len = MAX_I2C_SIZE,
+ .max_read_len = MAX_I2C_SIZE,
+ .flags = I2C_AQ_COMB_WRITE_THEN_READ,
+ .max_comb_1st_msg_len = MAX_I2C_SIZE,
+ .max_comb_2nd_msg_len = MAX_I2C_SIZE
+};
+
+static void
+cp2615_i2c_remove(struct usb_interface *usbif)
+{
+ struct i2c_adapter *adap = usb_get_intfdata(usbif);
+
+ usb_set_intfdata(usbif, NULL);
+ i2c_del_adapter(adap);
+}
+
+static int
+cp2615_i2c_probe(struct usb_interface *usbif, const struct usb_device_id *id)
+{
+ int ret = 0;
+ struct i2c_adapter *adap;
+ struct usb_device *usbdev = interface_to_usbdev(usbif);
+
+ ret = usb_set_interface(usbdev, IOP_IFN, IOP_ALTSETTING);
+ if (ret)
+ return ret;
+
+ adap = devm_kzalloc(&usbif->dev, sizeof(struct i2c_adapter), GFP_KERNEL);
+ if (!adap)
+ return -ENOMEM;
+
+ strncpy(adap->name, usbdev->serial, sizeof(adap->name));
+ adap->owner = THIS_MODULE;
+ adap->dev.parent = &usbif->dev;
+ adap->dev.of_node = usbif->dev.of_node;
+ adap->timeout = HZ;
+ adap->algo = &cp2615_i2c_algo;
+ adap->quirks = &cp2615_i2c_quirks;
+ adap->algo_data = usbif;
+
+ ret = i2c_add_adapter(adap);
+ if (ret)
+ return ret;
+
+ usb_set_intfdata(usbif, adap);
+ return ret;
+}
+
+static const struct usb_device_id id_table[] = {
+ { USB_DEVICE(CP2615_VID, CP2615_PID) },
+ { }
+};
+
+MODULE_DEVICE_TABLE(usb, id_table);
+
+static struct usb_driver cp2615_i2c_driver = {
+ .name = "i2c-cp2615",
+ .probe = cp2615_i2c_probe,
+ .disconnect = cp2615_i2c_remove,
+ .id_table = id_table,
+};
+
+module_usb_driver(cp2615_i2c_driver);
+
+MODULE_AUTHOR("Bence Csókás <[email protected]>");
+MODULE_DESCRIPTION("CP2615 I2C bus driver");
+MODULE_LICENSE("GPL");
--
2.30.1

2021-03-17 12:27:43

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

Hi "Bence,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc3 next-20210317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Bence-Cs-k-s/Adding-i2c-cp2615-i2c-support-for-Silicon-Labs-CP2615-Digital-Audio-Bridge/20210317-181539
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/c8c005a08175789b1874e69abf4c6da690d5b323
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bence-Cs-k-s/Adding-i2c-cp2615-i2c-support-for-Silicon-Labs-CP2615-Digital-Audio-Bridge/20210317-181539
git checkout c8c005a08175789b1874e69abf4c6da690d5b323
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-cp2615.c:82:5: warning: no previous prototype for 'cp2615_init_iop_msg' [-Wmissing-prototypes]
82 | int cp2615_init_iop_msg(struct cp2615_iop_msg *ret, enum cp2615_iop_msg_type msg, const void *data, size_t data_len)
| ^~~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-cp2615.c:99:5: warning: no previous prototype for 'cp2615_init_i2c_msg' [-Wmissing-prototypes]
99 | int cp2615_init_i2c_msg(struct cp2615_iop_msg *ret, const struct cp2615_i2c_transfer *data)
| ^~~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-cp2615.c:105:5: warning: no previous prototype for 'cp2615_check_status' [-Wmissing-prototypes]
105 | int cp2615_check_status(enum cp2615_i2c_status status)
| ^~~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-cp2615.c:269:1: warning: data definition has no type or storage class
269 | MODULE_DEVICE_TABLE(usb, id_table);
| ^~~~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:269:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/i2c/busses/i2c-cp2615.c:269:1: warning: parameter names (without types) in function declaration
In file included from include/linux/device.h:32,
from include/linux/acpi.h:15,
from include/linux/i2c.h:13,
from drivers/i2c/busses/i2c-cp2615.c:11:
>> include/linux/device/driver.h:263:1: warning: data definition has no type or storage class
263 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:263:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int]
263 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:7,
from drivers/i2c/busses/i2c-cp2615.c:9:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/usb.h:1290:30: note: in expansion of macro 'THIS_MODULE'
1290 | usb_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'usb_register'
261 | return __register(&(__driver) , ##__VA_ARGS__); \
| ^~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:32,
from include/linux/acpi.h:15,
from include/linux/i2c.h:13,
from drivers/i2c/busses/i2c-cp2615.c:11:
include/linux/device/driver.h:268:1: warning: data definition has no type or storage class
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:268:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int]
268 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:7,
from include/linux/kernel.h:7,
from drivers/i2c/busses/i2c-cp2615.c:9:
>> include/linux/export.h:19:30: warning: parameter names (without types) in function declaration
19 | #define THIS_MODULE ((struct module *)0)
| ^~~~~~
include/linux/usb.h:1290:30: note: in expansion of macro 'THIS_MODULE'
1290 | usb_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
| ^~~~~~~~~~~
include/linux/device/driver.h:261:9: note: in expansion of macro 'usb_register'
261 | return __register(&(__driver) , ##__VA_ARGS__); \
| ^~~~~~~~~~
include/linux/usb.h:1303:2: note: in expansion of macro 'module_driver'
1303 | module_driver(__usb_driver, usb_register, \
| ^~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:280:15: error: expected declaration specifiers or '...' before string constant
280 | MODULE_AUTHOR("Bence Cs?k?s <[email protected]>");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:281:20: error: expected declaration specifiers or '...' before string constant
281 | MODULE_DESCRIPTION("CP2615 I2C bus driver");
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:282:16: error: expected declaration specifiers or '...' before string constant
282 | MODULE_LICENSE("GPL");
| ^~~~~
In file included from include/linux/device.h:32,
from include/linux/acpi.h:15,
from include/linux/i2c.h:13,
from drivers/i2c/busses/i2c-cp2615.c:11:
drivers/i2c/busses/i2c-cp2615.c:278:19: warning: 'cp2615_i2c_driver_init' defined but not used [-Wunused-function]
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/device/driver.h:259:19: note: in definition of macro 'module_driver'
259 | static int __init __driver##_init(void) \
| ^~~~~~~~
drivers/i2c/busses/i2c-cp2615.c:278:1: note: in expansion of macro 'module_usb_driver'
278 | module_usb_driver(cp2615_i2c_driver);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/cp2615_init_iop_msg +82 drivers/i2c/busses/i2c-cp2615.c

81
> 82 int cp2615_init_iop_msg(struct cp2615_iop_msg *ret, enum cp2615_iop_msg_type msg, const void *data, size_t data_len)
83 {
84 if (data_len > MAX_IOP_PAYLOAD_SIZE)
85 return -EFBIG;
86
87 if (ret) {
88 ret->preamble = 0x2A2A;
89 ret->length = htons(data_len+6);
90 ret->msg = htons(msg);
91 if(data && data_len)
92 memcpy(&ret->data, data, data_len);
93 return 0;
94 } else {
95 return -EINVAL;
96 }
97 }
98
> 99 int cp2615_init_i2c_msg(struct cp2615_iop_msg *ret, const struct cp2615_i2c_transfer *data)
100 {
101 return cp2615_init_iop_msg(ret, iop_DoI2cTransfer, data, 4 + data->write_len);
102 }
103
104 /* Translates status codes to Linux errno's */
> 105 int cp2615_check_status(enum cp2615_i2c_status status)
106 {
107 switch (status) {
108 case CP2615_SUCCESS:
109 return 0;
110 case CP2615_BUS_ERROR:
111 return -ECOMM;
112 case CP2615_BUS_BUSY:
113 return -EAGAIN;
114 case CP2615_TIMEOUT:
115 return -ETIMEDOUT;
116 case CP2615_INVALID_PARAM:
117 return -EINVAL;
118 case CP2615_CFG_LOCKED:
119 return -EPERM;
120 }
121 /* Unknown error code */
122 return -EPROTO;
123 }
124
125
126 static int
127 cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
128 {
129 struct cp2615_iop_msg *msg = kzalloc(sizeof(struct cp2615_iop_msg), GFP_KERNEL);
130 struct usb_device *usbdev = interface_to_usbdev(usbif);
131 int res = cp2615_init_i2c_msg(msg, i2c_w);
132 if (!res)
133 res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT), msg, ntohs(msg->length), NULL, 0);
134 kfree(msg);
135 return res;
136 }
137
138 static int
139 cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
140 {
141 struct cp2615_iop_msg *msg = kzalloc(sizeof(struct cp2615_iop_msg), GFP_KERNEL);
142 struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result*) &msg->data;
143 struct usb_device *usbdev = interface_to_usbdev(usbif);
144 int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN), msg, sizeof(struct cp2615_iop_msg), NULL, 0);
145 if (res < 0)
146 return res;
147
148 if (msg->msg != htons(iop_I2cTransferResult) || i2c_r->tag != tag)
149 return -EIO;
150
151 res = cp2615_check_status(i2c_r->status);
152 if (res < 0)
153 return res;
154
155 memcpy(buf, &i2c_r->data, i2c_r->read_len);
156 kfree(msg);
157 return 0;
158 }
159
160 static int
161 cp2615_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
162 {
163 struct usb_interface *usbif = adap->algo_data;
164 int i = 0, ret = 0;
165 struct i2c_msg *msg;
166 struct cp2615_i2c_transfer i2c_w = {0};
167 dev_dbg(&usbif->dev, "Doing %d I2C transactions\n", num);
168
169 for(; !ret && i < num; i++) {
170 msg = &msgs[i];
171
172 i2c_w.tag = 0xdd;
173 i2c_w.i2caddr = i2c_8bit_addr_from_msg(msg);
174 if (msg->flags & I2C_M_RD) {
175 i2c_w.read_len = msg->len;
176 i2c_w.write_len = 0;
177 } else {
178 i2c_w.read_len = 0;
179 i2c_w.write_len = msg->len;
180 memcpy(&i2c_w.data, msg->buf, i2c_w.write_len);
181 }
182 ret = cp2615_i2c_send(usbif, &i2c_w);
183 if (ret)
184 break;
185 ret = cp2615_i2c_recv(usbif, i2c_w.tag, msg->buf);
186 }
187 if (ret < 0)
188 return ret;
189 return i;
190 }
191
192 static u32
193 cp2615_i2c_func(struct i2c_adapter *adap)
194 {
195 return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
196 }
197
198 static const struct i2c_algorithm cp2615_i2c_algo = {
199 .master_xfer = cp2615_i2c_master_xfer,
200 .functionality = cp2615_i2c_func,
201 };
202
203 /*
204 * This chip has some limitations: one is that the USB endpoint
205 * can only receive 64 bytes/transfer, that leaves 54 bytes for
206 * the I2C transfer. On top of that, EITHER read_len OR write_len
207 * may be zero, but not both. If both are non-zero, the adapter
208 * issues a write followed by a read. And the chip does not
209 * support repeated START between the write and read phases.
210 *
211 * FIXME: There in no quirk flag for specifying that the adapter
212 * does not support empty transfers, or that it cannot emit a
213 * START condition between the combined phases.
214 */
215 struct i2c_adapter_quirks cp2615_i2c_quirks = {
216 .max_write_len = MAX_I2C_SIZE,
217 .max_read_len = MAX_I2C_SIZE,
218 .flags = I2C_AQ_COMB_WRITE_THEN_READ,
219 .max_comb_1st_msg_len = MAX_I2C_SIZE,
220 .max_comb_2nd_msg_len = MAX_I2C_SIZE
221 };
222
223 static void
224 cp2615_i2c_remove(struct usb_interface *usbif)
225 {
226 struct i2c_adapter *adap = usb_get_intfdata(usbif);
227
228 usb_set_intfdata(usbif, NULL);
229 i2c_del_adapter(adap);
230 }
231
232 static int
233 cp2615_i2c_probe(struct usb_interface *usbif, const struct usb_device_id *id)
234 {
235 int ret = 0;
236 struct i2c_adapter *adap;
237 struct usb_device *usbdev = interface_to_usbdev(usbif);
238
239 ret = usb_set_interface(usbdev, IOP_IFN, IOP_ALTSETTING);
240 if (ret)
241 return ret;
242
243 adap = devm_kzalloc(&usbif->dev, sizeof(struct i2c_adapter), GFP_KERNEL);
244 if (!adap)
245 return -ENOMEM;
246
247 strncpy(adap->name, usbdev->serial, sizeof(adap->name));
248 adap->owner = THIS_MODULE;
249 adap->dev.parent = &usbif->dev;
250 adap->dev.of_node = usbif->dev.of_node;
251 adap->timeout = HZ;
252 adap->algo = &cp2615_i2c_algo;
253 adap->quirks = &cp2615_i2c_quirks;
254 adap->algo_data = usbif;
255
256 ret = i2c_add_adapter(adap);
257 if (ret)
258 return ret;
259
260 usb_set_intfdata(usbif, adap);
261 return ret;
262 }
263
264 static const struct usb_device_id id_table[] = {
265 { USB_DEVICE(CP2615_VID, CP2615_PID) },
266 { }
267 };
268
> 269 MODULE_DEVICE_TABLE(usb, id_table);
270

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (15.13 kB)
.config.gz (65.79 kB)
Download all attachments

2021-03-17 12:35:49

by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

On Wed, Mar 17, 2021 at 10:30:21AM +0000, Bence Csókás wrote:
> Signed-off-by: Bence Csókás <[email protected]>

Thanks, this looks good now and I think we are very close.

> ---

Next, time please provide a small summary of changes since last version.
I get enough patches that it becomes confusing otherwise.

> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cp2615.c
> @@ -0,0 +1,282 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

If you want GPL v2 only, then you need to say in MODULE_LICENSE also
"GPL v2".

> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/usb.h>

Please sort this to avoid double entries get added in the future.

> +enum cp2615_iop_msg_type {
> + iop_GetAccessoryInfo = 0xD100,
> + iop_AccessoryInfo = 0xA100,
> + iop_GetPortConfiguration = 0xD203,
> + iop_PortConfiguration = 0xA203,
> + // ...

This comment can go?

> +/** Possible values for struct cp2615_i2c_transfer_result.status
> + *
> + * Values extracted from the USBXpress(r) SDK
> + */

I'd think this can go, too.

> +int cp2615_init_iop_msg(struct cp2615_iop_msg *ret, enum cp2615_iop_msg_type msg, const void *data, size_t data_len)

Please break it into two lines. And please run 'checkpatch --strict' on
your patch to find things like too long lines and spaces around
operators etc... I will skip pointing them out here.

> +{
> + if (data_len > MAX_IOP_PAYLOAD_SIZE)
> + return -EFBIG;
> +
> + if (ret) {

Minor nit:
if (!ret)
return -EINVAL;

and then save the indentation for the main code path.

> + ret->preamble = 0x2A2A;
> + ret->length = htons(data_len+6);
> + ret->msg = htons(msg);
> + if(data && data_len)
> + memcpy(&ret->data, data, data_len);
> + return 0;
> + } else {
> + return -EINVAL;
> + }
> +}
> +
> +int cp2615_init_i2c_msg(struct cp2615_iop_msg *ret, const struct cp2615_i2c_transfer *data)
> +{
> + return cp2615_init_iop_msg(ret, iop_DoI2cTransfer, data, 4 + data->write_len);
> +}

I'll leave it to you but maybe this function can go and
cp2615_init_iop_msg can be used directly? Both are only called once.

> +
> +/* Translates status codes to Linux errno's */
> +int cp2615_check_status(enum cp2615_i2c_status status)
> +{
> + switch (status) {
> + case CP2615_SUCCESS:
> + return 0;
> + case CP2615_BUS_ERROR:
> + return -ECOMM;

-EIO probably.

We have 'Documentation/i2c/fault-codes.rst' as a guide.

> + case CP2615_BUS_BUSY:
> + return -EAGAIN;
> + case CP2615_TIMEOUT:
> + return -ETIMEDOUT;
> + case CP2615_INVALID_PARAM:
> + return -EINVAL;
> + case CP2615_CFG_LOCKED:
> + return -EPERM;
> + }
> + /* Unknown error code */
> + return -EPROTO;
> +}
> +

...

> +/*
> + * This chip has some limitations: one is that the USB endpoint
> + * can only receive 64 bytes/transfer, that leaves 54 bytes for
> + * the I2C transfer. On top of that, EITHER read_len OR write_len
> + * may be zero, but not both. If both are non-zero, the adapter
> + * issues a write followed by a read. And the chip does not
> + * support repeated START between the write and read phases.

Good and useful paragraph!

> + * FIXME: There in no quirk flag for specifying that the adapter
> + * does not support empty transfers, or that it cannot emit a

Can't we use I2C_AQ_NO_ZERO_LEN here?

> + * START condition between the combined phases.

True! But it makes sense, so we can fix that. We just need to add
I2C_AQ_NO_REP_START and a short explanation to i2c.h. If you want, you
can do it in a seperate patch. I can do it, too, if you prefer.

> + */
> +struct i2c_adapter_quirks cp2615_i2c_quirks = {
> + .max_write_len = MAX_I2C_SIZE,
> + .max_read_len = MAX_I2C_SIZE,
> + .flags = I2C_AQ_COMB_WRITE_THEN_READ,
> + .max_comb_1st_msg_len = MAX_I2C_SIZE,
> + .max_comb_2nd_msg_len = MAX_I2C_SIZE
> +};

quirks struct looks good otherwise!

> +static const struct usb_device_id id_table[] = {
> + { USB_DEVICE(CP2615_VID, CP2615_PID) },

Maybe skip the defines for VID and PID and use the values directly?
I am not a USB expert, not really sure what the consistent way is.

So, this and the checkpatch issues and I think we are done.

Thanks,

Wolfram

Attachments:

(No filename) (4.23 kB)
signature.asc (849.00 B)
Download all attachments

2021-03-17 17:18:26

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

Attachments:

(No filename) (4.12 kB)
.config.gz (62.01 kB)
Download all attachments

2021-03-18 02:00:33

by Bence Csókás

[permalink] [raw]

Subject: Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

Thanks for the lightning quick response!

Wolfram Sang <[email protected]> ezt írta (időpont: 2021. márc. 17., Sze, 13:34):
>
> On Wed, Mar 17, 2021 at 10:30:21AM +0000, Bence Csókás wrote:
> > Signed-off-by: Bence Csókás <[email protected]>
>
> Thanks, this looks good now and I think we are very close.
>
> > ---
>
> Next, time please provide a small summary of changes since last version.
> I get enough patches that it becomes confusing otherwise.
>

You are right, sorry, I am still familiarizing myself with `git send-email`

> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > @@ -0,0 +1,282 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> If you want GPL v2 only, then you need to say in MODULE_LICENSE also
> "GPL v2".
>

GPLv2 or later is fine by me. If I change this to "//
SPDX-License-Identifier: GPL-2.0-or-later", is that OK?

> > +enum cp2615_iop_msg_type {
> > + iop_GetAccessoryInfo = 0xD100,
> > + iop_AccessoryInfo = 0xA100,
> > + iop_GetPortConfiguration = 0xD203,
> > + iop_PortConfiguration = 0xA203,
> > + // ...
>
> This comment can go?

Sorry, this slipped in from before... I shall remove it.

> > +/*
> > + * This chip has some limitations: one is that the USB endpoint
> > + * can only receive 64 bytes/transfer, that leaves 54 bytes for
> > + * the I2C transfer. On top of that, EITHER read_len OR write_len
> > + * may be zero, but not both. If both are non-zero, the adapter
> > + * issues a write followed by a read. And the chip does not
> > + * support repeated START between the write and read phases.
>
> Good and useful paragraph!

Thank you!

>
> > + * FIXME: There in no quirk flag for specifying that the adapter
> > + * does not support empty transfers, or that it cannot emit a
>
> Can't we use I2C_AQ_NO_ZERO_LEN here?

I thought that meant the adapter cannot handle NEITHER zero-length
reads NOR writes, but the CP2615 can do a zero read combined with a
non-zero write or the other way around, just both cannot be zero. If
both are zero, the chip just ignores the request, as I've learned from
a very confusing situation with `i2cdetect`.

>
> > + * START condition between the combined phases.
>
> True! But it makes sense, so we can fix that. We just need to add
> I2C_AQ_NO_REP_START and a short explanation to i2c.h. If you want, you
> can do it in a seperate patch. I can do it, too, if you prefer.

Sure! I should just define it as BIT(7) or something, right? Should I
do it in a completely different patchset, or is it OK if I submit it
as the 2/2 of PATCH v3? Are there maybe other adapters that would be
affected?

> Maybe skip the defines for VID and PID and use the values directly?
> I am not a USB expert, not really sure what the consistent way is.

I think this is how they usually do it, or at least from what I've seen.

>
> So, this and the checkpatch issues and I think we are done.
>
> Thanks,
>
> Wolfram
>

Thanks,
Bence

2021-03-18 06:01:35

by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

Hi Bence,

> You are right, sorry, I am still familiarizing myself with `git send-email`

No worries, the progress from your first to your second version was
really great! You can add such information e.g. using the "--annotate"
parameter og git-send-email.

> GPLv2 or later is fine by me. If I change this to "//
> SPDX-License-Identifier: GPL-2.0-or-later", is that OK?

Yes, also perfect.

> > > + * FIXME: There in no quirk flag for specifying that the adapter
> > > + * does not support empty transfers, or that it cannot emit a
> >
> > Can't we use I2C_AQ_NO_ZERO_LEN here?
>
> I thought that meant the adapter cannot handle NEITHER zero-length
> reads NOR writes, but the CP2615 can do a zero read combined with a
> non-zero write or the other way around, just both cannot be zero. If
> both are zero, the chip just ignores the request, as I've learned from
> a very confusing situation with `i2cdetect`.

I think we still should use I2C_AQ_NO_ZERO_LEN. The almost only use case
is a standalone zero-len read or write. This is not supported by your
adapter as you found out.

> > True! But it makes sense, so we can fix that. We just need to add
> > I2C_AQ_NO_REP_START and a short explanation to i2c.h. If you want, you
> > can do it in a seperate patch. I can do it, too, if you prefer.
>
> Sure! I should just define it as BIT(7) or something, right? Should I
> do it in a completely different patchset, or is it OK if I submit it
> as the 2/2 of PATCH v3? Are there maybe other adapters that would be

Yes, BIT(7) and same patchset please. But you need to make it 1/2
because you will use it in you driver which is then 2/2.

> affected?

Maybe, but we can fix them later incrementally. Currently, there is also
no client testing the flag, so we have a bit of time here.

> > Maybe skip the defines for VID and PID and use the values directly?
> > I am not a USB expert, not really sure what the consistent way is.
>
> I think this is how they usually do it, or at least from what I've seen.

Then keep it.

All the best and happy hacking,

Wolfram

Attachments:

(No filename) (2.07 kB)
signature.asc (849.00 B)
Download all attachments