[PATCH v4 0/3] mfd: add support for Diolan DLN-2

[PATCH v4 0/3] mfd: add support for Diolan DLN-2

[Octavian Purdila]

This patch series adds support for Diolan USB-I2C/GPIO Master Adapter
DLN-2. Details about device can be found here:

https://www.diolan.com/i2c/i2c_interface.html.

Changes since v3:

 * Mostly addressing latest review comments from Johan

 * MFD driver: more message size checks for responses, switched to
   using unsigned for buffer sizes and use const void * for output
   buffer for the dln2_transfer() API, more codying style cleanups,
   renamed DLN2_HANDLE_GPIO_EVENT to DLN2_HANDLE_EVENT

 * MFD driver: turns out the RX buf size was too low for certain I2C
   drivers (I was seeing request up to 257 bytes), so increase it from
   64 to 512

 * GPIO driver: revert the retry chip remove change, as there are
   still issues if the gpio if exported to sysfs; coding style
   cleanups, correctly clamp no pin numbers to 32, use devm_kzalloc
   and simplify the error path in probe, cache pin direction to save
   one transfer when reading the pin value

 * I2C driver: add an i2c port parameter in the platform data
   (although only one port seems to be supported), make frequency a
   sysfs attribute instead of a module paramter, coding style
   cleanups, use devm_kzalloc and simplify the error path in probe,
   move the I2C buffers from the stack to the dln2_i2c structure (it
   is enough as we can have only one transfer at a time on the i2c
   adapter)

Changes since v2:

 * MFD driver: fix a few obsolete comments for the DLN2 I/O API

 * GPIO driver: retry the chip remove call if -EBUSY is returned, see
   the comments in dln2_do_remove for more details; also removed a
   redundant !dln2->pdev check from dln2_irq_event - we do it in
   dln2_transfer already

 * I2C driver: add I2C_FUNC_SMBUS_I2C_BLOCK support

Changes since v1:

 * rewrite the drivers as an MFD

 * rewrite the irq part of the gpio driver to use GPIOLIB_IRQCHIP

 * cleanup the I/O interface

 * various fixes and cleanps: check received message sizes before
   parsing, error handling for usb_submit_urb, check URB status, use
   single bit manipulation functions instead of bitmap_*, move
   GFP_KERNEL URB submit away from under lock


Daniel Baluta (1):
  gpio: add support for the Diolan DLN-2 USB GPIO driver

Laurentiu Palcu (1):
  i2c: add support for Diolan DLN-2 USB-I2C adapter

Octavian Purdila (1):
  mfd: add support for Diolan DLN-2 devices

 drivers/gpio/Kconfig          |  12 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-dln2.c      | 529 ++++++++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig    |  10 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-dln2.c | 390 ++++++++++++++++++++++++
 drivers/mfd/Kconfig           |   9 +
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/dln2.c            | 681 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/dln2.h      |  71 +++++
 10 files changed, 1705 insertions(+)
 create mode 100644 drivers/gpio/gpio-dln2.c
 create mode 100644 drivers/i2c/busses/i2c-dln2.c
 create mode 100644 drivers/mfd/dln2.c
 create mode 100644 include/linux/mfd/dln2.h

-- 
1.9.1

[PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Octavian Purdila]

This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
Master Adapter DLN-2. Details about the device can be found here:

https://www.diolan.com/i2c/i2c_interface.html.

Information about the USB protocol can be found in the Programmer's
Reference Manual [1], see section 1.7.

Because the hardware has a single transmit endpoint and a single
receive endpoint the communication between the various DLN2 drivers
and the hardware will be muxed/demuxed by this driver.

Each DLN2 module will be identified by the handle field within the DLN2
message header. If a DLN2 module issues multiple commands in parallel
they will be identified by the echo counter field in the message header.

The DLN2 modules can use the dln2_transfer() function to issue a
command and wait for its response. They can also register a callback
that is going to be called when a specific event id is generated by
the device (e.g. GPIO interrupts). The device uses handle 0 for
sending events.

[1] https://www.diolan.com/downloads/dln-api-manual.pdf

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/mfd/Kconfig      |   9 +
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/dln2.h |  71 +++++
 4 files changed, 762 insertions(+)
 create mode 100644 drivers/mfd/dln2.c
 create mode 100644 include/linux/mfd/dln2.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index de5abf2..7bcf895 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -183,6 +183,15 @@ config MFD_DA9063
 	  Additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_DLN2
+	tristate "Diolan DLN2 support"
+	select MFD_CORE
+	depends on USB
+	help
+	  This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
+	  Additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 config MFD_MC13XXX
 	tristate
 	depends on (SPI_MASTER || I2C)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f001487..591988d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
 obj-$(CONFIG_MFD_AS3722)	+= as3722.o
 obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
 obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
+obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
new file mode 100644
index 0000000..b551b5e
--- /dev/null
+++ b/drivers/mfd/dln2.c
@@ -0,0 +1,681 @@
+/*
+ * Driver for the Diolan DLN-2 USB 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/errno.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/dln2.h>
+
+#define DRIVER_NAME			"usb-dln2"
+
+struct dln2_header {
+	__le16 size;
+	__le16 id;
+	__le16 echo;
+	__le16 handle;
+} __packed;
+
+struct dln2_response {
+	struct dln2_header hdr;
+	__le16 result;
+} __packed;
+
+#define DLN2_GENERIC_MODULE_ID		0x00
+#define DLN2_GENERIC_CMD(cmd)		DLN2_CMD(cmd, DLN2_GENERIC_MODULE_ID)
+#define CMD_GET_DEVICE_VER		DLN2_GENERIC_CMD(0x30)
+#define CMD_GET_DEVICE_SN		DLN2_GENERIC_CMD(0x31)
+
+#define DLN2_HW_ID			0x200
+#define DLN2_USB_TIMEOUT		200	/* in ms */
+#define DLN2_MAX_RX_SLOTS		16
+#define DLN2_MAX_MODULES		5
+#define DLN2_MAX_URBS			16
+#define DLN2_RX_BUF_SIZE		512
+
+#define DLN2_HANDLE_EVENT		0
+#define DLN2_HANDLE_CTRL		1
+#define DLN2_HANDLE_GPIO		2
+#define DLN2_HANDLE_I2C			3
+
+/*
+ * Receive context used between the receive demultiplexer and the
+ * transfer routine. While sending a request the transfer routine
+ * will look for a free receive context and use it to wait for a
+ * response and to receive the URB and thus the response data.
+ */
+struct dln2_rx_context {
+	struct completion done;
+	struct urb *urb;
+	bool connected;
+};
+
+/*
+ * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
+ * use the handle header field to indentify the module in
+ * dln2_dev.mod_rx_slots and then the echo header field to index the
+ * slots field and find the receive context for a particular
+ * request.
+ */
+struct dln2_mod_rx_slots {
+	/* RX slots bitmap */
+	unsigned long bmap;
+
+	/* used to wait for a free RX slot */
+	wait_queue_head_t wq;
+
+	/* used to wait for an RX operation to complete */
+	struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
+
+	/* avoid races between free_rx_slot and dln2_rx_transfer */
+	spinlock_t lock;
+};
+
+struct dln2_dev {
+	struct usb_device *usb_dev;
+	struct usb_interface *interface;
+	u8 ep_in;
+	u8 ep_out;
+
+	struct urb *rx_urb[DLN2_MAX_URBS];
+	void *rx_buf[DLN2_MAX_URBS];
+
+	struct dln2_mod_rx_slots mod_rx_slots[DLN2_MAX_MODULES];
+
+	struct list_head rx_cb_list;
+	spinlock_t rx_cb_lock;
+};
+
+static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&rxs->lock, flags);
+
+	*slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
+
+	if (*slot < DLN2_MAX_RX_SLOTS) {
+		struct dln2_rx_context *rxc = &rxs->slots[*slot];
+
+		set_bit(*slot, &rxs->bmap);
+		rxc->connected = true;
+	}
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	return *slot < DLN2_MAX_RX_SLOTS;
+}
+
+static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
+{
+	int ret;
+	int slot;
+
+	/* No need to timeout here, the wait is bounded by the timeout
+	 * in _dln2_transfer
+	 */
+	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
+	if (ret < 0)
+		return ret;
+
+	return slot;
+}
+
+static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots *rxs,
+			 int slot)
+{
+	struct urb *urb = NULL;
+	unsigned long flags;
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+	int ret;
+
+	spin_lock_irqsave(&rxs->lock, flags);
+
+	clear_bit(slot, &rxs->bmap);
+
+	rxc = &rxs->slots[slot];
+	rxc->connected = false;
+	urb = rxc->urb;
+	rxc->urb = NULL;
+	reinit_completion(&rxc->done);
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	if (urb)  {
+		ret = usb_submit_urb(urb, GFP_KERNEL);
+		if (ret < 0)
+			dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
+	}
+
+	wake_up_interruptible(&rxs->wq);
+}
+
+struct dln2_rx_cb_entry {
+	struct list_head list;
+	u16 id;
+	struct platform_device *pdev;
+	dln2_rx_cb_t callback;
+};
+
+int dln2_register_event_cb(struct platform_device *pdev, u16 id,
+			   dln2_rx_cb_t rx_cb)
+{
+	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
+	struct dln2_rx_cb_entry *i, *new;
+	unsigned long flags;
+	int ret = 0;
+
+	new = kmalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	new->id = id;
+	new->callback = rx_cb;
+	new->pdev = pdev;
+
+	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
+
+	list_for_each_entry(i, &dln2->rx_cb_list, list) {
+		if (i->id == id) {
+			ret = -EBUSY;
+			break;
+		}
+	}
+
+	if (!ret)
+		list_add(&new->list, &dln2->rx_cb_list);
+
+	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
+
+	if (ret)
+		kfree(new);
+
+	return ret;
+}
+EXPORT_SYMBOL(dln2_register_event_cb);
+
+void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
+{
+	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
+	struct dln2_rx_cb_entry *i, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
+
+	list_for_each_entry_safe(i, tmp, &dln2->rx_cb_list, list) {
+		if (i->id == id) {
+			list_del(&i->list);
+			kfree(i);
+		}
+	}
+
+	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
+}
+EXPORT_SYMBOL(dln2_unregister_event_cb);
+
+static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
+			     u16 handle, u16 rx_slot)
+{
+	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+	int err;
+
+	spin_lock(&rxs->lock);
+	rxc = &rxs->slots[rx_slot];
+	if (rxc->connected) {
+		rxc->urb = urb;
+		complete(&rxc->done);
+	} else {
+		dev_warn(dev, "dropping response %d/%d", handle, rx_slot);
+		err = usb_submit_urb(urb, GFP_ATOMIC);
+		if (err < 0)
+			dev_err(dev, "failed to re-submit RX URB: %d\n", err);
+	}
+	spin_unlock(&rxs->lock);
+}
+
+static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
+				  void *data, int len)
+{
+	struct dln2_rx_cb_entry *i;
+
+	spin_lock(&dln2->rx_cb_lock);
+
+	list_for_each_entry(i, &dln2->rx_cb_list, list)
+		if (i->id == id)
+			i->callback(i->pdev, echo, data, len);
+
+	spin_unlock(&dln2->rx_cb_lock);
+}
+
+static void dln2_rx(struct urb *urb)
+{
+	int err;
+	struct dln2_dev *dln2 = urb->context;
+	struct dln2_header *hdr = urb->transfer_buffer;
+	struct device *dev = &dln2->interface->dev;
+	u16 id, echo, handle, size;
+	u8 *data;
+	int len;
+
+	switch (urb->status) {
+	case 0:
+		/* success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+	case -EPIPE:
+		/* this urb is terminated, clean up */
+		dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
+		return;
+	default:
+		dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
+		goto out;
+	}
+
+	if (urb->actual_length < sizeof(struct dln2_header)) {
+		dev_err(dev, "short response: %d\n", urb->actual_length);
+		goto out;
+	}
+
+	handle = le16_to_cpu(hdr->handle);
+	id = le16_to_cpu(hdr->id);
+	echo = le16_to_cpu(hdr->echo);
+	size = le16_to_cpu(hdr->size);
+
+	if (size != urb->actual_length) {
+		dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
+			handle, id, echo, size, urb->actual_length);
+		goto out;
+	}
+
+	if (handle >= DLN2_MAX_MODULES) {
+		dev_warn(dev, "RX: invalid handle %d\n", handle);
+		goto out;
+	}
+
+	data = urb->transfer_buffer + sizeof(struct dln2_header);
+	len = urb->actual_length - sizeof(struct dln2_header);
+
+	if (handle == DLN2_HANDLE_EVENT) {
+		dln2_run_rx_callbacks(dln2, id, echo, data, len);
+		err = usb_submit_urb(urb, GFP_ATOMIC);
+		if (err < 0)
+			goto out_submit_failed;
+	} else {
+		dln2_rx_transfer(dln2, urb, handle, echo);
+	}
+
+	return;
+
+out:
+	err = usb_submit_urb(urb, GFP_ATOMIC);
+out_submit_failed:
+	if (err < 0)
+		dev_err(dev, "failed to re-submit RX URB: %d\n", err);
+}
+
+static void *dln2_prep_buf(u16 handle, u16 cmd, u16 echo, const void *obuf,
+			   int *obuf_len, gfp_t gfp)
+{
+	int len;
+	void *buf;
+	struct dln2_header *hdr;
+
+	len = *obuf_len + sizeof(*hdr);
+	buf = kmalloc(len, gfp);
+	if (!buf)
+		return NULL;
+
+	hdr = (struct dln2_header *)buf;
+	hdr->id = cpu_to_le16(cmd);
+	hdr->size = cpu_to_le16(len);
+	hdr->echo = cpu_to_le16(echo);
+	hdr->handle = cpu_to_le16(handle);
+
+	memcpy(buf + sizeof(*hdr), obuf, *obuf_len);
+
+	*obuf_len = len;
+
+	return buf;
+}
+
+static int dln2_send_wait(struct dln2_dev *dln2, u16 handle, u16 cmd, u16 echo,
+			  const void *obuf, int obuf_len)
+{
+	int ret = 0;
+	int len = obuf_len;
+	void *buf;
+	int actual;
+
+	buf = dln2_prep_buf(handle, cmd, echo, obuf, &len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = usb_bulk_msg(dln2->usb_dev,
+			   usb_sndbulkpipe(dln2->usb_dev, dln2->ep_out),
+			   buf, len, &actual, DLN2_USB_TIMEOUT);
+
+	kfree(buf);
+
+	return ret;
+}
+
+static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
+			  const void *obuf, unsigned obuf_len,
+			  void *ibuf, unsigned *ibuf_len)
+{
+	int ret = 0;
+	u16 result;
+	int rx_slot;
+	struct dln2_response *rsp;
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
+	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
+
+	rx_slot = alloc_rx_slot(rxs);
+	if (rx_slot < 0)
+		return rx_slot;
+
+	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
+	if (ret < 0) {
+		free_rx_slot(dln2, rxs, rx_slot);
+		dev_err(dev, "USB write failed: %d", ret);
+		return ret;
+	}
+
+	rxc = &rxs->slots[rx_slot];
+
+	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
+	if (ret <= 0) {
+		if (!ret)
+			ret = -ETIMEDOUT;
+		goto out_free_rx_slot;
+	}
+
+	/* if we got here we know that the response header has been checked */
+	rsp = rxc->urb->transfer_buffer;
+	if (rsp->hdr.size < sizeof(*rsp)) {
+		ret = -EPROTO;
+		goto out_free_rx_slot;
+	}
+
+	result = le16_to_cpu(rsp->result);
+	if (result) {
+		dev_dbg(dev, "%d received response with error %d\n",
+			handle, result);
+		ret = -EREMOTEIO;
+		goto out_free_rx_slot;
+	}
+
+	if (!ibuf) {
+		ret = 0;
+		goto out_free_rx_slot;
+	}
+
+	if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
+		*ibuf_len = rsp->hdr.size - sizeof(*rsp);
+
+	memcpy(ibuf, rsp + 1, *ibuf_len);
+
+out_free_rx_slot:
+	free_rx_slot(dln2, rxs, rx_slot);
+
+	return ret;
+}
+
+int dln2_transfer(struct platform_device *pdev, u16 cmd,
+		  const void *obuf, unsigned obuf_len,
+		  void *ibuf, unsigned *ibuf_len)
+{
+	struct dln2_platform_data *dln2_pdata;
+	struct dln2_dev *dln2;
+	u16 h;
+
+	/* USB device has been disconnected, bail out */
+	if (!pdev)
+		return -ENODEV;
+
+	dln2 = dev_get_drvdata(pdev->dev.parent);
+	dln2_pdata = dev_get_platdata(&pdev->dev);
+	h = dln2_pdata->handle;
+
+	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
+}
+EXPORT_SYMBOL(dln2_transfer);
+
+static int dln2_check_hw(struct dln2_dev *dln2)
+{
+	int ret;
+	__le32 hw_type;
+	int len = sizeof(hw_type);
+
+	ret = _dln2_transfer(dln2, DLN2_HANDLE_CTRL, CMD_GET_DEVICE_VER,
+			     NULL, 0, &hw_type, &len);
+	if (ret < 0)
+		return ret;
+	if (len < sizeof(hw_type))
+		return -EREMOTEIO;
+
+	if (le32_to_cpu(hw_type) != DLN2_HW_ID) {
+		dev_err(&dln2->interface->dev, "Device ID 0x%x not supported",
+			le32_to_cpu(hw_type));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int dln2_print_serialno(struct dln2_dev *dln2)
+{
+	int ret;
+	__le32 serial_no;
+	int len = sizeof(serial_no);
+	struct device *dev = &dln2->interface->dev;
+
+	ret = _dln2_transfer(dln2, DLN2_HANDLE_CTRL, CMD_GET_DEVICE_SN, NULL, 0,
+			     &serial_no, &len);
+	if (ret < 0)
+		return ret;
+	if (len < sizeof(serial_no))
+		return -EREMOTEIO;
+
+	dev_info(dev, "Diolan DLN2 serial 0x%x\n", le32_to_cpu(serial_no));
+
+	return 0;
+}
+
+static int dln2_hw_init(struct dln2_dev *dln2)
+{
+	int ret;
+
+	ret = dln2_check_hw(dln2);
+	if (ret < 0)
+		return ret;
+
+	return dln2_print_serialno(dln2);
+}
+
+static void dln2_free_rx_urbs(struct dln2_dev *dln2)
+{
+	int i;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		usb_kill_urb(dln2->rx_urb[i]);
+		usb_free_urb(dln2->rx_urb[i]);
+		kfree(dln2->rx_buf[i]);
+	}
+}
+
+static void dln2_free(struct dln2_dev *dln2)
+{
+	dln2_free_rx_urbs(dln2);
+	usb_put_dev(dln2->usb_dev);
+	kfree(dln2);
+}
+
+static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
+			      struct usb_host_interface *hostif)
+{
+	int i;
+	const int rx_max_size = DLN2_RX_BUF_SIZE;
+	struct device *dev = &dln2->interface->dev;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		int ret;
+
+		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
+		if (!dln2->rx_buf[i])
+			return -ENOMEM;
+
+		dln2->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!dln2->rx_urb[i])
+			return -ENOMEM;
+
+		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
+				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
+				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
+
+		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
+		if (ret < 0) {
+			dev_err(dev, "failed to submit RX URB: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static struct dln2_platform_data dln2_pdata_gpio = {
+	.handle = DLN2_HANDLE_GPIO,
+};
+
+/* Right now only one I2C port seems to be supported */
+static struct dln2_platform_data dln2_pdata_i2c = {
+	.handle = DLN2_HANDLE_I2C,
+	.i2c = {
+		.port = 0,
+	},
+};
+
+static const struct mfd_cell dln2_devs[] = {
+	{
+		.name	= "dln2-gpio",
+		.platform_data = &dln2_pdata_gpio,
+		.pdata_size = sizeof(struct dln2_platform_data),
+	},
+	{
+		.name	= "dln2-i2c",
+		.platform_data = &dln2_pdata_i2c,
+		.pdata_size = sizeof(struct dln2_platform_data),
+	},
+};
+
+static void dln2_disconnect(struct usb_interface *interface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(interface);
+
+	mfd_remove_devices(&interface->dev);
+	dln2_free(dln2);
+}
+
+static int dln2_probe(struct usb_interface *interface,
+		      const struct usb_device_id *id)
+{
+	struct usb_host_interface *hostif = interface->cur_altsetting;
+	struct device *dev = &interface->dev;
+	struct dln2_dev *dln2;
+	int ret;
+	int i, j;
+
+	if (hostif->desc.bInterfaceNumber != 0 ||
+	    hostif->desc.bNumEndpoints < 2)
+		return -ENODEV;
+
+	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
+	if (!dln2)
+		return -ENOMEM;
+
+	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
+	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
+	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
+	dln2->interface = interface;
+	usb_set_intfdata(interface, dln2);
+
+	for (i = 0; i < DLN2_MAX_MODULES; i++) {
+		init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
+		spin_lock_init(&dln2->mod_rx_slots[i].lock);
+		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++)
+			init_completion(&dln2->mod_rx_slots[i].slots[j].done);
+	}
+
+	spin_lock_init(&dln2->rx_cb_lock);
+	INIT_LIST_HEAD(&dln2->rx_cb_list);
+
+	ret = dln2_setup_rx_urbs(dln2, hostif);
+	if (ret) {
+		dln2_free(dln2);
+		return ret;
+	}
+
+	ret = dln2_hw_init(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize hardware\n");
+		goto out_cleanup;
+	}
+
+	ret = mfd_add_devices(dev, -1, dln2_devs, ARRAY_SIZE(dln2_devs),
+			      NULL, 0, NULL);
+	if (ret != 0) {
+		dev_err(dev, "Failed to add mfd devices to core.\n");
+		goto out_cleanup;
+	}
+
+	return 0;
+
+out_cleanup:
+	dln2_free(dln2);
+
+	return ret;
+}
+
+static const struct usb_device_id dln2_table[] = {
+	{ USB_DEVICE(0xa257, 0x2013) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(usb, dln2_table);
+
+static struct usb_driver dln2_driver = {
+	.name = DRIVER_NAME,
+	.probe = dln2_probe,
+	.disconnect = dln2_disconnect,
+	.id_table = dln2_table,
+};
+
+module_usb_driver(dln2_driver);
+
+MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
new file mode 100644
index 0000000..197565d
--- /dev/null
+++ b/include/linux/mfd/dln2.h
@@ -0,0 +1,71 @@
+#ifndef __LINUX_USB_DLN2_H
+#define __LINUX_USB_DLN2_H
+
+#define DLN2_CMD(cmd, id)		((cmd) | ((id) << 8))
+
+struct dln2_platform_data {
+	u16 handle;
+	union {
+		struct {
+			u8 port;
+		} i2c;
+	};
+};
+
+/**
+ * dln2_rx_cb - event callback function signature
+ *
+ * @pdev - the sub-device that registered this callback
+ * @echo - the echo header field received in the message
+ * @data - the data payload
+ * @len  - the data payload length
+ *
+ * The callback function is called in interrupt context and the data
+ * payload is only valid during the call. If the user needs later
+ * access of the data, it must copy it.
+ */
+
+typedef void (*dln2_rx_cb_t)(struct platform_device *pdev, u16 echo,
+			     const void *data, int len);
+
+/**
+ * dl2n_register_event_cb - register a callback function for an event
+ *
+ * @pdev - the sub-device that registers the callback
+ * @event - the event for which to register a callback
+ * @rx_cb - the callback function
+ *
+ * @return 0 in case of success, negative value in case of error
+ */
+int dln2_register_event_cb(struct platform_device *pdev, u16 event,
+			   dln2_rx_cb_t rx_cb);
+
+/**
+ * dln2_unregister_event_cb - unregister the callback function for an event
+ *
+ * @pdev - the sub-device that registered the callback
+ * @event - the event for which to register a callback
+ */
+void dln2_unregister_event_cb(struct platform_device *pdev, u16 event);
+
+/**
+ * dln2_transfer - issue a DLN2 command and wait for a response and
+ * the associated data
+ *
+ * @pdev - the sub-device which is issuing this transfer
+ * @cmd - the command to be sent to the device
+ * @obuf - the buffer to be sent to the device; can be NULL if the
+ * user doesn't need to transmit data with this command
+ * @obuf_len - the size of the buffer to be sent to the device
+ * @ibuf - any data associated with the response will be copied here;
+ * it can be NULL if the user doesn't need the response data
+ * @ibuf_len - must be initialized to the input buffer size; it will
+ * be modified to indicate the actual data transfered
+ *
+ * @returns 0 for success, negative value for errors
+ */
+int dln2_transfer(struct platform_device *pdev, u16 cmd,
+		  const void *obuf, unsigned obuf_len,
+		  void *ibuf, unsigned *ibuf_len);
+
+#endif
-- 
1.9.1

[PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Octavian Purdila]

From: Laurentiu Palcu <laurentiu.palcu@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.palcu@intel.com>
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/i2c/busses/Kconfig    |  10 ++
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-dln2.c | 390 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 401 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-dln2.c

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 0000000..ac09ec4
--- /dev/null
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -0,0 +1,390 @@
+/*
+ * 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/errno.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 DRIVER_NAME			"dln2-i2c"
+
+#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_COUNT		DLN2_I2C_CMD(0x00)
+#define DLN2_I2C_ENABLE			DLN2_I2C_CMD(0x01)
+#define DLN2_I2C_DISABLE		DLN2_I2C_CMD(0x02)
+#define DLN2_I2C_IS_ENABLED		DLN2_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_DISABLE		DLN2_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		100000
+
+#define DLN2_I2C_MAX_XFER_SIZE		256
+
+struct dln2_i2c {
+	struct platform_device *pdev;
+	struct i2c_adapter adapter;
+	uint32_t freq;
+	uint32_t min_freq;
+	uint32_t max_freq;
+	/*
+	 * 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.
+	 */
+	union {
+		struct {
+			u8 port;
+			u8 addr;
+			u8 mem_addr_len;
+			__le32 mem_addr;
+			__le16 buf_len;
+			u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+		} __packed tx;
+		struct {
+			__le16 buf_len;
+			u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+		} __packed rx;
+	} buf;
+};
+
+static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
+{
+	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
+	int ret;
+	u8 port;
+	u16 cmd;
+
+	port = pdata->i2c.port;
+
+	if (enable)
+		cmd = DLN2_I2C_ENABLE;
+	else
+		cmd = DLN2_I2C_DISABLE;
+
+	ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(port), NULL, NULL);
+	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;
+	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
+
+	tx.port = pdata->i2c.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, uint32_t *res)
+{
+	int ret;
+	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
+	u8 port = pdata->i2c.port;
+	__le32 freq;
+	unsigned len = sizeof(freq);
+
+	ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(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_setup(struct dln2_i2c *dln2)
+{
+	int ret;
+
+	ret = dln2_i2c_enable(dln2, false);
+	if (ret < 0)
+		return ret;
+
+	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MIN_FREQUENCY,
+				&dln2->min_freq);
+	if (ret < 0)
+		return 0;
+
+	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MAX_FREQUENCY,
+				&dln2->max_freq);
+	if (ret < 0)
+		return 0;
+
+	ret = dln2_i2c_set_frequency(dln2, DLN2_I2C_FREQ_STD);
+	if (ret < 0)
+		return ret;
+
+	ret = dln2_i2c_enable(dln2, true);
+
+	return ret;
+}
+
+static int dln2_i2c_write(struct dln2_i2c *dln2, u8 addr,
+			  u8 *data, u16 data_len)
+{
+	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
+	int ret;
+	unsigned len;
+
+	dln2->buf.tx.port = pdata->i2c.port;
+	dln2->buf.tx.addr = addr;
+	dln2->buf.tx.mem_addr_len = 0;
+	dln2->buf.tx.mem_addr = 0;
+	dln2->buf.tx.buf_len = cpu_to_le16(data_len);
+	memcpy(dln2->buf.tx.buf, data, data_len);
+
+	len = sizeof(dln2->buf.tx) + data_len - DLN2_I2C_MAX_XFER_SIZE;
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_WRITE, &dln2->buf.tx, len,
+			    NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	return data_len;
+}
+
+static int dln2_i2c_read(struct dln2_i2c *dln2, u16 addr, u8 *data,
+			 u16 data_len)
+{
+	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
+	int ret;
+	struct {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+	} __packed tx;
+	unsigned rx_len, rx_buf_len;
+
+	tx.port = pdata->i2c.port;
+	tx.addr = addr;
+	tx.mem_addr_len = 0;
+	tx.mem_addr = 0;
+	tx.buf_len = cpu_to_le16(data_len);
+
+	rx_len = sizeof(dln2->buf.rx);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_READ, &tx, sizeof(tx),
+			    &dln2->buf.rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < 2)
+		return -EPROTO;
+
+	rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
+	if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
+		return -EPROTO;
+
+	if (data_len > rx_buf_len)
+		data_len = rx_buf_len;
+
+	memcpy(data, dln2->buf.rx.buf, data_len);
+
+	return data_len;
+}
+
+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;
+
+		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;
+}
+
+static u32 dln2_i2c_func(struct i2c_adapter *a)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
+		I2C_FUNC_SMBUS_I2C_BLOCK;
+}
+
+static const struct i2c_algorithm dln2_i2c_usb_algorithm = {
+	.master_xfer = dln2_i2c_xfer,
+	.functionality = dln2_i2c_func,
+};
+
+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(freq, S_IRUGO|S_IWUSR, dln2_i2c_freq_show,
+		   dln2_i2c_freq_store);
+
+static int dln2_i2c_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct dln2_i2c *dln2;
+	struct device *dev = &pdev->dev;
+	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
+
+	dln2 = devm_kzalloc(dev, sizeof(*dln2), GFP_KERNEL);
+	if (!dln2)
+		return -ENOMEM;
+
+	dln2->pdev = pdev;
+
+	/* setup i2c adapter description */
+	dln2->adapter.owner = THIS_MODULE;
+	dln2->adapter.class = I2C_CLASS_HWMON;
+	dln2->adapter.algo = &dln2_i2c_usb_algorithm;
+	dln2->adapter.dev.parent = dev;
+	i2c_set_adapdata(&dln2->adapter, dln2);
+	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), "%s-%s-%d",
+		 DRIVER_NAME, dev_name(pdev->dev.parent), pdata->i2c.port);
+
+	platform_set_drvdata(pdev, dln2);
+
+	ret = device_create_file(dev, &dev_attr_freq);
+	if (ret < 0) {
+		dev_err(dev, "failed to add freq attribute\n");
+		return ret;
+	}
+
+	/* initialize the i2c interface */
+	ret = dln2_i2c_setup(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize adapter: %d\n", ret);
+		goto out_remove_freq_attr;
+	}
+
+	/* and finally attach to i2c layer */
+	ret = i2c_add_adapter(&dln2->adapter);
+	if (ret < 0) {
+		dev_err(dev, "failed to add I2C adapter: %d\n", ret);
+		goto out_disable;
+	}
+
+	return 0;
+
+out_disable:
+	dln2_i2c_enable(dln2, false);
+out_remove_freq_attr:
+	device_remove_file(dev, &dev_attr_freq);
+
+	return ret;
+}
+
+static int dln2_i2c_remove(struct platform_device *pdev)
+{
+	struct dln2_i2c *dln2 = platform_get_drvdata(pdev);
+
+	device_remove_file(&pdev->dev, &dev_attr_freq);
+	i2c_del_adapter(&dln2->adapter);
+	dln2_i2c_enable(dln2, false);
+
+	return 0;
+}
+
+static struct platform_driver dln2_i2c_driver = {
+	.driver.name	= DRIVER_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= dln2_i2c_probe,
+	.remove		= dln2_i2c_remove,
+};
+
+module_platform_driver(dln2_i2c_driver);
+
+MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Octavian Purdila]

From: Daniel Baluta <daniel.baluta@intel.com>

This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module.

Information about the USB protocol interface can be found in the
Programmer's Reference Manual [1], see section 2.9 for the GPIO
module commands and responses.

[1] https://www.diolan.com/downloads/dln-api-manual.pdf

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/Kconfig     |  12 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-dln2.c | 529 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 542 insertions(+)
 create mode 100644 drivers/gpio/gpio-dln2.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..44ec206 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -897,4 +897,16 @@ config GPIO_VIPERBOARD
           River Tech's viperboard.h for detailed meaning
           of the module parameters.
 
+config GPIO_DLN2
+	tristate "Diolan DLN2 GPIO support"
+	depends on MFD_DLN2
+	select GPIOLIB_IRQCHIP
+
+	help
+	  Select this option to enable GPIO driver for the Diolan DLN2
+	  board.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called gpio-dln2.
+
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..eaa97a0 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
+obj-$(CONFIG_GPIO_DLN2)		+= gpio-dln2.o
 obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
new file mode 100644
index 0000000..23ed2ed
--- /dev/null
+++ b/drivers/gpio/gpio-dln2.c
@@ -0,0 +1,529 @@
+/*
+ * Driver for the Diolan DLN-2 USB-GPIO adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * 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/errno.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+
+#define DRIVER_NAME "dln2-gpio"
+
+#define DLN2_GPIO_ID			0x01
+
+#define DLN2_GPIO_GET_PORT_COUNT	DLN2_CMD(0x00, DLN2_GPIO_ID)
+#define DLN2_GPIO_GET_PIN_COUNT		DLN2_CMD(0x01, DLN2_GPIO_ID)
+#define DLN2_GPIO_SET_DEBOUNCE		DLN2_CMD(0x04, DLN2_GPIO_ID)
+#define DLN2_GPIO_GET_DEBOUNCE		DLN2_CMD(0x05, DLN2_GPIO_ID)
+#define DLN2_GPIO_PORT_GET_VAL		DLN2_CMD(0x06, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_VAL		DLN2_CMD(0x0B, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_OUT_VAL	DLN2_CMD(0x0C, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_OUT_VAL	DLN2_CMD(0x0D, DLN2_GPIO_ID)
+#define DLN2_GPIO_CONDITION_MET_EV	DLN2_CMD(0x0F, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_ENABLE		DLN2_CMD(0x10, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_DISABLE		DLN2_CMD(0x11, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_DIRECTION	DLN2_CMD(0x13, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_DIRECTION	DLN2_CMD(0x14, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_EVENT_CFG	DLN2_CMD(0x1E, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_EVENT_CFG	DLN2_CMD(0x1F, DLN2_GPIO_ID)
+
+#define DLN2_GPIO_EVENT_NONE		0
+#define DLN2_GPIO_EVENT_CHANGE		1
+#define DLN2_GPIO_EVENT_LVL_HIGH	2
+#define DLN2_GPIO_EVENT_LVL_LOW		3
+#define DLN2_GPIO_EVENT_CHANGE_RISING	0x11
+#define DLN2_GPIO_EVENT_CHANGE_FALLING  0x21
+#define DLN2_GPIO_EVENT_MASK		0x0F
+
+#define DLN2_GPIO_MAX_PINS 32
+
+struct dln2_irq_work {
+	struct work_struct work;
+	struct dln2_gpio *dln2;
+	int pin;
+	int type;
+};
+
+struct dln2_gpio {
+	struct platform_device *pdev;
+	struct gpio_chip gpio;
+
+	/*
+	 * Cache pin direction to save us one transfer, since the
+	 * hardware has separate commands to read the in and out
+	 * values.
+	 */
+	DECLARE_BITMAP(pin_dir_set, DLN2_GPIO_MAX_PINS);
+	DECLARE_BITMAP(pin_dir, DLN2_GPIO_MAX_PINS);
+
+	DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
+	DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
+	DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
+	struct dln2_irq_work irq_work[DLN2_GPIO_MAX_PINS];
+};
+
+struct dln2_gpio_pin {
+	__le16 pin;
+} __packed;
+
+struct dln2_gpio_pin_val {
+	__le16 pin;
+	u8 value;
+} __packed;
+
+static int dln2_gpio_get_pin_count(struct platform_device *pdev)
+{
+	int ret;
+	__le16 count;
+	int len = sizeof(count);
+
+	ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
+			    &len);
+	if (ret < 0)
+		return ret;
+	if (len < sizeof(count))
+		return -EPROTO;
+
+	return le16_to_cpu(count);
+}
+
+static int dln2_gpio_pin_cmd(struct dln2_gpio *dln2, int cmd, unsigned pin)
+{
+	struct dln2_gpio_pin req = {
+		.pin = cpu_to_le16(pin),
+	};
+
+	return dln2_transfer(dln2->pdev, cmd, &req, sizeof(req), NULL, NULL);
+}
+
+static int dln2_gpio_pin_val(struct dln2_gpio *dln2, int cmd, unsigned int pin)
+{
+	int ret;
+	struct dln2_gpio_pin req = {
+		.pin = cpu_to_le16(pin),
+	};
+	struct dln2_gpio_pin_val rsp;
+	int len = sizeof(rsp);
+
+	ret = dln2_transfer(dln2->pdev, cmd, &req, sizeof(req), &rsp, &len);
+	if (ret < 0)
+		return ret;
+	if (len < sizeof(rsp) || req.pin != rsp.pin)
+		return -EPROTO;
+
+	return rsp.value;
+}
+
+static int dln2_gpio_pin_get_in_val(struct dln2_gpio *dln2, unsigned int pin)
+{
+	int ret;
+
+	ret = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_VAL, pin);
+	if (ret < 0)
+		return ret;
+	return !!ret;
+}
+
+static int dln2_gpio_pin_get_out_val(struct dln2_gpio *dln2, unsigned int pin)
+{
+	int ret;
+
+	ret = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_OUT_VAL, pin);
+	if (ret < 0)
+		return ret;
+	return !!ret;
+}
+
+static void dln2_gpio_pin_set_out_val(struct dln2_gpio *dln2,
+				      unsigned int pin, int value)
+{
+	struct dln2_gpio_pin_val req = {
+		.pin = cpu_to_le16(pin),
+		.value = cpu_to_le16(value),
+	};
+
+	dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_OUT_VAL, &req, sizeof(req),
+		      NULL, NULL);
+}
+
+static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	return dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
+}
+
+static void dln2_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
+}
+
+#define DLN2_GPIO_DIRECTION_IN		0
+#define DLN2_GPIO_DIRECTION_OUT		1
+
+static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	int ret;
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	struct dln2_gpio_pin req = {
+		.pin = cpu_to_le16(offset),
+	};
+	struct dln2_gpio_pin_val rsp;
+	int len = sizeof(rsp);
+
+	if (test_bit(offset, dln2->pin_dir_set)) {
+		rsp.value = !!test_bit(offset, dln2->pin_dir);
+		goto report;
+	}
+
+	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
+			    &req, sizeof(req), &rsp, &len);
+	if (ret < 0)
+		return ret;
+	if (len < sizeof(rsp) || req.pin != rsp.pin)
+		return -EPROTO;
+	set_bit(offset, dln2->pin_dir_set);
+
+report:
+	switch (rsp.value) {
+	case DLN2_GPIO_DIRECTION_IN:
+		clear_bit(offset, dln2->pin_dir);
+		return 1;
+	case DLN2_GPIO_DIRECTION_OUT:
+		set_bit(offset, dln2->pin_dir);
+		return 0;
+	default:
+		return -EPROTO;
+	}
+}
+
+static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	int dir;
+
+	dir = dln2_gpio_get_direction(chip, offset);
+	if (dir < 0)
+		return dir;
+
+	if (dir)
+		return dln2_gpio_pin_get_in_val(dln2, offset);
+
+	return dln2_gpio_pin_get_out_val(dln2, offset);
+}
+
+static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	dln2_gpio_pin_set_out_val(dln2, offset, value);
+}
+
+static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
+				   unsigned dir)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	struct dln2_gpio_pin_val req = {
+		.pin = cpu_to_le16(offset),
+		.value = cpu_to_le16(dir),
+	};
+
+	set_bit(offset, dln2->pin_dir_set);
+	if (dir == DLN2_GPIO_DIRECTION_OUT)
+		set_bit(offset, dln2->pin_dir);
+	else
+		clear_bit(offset, dln2->pin_dir);
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static int dln2_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_IN);
+}
+
+static int dln2_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+				      int value)
+{
+	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_OUT);
+}
+
+static int dln2_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
+				  unsigned debounce)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	struct {
+		__le32 duration;
+	} __packed req = {
+		.duration = cpu_to_le32(debounce),
+	};
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
+				   unsigned type, unsigned period)
+{
+	struct {
+		__le16 pin;
+		u8 type;
+		__le16 period;
+	} __packed req = {
+		.pin = cpu_to_le16(pin),
+		.type = type,
+		.period = cpu_to_le16(period),
+	};
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static void dln2_irq_work(struct work_struct *w)
+{
+	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
+	struct dln2_gpio *dln2 = iw->dln2;
+	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
+
+	if (test_bit(iw->pin, dln2->irqs_enabled))
+		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
+	else
+		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
+}
+
+static void dln2_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	set_bit(pin, dln2->irqs_enabled);
+	schedule_work(&dln2->irq_work[pin].work);
+}
+
+static void dln2_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	clear_bit(pin, dln2->irqs_enabled);
+	schedule_work(&dln2->irq_work[pin].work);
+}
+
+static void dln2_irq_mask(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	set_bit(pin, dln2->irqs_masked);
+}
+
+static void dln2_irq_unmask(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	if (test_and_clear_bit(pin, dln2->irqs_pending))
+		generic_handle_irq(pin);
+}
+
+static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_LOW;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_FALLING;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct irq_chip dln2_gpio_irqchip = {
+	.name = "dln2-irq",
+	.irq_enable = dln2_irq_enable,
+	.irq_disable = dln2_irq_disable,
+	.irq_mask = dln2_irq_mask,
+	.irq_unmask = dln2_irq_unmask,
+	.irq_set_type = dln2_irq_set_type,
+};
+
+static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
+			    const void *data, int len)
+{
+	int pin, irq;
+	const struct {
+		__le16 count;
+		__u8 type;
+		__le16 pin;
+		__u8 value;
+	} __packed *event = data;
+	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
+
+	if (len < sizeof(*event)) {
+		dev_err(dln2->gpio.dev, "short event message\n");
+		return;
+	}
+
+	pin = le16_to_cpu(event->pin);
+	irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
+
+	if (!irq) {
+		dev_err(dln2->gpio.dev, "pin %d not mapped to IRQ\n", pin);
+		return;
+	}
+
+	if (!test_bit(pin, dln2->irqs_enabled))
+		return;
+	if (test_bit(pin, dln2->irqs_masked)) {
+		set_bit(pin, dln2->irqs_pending);
+		return;
+	}
+
+	switch (dln2->irq_work[pin].type) {
+	case DLN2_GPIO_EVENT_CHANGE_RISING:
+		if (event->value)
+			generic_handle_irq(irq);
+		break;
+	case DLN2_GPIO_EVENT_CHANGE_FALLING:
+		if (!event->value)
+			generic_handle_irq(irq);
+		break;
+	default:
+		generic_handle_irq(irq);
+	}
+}
+
+static int dln2_gpio_probe(struct platform_device *pdev)
+{
+	struct dln2_gpio *dln2;
+	struct device *dev = &pdev->dev;
+	int pins = dln2_gpio_get_pin_count(pdev);
+	int i, ret;
+
+	if (pins < 0) {
+		dev_err(dev, "failed to get pin count: %d\n", pins);
+		return pins;
+	}
+	if (pins > DLN2_GPIO_MAX_PINS) {
+		pins = DLN2_GPIO_MAX_PINS;
+		dev_warn(dev, "clamping pins to %d\n", DLN2_GPIO_MAX_PINS);
+	}
+
+	dln2 = devm_kzalloc(&pdev->dev, sizeof(*dln2), GFP_KERNEL);
+	if (!dln2)
+		return -ENOMEM;
+
+	for (i = 0; i < DLN2_GPIO_MAX_PINS; i++) {
+		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
+		dln2->irq_work[i].pin = i;
+		dln2->irq_work[i].dln2 = dln2;
+	}
+
+	dln2->pdev = pdev;
+
+	dln2->gpio.label = "dln2";
+	dln2->gpio.dev = dev;
+	dln2->gpio.owner = THIS_MODULE;
+	dln2->gpio.base = -1;
+	dln2->gpio.ngpio = pins;
+	dln2->gpio.exported = 1;
+	dln2->gpio.set = dln2_gpio_set;
+	dln2->gpio.get = dln2_gpio_get;
+	dln2->gpio.request = dln2_gpio_request;
+	dln2->gpio.free = dln2_gpio_free;
+	dln2->gpio.get_direction = dln2_gpio_get_direction;
+	dln2->gpio.direction_input = dln2_gpio_direction_input;
+	dln2->gpio.direction_output = dln2_gpio_direction_output;
+	dln2->gpio.set_debounce = dln2_gpio_set_debounce;
+
+	platform_set_drvdata(pdev, dln2);
+
+	ret = gpiochip_add(&dln2->gpio);
+	if (ret < 0) {
+		dev_err(dev, "failed to add gpio chip: %d\n", ret);
+		goto out;
+	}
+
+	ret = gpiochip_irqchip_add(&dln2->gpio, &dln2_gpio_irqchip, 0,
+				   handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret < 0) {
+		dev_err(dev, "failed to add irq chip: %d\n", ret);
+		goto out_gpiochip_remove;
+	}
+
+	ret = dln2_register_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV,
+				     dln2_gpio_event);
+	if (ret) {
+		dev_err(dev, "failed to register event cb: %d\n", ret);
+		goto out_gpiochip_remove;
+	}
+
+	return 0;
+
+out_gpiochip_remove:
+	gpiochip_remove(&dln2->gpio);
+out:
+	return ret;
+}
+
+static int dln2_gpio_remove(struct platform_device *pdev)
+{
+	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
+
+	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
+	dln2->pdev = NULL;
+	gpiochip_remove(&dln2->gpio);
+
+	return 0;
+}
+
+static struct platform_driver dln2_gpio_driver = {
+	.driver.name	= DRIVER_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= dln2_gpio_probe,
+	.remove		= dln2_gpio_remove,
+};
+
+module_platform_driver(dln2_gpio_driver);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
+MODULE_DESCRIPTION(DRIVER_NAME "driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Lee Jones]

On Tue, 09 Sep 2014, Octavian Purdila wrote:

> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
> 
> https://www.diolan.com/i2c/i2c_interface.html.
> 
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.

This driver really needs a USB Ack before I can accept it.

> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
> 
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
> 
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
> 
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/mfd/Kconfig      |   9 +
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h |  71 +++++
>  4 files changed, 762 insertions(+)
>  create mode 100644 drivers/mfd/dln2.c
>  create mode 100644 include/linux/mfd/dln2.h

[...]

> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 0000000..b551b5e
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,681 @@
> +/*
> + * Driver for the Diolan DLN-2 USB 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/errno.h>

What are you using this for?

> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME			"usb-dln2"

Don't do this, just use "usb-dln2" where it needs to be used.

[...]

> +static struct usb_driver dln2_driver = {
> +	.name = DRIVER_NAME,
> +	.probe = dln2_probe,
> +	.disconnect = dln2_disconnect,
> +	.id_table = dln2_table,
> +};
> +
> +module_usb_driver(dln2_driver);
> +
> +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
> +MODULE_DESCRIPTION(DRIVER_NAME " driver");

This is not a description.

> +MODULE_LICENSE("GPL");

Header says v2.

> diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> new file mode 100644
> index 0000000..197565d
> --- /dev/null
> +++ b/include/linux/mfd/dln2.h
> @@ -0,0 +1,71 @@
> +#ifndef __LINUX_USB_DLN2_H
> +#define __LINUX_USB_DLN2_H
> +
> +#define DLN2_CMD(cmd, id)		((cmd) | ((id) << 8))
> +
> +struct dln2_platform_data {
> +	u16 handle;
> +	union {
> +		struct {
> +			u8 port;
> +		} i2c;
> +	};
> +};

Why this complexity?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Octavian Purdila]

On Wed, Sep 17, 2014 at 2:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 09 Sep 2014, Octavian Purdila wrote:
>
> > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> > Master Adapter DLN-2. Details about the device can be found here:
> >
> > https://www.diolan.com/i2c/i2c_interface.html.
> >
> > Information about the USB protocol can be found in the Programmer's
> > Reference Manual [1], see section 1.7.
>
> This driver really needs a USB Ack before I can accept it.
>

Greg, Johan, is the driver acceptable now?

> > Because the hardware has a single transmit endpoint and a single
> > receive endpoint the communication between the various DLN2 drivers
> > and the hardware will be muxed/demuxed by this driver.
> >
> > Each DLN2 module will be identified by the handle field within the DLN2
> > message header. If a DLN2 module issues multiple commands in parallel
> > they will be identified by the echo counter field in the message header.
> >
> > The DLN2 modules can use the dln2_transfer() function to issue a
> > command and wait for its response. They can also register a callback
> > that is going to be called when a specific event id is generated by
> > the device (e.g. GPIO interrupts). The device uses handle 0 for
> > sending events.
> >
> > [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > ---
> >  drivers/mfd/Kconfig      |   9 +
> >  drivers/mfd/Makefile     |   1 +
> >  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/dln2.h |  71 +++++
> >  4 files changed, 762 insertions(+)
> >  create mode 100644 drivers/mfd/dln2.c
> >  create mode 100644 include/linux/mfd/dln2.h
>
> [...]
>
> > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > new file mode 100644
> > index 0000000..b551b5e
> > --- /dev/null
> > +++ b/drivers/mfd/dln2.c
> > @@ -0,0 +1,681 @@
> > +/*
> > + * Driver for the Diolan DLN-2 USB 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/errno.h>
>
> What are you using this for?
>

Not needed, I will remove it.

> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/dln2.h>
> > +
> > +#define DRIVER_NAME                  "usb-dln2"
>
> Don't do this, just use "usb-dln2" where it needs to be used.
>

Will do.

> [...]
>
> > +static struct usb_driver dln2_driver = {
> > +     .name = DRIVER_NAME,
> > +     .probe = dln2_probe,
> > +     .disconnect = dln2_disconnect,
> > +     .id_table = dln2_table,
> > +};
> > +
> > +module_usb_driver(dln2_driver);
> > +
> > +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
> > +MODULE_DESCRIPTION(DRIVER_NAME " driver");
>
> This is not a description.
>
> > +MODULE_LICENSE("GPL");
>
> Header says v2.
>

I will change it to GPL v2.

> > diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> > new file mode 100644
> > index 0000000..197565d
> > --- /dev/null
> > +++ b/include/linux/mfd/dln2.h
> > @@ -0,0 +1,71 @@
> > +#ifndef __LINUX_USB_DLN2_H
> > +#define __LINUX_USB_DLN2_H
> > +
> > +#define DLN2_CMD(cmd, id)            ((cmd) | ((id) << 8))
> > +
> > +struct dln2_platform_data {
> > +     u16 handle;
> > +     union {
> > +             struct {
> > +                     u8 port;
> > +             } i2c;
> > +     };
> > +};
>
> Why this complexity?
>

There is also an SPI interface on this adapter which will probably the
port information and maybe some additional information. Would you
prefer to add the union later, when we add the SPI driver?

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Johan Hovold]

On Wed, Sep 17, 2014 at 10:25:18AM +0300, Octavian Purdila wrote:
> On Wed, Sep 17, 2014 at 2:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 09 Sep 2014, Octavian Purdila wrote:
> >
> > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> > > Master Adapter DLN-2. Details about the device can be found here:
> > >
> > > https://www.diolan.com/i2c/i2c_interface.html.
> > >
> > > Information about the USB protocol can be found in the Programmer's
> > > Reference Manual [1], see section 1.7.
> >
> > This driver really needs a USB Ack before I can accept it.
> >
> 
> Greg, Johan, is the driver acceptable now?

I started looking through v4 yesterday and found a few more things. Will
send you some you comments shortly.

Johan

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Johan Hovold]

On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:
> This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> Master Adapter DLN-2. Details about the device can be found here:
> 
> https://www.diolan.com/i2c/i2c_interface.html.
> 
> Information about the USB protocol can be found in the Programmer's
> Reference Manual [1], see section 1.7.
> 
> Because the hardware has a single transmit endpoint and a single
> receive endpoint the communication between the various DLN2 drivers
> and the hardware will be muxed/demuxed by this driver.
> 
> Each DLN2 module will be identified by the handle field within the DLN2
> message header. If a DLN2 module issues multiple commands in parallel
> they will be identified by the echo counter field in the message header.
> 
> The DLN2 modules can use the dln2_transfer() function to issue a
> command and wait for its response. They can also register a callback
> that is going to be called when a specific event id is generated by
> the device (e.g. GPIO interrupts). The device uses handle 0 for
> sending events.
> 
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/mfd/Kconfig      |   9 +
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h |  71 +++++
>  4 files changed, 762 insertions(+)
>  create mode 100644 drivers/mfd/dln2.c
>  create mode 100644 include/linux/mfd/dln2.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..7bcf895 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -183,6 +183,15 @@ config MFD_DA9063
>  	  Additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_DLN2
> +	tristate "Diolan DLN2 support"
> +	select MFD_CORE
> +	depends on USB
> +	help
> +	  This adds support for Diolan USB-I2C/SPI/GPIO Master Adapter DLN-2.
> +	  Additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
>  config MFD_MC13XXX
>  	tristate
>  	depends on (SPI_MASTER || I2C)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..591988d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> +obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> new file mode 100644
> index 0000000..b551b5e
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,681 @@
> +/*
> + * Driver for the Diolan DLN-2 USB 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/errno.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME			"usb-dln2"
> +
> +struct dln2_header {
> +	__le16 size;
> +	__le16 id;
> +	__le16 echo;
> +	__le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> +	struct dln2_header hdr;
> +	__le16 result;
> +} __packed;
> +
> +#define DLN2_GENERIC_MODULE_ID		0x00
> +#define DLN2_GENERIC_CMD(cmd)		DLN2_CMD(cmd, DLN2_GENERIC_MODULE_ID)
> +#define CMD_GET_DEVICE_VER		DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SN		DLN2_GENERIC_CMD(0x31)
> +
> +#define DLN2_HW_ID			0x200
> +#define DLN2_USB_TIMEOUT		200	/* in ms */
> +#define DLN2_MAX_RX_SLOTS		16
> +#define DLN2_MAX_MODULES		5

Reduce to 4 until you implement support for more modules and save some
memory meanwhile? (Or is id 4 already used?)

> +#define DLN2_MAX_URBS			16
> +#define DLN2_RX_BUF_SIZE		512
> +
> +#define DLN2_HANDLE_EVENT		0
> +#define DLN2_HANDLE_CTRL		1
> +#define DLN2_HANDLE_GPIO		2
> +#define DLN2_HANDLE_I2C			3
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> +	struct completion done;
> +	struct urb *urb;
> +	bool connected;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to indentify the module in
> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> + * slots field and find the receive context for a particular
> + * request.
> + */
> +struct dln2_mod_rx_slots {
> +	/* RX slots bitmap */
> +	unsigned long bmap;
> +
> +	/* used to wait for a free RX slot */
> +	wait_queue_head_t wq;
> +
> +	/* used to wait for an RX operation to complete */
> +	struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> +	/* avoid races between free_rx_slot and dln2_rx_transfer */
> +	spinlock_t lock;
> +};
> +
> +struct dln2_dev {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *interface;
> +	u8 ep_in;
> +	u8 ep_out;
> +
> +	struct urb *rx_urb[DLN2_MAX_URBS];
> +	void *rx_buf[DLN2_MAX_URBS];
> +
> +	struct dln2_mod_rx_slots mod_rx_slots[DLN2_MAX_MODULES];
> +
> +	struct list_head rx_cb_list;
> +	spinlock_t rx_cb_lock;
> +};
> +
> +static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	*slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
> +
> +	if (*slot < DLN2_MAX_RX_SLOTS) {
> +		struct dln2_rx_context *rxc = &rxs->slots[*slot];
> +
> +		set_bit(*slot, &rxs->bmap);
> +		rxc->connected = true;
> +	}
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	return *slot < DLN2_MAX_RX_SLOTS;
> +}
> +
> +static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
> +{
> +	int ret;
> +	int slot;
> +
> +	/* No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer
> +	 */

Multi-comment style, again.

> +	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}

<snip>

> +static struct dln2_platform_data dln2_pdata_gpio = {
> +	.handle = DLN2_HANDLE_GPIO,
> +};
> +
> +/* Right now only one I2C port seems to be supported */
> +static struct dln2_platform_data dln2_pdata_i2c = {
> +	.handle = DLN2_HANDLE_I2C,
> +	.i2c = {
> +		.port = 0,
> +	},
> +};
> +
> +static const struct mfd_cell dln2_devs[] = {
> +	{
> +		.name	= "dln2-gpio",

Drop indentation     ^

> +		.platform_data = &dln2_pdata_gpio,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +	{
> +		.name	= "dln2-i2c",

Ditto

> +		.platform_data = &dln2_pdata_i2c,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +};
> +
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	mfd_remove_devices(&interface->dev);
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret;
> +	int i, j;
> +
> +	if (hostif->desc.bInterfaceNumber != 0 ||
> +	    hostif->desc.bNumEndpoints < 2)
> +		return -ENODEV;
> +
> +	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
> +	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
> +	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	dln2->interface = interface;
> +	usb_set_intfdata(interface, dln2);
> +
> +	for (i = 0; i < DLN2_MAX_MODULES; i++) {
> +		init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
> +		spin_lock_init(&dln2->mod_rx_slots[i].lock);
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++)
> +			init_completion(&dln2->mod_rx_slots[i].slots[j].done);
> +	}
> +
> +	spin_lock_init(&dln2->rx_cb_lock);
> +	INIT_LIST_HEAD(&dln2->rx_cb_list);
> +
> +	ret = dln2_setup_rx_urbs(dln2, hostif);
> +	if (ret) {
> +		dln2_free(dln2);
> +		return ret;
> +	}
> +
> +	ret = dln2_hw_init(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize hardware\n");
> +		goto out_cleanup;
> +	}
> +
> +	ret = mfd_add_devices(dev, -1, dln2_devs, ARRAY_SIZE(dln2_devs),
> +			      NULL, 0, NULL);
> +	if (ret != 0) {
> +		dev_err(dev, "Failed to add mfd devices to core.\n");
> +		goto out_cleanup;
> +	}

You cannot use id -1 (PLATFORM_DEVID_NONE) here. You either need to use
a unique id base or use -2 (PLATFORM_DEVID_AUTO) so that a platform
device id is assigned automatically.

Currently, the platform devices end up being named

	dln2-gpio
	dln2-i2c

without unique ids in sysfs. When you connect a second DLN2 device your
driver will try to register the same names and fail:

[ 2146.320648] ------------[ cut here ]------------
[ 2146.320831] WARNING: CPU: 0 PID: 178 at /home/johan/work/omicron/src/linux/fs/sysfs/dir.c:31 sysfs_warn_dup+0x74/0x84()
[ 2146.320922] sysfs: cannot create duplicate filename '/bus/platform/devices/dln2-gpio'
[ 2146.321014] Modules linked in: i2c_dln2 gpio_dln2 dln2 netconsole [last unloaded: dln2]
[ 2146.323974] CPU: 0 PID: 178 Comm: bash Tainted: G        W      3.17.0-rc3 #9
[ 2146.324737] [<c0016bec>] (unwind_backtrace) from [<c0013850>] (show_stack+0x20/0x24)
[ 2146.324890] [<c0013850>] (show_stack) from [<c042cb74>] (dump_stack+0x24/0x28)
[ 2146.326538] [<c042cb74>] (dump_stack) from [<c0040ff4>] (warn_slowpath_common+0x80/0x98)
[ 2146.326721] [<c0040ff4>] (warn_slowpath_common) from [<c004104c>] (warn_slowpath_fmt+0x40/0x48)
[ 2146.326812] [<c004104c>] (warn_slowpath_fmt) from [<c016ee54>] (sysfs_warn_dup+0x74/0x84)
[ 2146.326904] [<c016ee54>] (sysfs_warn_dup) from [<c016f1e0>] (sysfs_do_create_link_sd.isra.2+0xcc/0xd0)
[ 2146.327026] [<c016f1e0>] (sysfs_do_create_link_sd.isra.2) from [<c016f220>] (sysfs_create_link+0x3c/0x48)
[ 2146.328338] [<c016f220>] (sysfs_create_link) from [<c0286474>] (bus_add_device+0x12c/0x1e0)
[ 2146.328460] [<c0286474>] (bus_add_device) from [<c02844a8>] (device_add+0x410/0x584)
[ 2146.328552] [<c02844a8>] (device_add) from [<c02890c8>] (platform_device_add+0xd8/0x26c)
[ 2146.328643] [<c02890c8>] (platform_device_add) from [<c02a571c>] (mfd_add_device+0x23c/0x3a0)
[ 2146.328735] [<c02a571c>] (mfd_add_device) from [<c02a5978>] (mfd_add_devices+0xb8/0x110)
[ 2146.328857] [<c02a5978>] (mfd_add_devices) from [<bf0ac800>] (dln2_probe+0x1f4/0x250 [dln2])
[ 2146.328948] [<bf0ac800>] (dln2_probe [dln2]) from [<c030bc98>] (usb_probe_interface+0x1bc/0x2a8)
[ 2146.329071] [<c030bc98>] (usb_probe_interface) from [<c0287314>] (driver_probe_device+0x14c/0x3ac)
[ 2146.330718] [<c0287314>] (driver_probe_device) from [<c028766c>] (__driver_attach+0xa4/0xa8)
[ 2146.330810] [<c028766c>] (__driver_attach) from [<c0285320>] (bus_for_each_dev+0x70/0xa4)
[ 2146.330902] [<c0285320>] (bus_for_each_dev) from [<c0286cb8>] (driver_attach+0x2c/0x30)
[ 2146.330993] [<c0286cb8>] (driver_attach) from [<c0309f20>] (usb_store_new_id+0x170/0x1ac)
[ 2146.331909] [<c0309f20>] (usb_store_new_id) from [<c0309f90>] (new_id_store+0x34/0x3c)
[ 2146.332000] [<c0309f90>] (new_id_store) from [<c0285074>] (drv_attr_store+0x30/0x3c)
[ 2146.332611] [<c0285074>] (drv_attr_store) from [<c016e740>] (sysfs_kf_write+0x5c/0x60)
[ 2146.332702] [<c016e740>] (sysfs_kf_write) from [<c016d900>] (kernfs_fop_write+0xd4/0x194)
[ 2146.332794] [<c016d900>] (kernfs_fop_write) from [<c010fde8>] (vfs_write+0xb4/0x1c0)
[ 2146.332916] [<c010fde8>] (vfs_write) from [<c0110450>] (SyS_write+0x4c/0xa0)
[ 2146.333007] [<c0110450>] (SyS_write) from [<c000f900>] (ret_fast_syscall+0x0/0x48)
[ 2146.333984] ---[ end trace ad1c64348c92978a ]---
[ 2146.334625] usb-dln2 1-2.1:1.0: Failed to add mfd devices to core.
[ 2146.334930] usb-dln2: probe of 1-2.1:1.0 failed with error -17


I noticed that the other two USB MFD drivers suffer from similar
problems. The rtsx_usb driver almost gets it right by using the usb
device number (address) but that is only unique per bus. I suggest using

	bus_num << 8 | devnum

as a base for USB MFD device ids instead. Note however that this might
still not be sufficient if there are ever USB-MFD drivers with multiple
cells of the same type (with increasing cell ids). (That could of course
then be solved by further shifting.)

Unless anyone suggests otherwise (e.g. to stick with auto id), I'll add
a helper function for this and fix up those two drivers.

Johan

Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Johan Hovold]

On Tue, Sep 09, 2014 at 10:24:45PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu <laurentiu.palcu@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.palcu@intel.com>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/i2c/busses/Kconfig    |  10 ++
>  drivers/i2c/busses/Makefile   |   1 +
>  drivers/i2c/busses/i2c-dln2.c | 390 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 401 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-dln2.c
> 
> 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 0000000..ac09ec4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dln2.c
> @@ -0,0 +1,390 @@
> +/*
> + * 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/errno.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 DRIVER_NAME			"dln2-i2c"
> +
> +#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_COUNT		DLN2_I2C_CMD(0x00)
> +#define DLN2_I2C_ENABLE			DLN2_I2C_CMD(0x01)
> +#define DLN2_I2C_DISABLE		DLN2_I2C_CMD(0x02)
> +#define DLN2_I2C_IS_ENABLED		DLN2_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_DISABLE		DLN2_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		100000
> +
> +#define DLN2_I2C_MAX_XFER_SIZE		256
> +
> +struct dln2_i2c {
> +	struct platform_device *pdev;
> +	struct i2c_adapter adapter;
> +	uint32_t freq;
> +	uint32_t min_freq;
> +	uint32_t max_freq;

Please use u32 throughout for consistency.

> +	/*
> +	 * 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.
> +	 */
> +	union {
> +		struct {
> +			u8 port;
> +			u8 addr;
> +			u8 mem_addr_len;
> +			__le32 mem_addr;
> +			__le16 buf_len;
> +			u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> +		} __packed tx;
> +		struct {
> +			__le16 buf_len;
> +			u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> +		} __packed rx;
> +	} buf;

While this works in this case due to the extra copy you do in
dln2_transfer, allocating buffers that would (generally) be used for DMA
transfers as part of a larger structure is a recipe for trouble.

It's probably better to allocate separately, if only to prevent people
from thinking there might be a bug here.

> +};
> +
> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +	int ret;
> +	u8 port;
> +	u16 cmd;
> +
> +	port = pdata->i2c.port;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(port), NULL, NULL);
> +	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;
> +	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +
> +	tx.port = pdata->i2c.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, uint32_t *res)

u32

> +{
> +	int ret;
> +	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +	u8 port = pdata->i2c.port;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &port, sizeof(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_setup(struct dln2_i2c *dln2)
> +{
> +	int ret;
> +
> +	ret = dln2_i2c_enable(dln2, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MIN_FREQUENCY,
> +				&dln2->min_freq);
> +	if (ret < 0)
> +		return 0;
> +
> +	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MAX_FREQUENCY,
> +				&dln2->max_freq);
> +	if (ret < 0)
> +		return 0;
> +
> +	ret = dln2_i2c_set_frequency(dln2, DLN2_I2C_FREQ_STD);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_enable(dln2, true);
> +
> +	return ret;
> +}
> +
> +static int dln2_i2c_write(struct dln2_i2c *dln2, u8 addr,
> +			  u8 *data, u16 data_len)
> +{
> +	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +	int ret;
> +	unsigned len;
> +
> +	dln2->buf.tx.port = pdata->i2c.port;
> +	dln2->buf.tx.addr = addr;
> +	dln2->buf.tx.mem_addr_len = 0;
> +	dln2->buf.tx.mem_addr = 0;
> +	dln2->buf.tx.buf_len = cpu_to_le16(data_len);
> +	memcpy(dln2->buf.tx.buf, data, data_len);
> +
> +	len = sizeof(dln2->buf.tx) + data_len - DLN2_I2C_MAX_XFER_SIZE;
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_WRITE, &dln2->buf.tx, len,
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return data_len;
> +}
> +
> +static int dln2_i2c_read(struct dln2_i2c *dln2, u16 addr, u8 *data,
> +			 u16 data_len)
> +{
> +	struct dln2_platform_data *pdata = dev_get_platdata(&dln2->pdev->dev);
> +	int ret;
> +	struct {
> +		u8 port;
> +		u8 addr;
> +		u8 mem_addr_len;
> +		__le32 mem_addr;
> +		__le16 buf_len;
> +	} __packed tx;
> +	unsigned rx_len, rx_buf_len;
> +
> +	tx.port = pdata->i2c.port;
> +	tx.addr = addr;
> +	tx.mem_addr_len = 0;
> +	tx.mem_addr = 0;
> +	tx.buf_len = cpu_to_le16(data_len);
> +
> +	rx_len = sizeof(dln2->buf.rx);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_READ, &tx, sizeof(tx),
> +			    &dln2->buf.rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len < 2)

Use sizeof(dln2->buf.rx.buf_len) here as well.

> +		return -EPROTO;
> +
> +	rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> +	if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> +		return -EPROTO;
> +
> +	if (data_len > rx_buf_len)
> +		data_len = rx_buf_len;

You're still not checking that the received data does not overflow the
supplied buffer as I already commented on v3.

> +
> +	memcpy(data, dln2->buf.rx.buf, data_len);
> +
> +	return data_len;
> +}
> +
> +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;
> +
> +		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;
> +}
> +
> +static u32 dln2_i2c_func(struct i2c_adapter *a)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
> +		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> +		I2C_FUNC_SMBUS_I2C_BLOCK;
> +}
> +
> +static const struct i2c_algorithm dln2_i2c_usb_algorithm = {
> +	.master_xfer = dln2_i2c_xfer,
> +	.functionality = dln2_i2c_func,
> +};
> +
> +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(freq, S_IRUGO|S_IWUSR, dln2_i2c_freq_show,
> +		   dln2_i2c_freq_store);

Use DEVICE_ATTR_RW

> +
> +static int dln2_i2c_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct dln2_i2c *dln2;
> +	struct device *dev = &pdev->dev;
> +	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> +	dln2 = devm_kzalloc(dev, sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	dln2->pdev = pdev;
> +
> +	/* setup i2c adapter description */
> +	dln2->adapter.owner = THIS_MODULE;
> +	dln2->adapter.class = I2C_CLASS_HWMON;
> +	dln2->adapter.algo = &dln2_i2c_usb_algorithm;
> +	dln2->adapter.dev.parent = dev;
> +	i2c_set_adapdata(&dln2->adapter, dln2);
> +	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), "%s-%s-%d",
> +		 DRIVER_NAME, dev_name(pdev->dev.parent), pdata->i2c.port);
> +
> +	platform_set_drvdata(pdev, dln2);
> +
> +	ret = device_create_file(dev, &dev_attr_freq);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add freq attribute\n");
> +		return ret;
> +	}

There are a couple of problems here. First, you should not make this an
attribute of the platform device, which is created before any driver is
bound (might not ever happen).

Instead add the attribute to the i2c adapter below. However, you need to
do this using device attribute groups to avoid racing with userspace (as
you are when using device_create_file after the device itself has been
created).

You should probably also make your attribute name less generic by adding
a "dln2_"-prefix.

> +
> +	/* initialize the i2c interface */
> +	ret = dln2_i2c_setup(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize adapter: %d\n", ret);
> +		goto out_remove_freq_attr;
> +	}
> +
> +	/* and finally attach to i2c layer */
> +	ret = i2c_add_adapter(&dln2->adapter);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add I2C adapter: %d\n", ret);
> +		goto out_disable;
> +	}
> +
> +	return 0;
> +
> +out_disable:
> +	dln2_i2c_enable(dln2, false);
> +out_remove_freq_attr:
> +	device_remove_file(dev, &dev_attr_freq);
> +
> +	return ret;
> +}

I'll try to look through v4 of the gpio driver tomorrow as well.

Johan

Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Octavian Purdila]

On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:

<snip>

>> +     /*
>> +      * 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.
>> +      */
>> +     union {
>> +             struct {
>> +                     u8 port;
>> +                     u8 addr;
>> +                     u8 mem_addr_len;
>> +                     __le32 mem_addr;
>> +                     __le16 buf_len;
>> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> +             } __packed tx;
>> +             struct {
>> +                     __le16 buf_len;
>> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> +             } __packed rx;
>> +     } buf;
>
> While this works in this case due to the extra copy you do in
> dln2_transfer, allocating buffers that would (generally) be used for DMA
> transfers as part of a larger structure is a recipe for trouble.
>
> It's probably better to allocate separately, if only to prevent people
> from thinking there might be a bug here.
>

Just to make sure I understand this, what could the issues be? The
buffers not being aligned or not allocated in continuous physical
memory?

<snip>

>> +
>> +     rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
>> +     if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
>> +             return -EPROTO;
>> +
>> +     if (data_len > rx_buf_len)
>> +             data_len = rx_buf_len;
>
> You're still not checking that the received data does not overflow the
> supplied buffer as I already commented on v3.
>
>> +
>> +     memcpy(data, dln2->buf.rx.buf, data_len);
>> +
>> +     return data_len;
>> +}

Hmm, perhaps I am missing something, but we never transfer more then
data_len, where data_len is the size of the buffer supplied by the
user.

<snip>

>> +
>> +     platform_set_drvdata(pdev, dln2);
>> +
>> +     ret = device_create_file(dev, &dev_attr_freq);
>> +     if (ret < 0) {
>> +             dev_err(dev, "failed to add freq attribute\n");
>> +             return ret;
>> +     }
>
> There are a couple of problems here. First, you should not make this an
> attribute of the platform device, which is created before any driver is
> bound (might not ever happen).
>
> Instead add the attribute to the i2c adapter below. However, you need to
> do this using device attribute groups to avoid racing with userspace (as
> you are when using device_create_file after the device itself has been
> created).
>
> You should probably also make your attribute name less generic by adding
> a "dln2_"-prefix.

Thanks for the detailed review and explanations, as always :)

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Lee Jones]

On Wed, 17 Sep 2014, Johan Hovold wrote:
> On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:
> > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> > Master Adapter DLN-2. Details about the device can be found here:
> > 
> > https://www.diolan.com/i2c/i2c_interface.html.
> > 
> > Information about the USB protocol can be found in the Programmer's
> > Reference Manual [1], see section 1.7.
> > 
> > Because the hardware has a single transmit endpoint and a single
> > receive endpoint the communication between the various DLN2 drivers
> > and the hardware will be muxed/demuxed by this driver.
> > 
> > Each DLN2 module will be identified by the handle field within the DLN2
> > message header. If a DLN2 module issues multiple commands in parallel
> > they will be identified by the echo counter field in the message header.
> > 
> > The DLN2 modules can use the dln2_transfer() function to issue a
> > command and wait for its response. They can also register a callback
> > that is going to be called when a specific event id is generated by
> > the device (e.g. GPIO interrupts). The device uses handle 0 for
> > sending events.
> > 
> > [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> > 
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > ---
> >  drivers/mfd/Kconfig      |   9 +
> >  drivers/mfd/Makefile     |   1 +
> >  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/dln2.h |  71 +++++
> >  4 files changed, 762 insertions(+)
> >  create mode 100644 drivers/mfd/dln2.c
> >  create mode 100644 include/linux/mfd/dln2.h

[...]

> Unless anyone suggests otherwise (e.g. to stick with auto id), I'll add
> a helper function for this and fix up those two drivers.

Appreciated.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Lee Jones]

On Wed, 17 Sep 2014, Octavian Purdila wrote:
> On Wed, Sep 17, 2014 at 2:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 09 Sep 2014, Octavian Purdila wrote:
> >
> > > This patch implements the USB part of the Diolan USB-I2C/SPI/GPIO
> > > Master Adapter DLN-2. Details about the device can be found here:
> > >
> > > https://www.diolan.com/i2c/i2c_interface.html.
> > >
> > > Information about the USB protocol can be found in the Programmer's
> > > Reference Manual [1], see section 1.7.
> > >
> > > Because the hardware has a single transmit endpoint and a single
> > > receive endpoint the communication between the various DLN2 drivers
> > > and the hardware will be muxed/demuxed by this driver.
> > >
> > > Each DLN2 module will be identified by the handle field within the DLN2
> > > message header. If a DLN2 module issues multiple commands in parallel
> > > they will be identified by the echo counter field in the message header.
> > >
> > > The DLN2 modules can use the dln2_transfer() function to issue a
> > > command and wait for its response. They can also register a callback
> > > that is going to be called when a specific event id is generated by
> > > the device (e.g. GPIO interrupts). The device uses handle 0 for
> > > sending events.
> > >
> > > [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> > >
> > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > > ---
> > >  drivers/mfd/Kconfig      |   9 +
> > >  drivers/mfd/Makefile     |   1 +
> > >  drivers/mfd/dln2.c       | 681 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/dln2.h |  71 +++++
> > >  4 files changed, 762 insertions(+)
> > >  create mode 100644 drivers/mfd/dln2.c
> > >  create mode 100644 include/linux/mfd/dln2.h

[...]

> > > diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> > > new file mode 100644
> > > index 0000000..197565d
> > > --- /dev/null
> > > +++ b/include/linux/mfd/dln2.h
> > > @@ -0,0 +1,71 @@
> > > +#ifndef __LINUX_USB_DLN2_H
> > > +#define __LINUX_USB_DLN2_H
> > > +
> > > +#define DLN2_CMD(cmd, id)            ((cmd) | ((id) << 8))
> > > +
> > > +struct dln2_platform_data {
> > > +     u16 handle;
> > > +     union {
> > > +             struct {
> > > +                     u8 port;
> > > +             } i2c;
> > > +     };
> > > +};
> >
> > Why this complexity?
> 
> There is also an SPI interface on this adapter which will probably the
> port information and maybe some additional information. Would you
> prefer to add the union later, when we add the SPI driver?

Yes please.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Johan Hovold]

On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> <snip>
> 
> >> +     /*
> >> +      * 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.
> >> +      */
> >> +     union {
> >> +             struct {
> >> +                     u8 port;
> >> +                     u8 addr;
> >> +                     u8 mem_addr_len;
> >> +                     __le32 mem_addr;
> >> +                     __le16 buf_len;
> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> +             } __packed tx;
> >> +             struct {
> >> +                     __le16 buf_len;
> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> +             } __packed rx;
> >> +     } buf;
> >
> > While this works in this case due to the extra copy you do in
> > dln2_transfer, allocating buffers that would (generally) be used for DMA
> > transfers as part of a larger structure is a recipe for trouble.
> >
> > It's probably better to allocate separately, if only to prevent people
> > from thinking there might be a bug here.
> >
> 
> Just to make sure I understand this, what could the issues be? The
> buffers not being aligned or not allocated in continuous physical
> memory?

Yes, the buffer (and any subsequent field) would have to be cache-line
aligned to avoid corruption due to cache-line sharing on some systems.

> <snip>
> 
> >> +
> >> +     rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> >> +     if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> >> +             return -EPROTO;
> >> +
> >> +     if (data_len > rx_buf_len)
> >> +             data_len = rx_buf_len;
> >
> > You're still not checking that the received data does not overflow the
> > supplied buffer as I already commented on v3.
> >
> >> +
> >> +     memcpy(data, dln2->buf.rx.buf, data_len);
> >> +
> >> +     return data_len;
> >> +}
> 
> Hmm, perhaps I am missing something, but we never transfer more then
> data_len, where data_len is the size of the buffer supplied by the
> user.

That is the amount of data you request from the device, but you never
check how much is actually returned.

You really should clean up the error handling of this function as it is
currently not very readable.

> <snip>
> 
> >> +
> >> +     platform_set_drvdata(pdev, dln2);
> >> +
> >> +     ret = device_create_file(dev, &dev_attr_freq);
> >> +     if (ret < 0) {
> >> +             dev_err(dev, "failed to add freq attribute\n");
> >> +             return ret;
> >> +     }
> >
> > There are a couple of problems here. First, you should not make this an
> > attribute of the platform device, which is created before any driver is
> > bound (might not ever happen).
> >
> > Instead add the attribute to the i2c adapter below. However, you need to
> > do this using device attribute groups to avoid racing with userspace (as
> > you are when using device_create_file after the device itself has been
> > created).
> >
> > You should probably also make your attribute name less generic by adding
> > a "dln2_"-prefix.
> 
> Thanks for the detailed review and explanations, as always :)

You're welcome.

Johan

Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Octavian Purdila]

On Thu, Sep 18, 2014 at 11:19 AM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
>> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:
>>
>> <snip>
>>
>> >> +     /*
>> >> +      * 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.
>> >> +      */
>> >> +     union {
>> >> +             struct {
>> >> +                     u8 port;
>> >> +                     u8 addr;
>> >> +                     u8 mem_addr_len;
>> >> +                     __le32 mem_addr;
>> >> +                     __le16 buf_len;
>> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> >> +             } __packed tx;
>> >> +             struct {
>> >> +                     __le16 buf_len;
>> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> >> +             } __packed rx;
>> >> +     } buf;
>> >
>> > While this works in this case due to the extra copy you do in
>> > dln2_transfer, allocating buffers that would (generally) be used for DMA
>> > transfers as part of a larger structure is a recipe for trouble.
>> >
>> > It's probably better to allocate separately, if only to prevent people
>> > from thinking there might be a bug here.
>> >
>>
>> Just to make sure I understand this, what could the issues be? The
>> buffers not being aligned or not allocated in continuous physical
>> memory?
>
> Yes, the buffer (and any subsequent field) would have to be cache-line
> aligned to avoid corruption due to cache-line sharing on some systems.
>

Ah, ok, makes sense now. But is it safe to use kmalloc() in this case?
Does kmalloc() prevent cache line sharing?

>> <snip>
>>
>> >> +
>> >> +     rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
>> >> +     if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
>> >> +             return -EPROTO;
>> >> +
>> >> +     if (data_len > rx_buf_len)
>> >> +             data_len = rx_buf_len;
>> >
>> > You're still not checking that the received data does not overflow the
>> > supplied buffer as I already commented on v3.
>> >
>> >> +
>> >> +     memcpy(data, dln2->buf.rx.buf, data_len);
>> >> +
>> >> +     return data_len;
>> >> +}
>>
>> Hmm, perhaps I am missing something, but we never transfer more then
>> data_len, where data_len is the size of the buffer supplied by the
>> user.
>
> That is the amount of data you request from the device, but you never
> check how much is actually returned.
>

Actually we check the receive buffer size here:

    if (data_len > rx_buf_len)
        data_len = rx_buf_len;

rx_buf_len is the i2c received payload size while rx_len is the length
of received message

> You really should clean up the error handling of this function as it is
> currently not very readable.
>

Perhaps adding some comments similar to the the explanation above would help?

Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Johan Hovold]

On Thu, Sep 18, 2014 at 11:49:19AM +0300, Octavian Purdila wrote:
> On Thu, Sep 18, 2014 at 11:19 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Sep 17, 2014 at 01:07:51PM +0300, Octavian Purdila wrote:
> >> On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:
> >>
> >> <snip>
> >>
> >> >> +     /*
> >> >> +      * 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.
> >> >> +      */
> >> >> +     union {
> >> >> +             struct {
> >> >> +                     u8 port;
> >> >> +                     u8 addr;
> >> >> +                     u8 mem_addr_len;
> >> >> +                     __le32 mem_addr;
> >> >> +                     __le16 buf_len;
> >> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> +             } __packed tx;
> >> >> +             struct {
> >> >> +                     __le16 buf_len;
> >> >> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> >> +             } __packed rx;
> >> >> +     } buf;
> >> >
> >> > While this works in this case due to the extra copy you do in
> >> > dln2_transfer, allocating buffers that would (generally) be used for DMA
> >> > transfers as part of a larger structure is a recipe for trouble.
> >> >
> >> > It's probably better to allocate separately, if only to prevent people
> >> > from thinking there might be a bug here.
> >> >
> >>
> >> Just to make sure I understand this, what could the issues be? The
> >> buffers not being aligned or not allocated in continuous physical
> >> memory?
> >
> > Yes, the buffer (and any subsequent field) would have to be cache-line
> > aligned to avoid corruption due to cache-line sharing on some systems.
> >
> 
> Ah, ok, makes sense now. But is it safe to use kmalloc() in this case?
> Does kmalloc() prevent cache line sharing?

Yes, it does (as long as you allocate the buffer separately from the
containing struct).

> >> <snip>
> >>
> >> >> +
> >> >> +     rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
> >> >> +     if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
> >> >> +             return -EPROTO;
> >> >> +
> >> >> +     if (data_len > rx_buf_len)
> >> >> +             data_len = rx_buf_len;
> >> >
> >> > You're still not checking that the received data does not overflow the
> >> > supplied buffer as I already commented on v3.
> >> >
> >> >> +
> >> >> +     memcpy(data, dln2->buf.rx.buf, data_len);
> >> >> +
> >> >> +     return data_len;
> >> >> +}
> >>
> >> Hmm, perhaps I am missing something, but we never transfer more then
> >> data_len, where data_len is the size of the buffer supplied by the
> >> user.
> >
> > That is the amount of data you request from the device, but you never
> > check how much is actually returned.
> >
> 
> Actually we check the receive buffer size here:
> 
>     if (data_len > rx_buf_len)
>         data_len = rx_buf_len;
> 
> rx_buf_len is the i2c received payload size while rx_len is the length
> of received message

Bah, you're right. You never explicitly check for overflow, but also
never use more than data_len bytes of the returned buffer.

I think you should add explicit checks, and return an error in this case
rather than silently truncate the data.

> > You really should clean up the error handling of this function as it is
> > currently not very readable.
> >
> 
> Perhaps adding some comments similar to the the explanation above would help?

It's more the logic of this function I find a bit twisted. I think you
should clean it up and consider adding appropriately named (temporary)
variables to make it more readable.

Johan

Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Johan Hovold]

On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:

<snip>

> +#define DLN2_GPIO_DIRECTION_IN		0
> +#define DLN2_GPIO_DIRECTION_OUT		1
> +
> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +	int ret;
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct dln2_gpio_pin req = {
> +		.pin = cpu_to_le16(offset),
> +	};
> +	struct dln2_gpio_pin_val rsp;
> +	int len = sizeof(rsp);
> +
> +	if (test_bit(offset, dln2->pin_dir_set)) {
> +		rsp.value = !!test_bit(offset, dln2->pin_dir);
> +		goto report;
> +	}
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> +			    &req, sizeof(req), &rsp, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(rsp) || req.pin != rsp.pin)
> +		return -EPROTO;
> +	set_bit(offset, dln2->pin_dir_set);

You shouldn't set this flag until you've stored the direction.

> +
> +report:
> +	switch (rsp.value) {
> +	case DLN2_GPIO_DIRECTION_IN:
> +		clear_bit(offset, dln2->pin_dir);
> +		return 1;
> +	case DLN2_GPIO_DIRECTION_OUT:
> +		set_bit(offset, dln2->pin_dir);
> +		return 0;
> +	default:
> +		return -EPROTO;
> +	}
> +}
> +
> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	int dir;
> +
> +	dir = dln2_gpio_get_direction(chip, offset);
> +	if (dir < 0)
> +		return dir;
> +
> +	if (dir)
> +		return dln2_gpio_pin_get_in_val(dln2, offset);
> +
> +	return dln2_gpio_pin_get_out_val(dln2, offset);
> +}
> +
> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	dln2_gpio_pin_set_out_val(dln2, offset, value);
> +}
> +
> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
> +				   unsigned dir)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct dln2_gpio_pin_val req = {
> +		.pin = cpu_to_le16(offset),
> +		.value = cpu_to_le16(dir),
> +	};
> +
> +	set_bit(offset, dln2->pin_dir_set);

Set flag after you store the direction below.

> +	if (dir == DLN2_GPIO_DIRECTION_OUT)
> +		set_bit(offset, dln2->pin_dir);
> +	else
> +		clear_bit(offset, dln2->pin_dir);

Either way, it looks like this could race with get_direction() if you
get a set_direction() while get_direction() is retrieving the direction
from the device.

This would break gpio_get().

> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
> +			     &req, sizeof(req), NULL, NULL);
> +}

<snip>

> +static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
> +				   unsigned type, unsigned period)
> +{
> +	struct {
> +		__le16 pin;
> +		u8 type;
> +		__le16 period;
> +	} __packed req = {
> +		.pin = cpu_to_le16(pin),
> +		.type = type,
> +		.period = cpu_to_le16(period),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static void dln2_irq_work(struct work_struct *w)
> +{
> +	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
> +	struct dln2_gpio *dln2 = iw->dln2;
> +	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
> +
> +	if (test_bit(iw->pin, dln2->irqs_enabled))
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
> +	else
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
> +}
> +
> +static void dln2_irq_enable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	set_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_disable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	clear_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}

<snip>

> +static int dln2_gpio_probe(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2;
> +	struct device *dev = &pdev->dev;
> +	int pins = dln2_gpio_get_pin_count(pdev);

Again, don't do non-trivial intialisations when declaring your variables.

> +	int i, ret;
> +
> +	if (pins < 0) {
> +		dev_err(dev, "failed to get pin count: %d\n", pins);
> +		return pins;
> +	}
> +	if (pins > DLN2_GPIO_MAX_PINS) {
> +		pins = DLN2_GPIO_MAX_PINS;
> +		dev_warn(dev, "clamping pins to %d\n", DLN2_GPIO_MAX_PINS);
> +	}
> +
> +	dln2 = devm_kzalloc(&pdev->dev, sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DLN2_GPIO_MAX_PINS; i++) {
> +		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
> +		dln2->irq_work[i].pin = i;
> +		dln2->irq_work[i].dln2 = dln2;
> +	}
> +
> +	dln2->pdev = pdev;
> +
> +	dln2->gpio.label = "dln2";
> +	dln2->gpio.dev = dev;
> +	dln2->gpio.owner = THIS_MODULE;
> +	dln2->gpio.base = -1;
> +	dln2->gpio.ngpio = pins;
> +	dln2->gpio.exported = 1;
> +	dln2->gpio.set = dln2_gpio_set;
> +	dln2->gpio.get = dln2_gpio_get;
> +	dln2->gpio.request = dln2_gpio_request;
> +	dln2->gpio.free = dln2_gpio_free;
> +	dln2->gpio.get_direction = dln2_gpio_get_direction;
> +	dln2->gpio.direction_input = dln2_gpio_direction_input;
> +	dln2->gpio.direction_output = dln2_gpio_direction_output;
> +	dln2->gpio.set_debounce = dln2_gpio_set_debounce;
> +
> +	platform_set_drvdata(pdev, dln2);
> +
> +	ret = gpiochip_add(&dln2->gpio);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add gpio chip: %d\n", ret);
> +		goto out;
> +	}
> +
> +	ret = gpiochip_irqchip_add(&dln2->gpio, &dln2_gpio_irqchip, 0,
> +				   handle_simple_irq, IRQ_TYPE_NONE);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add irq chip: %d\n", ret);
> +		goto out_gpiochip_remove;
> +	}
> +
> +	ret = dln2_register_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV,
> +				     dln2_gpio_event);
> +	if (ret) {
> +		dev_err(dev, "failed to register event cb: %d\n", ret);
> +		goto out_gpiochip_remove;
> +	}
> +
> +	return 0;
> +
> +out_gpiochip_remove:
> +	gpiochip_remove(&dln2->gpio);
> +out:
> +	return ret;
> +}
> +
> +static int dln2_gpio_remove(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
> +
> +	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +	dln2->pdev = NULL;
> +	gpiochip_remove(&dln2->gpio);

You probably need to flush all your work structs here.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver dln2_gpio_driver = {
> +	.driver.name	= DRIVER_NAME,
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= dln2_gpio_probe,
> +	.remove		= dln2_gpio_remove,
> +};
> +
> +module_platform_driver(dln2_gpio_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
> +MODULE_DESCRIPTION(DRIVER_NAME "driver");
> +MODULE_LICENSE("GPL");

Johan

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Johan Hovold]

On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:

> +static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
> +{
> +	int ret;
> +	int slot;
> +
> +	/* No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer
> +	 */
> +	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots *rxs,
> +			 int slot)
> +{
> +	struct urb *urb = NULL;
> +	unsigned long flags;
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int ret;
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	clear_bit(slot, &rxs->bmap);
> +
> +	rxc = &rxs->slots[slot];
> +	rxc->connected = false;
> +	urb = rxc->urb;
> +	rxc->urb = NULL;
> +	reinit_completion(&rxc->done);
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	if (urb)  {
> +		ret = usb_submit_urb(urb, GFP_KERNEL);
> +		if (ret < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
> +	}
> +
> +	wake_up_interruptible(&rxs->wq);
> +}

> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> +			     u16 handle, u16 rx_slot)
> +{
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int err;
> +
> +	spin_lock(&rxs->lock);
> +	rxc = &rxs->slots[rx_slot];
> +	if (rxc->connected) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +	} else {
> +		dev_warn(dev, "dropping response %d/%d", handle, rx_slot);
> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +	}
> +	spin_unlock(&rxs->lock);
> +}

> +static void dln2_rx(struct urb *urb)
> +{
> +	int err;
> +	struct dln2_dev *dln2 = urb->context;
> +	struct dln2_header *hdr = urb->transfer_buffer;
> +	struct device *dev = &dln2->interface->dev;
> +	u16 id, echo, handle, size;
> +	u8 *data;
> +	int len;
> +
> +	switch (urb->status) {
> +	case 0:
> +		/* success */
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +	case -EPIPE:
> +		/* this urb is terminated, clean up */
> +		dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> +		return;
> +	default:
> +		dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> +		goto out;
> +	}
> +
> +	if (urb->actual_length < sizeof(struct dln2_header)) {
> +		dev_err(dev, "short response: %d\n", urb->actual_length);
> +		goto out;
> +	}
> +
> +	handle = le16_to_cpu(hdr->handle);
> +	id = le16_to_cpu(hdr->id);
> +	echo = le16_to_cpu(hdr->echo);
> +	size = le16_to_cpu(hdr->size);
> +
> +	if (size != urb->actual_length) {
> +		dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
> +			handle, id, echo, size, urb->actual_length);
> +		goto out;
> +	}
> +
> +	if (handle >= DLN2_MAX_MODULES) {
> +		dev_warn(dev, "RX: invalid handle %d\n", handle);
> +		goto out;
> +	}
> +
> +	data = urb->transfer_buffer + sizeof(struct dln2_header);
> +	len = urb->actual_length - sizeof(struct dln2_header);
> +
> +	if (handle == DLN2_HANDLE_EVENT) {
> +		dln2_run_rx_callbacks(dln2, id, echo, data, len);
> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			goto out_submit_failed;
> +	} else {
> +		dln2_rx_transfer(dln2, urb, handle, echo);
> +	}
> +
> +	return;
> +
> +out:
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +out_submit_failed:
> +	if (err < 0)
> +		dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +}

> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> +			  const void *obuf, unsigned obuf_len,
> +			  void *ibuf, unsigned *ibuf_len)
> +{
> +	int ret = 0;
> +	u16 result;
> +	int rx_slot;
> +	struct dln2_response *rsp;
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> +	rx_slot = alloc_rx_slot(rxs);
> +	if (rx_slot < 0)
> +		return rx_slot;
> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		free_rx_slot(dln2, rxs, rx_slot);
> +		dev_err(dev, "USB write failed: %d", ret);
> +		return ret;
> +	}
> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		goto out_free_rx_slot;
> +	}
> +
> +	/* if we got here we know that the response header has been checked */
> +	rsp = rxc->urb->transfer_buffer;
> +	if (rsp->hdr.size < sizeof(*rsp)) {
> +		ret = -EPROTO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	result = le16_to_cpu(rsp->result);
> +	if (result) {
> +		dev_dbg(dev, "%d received response with error %d\n",
> +			handle, result);
> +		ret = -EREMOTEIO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (!ibuf) {
> +		ret = 0;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
> +		*ibuf_len = rsp->hdr.size - sizeof(*rsp);
> +
> +	memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> +	free_rx_slot(dln2, rxs, rx_slot);
> +
> +	return ret;
> +}
> +
> +int dln2_transfer(struct platform_device *pdev, u16 cmd,
> +		  const void *obuf, unsigned obuf_len,
> +		  void *ibuf, unsigned *ibuf_len)
> +{
> +	struct dln2_platform_data *dln2_pdata;
> +	struct dln2_dev *dln2;
> +	u16 h;
> +
> +	/* USB device has been disconnected, bail out */
> +	if (!pdev)
> +		return -ENODEV;

This isn't sufficient to prevent I/O after disconnect, or worse.

You set pdev to NULL in the platform device's remove callbacks, but you
have no synchronisation.

To take one example: in _dln2_transfer above you wait for completion,
but you never wake the process up in case the urb is killed due to a
disconnect. Hence you get a timeout much later, call free_rx_slot which
tries to send of another urb using potentially already freed data.

> +
> +	dln2 = dev_get_drvdata(pdev->dev.parent);
> +	dln2_pdata = dev_get_platdata(&pdev->dev);
> +	h = dln2_pdata->handle;
> +
> +	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
> +}
> +EXPORT_SYMBOL(dln2_transfer);

> +static void dln2_free_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		usb_kill_urb(dln2->rx_urb[i]);
> +		usb_free_urb(dln2->rx_urb[i]);
> +		kfree(dln2->rx_buf[i]);
> +	}
> +}
> +
> +static void dln2_free(struct dln2_dev *dln2)
> +{
> +	dln2_free_rx_urbs(dln2);
> +	usb_put_dev(dln2->usb_dev);
> +	kfree(dln2);
> +}

> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	mfd_remove_devices(&interface->dev);

You need to make sure all I/O has stopped here.

> +	dln2_free(dln2);
> +}

Johan

Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Octavian Purdila]

On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:
>
> <snip>
>
>> +#define DLN2_GPIO_DIRECTION_IN               0
>> +#define DLN2_GPIO_DIRECTION_OUT              1
>> +
>> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>> +{
>> +     int ret;
>> +     struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
>> +     struct dln2_gpio_pin req = {
>> +             .pin = cpu_to_le16(offset),
>> +     };
>> +     struct dln2_gpio_pin_val rsp;
>> +     int len = sizeof(rsp);
>> +
>> +     if (test_bit(offset, dln2->pin_dir_set)) {
>> +             rsp.value = !!test_bit(offset, dln2->pin_dir);
>> +             goto report;
>> +     }
>> +
>> +     ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
>> +                         &req, sizeof(req), &rsp, &len);
>> +     if (ret < 0)
>> +             return ret;
>> +     if (len < sizeof(rsp) || req.pin != rsp.pin)
>> +             return -EPROTO;
>> +     set_bit(offset, dln2->pin_dir_set);
>
> You shouldn't set this flag until you've stored the direction.
>
>> +
>> +report:
>> +     switch (rsp.value) {
>> +     case DLN2_GPIO_DIRECTION_IN:
>> +             clear_bit(offset, dln2->pin_dir);
>> +             return 1;
>> +     case DLN2_GPIO_DIRECTION_OUT:
>> +             set_bit(offset, dln2->pin_dir);
>> +             return 0;
>> +     default:
>> +             return -EPROTO;
>> +     }
>> +}
>> +
>> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +     struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
>> +     int dir;
>> +
>> +     dir = dln2_gpio_get_direction(chip, offset);
>> +     if (dir < 0)
>> +             return dir;
>> +
>> +     if (dir)
>> +             return dln2_gpio_pin_get_in_val(dln2, offset);
>> +
>> +     return dln2_gpio_pin_get_out_val(dln2, offset);
>> +}
>> +
>> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> +{
>> +     struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
>> +
>> +     dln2_gpio_pin_set_out_val(dln2, offset, value);
>> +}
>> +
>> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
>> +                                unsigned dir)
>> +{
>> +     struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
>> +     struct dln2_gpio_pin_val req = {
>> +             .pin = cpu_to_le16(offset),
>> +             .value = cpu_to_le16(dir),
>> +     };
>> +
>> +     set_bit(offset, dln2->pin_dir_set);
>
> Set flag after you store the direction below.
>
>> +     if (dir == DLN2_GPIO_DIRECTION_OUT)
>> +             set_bit(offset, dln2->pin_dir);
>> +     else
>> +             clear_bit(offset, dln2->pin_dir);
>
> Either way, it looks like this could race with get_direction() if you
> get a set_direction() while get_direction() is retrieving the direction
> from the device.
>
> This would break gpio_get().
>

I don't think gpio_set_direction() and gpio_get() are allowed to race.
I think the user must make sure the right direction is set before
issuing gpio_get(), otherwise it can read invalid values even if
gpio_get() would not depend on get_direction().

I will fix the calls to pin_dir_set to be made after the pin_dir calls.

Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Johan Hovold]

On Thu, Sep 18, 2014 at 03:43:07PM +0300, Octavian Purdila wrote:
> On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:

> > Either way, it looks like this could race with get_direction() if you
> > get a set_direction() while get_direction() is retrieving the direction
> > from the device.
> >
> > This would break gpio_get().
> >
> I don't think gpio_set_direction() and gpio_get() are allowed to race.

I wrote that set_direction() and get_direction() could race, which in
turn would break gpio_get() as you would be caching the wrong
direction setting.

But perhaps gpiolib prevents this from happening.

Johan

Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Johan Hovold]

On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:

> +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
> +MODULE_DESCRIPTION(DRIVER_NAME " driver");
> +MODULE_LICENSE("GPL");

Just noticed both the i2c and gpio driver is lacking a MODULE_ALIAS to
enable module auto-loading.

Johan

Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Octavian Purdila]

On Thu, Sep 18, 2014 at 3:46 PM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Sep 18, 2014 at 03:43:07PM +0300, Octavian Purdila wrote:
>> On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:
>
>> > Either way, it looks like this could race with get_direction() if you
>> > get a set_direction() while get_direction() is retrieving the direction
>> > from the device.
>> >
>> > This would break gpio_get().
>> >
>> I don't think gpio_set_direction() and gpio_get() are allowed to race.
>
> I wrote that set_direction() and get_direction() could race, which in
> turn would break gpio_get() as you would be caching the wrong
> direction setting.
>

OK, I now see the problem. I think doing this in get_direction() will
fix the issue:

                if (!test_and_set_bit(offset, dln2->pin_dir_set))
                        set/clear_bit(offset, dln2->pin_dir);

because gpiolib calls get_direction() while requesting a pin and
request cannot race with itself. Which means that get_direction() can
not race with itself the first time it is called, when the set/clear
operation will be run.

And because we know that get_direction() is called first, we can even
remove the set/clear operation from set_direction().

Re: [PATCH v4 3/3] gpio: add support for the Diolan DLN-2 USB GPIO driver

[Johan Hovold]

On Thu, Sep 18, 2014 at 06:54:34PM +0300, Octavian Purdila wrote:
> On Thu, Sep 18, 2014 at 3:46 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Sep 18, 2014 at 03:43:07PM +0300, Octavian Purdila wrote:
> >> On Thu, Sep 18, 2014 at 1:54 PM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Tue, Sep 09, 2014 at 10:24:46PM +0300, Octavian Purdila wrote:
> >
> >> > Either way, it looks like this could race with get_direction() if you
> >> > get a set_direction() while get_direction() is retrieving the direction
> >> > from the device.
> >> >
> >> > This would break gpio_get().
> >> >
> >> I don't think gpio_set_direction() and gpio_get() are allowed to race.
> >
> > I wrote that set_direction() and get_direction() could race, which in
> > turn would break gpio_get() as you would be caching the wrong
> > direction setting.
> >
> 
> OK, I now see the problem. I think doing this in get_direction() will
> fix the issue:
> 
>                 if (!test_and_set_bit(offset, dln2->pin_dir_set))
>                         set/clear_bit(offset, dln2->pin_dir);
> 
> because gpiolib calls get_direction() while requesting a pin and
> request cannot race with itself. Which means that get_direction() can
> not race with itself the first time it is called, when the set/clear
> operation will be run.
> 
> And because we know that get_direction() is called first, we can even
> remove the set/clear operation from set_direction().

Why not simply fetch the direction in request() and get rid of the
pin_dir_set bitmask?

Johan