Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 new file mode 100644 index 000..ad55af6 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 This is a nonstandard path that does not match the attribute path (or any other convention followed under ABI/). I suggest you rename it sysfs-class-i2c-adapter-dln2 @@ -0,0 +1,5 @@ +What:/sys/bus/i2c/devices/.../dln2_i2c_freq I think you should rename the attribute bus_frequency and name the attribute group dln2. That way the attribute will show up as .../device/dln2/bus_frequency Then the What: field should read What: /sys/class/i2c-adapter/device/dln2/bus_frequency +Date:September 2014 +Contact: Octavian Purdila octavian.purd...@intel.com +Description: + This attribute shows/sets the frequency (in Hz) of the I2C bus. I'd reword this Set the frequency Please also describe under what circumstances this may fail, that is, when setting a frequency less than or greater than the min or max frequencies reported by the device. Perhaps you should even consider exporting those two values? Johan -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Wed, Oct 8, 2014 at 1:42 PM, Johan Hovold jo...@kernel.org wrote: On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 new file mode 100644 index 000..ad55af6 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 This is a nonstandard path that does not match the attribute path (or any other convention followed under ABI/). I suggest you rename it sysfs-class-i2c-adapter-dln2 @@ -0,0 +1,5 @@ +What:/sys/bus/i2c/devices/.../dln2_i2c_freq I think you should rename the attribute bus_frequency and name the attribute group dln2. That way the attribute will show up as .../device/dln2/bus_frequency Then the What: field should read What: /sys/class/i2c-adapter/device/dln2/bus_frequency +Date:September 2014 +Contact: Octavian Purdila octavian.purd...@intel.com +Description: + This attribute shows/sets the frequency (in Hz) of the I2C bus. I'd reword this Set the frequency Please also describe under what circumstances this may fail, that is, when setting a frequency less than or greater than the min or max frequencies reported by the device. Perhaps you should even consider exporting those two values? I noticed that a few other USB boards support this. Perhaps it is time to add these entries to the core i2c layer? In that case I think is better to remove the setting and update the drievr when the patches for the i2c layer are ready? -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: From: Laurentiu Palcu laurentiu.pa...@intel.com This patch adds support for the Diolan DLN-2 I2C master module. Due to hardware limitations it does not support SMBUS quick commands. Information about the USB protocol interface can be found in the Programmer's Reference Manual [1], see section 6.2.2 for the I2C master module commands and responses. [1] https://www.diolan.com/downloads/dln-api-manual.pdf Signed-off-by: Laurentiu Palcu laurentiu.pa...@intel.com Signed-off-by: Octavian Purdila octavian.purd...@intel.com --- [...] +#define DLN2_I2C_MAX_XFER_SIZE 256 +#define DLN2_I2C_BUF_SIZE(DLN2_I2C_MAX_XFER_SIZE + 16) + +struct dln2_i2c { + struct platform_device *pdev; + struct i2c_adapter adapter; + u32 freq; + u32 min_freq; + u32 max_freq; + u8 port; + /* + * Buffer to hold the packet for read or write transfers. One + * is enough since we can't have multiple transfers in + * parallel on the i2c adapter. + */ + void *buf; +}; + +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable) +{ + int ret; + u16 cmd; + + if (enable) + cmd = DLN2_I2C_ENABLE; + else + cmd = DLN2_I2C_DISABLE; + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + NULL, NULL); Don't use the port member of dln2 directly here. Encode the protocol in the function as you already do for most other messages (and did in previous versions). You could even declare a struct { u8 port; } tx; for consistency. + if (ret 0) + return ret; + + return 0; +} + +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq) +{ + int ret; + struct tx_data { + u8 port; + __le32 freq; + } __packed tx; + + tx.port = dln2-port; + tx.freq = cpu_to_le32(freq); + + ret = dln2_transfer(dln2-pdev, DLN2_I2C_SET_FREQUENCY, tx, sizeof(tx), + NULL, NULL); + if (ret 0) + return ret; + + dln2-freq = freq; + + return 0; +} + +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res) +{ + int ret; + __le32 freq; + unsigned len = sizeof(freq); + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + freq, len); Same here. + if (ret 0) + return ret; + if (len sizeof(freq)) + return -EPROTO; + + *res = le32_to_cpu(freq); + + return 0; +} + [...] +static ssize_t dln2_i2c_freq_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct dln2_i2c *dln2 = dev_get_drvdata(dev); + + return snprintf(buf, PAGE_SIZE, %d\n, dln2-freq); +} + +static ssize_t dln2_i2c_freq_store(struct device *dev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct dln2_i2c *dln2 = dev_get_drvdata(dev); + unsigned long freq; + int ret; + + if (kstrtoul(buf, 0, freq) || freq dln2-min_freq || + freq dln2-max_freq) + return -EINVAL; + + ret = dln2_i2c_enable(dln2, false); + if (ret 0) + return ret; + + ret = dln2_i2c_set_frequency(dln2, freq); + if (ret 0) + return ret; + + ret = dln2_i2c_enable(dln2, true); + + return count; +} + +static DEVICE_ATTR_RW(dln2_i2c_freq); You forgot to add the required documentation of the attribute to Documentation/ABI as requested. Johan -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold jo...@kernel.org wrote: On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: From: Laurentiu Palcu laurentiu.pa...@intel.com This patch adds support for the Diolan DLN-2 I2C master module. Due to hardware limitations it does not support SMBUS quick commands. Information about the USB protocol interface can be found in the Programmer's Reference Manual [1], see section 6.2.2 for the I2C master module commands and responses. [1] https://www.diolan.com/downloads/dln-api-manual.pdf Signed-off-by: Laurentiu Palcu laurentiu.pa...@intel.com Signed-off-by: Octavian Purdila octavian.purd...@intel.com --- [...] +#define DLN2_I2C_MAX_XFER_SIZE 256 +#define DLN2_I2C_BUF_SIZE(DLN2_I2C_MAX_XFER_SIZE + 16) + +struct dln2_i2c { + struct platform_device *pdev; + struct i2c_adapter adapter; + u32 freq; + u32 min_freq; + u32 max_freq; + u8 port; + /* + * Buffer to hold the packet for read or write transfers. One + * is enough since we can't have multiple transfers in + * parallel on the i2c adapter. + */ + void *buf; +}; + +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable) +{ + int ret; + u16 cmd; + + if (enable) + cmd = DLN2_I2C_ENABLE; + else + cmd = DLN2_I2C_DISABLE; + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + NULL, NULL); Don't use the port member of dln2 directly here. Encode the protocol in the function as you already do for most other messages (and did in previous versions). You could even declare a struct { u8 port; } tx; for consistency. Yes, I think I did this after Arnd's review on the gpio stuff where I thought he suggested to remove the structure, when in fact he asked to remove the __packed attribute. I will revert it back. + if (ret 0) + return ret; + + return 0; +} + +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq) +{ + int ret; + struct tx_data { + u8 port; + __le32 freq; + } __packed tx; + + tx.port = dln2-port; + tx.freq = cpu_to_le32(freq); + + ret = dln2_transfer(dln2-pdev, DLN2_I2C_SET_FREQUENCY, tx, sizeof(tx), + NULL, NULL); + if (ret 0) + return ret; + + dln2-freq = freq; + + return 0; +} + +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res) +{ + int ret; + __le32 freq; + unsigned len = sizeof(freq); + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + freq, len); Same here. OK. + if (ret 0) + return ret; + if (len sizeof(freq)) + return -EPROTO; + + *res = le32_to_cpu(freq); + + return 0; +} + [...] +static ssize_t dln2_i2c_freq_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct dln2_i2c *dln2 = dev_get_drvdata(dev); + + return snprintf(buf, PAGE_SIZE, %d\n, dln2-freq); +} + +static ssize_t dln2_i2c_freq_store(struct device *dev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct dln2_i2c *dln2 = dev_get_drvdata(dev); + unsigned long freq; + int ret; + + if (kstrtoul(buf, 0, freq) || freq dln2-min_freq || + freq dln2-max_freq) + return -EINVAL; + + ret = dln2_i2c_enable(dln2, false); + if (ret 0) + return ret; + + ret = dln2_i2c_set_frequency(dln2, freq); + if (ret 0) + return ret; + + ret = dln2_i2c_enable(dln2, true); + + return count; +} + +static DEVICE_ATTR_RW(dln2_i2c_freq); You forgot to add the required documentation of the attribute to Documentation/ABI as requested. Hmm, I did add a new entry in Documentation/ABI/ at testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning at the patch. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
On Tue, Oct 07, 2014 at 08:52:35PM +0300, Octavian Purdila wrote: On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold jo...@kernel.org wrote: On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote: +static DEVICE_ATTR_RW(dln2_i2c_freq); You forgot to add the required documentation of the attribute to Documentation/ABI as requested. Hmm, I did add a new entry in Documentation/ABI/ at testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning at the patch. Ah, sorry, I missed that. I'll look at it tomorrow. Johan -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
Hi, diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 2ac87fa..6afc17e 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -1021,4 +1021,14 @@ config SCx200_ACB This support is also available as a module. If so, the module will be called scx200_acb. +config I2C_DLN2 + tristate Diolan DLN-2 USB I2C adapter + depends on MFD_DLN2 + help + If you say yes to this option, support will be included for Diolan + DLN2, a USB to I2C interface. + + This driver can also be built as a module. If so, the module + will be called i2c-dln2. + Please keep to the existing sorting. endmenu diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 49bf07e..3118fea 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA)+= i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_DLN2) += i2c-dln2.o Ditto! +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable) +{ + int ret; + u16 cmd; + + if (enable) + cmd = DLN2_I2C_ENABLE; + else + cmd = DLN2_I2C_DISABLE; Ternary operator? + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + NULL, NULL); + if (ret 0) + return ret; + + return 0; +} + +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq) Here you have 'set_frequency'... +{ + int ret; + struct tx_data { + u8 port; + __le32 freq; + } __packed tx; + + tx.port = dln2-port; + tx.freq = cpu_to_le32(freq); + + ret = dln2_transfer(dln2-pdev, DLN2_I2C_SET_FREQUENCY, tx, sizeof(tx), + NULL, NULL); + if (ret 0) + return ret; + + dln2-freq = freq; + + return 0; +} + +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res) ... here 'get_freq'. Please keep it consistent. +{ + int ret; + __le32 freq; + unsigned len = sizeof(freq); + + ret = dln2_transfer(dln2-pdev, cmd, dln2-port, sizeof(dln2-port), + freq, len); + if (ret 0) + return ret; + if (len sizeof(freq)) + return -EPROTO; + + *res = le32_to_cpu(freq); + + return 0; +} ... + +static int dln2_i2c_xfer(struct i2c_adapter *adapter, + struct i2c_msg *msgs, int num) +{ + struct dln2_i2c *dln2 = i2c_get_adapdata(adapter); + struct i2c_msg *pmsg; + int i; + + for (i = 0; i num; i++) { + int ret; + + pmsg = msgs[i]; + + if (pmsg-len DLN2_I2C_MAX_XFER_SIZE) + return -EINVAL; Rather -EOPNOTSUPP. And we should add a warning here since I2C transfers can be bigger than 256 byte and this is a flaw of the controller. + + if (pmsg-flags I2C_M_RD) { + ret = dln2_i2c_read(dln2, pmsg-addr, pmsg-buf, + pmsg-len); + if (ret 0) + return ret; + + pmsg-len = ret; + } else { + ret = dln2_i2c_write(dln2, pmsg-addr, pmsg-buf, + pmsg-len); + if (ret != pmsg-len) + return -EPROTO; + } + } + + return num; +} + Thanks, Wolfram signature.asc Description: Digital signature
[PATCH v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter
From: Laurentiu Palcu laurentiu.pa...@intel.com This patch adds support for the Diolan DLN-2 I2C master module. Due to hardware limitations it does not support SMBUS quick commands. Information about the USB protocol interface can be found in the Programmer's Reference Manual [1], see section 6.2.2 for the I2C master module commands and responses. [1] https://www.diolan.com/downloads/dln-api-manual.pdf Signed-off-by: Laurentiu Palcu laurentiu.pa...@intel.com Signed-off-by: Octavian Purdila octavian.purd...@intel.com --- .../ABI/testing/sysfs-bus-i2c-busses-dln2 | 5 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-dln2.c | 378 + 4 files changed, 394 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 create mode 100644 drivers/i2c/busses/i2c-dln2.c diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 new file mode 100644 index 000..ad55af6 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 @@ -0,0 +1,5 @@ +What: /sys/bus/i2c/devices/.../dln2_i2c_freq +Date: September 2014 +Contact: Octavian Purdila octavian.purd...@intel.com +Description: + This attribute shows/sets the frequency (in Hz) of the I2C bus. diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 2ac87fa..6afc17e 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -1021,4 +1021,14 @@ config SCx200_ACB This support is also available as a module. If so, the module will be called scx200_acb. +config I2C_DLN2 + tristate Diolan DLN-2 USB I2C adapter + depends on MFD_DLN2 + help + If you say yes to this option, support will be included for Diolan + DLN2, a USB to I2C interface. + + This driver can also be built as a module. If so, the module + will be called i2c-dln2. + endmenu diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 49bf07e..3118fea 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o +obj-$(CONFIG_I2C_DLN2) += i2c-dln2.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c new file mode 100644 index 000..15b7f35b --- /dev/null +++ b/drivers/i2c/busses/i2c-dln2.c @@ -0,0 +1,378 @@ +/* + * Driver for the Diolan DLN-2 USB-I2C adapter + * + * Copyright (c) 2014 Intel Corporation + * + * Derived from: + * i2c-diolan-u2c.c + * Copyright (c) 2010-2011 Ericsson AB + * + * 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, version 2. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/types.h +#include linux/slab.h +#include linux/i2c.h +#include linux/platform_device.h +#include linux/mfd/dln2.h + +#define DLN2_I2C_MODULE_ID 0x03 +#define DLN2_I2C_CMD(cmd) DLN2_CMD(cmd, DLN2_I2C_MODULE_ID) + +/* I2C commands */ +#define DLN2_I2C_GET_PORT_COUNTDLN2_I2C_CMD(0x00) +#define DLN2_I2C_ENABLEDLN2_I2C_CMD(0x01) +#define DLN2_I2C_DISABLE DLN2_I2C_CMD(0x02) +#define DLN2_I2C_IS_ENABLEDDLN2_I2C_CMD(0x03) +#define DLN2_I2C_SET_FREQUENCY DLN2_I2C_CMD(0x04) +#define DLN2_I2C_GET_FREQUENCY DLN2_I2C_CMD(0x05) +#define DLN2_I2C_WRITE DLN2_I2C_CMD(0x06) +#define DLN2_I2C_READ DLN2_I2C_CMD(0x07) +#define DLN2_I2C_SCAN_DEVICES DLN2_I2C_CMD(0x08) +#define DLN2_I2C_PULLUP_ENABLE DLN2_I2C_CMD(0x09) +#define DLN2_I2C_PULLUP_DISABLEDLN2_I2C_CMD(0x0A) +#define DLN2_I2C_PULLUP_IS_ENABLED DLN2_I2C_CMD(0x0B) +#define DLN2_I2C_TRANSFER DLN2_I2C_CMD(0x0C) +#define DLN2_I2C_SET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0D) +#define DLN2_I2C_GET_MAX_REPLY_COUNT DLN2_I2C_CMD(0x0E) +#define DLN2_I2C_GET_MIN_FREQUENCY DLN2_I2C_CMD(0x40) +#define DLN2_I2C_GET_MAX_FREQUENCY DLN2_I2C_CMD(0x41) + +#define DLN2_I2C_FREQ_STD 10 + +#define DLN2_I2C_MAX_XFER_SIZE 256 +#define DLN2_I2C_BUF_SIZE (DLN2_I2C_MAX_XFER_SIZE + 16) + +struct dln2_i2c { + struct platform_device *pdev; + struct i2c_adapter adapter; + u32 freq; + u32 min_freq; + u32 max_freq; + u8 port; + /* +* Buffer to hold the packet for read or write transfers. One +* is enough since we can't have multiple transfers in +* parallel on