[PATCH v6 0/4] mfd: add support for Diolan DLN-2

[PATCH v6 0/4] 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 v5:

* MFD: use enum for handles, fix a couple of miss spells, rename a few
  fields for clarity, rework the disconnect mechanism, fix a couple of
  missing newlines in printks, use straight list_for_each instead of
  RCU versions in the update sides, used busnum << 8 | devnum as id in
  mfd_add_devices()

* GPIO: renamed pin_dir to output_enabled, disable GPIO if we failed
  getting the direction, don't set platform's driver owner field

* I2C: update Documentation/ABI for the added sysfs attributed, stored
  the port number in the dln2_i2c structure, used a temporary pointer
  to the tx/rx struct for readability, don't set platform's driver
  owner field

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 (2):
  mfd: add support for Diolan DLN-2 devices
  gpiolib: add irq_not_threaded flag to gpio_chip

 .../ABI/testing/sysfs-bus-i2c-busses-dln2          |   5 +
 drivers/gpio/Kconfig                               |  12 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-dln2.c                           | 556 +++++++++++++++
 drivers/gpio/gpiolib.c                             |   2 +-
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-dln2.c                      | 378 +++++++++++
 drivers/mfd/Kconfig                                |   9 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/dln2.c                                 | 752 +++++++++++++++++++++
 include/linux/gpio/driver.h                        |   3 +
 include/linux/mfd/dln2.h                           |  67 ++
 13 files changed, 1796 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
 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 v6 1/4] 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       | 751 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/dln2.h |  67 +++++
 4 files changed, 828 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..3ff8c6d
--- /dev/null
+++ b/drivers/mfd/dln2.c
@@ -0,0 +1,751 @@
+/*
+ * 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/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>
+#include <linux/rculist.h>
+
+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_URBS			16
+#define DLN2_RX_BUF_SIZE		512
+
+enum dln2_handle {
+	DLN2_HANDLE_EVENT,
+	DLN2_HANDLE_CTRL,
+	DLN2_HANDLE_GPIO,
+	DLN2_HANDLE_I2C,
+	DLN2_HANDLES
+};
+
+/*
+ * 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 {
+	/* completion used to wait for a response */
+	struct completion done;
+
+	/* if non-NULL the URB contains the response */
+	struct urb *urb;
+
+	/* if true then this context is used to wait for a response */
+	bool in_use;
+};
+
+/*
+ * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
+ * use the handle header field to identify 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 alloc/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_HANDLES];
+
+	struct list_head event_cb_list;
+	spinlock_t event_cb_lock;
+
+	bool disconnect;
+	int active_transfers;
+	wait_queue_head_t disconnect_wq;
+	spinlock_t disconnect_lock;
+};
+
+struct dln2_event_cb_entry {
+	struct list_head list;
+	u16 id;
+	struct platform_device *pdev;
+	dln2_event_cb_t callback;
+};
+
+int dln2_register_event_cb(struct platform_device *pdev, u16 id,
+			   dln2_event_cb_t rx_cb)
+{
+	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
+	struct dln2_event_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->event_cb_lock, flags);
+
+	list_for_each_entry(i, &dln2->event_cb_list, list) {
+		if (i->id == id) {
+			ret = -EBUSY;
+			break;
+		}
+	}
+
+	if (!ret)
+		list_add_rcu(&new->list, &dln2->event_cb_list);
+
+	spin_unlock_irqrestore(&dln2->event_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_event_cb_entry *i;
+	unsigned long flags;
+	bool found = false;
+
+	spin_lock_irqsave(&dln2->event_cb_lock, flags);
+
+	list_for_each_entry(i, &dln2->event_cb_list, list) {
+		if (i->id == id) {
+			list_del_rcu(&i->list);
+			found = true;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
+
+	if (found) {
+		synchronize_rcu();
+		kfree(i);
+	}
+}
+EXPORT_SYMBOL(dln2_unregister_event_cb);
+
+static int dln2_submit_urb(struct dln2_dev *dln2, struct urb *urb, gfp_t gfp)
+{
+	int ret;
+	struct device *dev = &dln2->interface->dev;
+
+	ret = usb_submit_urb(urb, gfp);
+	if (ret < 0)
+		dev_err(dev, "failed to (re)submit RX URB: %d\n", ret);
+	return ret;
+}
+
+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;
+
+	spin_lock(&rxs->lock);
+
+	rxc = &rxs->slots[rx_slot];
+
+	if (rxc->in_use && !rxc->urb) {
+		rxc->urb = urb;
+		complete(&rxc->done);
+		/* URB will be resubmitted in free_rx_slot */
+	} else {
+		dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
+		dln2_submit_urb(dln2, urb, GFP_ATOMIC);
+	}
+
+	spin_unlock(&rxs->lock);
+}
+
+static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
+				  void *data, int len)
+{
+	struct dln2_event_cb_entry *i;
+
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
+		if (i->id == id)
+			i->callback(i->pdev, echo, data, len);
+
+	rcu_read_unlock();
+}
+
+static void dln2_rx(struct urb *urb)
+{
+	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_HANDLES) {
+		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);
+	} else {
+		dln2_rx_transfer(dln2, urb, handle, echo);
+		/* URB will be re-submitted in dln2_rx_transfer */
+		return;
+	}
+
+out:
+	dln2_submit_urb(dln2, urb, GFP_ATOMIC);
+}
+
+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 bool find_free_slot(struct dln2_dev *dln2, u16 handle, int *slot)
+{
+	struct dln2_mod_rx_slots *rxs;
+	unsigned long flags;
+
+	rxs = &dln2->mod_rx_slots[handle];
+
+	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->in_use = true;
+	}
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	return *slot < DLN2_MAX_RX_SLOTS;
+}
+
+static int alloc_rx_slot(struct dln2_dev *dln2, u16 handle)
+{
+	int ret;
+	int slot;
+
+	/*
+	 * No need to timeout here, the wait is bounded by the timeout
+	 * in _dln2_transfer.
+	 */
+	ret = wait_event_interruptible(dln2->mod_rx_slots[handle].wq,
+				       find_free_slot(dln2, handle, &slot));
+	if (ret < 0)
+		return ret;
+
+	return slot;
+}
+
+static void free_rx_slot(struct dln2_dev *dln2, u16 handle, int slot)
+{
+	struct dln2_mod_rx_slots *rxs;
+	struct urb *urb = NULL;
+	unsigned long flags;
+	struct dln2_rx_context *rxc;
+
+	rxs = &dln2->mod_rx_slots[handle];
+
+	spin_lock_irqsave(&rxs->lock, flags);
+
+	clear_bit(slot, &rxs->bmap);
+
+	rxc = &rxs->slots[slot];
+	rxc->in_use = false;
+	urb = rxc->urb;
+	rxc->urb = NULL;
+	reinit_completion(&rxc->done);
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	if (urb)
+		dln2_submit_urb(dln2, urb, GFP_KERNEL);
+
+	wake_up_interruptible(&rxs->wq);
+}
+
+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;
+	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
+	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
+
+	spin_lock(&dln2->disconnect_lock);
+	if (!dln2->disconnect)
+		dln2->active_transfers++;
+	else
+		ret = -ENODEV;
+	spin_unlock(&dln2->disconnect_lock);
+
+	if (ret)
+		return ret;
+
+	rx_slot = alloc_rx_slot(dln2, handle);
+	if (rx_slot < 0) {
+		ret = rx_slot;
+		goto out_decr;
+	}
+
+	dev = &dln2->interface->dev;
+
+	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
+	if (ret < 0) {
+		dev_err(dev, "USB write failed: %d\n", ret);
+		goto out_free_rx_slot;
+	}
+
+	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 (!rxc->urb) {
+		ret = -ENODEV;
+		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, handle, rx_slot);
+out_decr:
+	spin_lock(&dln2->disconnect_lock);
+	dln2->active_transfers--;
+	spin_unlock(&dln2->disconnect_lock);
+	if (dln2->disconnect)
+		wake_up(&dln2->disconnect_wq);
+
+	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;
+
+	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\n",
+			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;
+
+	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 = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct dln2_platform_data dln2_pdata_gpio = {
+	.handle = DLN2_HANDLE_GPIO,
+};
+
+/* Only one I2C port seems to be supported on current hardware */
+static struct dln2_platform_data dln2_pdata_i2c = {
+	.handle = DLN2_HANDLE_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);
+	int i, j;
+
+	/* don't allow starting new transfers */
+	spin_lock(&dln2->disconnect_lock);
+	dln2->disconnect = true;
+	spin_unlock(&dln2->disconnect_lock);
+
+	/* cancel in progress transfers */
+	for (i = 0; i < DLN2_HANDLES; i++) {
+		struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
+		unsigned long flags;
+
+		spin_lock_irqsave(&rxs->lock, flags);
+
+		/* cancel all response waiters */
+		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
+			struct dln2_rx_context *rxc = &rxs->slots[j];
+
+			if (rxc->in_use)
+				complete(&rxc->done);
+		}
+
+		spin_unlock_irqrestore(&rxs->lock, flags);
+	}
+
+	/* wait for transfers to end */
+	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
+
+	mfd_remove_devices(&interface->dev);
+
+	dln2_free(dln2);
+}
+
+static int dln2_probe(struct usb_interface *interface,
+		      const struct usb_device_id *usb_id)
+{
+	struct usb_host_interface *hostif = interface->cur_altsetting;
+	struct device *dev = &interface->dev;
+	struct dln2_dev *dln2;
+	int ret;
+	int i, j;
+	int id;
+
+	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);
+	init_waitqueue_head(&dln2->disconnect_wq);
+
+	for (i = 0; i < DLN2_HANDLES; 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->event_cb_lock);
+	INIT_LIST_HEAD(&dln2->event_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;
+	}
+
+	id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
+	ret = mfd_add_devices(dev, id, 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 = "usb-dln2",
+	.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("Core driver for the Diolan DLN2 interface adapter");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
new file mode 100644
index 0000000..0b0d822
--- /dev/null
+++ b/include/linux/mfd/dln2.h
@@ -0,0 +1,67 @@
+#ifndef __LINUX_USB_DLN2_H
+#define __LINUX_USB_DLN2_H
+
+#define DLN2_CMD(cmd, id)		((cmd) | ((id) << 8))
+
+struct dln2_platform_data {
+	u16 handle;		/* sub-driver handle (internally used only) */
+	u8 port;		/* I2C/SPI port */
+};
+
+/**
+ * dln2_event_cb_t - 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_event_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
+ * @event_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_event_cb_t event_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 v6 2/4] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Octavian Purdila]

From: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 .../ABI/testing/sysfs-bus-i2c-busses-dln2          |   5 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-dln2.c                      | 378 +++++++++++++++++++++
 4 files changed, 394 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
 create mode 100644 drivers/i2c/busses/i2c-dln2.c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
new file mode 100644
index 0000000..ad55af6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
@@ -0,0 +1,5 @@
+What:		/sys/bus/i2c/devices/.../dln2_i2c_freq
+Date:		September 2014
+Contact:	Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+Description:
+		This attribute shows/sets the frequency (in Hz) of the I2C bus.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..6afc17e 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1021,4 +1021,14 @@ config SCx200_ACB
 	  This support is also available as a module.  If so, the module
 	  will be called scx200_acb.
 
+config I2C_DLN2
+       tristate "Diolan DLN-2 USB I2C adapter"
+       depends on MFD_DLN2
+       help
+         If you say yes to this option, support will be included for Diolan
+         DLN2, a USB to I2C interface.
+
+         This driver can also be built as a module.  If so, the module
+         will be called i2c-dln2.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..3118fea 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
+obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
new file mode 100644
index 0000000..15b7f35b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -0,0 +1,378 @@
+/*
+ * Driver for the Diolan DLN-2 USB-I2C adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * Derived from:
+ *  i2c-diolan-u2c.c
+ *  Copyright (c) 2010-2011 Ericsson AB
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+
+#define DLN2_I2C_MODULE_ID		0x03
+#define DLN2_I2C_CMD(cmd)		DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
+
+/* I2C commands */
+#define DLN2_I2C_GET_PORT_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
+#define DLN2_I2C_BUF_SIZE		(DLN2_I2C_MAX_XFER_SIZE + 16)
+
+struct dln2_i2c {
+	struct platform_device *pdev;
+	struct i2c_adapter adapter;
+	u32 freq;
+	u32 min_freq;
+	u32 max_freq;
+	u8 port;
+	/*
+	 * Buffer to hold the packet for read or write transfers. One
+	 * is enough since we can't have multiple transfers in
+	 * parallel on the i2c adapter.
+	 */
+	void *buf;
+};
+
+static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
+{
+	int ret;
+	u16 cmd;
+
+	if (enable)
+		cmd = DLN2_I2C_ENABLE;
+	else
+		cmd = DLN2_I2C_DISABLE;
+
+	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
+			    NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
+{
+	int ret;
+	struct tx_data {
+		u8 port;
+		__le32 freq;
+	} __packed tx;
+
+	tx.port = dln2->port;
+	tx.freq = cpu_to_le32(freq);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
+			    NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	dln2->freq = freq;
+
+	return 0;
+}
+
+static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
+{
+	int ret;
+	__le32 freq;
+	unsigned len = sizeof(freq);
+
+	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
+			    &freq, &len);
+	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 ret;
+
+	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MAX_FREQUENCY,
+				&dln2->max_freq);
+	if (ret < 0)
+		return ret;
+
+	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)
+{
+	int ret;
+	struct {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed *tx = dln2->buf;
+	unsigned len;
+
+	BUILD_BUG_ON(sizeof(*tx) > DLN2_I2C_BUF_SIZE);
+
+	tx->port = dln2->port;
+	tx->addr = addr;
+	tx->mem_addr_len = 0;
+	tx->mem_addr = 0;
+	tx->buf_len = cpu_to_le16(data_len);
+	memcpy(tx->buf, data, data_len);
+
+	len = sizeof(*tx) + data_len - DLN2_I2C_MAX_XFER_SIZE;
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_WRITE, 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)
+{
+	int ret;
+	struct {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+	} __packed tx;
+	struct {
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+
+	BUILD_BUG_ON(sizeof(*rx) > DLN2_I2C_BUF_SIZE);
+
+	tx.port = dln2->port;
+	tx.addr = addr;
+	tx.mem_addr_len = 0;
+	tx.mem_addr = 0;
+	tx.buf_len = cpu_to_le16(data_len);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_READ, &tx, sizeof(tx),
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(rx->buf_len) + data_len)
+		return -EPROTO;
+	if (le16_to_cpu(rx->buf_len) != data_len)
+		return -EPROTO;
+
+	memcpy(data, 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_RW(dln2_i2c_freq);
+static struct attribute *dln2_i2c_freq_attrs[] = {
+	&dev_attr_dln2_i2c_freq.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(dln2_i2c_freq);
+
+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->buf = devm_kmalloc(dev, DLN2_I2C_BUF_SIZE, GFP_KERNEL);
+	if (!dln2->buf)
+		return -ENOMEM;
+
+	dln2->pdev = pdev;
+	dln2->port = pdata->port;
+
+	/* 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;
+	dln2->adapter.dev.groups = dln2_i2c_freq_groups;
+	i2c_set_adapdata(&dln2->adapter, dln2);
+	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), "%s-%s-%d",
+		 "dln2-i2c", dev_name(pdev->dev.parent), dln2->port);
+
+	platform_set_drvdata(pdev, dln2);
+
+	/* initialize the i2c interface */
+	ret = dln2_i2c_setup(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize adapter: %d\n", ret);
+		return ret;
+	}
+
+	/* 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);
+
+	return ret;
+}
+
+static int dln2_i2c_remove(struct platform_device *pdev)
+{
+	struct dln2_i2c *dln2 = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dln2->adapter);
+	dln2_i2c_enable(dln2, false);
+
+	return 0;
+}
+
+static struct platform_driver dln2_i2c_driver = {
+	.driver.name	= "dln2-i2c",
+	.probe		= dln2_i2c_probe,
+	.remove		= dln2_i2c_remove,
+};
+
+module_platform_driver(dln2_i2c_driver);
+
+MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Driver for the Diolan DLN2 I2C master interface");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dln2-i2c");
-- 
1.9.1

[PATCH v6 2/4] 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>
---
 .../ABI/testing/sysfs-bus-i2c-busses-dln2          |   5 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-dln2.c                      | 378 +++++++++++++++++++++
 4 files changed, 394 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
 create mode 100644 drivers/i2c/busses/i2c-dln2.c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
new file mode 100644
index 0000000..ad55af6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
@@ -0,0 +1,5 @@
+What:		/sys/bus/i2c/devices/.../dln2_i2c_freq
+Date:		September 2014
+Contact:	Octavian Purdila <octavian.purdila@intel.com>
+Description:
+		This attribute shows/sets the frequency (in Hz) of the I2C bus.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..6afc17e 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1021,4 +1021,14 @@ config SCx200_ACB
 	  This support is also available as a module.  If so, the module
 	  will be called scx200_acb.
 
+config I2C_DLN2
+       tristate "Diolan DLN-2 USB I2C adapter"
+       depends on MFD_DLN2
+       help
+         If you say yes to this option, support will be included for Diolan
+         DLN2, a USB to I2C interface.
+
+         This driver can also be built as a module.  If so, the module
+         will be called i2c-dln2.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..3118fea 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
+obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-dln2.c b/drivers/i2c/busses/i2c-dln2.c
new file mode 100644
index 0000000..15b7f35b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -0,0 +1,378 @@
+/*
+ * Driver for the Diolan DLN-2 USB-I2C adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * Derived from:
+ *  i2c-diolan-u2c.c
+ *  Copyright (c) 2010-2011 Ericsson AB
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+
+#define DLN2_I2C_MODULE_ID		0x03
+#define DLN2_I2C_CMD(cmd)		DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
+
+/* I2C commands */
+#define DLN2_I2C_GET_PORT_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
+#define DLN2_I2C_BUF_SIZE		(DLN2_I2C_MAX_XFER_SIZE + 16)
+
+struct dln2_i2c {
+	struct platform_device *pdev;
+	struct i2c_adapter adapter;
+	u32 freq;
+	u32 min_freq;
+	u32 max_freq;
+	u8 port;
+	/*
+	 * Buffer to hold the packet for read or write transfers. One
+	 * is enough since we can't have multiple transfers in
+	 * parallel on the i2c adapter.
+	 */
+	void *buf;
+};
+
+static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
+{
+	int ret;
+	u16 cmd;
+
+	if (enable)
+		cmd = DLN2_I2C_ENABLE;
+	else
+		cmd = DLN2_I2C_DISABLE;
+
+	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
+			    NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
+{
+	int ret;
+	struct tx_data {
+		u8 port;
+		__le32 freq;
+	} __packed tx;
+
+	tx.port = dln2->port;
+	tx.freq = cpu_to_le32(freq);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
+			    NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	dln2->freq = freq;
+
+	return 0;
+}
+
+static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
+{
+	int ret;
+	__le32 freq;
+	unsigned len = sizeof(freq);
+
+	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
+			    &freq, &len);
+	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 ret;
+
+	ret = dln2_i2c_get_freq(dln2, DLN2_I2C_GET_MAX_FREQUENCY,
+				&dln2->max_freq);
+	if (ret < 0)
+		return ret;
+
+	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)
+{
+	int ret;
+	struct {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed *tx = dln2->buf;
+	unsigned len;
+
+	BUILD_BUG_ON(sizeof(*tx) > DLN2_I2C_BUF_SIZE);
+
+	tx->port = dln2->port;
+	tx->addr = addr;
+	tx->mem_addr_len = 0;
+	tx->mem_addr = 0;
+	tx->buf_len = cpu_to_le16(data_len);
+	memcpy(tx->buf, data, data_len);
+
+	len = sizeof(*tx) + data_len - DLN2_I2C_MAX_XFER_SIZE;
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_WRITE, 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)
+{
+	int ret;
+	struct {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+	} __packed tx;
+	struct {
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+
+	BUILD_BUG_ON(sizeof(*rx) > DLN2_I2C_BUF_SIZE);
+
+	tx.port = dln2->port;
+	tx.addr = addr;
+	tx.mem_addr_len = 0;
+	tx.mem_addr = 0;
+	tx.buf_len = cpu_to_le16(data_len);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_I2C_READ, &tx, sizeof(tx),
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(rx->buf_len) + data_len)
+		return -EPROTO;
+	if (le16_to_cpu(rx->buf_len) != data_len)
+		return -EPROTO;
+
+	memcpy(data, 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_RW(dln2_i2c_freq);
+static struct attribute *dln2_i2c_freq_attrs[] = {
+	&dev_attr_dln2_i2c_freq.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(dln2_i2c_freq);
+
+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->buf = devm_kmalloc(dev, DLN2_I2C_BUF_SIZE, GFP_KERNEL);
+	if (!dln2->buf)
+		return -ENOMEM;
+
+	dln2->pdev = pdev;
+	dln2->port = pdata->port;
+
+	/* 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;
+	dln2->adapter.dev.groups = dln2_i2c_freq_groups;
+	i2c_set_adapdata(&dln2->adapter, dln2);
+	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), "%s-%s-%d",
+		 "dln2-i2c", dev_name(pdev->dev.parent), dln2->port);
+
+	platform_set_drvdata(pdev, dln2);
+
+	/* initialize the i2c interface */
+	ret = dln2_i2c_setup(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize adapter: %d\n", ret);
+		return ret;
+	}
+
+	/* 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);
+
+	return ret;
+}
+
+static int dln2_i2c_remove(struct platform_device *pdev)
+{
+	struct dln2_i2c *dln2 = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dln2->adapter);
+	dln2_i2c_enable(dln2, false);
+
+	return 0;
+}
+
+static struct platform_driver dln2_i2c_driver = {
+	.driver.name	= "dln2-i2c",
+	.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 for the Diolan DLN2 I2C master interface");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dln2-i2c");
-- 
1.9.1

[PATCH v6 3/4] gpiolib: add irq_not_threaded flag to gpio_chip

[Octavian Purdila]

Some GPIO chips (e.g. the DLN2 USB adapter) have blocking get/set
operation but do not need a threaded irq handler.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/gpiolib.c      | 2 +-
 include/linux/gpio/driver.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 15cc0bb..3fa7e73 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -447,7 +447,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
 	irq_set_lockdep_class(irq, &gpiochip_irq_lock_class);
 	irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler);
 	/* Chips that can sleep need nested thread handlers */
-	if (chip->can_sleep)
+	if (chip->can_sleep && !chip->irq_not_threaded)
 		irq_set_nested_thread(irq, 1);
 #ifdef CONFIG_ARM
 	set_irq_flags(irq, IRQF_VALID);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e78a237..44161ac 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -56,6 +56,8 @@ struct seq_file;
  *	as the chip access may sleep when e.g. reading out the IRQ status
  *	registers.
  * @exported: flags if the gpiochip is exported for use from sysfs. Private.
+ * @irq_not_threaded: flag must be set if @can_sleep is set but the
+ *	IRQs don't need to be threaded
  *
  * A gpio_chip can help platforms abstract various sources of GPIOs so
  * they can all be accessed through a common programing interface.
@@ -101,6 +103,7 @@ struct gpio_chip {
 	struct gpio_desc	*desc;
 	const char		*const *names;
 	bool			can_sleep;
+	bool			irq_not_threaded;
 	bool			exported;
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
-- 
1.9.1

[PATCH v6 4/4] 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 | 555 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 568 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..c047f3e
--- /dev/null
+++ b/drivers/gpio/gpio-dln2.c
@@ -0,0 +1,555 @@
+/*
+ * 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/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 DLN2_GPIO_ID			0x01
+
+#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(output_enabled, 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;
+};
+
+struct dln2_gpio_pin {
+	__le16 pin;
+};
+
+struct dln2_gpio_pin_val {
+	__le16 pin __packed;
+	u8 value;
+};
+
+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);
+}
+
+#define DLN2_GPIO_DIRECTION_IN		0
+#define DLN2_GPIO_DIRECTION_OUT		1
+
+static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	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);
+	int ret;
+
+	ret = dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
+	if (ret < 0)
+		return ret;
+
+	/* cache the pin direction */
+	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) {
+		ret = -EPROTO;
+		goto out_disable;
+	}
+
+	switch (rsp.value) {
+	case DLN2_GPIO_DIRECTION_IN:
+		clear_bit(offset, dln2->output_enabled);
+		return 0;
+	case DLN2_GPIO_DIRECTION_OUT:
+		set_bit(offset, dln2->output_enabled);
+		return 0;
+	default:
+		ret = -EPROTO;
+		goto out_disable;
+	}
+
+out_disable:
+	dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
+	return ret;
+}
+
+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);
+}
+
+static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	if (test_bit(offset, dln2->output_enabled))
+		return GPIOF_DIR_OUT;
+
+	return GPIOF_DIR_IN;
+}
+
+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 == GPIOF_DIR_IN)
+		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),
+	};
+	int ret;
+
+	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
+			    &req, sizeof(req), NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	if (dir == DLN2_GPIO_DIRECTION_OUT)
+		set_bit(offset, dln2->output_enabled);
+	else
+		clear_bit(offset, dln2->output_enabled);
+
+	return ret;
+}
+
+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);
+	__le32 duration = cpu_to_le32(debounce);
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
+			     &duration, sizeof(duration), 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);
+	struct device *dev = dln2->gpio.dev;
+	int pin = irqd_to_hwirq(irqd);
+
+	if (test_and_clear_bit(pin, dln2->irqs_pending)) {
+		int irq;
+
+		irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
+		if (!irq) {
+			dev_err(dev, "pin %d not mapped to IRQ\n", pin);
+			return;
+		}
+
+		generic_handle_irq(irq);
+	}
+}
+
+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);
+	if (pin >= dln2->gpio.ngpio) {
+		dev_err(dln2->gpio.dev, "out of bounds pin %d\n", pin);
+		return;
+	}
+
+	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;
+	int i, ret;
+
+	pins = dln2_gpio_get_pin_count(pdev);
+	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;
+
+	dln2->irq_work = devm_kcalloc(&pdev->dev, pins,
+				      sizeof(struct dln2_irq_work), GFP_KERNEL);
+	if (!dln2->irq_work)
+		return -ENOMEM;
+	for (i = 0; i < 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 = true;
+	dln2->gpio.can_sleep = true;
+	dln2->gpio.irq_not_threaded = true;
+	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);
+	int i;
+
+	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
+	for (i = 0; i < dln2->gpio.ngpio; i++)
+		flush_work(&dln2->irq_work[i].work);
+	gpiochip_remove(&dln2->gpio);
+
+	return 0;
+}
+
+static struct platform_driver dln2_gpio_driver = {
+	.driver.name	= "dln2-gpio",
+	.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 for the Diolan DLN2 GPIO interface");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dln2-gpio");
-- 
1.9.1

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

Hi,

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..6afc17e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1021,4 +1021,14 @@ config SCx200_ACB
>  	  This support is also available as a module.  If so, the module
>  	  will be called scx200_acb.
>  
> +config I2C_DLN2
> +       tristate "Diolan DLN-2 USB I2C adapter"
> +       depends on MFD_DLN2
> +       help
> +         If you say yes to this option, support will be included for Diolan
> +         DLN2, a USB to I2C interface.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-dln2.
> +

Please keep to the existing sorting.


>  endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..3118fea 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
>  obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
> +obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o

Ditto!

> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;

Ternary operator?

> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)

Here you have 'set_frequency'...

> +{
> +	int ret;
> +	struct tx_data {
> +		u8 port;
> +		__le32 freq;
> +	} __packed tx;
> +
> +	tx.port = dln2->port;
> +	tx.freq = cpu_to_le32(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	dln2->freq = freq;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)

... here 'get_freq'. Please keep it consistent.

> +{
> +	int ret;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    &freq, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(freq))
> +		return -EPROTO;
> +
> +	*res = le32_to_cpu(freq);
> +
> +	return 0;
> +}

...

> +
> +static int dln2_i2c_xfer(struct i2c_adapter *adapter,
> +			 struct i2c_msg *msgs, int num)
> +{
> +	struct dln2_i2c *dln2 = i2c_get_adapdata(adapter);
> +	struct i2c_msg *pmsg;
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		int ret;
> +
> +		pmsg = &msgs[i];
> +
> +		if (pmsg->len > DLN2_I2C_MAX_XFER_SIZE)
> +			return -EINVAL;

Rather -EOPNOTSUPP. And we should add a warning here since I2C transfers
can be bigger than 256 byte and this is a flaw of the controller.

> +
> +		if (pmsg->flags & I2C_M_RD) {
> +			ret = dln2_i2c_read(dln2, pmsg->addr, pmsg->buf,
> +					    pmsg->len);
> +			if (ret < 0)
> +				return ret;
> +
> +			pmsg->len = ret;
> +		} else {
> +			ret = dln2_i2c_write(dln2, pmsg->addr, pmsg->buf,
> +					     pmsg->len);
> +			if (ret != pmsg->len)
> +				return -EPROTO;
> +		}
> +	}
> +
> +	return num;
> +}
> +

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

Hi,

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..6afc17e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1021,4 +1021,14 @@ config SCx200_ACB
>  	  This support is also available as a module.  If so, the module
>  	  will be called scx200_acb.
>  
> +config I2C_DLN2
> +       tristate "Diolan DLN-2 USB I2C adapter"
> +       depends on MFD_DLN2
> +       help
> +         If you say yes to this option, support will be included for Diolan
> +         DLN2, a USB to I2C interface.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-dln2.
> +

Please keep to the existing sorting.


>  endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..3118fea 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
>  obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
> +obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o

Ditto!

> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;

Ternary operator?

> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)

Here you have 'set_frequency'...

> +{
> +	int ret;
> +	struct tx_data {
> +		u8 port;
> +		__le32 freq;
> +	} __packed tx;
> +
> +	tx.port = dln2->port;
> +	tx.freq = cpu_to_le32(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	dln2->freq = freq;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)

... here 'get_freq'. Please keep it consistent.

> +{
> +	int ret;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    &freq, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(freq))
> +		return -EPROTO;
> +
> +	*res = le32_to_cpu(freq);
> +
> +	return 0;
> +}

...

> +
> +static int dln2_i2c_xfer(struct i2c_adapter *adapter,
> +			 struct i2c_msg *msgs, int num)
> +{
> +	struct dln2_i2c *dln2 = i2c_get_adapdata(adapter);
> +	struct i2c_msg *pmsg;
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		int ret;
> +
> +		pmsg = &msgs[i];
> +
> +		if (pmsg->len > DLN2_I2C_MAX_XFER_SIZE)
> +			return -EINVAL;

Rather -EOPNOTSUPP. And we should add a warning here since I2C transfers
can be bigger than 256 byte and this is a flaw of the controller.

> +
> +		if (pmsg->flags & I2C_M_RD) {
> +			ret = dln2_i2c_read(dln2, pmsg->addr, pmsg->buf,
> +					    pmsg->len);
> +			if (ret < 0)
> +				return ret;
> +
> +			pmsg->len = ret;
> +		} else {
> +			ret = dln2_i2c_write(dln2, pmsg->addr, pmsg->buf,
> +					     pmsg->len);
> +			if (ret != pmsg->len)
> +				return -EPROTO;
> +		}
> +	}
> +
> +	return num;
> +}
> +

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Octavian Purdila]

On Fri, Oct 3, 2014 at 4:14 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> Hi,
>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 2ac87fa..6afc17e 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1021,4 +1021,14 @@ config SCx200_ACB
>>         This support is also available as a module.  If so, the module
>>         will be called scx200_acb.
>>
>> +config I2C_DLN2
>> +       tristate "Diolan DLN-2 USB I2C adapter"
>> +       depends on MFD_DLN2
>> +       help
>> +         If you say yes to this option, support will be included for Diolan
>> +         DLN2, a USB to I2C interface.
>> +
>> +         This driver can also be built as a module.  If so, the module
>> +         will be called i2c-dln2.
>> +
>
> Please keep to the existing sorting.
>
>
>>  endmenu
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 49bf07e..3118fea 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
>>  obj-$(CONFIG_I2C_PCA_ISA)    += i2c-pca-isa.o
>>  obj-$(CONFIG_I2C_SIBYTE)     += i2c-sibyte.o
>>  obj-$(CONFIG_SCx200_ACB)     += scx200_acb.o
>> +obj-$(CONFIG_I2C_DLN2)               += i2c-dln2.o
>
> Ditto!
>
>> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
>> +{
>> +     int ret;
>> +     u16 cmd;
>> +
>> +     if (enable)
>> +             cmd = DLN2_I2C_ENABLE;
>> +     else
>> +             cmd = DLN2_I2C_DISABLE;
>
> Ternary operator?
>

In the first versions of the patch it was used, but Johan suggested to avoid it.

I have addressed the rest of you comments, thanks for the review.

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

[Octavian Purdila]

On Fri, Oct 3, 2014 at 4:14 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hi,
>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 2ac87fa..6afc17e 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1021,4 +1021,14 @@ config SCx200_ACB
>>         This support is also available as a module.  If so, the module
>>         will be called scx200_acb.
>>
>> +config I2C_DLN2
>> +       tristate "Diolan DLN-2 USB I2C adapter"
>> +       depends on MFD_DLN2
>> +       help
>> +         If you say yes to this option, support will be included for Diolan
>> +         DLN2, a USB to I2C interface.
>> +
>> +         This driver can also be built as a module.  If so, the module
>> +         will be called i2c-dln2.
>> +
>
> Please keep to the existing sorting.
>
>
>>  endmenu
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 49bf07e..3118fea 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -100,5 +100,6 @@ obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
>>  obj-$(CONFIG_I2C_PCA_ISA)    += i2c-pca-isa.o
>>  obj-$(CONFIG_I2C_SIBYTE)     += i2c-sibyte.o
>>  obj-$(CONFIG_SCx200_ACB)     += scx200_acb.o
>> +obj-$(CONFIG_I2C_DLN2)               += i2c-dln2.o
>
> Ditto!
>
>> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
>> +{
>> +     int ret;
>> +     u16 cmd;
>> +
>> +     if (enable)
>> +             cmd = DLN2_I2C_ENABLE;
>> +     else
>> +             cmd = DLN2_I2C_DISABLE;
>
> Ternary operator?
>

In the first versions of the patch it was used, but Johan suggested to avoid it.

I have addressed the rest of you comments, thanks for the review.

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:31PM +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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mfd/Kconfig      |   9 +
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/dln2.c       | 751 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h |  67 +++++
>  4 files changed, 828 insertions(+)
>  create mode 100644 drivers/mfd/dln2.c
>  create mode 100644 include/linux/mfd/dln2.h

[...]

> +#define DLN2_HW_ID			0x200
> +#define DLN2_USB_TIMEOUT		200	/* in ms */
> +#define DLN2_MAX_RX_SLOTS		16
> +#define DLN2_MAX_URBS			16
> +#define DLN2_RX_BUF_SIZE		512
> +
> +enum dln2_handle {
> +	DLN2_HANDLE_EVENT,

This is the only handle that was fixed (0), right? Initialise this one
explicitly in case any one ever tries to reorder them.

> +	DLN2_HANDLE_CTRL,
> +	DLN2_HANDLE_GPIO,
> +	DLN2_HANDLE_I2C,
> +	DLN2_HANDLES
> +};
> +
> +/*
> + * 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 {
> +	/* completion used to wait for a response */
> +	struct completion done;
> +
> +	/* if non-NULL the URB contains the response */
> +	struct urb *urb;
> +
> +	/* if true then this context is used to wait for a response */
> +	bool in_use;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to identify 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 alloc/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_HANDLES];
> +
> +	struct list_head event_cb_list;
> +	spinlock_t event_cb_lock;
> +
> +	bool disconnect;
> +	int active_transfers;
> +	wait_queue_head_t disconnect_wq;
> +	spinlock_t disconnect_lock;
> +};
> +
> +struct dln2_event_cb_entry {
> +	struct list_head list;
> +	u16 id;
> +	struct platform_device *pdev;
> +	dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +			   dln2_event_cb_t rx_cb)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_event_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->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +
> +	if (!ret)
> +		list_add_rcu(&new->list, &dln2->event_cb_list);
> +
> +	spin_unlock_irqrestore(&dln2->event_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_event_cb_entry *i;
> +	unsigned long flags;
> +	bool found = false;
> +
> +	spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			list_del_rcu(&i->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> +	if (found) {
> +		synchronize_rcu();
> +		kfree(i);
> +	}
> +}
> +EXPORT_SYMBOL(dln2_unregister_event_cb);
> +
> +static int dln2_submit_urb(struct dln2_dev *dln2, struct urb *urb, gfp_t gfp)
> +{
> +	int ret;
> +	struct device *dev = &dln2->interface->dev;
> +
> +	ret = usb_submit_urb(urb, gfp);
> +	if (ret < 0)
> +		dev_err(dev, "failed to (re)submit RX URB: %d\n", ret);
> +	return ret;
> +}
> +
> +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;
> +
> +	spin_lock(&rxs->lock);
> +
> +	rxc = &rxs->slots[rx_slot];

This can be done outside the lock, right?

> +
> +	if (rxc->in_use && !rxc->urb) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +		/* URB will be resubmitted in free_rx_slot */
> +	} else {
> +		dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> +		dln2_submit_urb(dln2, urb, GFP_ATOMIC);

And so can the resubmission.

I think this might be easier to follow if you just do this inline in the
completion handler, and always resubmit there before returning (unless
the slot is in use).

> +	}
> +
> +	spin_unlock(&rxs->lock);
> +}
> +
> +static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> +				  void *data, int len)

Rename this one dln2_run_event_callbacks to match your new names (much
better by the way)?

> +{
> +	struct dln2_event_cb_entry *i;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
> +		if (i->id == id)
> +			i->callback(i->pdev, echo, data, len);
> +
> +	rcu_read_unlock();
> +}
> +
> +static void dln2_rx(struct urb *urb)
> +{
> +	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_HANDLES) {
> +		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);
> +	} else {
> +		dln2_rx_transfer(dln2, urb, handle, echo);
> +		/* URB will be re-submitted in dln2_rx_transfer */

This comment is a little misleading since the urb will not be
resubmitted if the corresponding slot is in use. See my comment to
dln2_rx_transfer above.

> +		return;
> +	}
> +
> +out:
> +	dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> +}
> +
> +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 bool find_free_slot(struct dln2_dev *dln2, u16 handle, int *slot)
> +{
> +	struct dln2_mod_rx_slots *rxs;
> +	unsigned long flags;
> +

You could still check the global disconnect flag here to speed up
disconnect (locking not needed). Return -ENODEV as I mentioned earlier.

> +	rxs = &dln2->mod_rx_slots[handle];
> +
> +	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->in_use = true;
> +	}
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	return *slot < DLN2_MAX_RX_SLOTS;
> +}
> +
> +static int alloc_rx_slot(struct dln2_dev *dln2, u16 handle)
> +{
> +	int ret;
> +	int slot;
> +
> +	/*
> +	 * No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer.
> +	 */
> +	ret = wait_event_interruptible(dln2->mod_rx_slots[handle].wq,
> +				       find_free_slot(dln2, handle, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, u16 handle, int slot)
> +{
> +	struct dln2_mod_rx_slots *rxs;
> +	struct urb *urb = NULL;
> +	unsigned long flags;
> +	struct dln2_rx_context *rxc;
> +
> +	rxs = &dln2->mod_rx_slots[handle];
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	clear_bit(slot, &rxs->bmap);
> +
> +	rxc = &rxs->slots[slot];
> +	rxc->in_use = false;
> +	urb = rxc->urb;
> +	rxc->urb = NULL;
> +	reinit_completion(&rxc->done);
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	if (urb)
> +		dln2_submit_urb(dln2, urb, GFP_KERNEL);
> +
> +	wake_up_interruptible(&rxs->wq);
> +}
> +
> +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;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> +	spin_lock(&dln2->disconnect_lock);
> +	if (!dln2->disconnect)
> +		dln2->active_transfers++;
> +	else
> +		ret = -ENODEV;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	rx_slot = alloc_rx_slot(dln2, handle);
> +	if (rx_slot < 0) {
> +		ret = rx_slot;
> +		goto out_decr;
> +	}
> +
> +	dev = &dln2->interface->dev;

This can be done when declaring dev.

> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		dev_err(dev, "USB write failed: %d\n", ret);
> +		goto out_free_rx_slot;
> +	}
> +
> +	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 (!rxc->urb) {
> +		ret = -ENODEV;
> +		goto out_free_rx_slot;
> +	}

This can only happen if disconnected, right? Perhaps add a comment
unless you want to the test the disconnected flag instead which would
make it self-explanatory.

> +
> +	/* 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, handle, rx_slot);
> +out_decr:
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->active_transfers--;
> +	spin_unlock(&dln2->disconnect_lock);
> +	if (dln2->disconnect)
> +		wake_up(&dln2->disconnect_wq);
> +
> +	return ret;
> +}
v> +
> +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;
> +
> +	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\n",
> +			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;
> +
> +	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 = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;

Is it really worth having this helper only to save a couple of lines on
a dev_err? If you do all resubmissions on completion inline in the
handler, you only have three places where usb_submit_urb is called.

> +	}
> +
> +	return 0;
> +}
> +
> +static struct dln2_platform_data dln2_pdata_gpio = {
> +	.handle = DLN2_HANDLE_GPIO,
> +};
> +
> +/* Only one I2C port seems to be supported on current hardware */
> +static struct dln2_platform_data dln2_pdata_i2c = {
> +	.handle = DLN2_HANDLE_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);
> +	int i, j;
> +
> +	/* don't allow starting new transfers */
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->disconnect = true;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	/* cancel in progress transfers */
> +	for (i = 0; i < DLN2_HANDLES; i++) {
> +		struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&rxs->lock, flags);
> +
> +		/* cancel all response waiters */
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> +			struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> +			if (rxc->in_use)
> +				complete(&rxc->done);
> +		}
> +
> +		spin_unlock_irqrestore(&rxs->lock, flags);
> +	}
> +
> +	/* wait for transfers to end */
> +	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> +	mfd_remove_devices(&interface->dev);
> +
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret;
> +	int i, j;
> +	int id;
> +
> +	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);
> +	init_waitqueue_head(&dln2->disconnect_wq);
> +
> +	for (i = 0; i < DLN2_HANDLES; 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->event_cb_lock);
> +	INIT_LIST_HEAD(&dln2->event_cb_list);
> +
> +	ret = dln2_setup_rx_urbs(dln2, hostif);
> +	if (ret) {
> +		dln2_free(dln2);

goto out_cleanup as mentioned earlier.

> +		return ret;
> +	}
> +
> +	ret = dln2_hw_init(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize hardware\n");
> +		goto out_cleanup;
> +	}
> +
> +	id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;

After giving this some more thought, I think you should just
use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
other USB MFD drivers and add a convenience helper to register
hotpluggable MFDs here:

        http://marc.info/?l=linux-kernel&m=141172912516884&w=2

> +	ret = mfd_add_devices(dev, id, 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;
> +}

I'll try to take a quick look on the subdrivers on Monday.

Johan

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:31PM +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       | 751 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h |  67 +++++
>  4 files changed, 828 insertions(+)
>  create mode 100644 drivers/mfd/dln2.c
>  create mode 100644 include/linux/mfd/dln2.h

[...]

> +#define DLN2_HW_ID			0x200
> +#define DLN2_USB_TIMEOUT		200	/* in ms */
> +#define DLN2_MAX_RX_SLOTS		16
> +#define DLN2_MAX_URBS			16
> +#define DLN2_RX_BUF_SIZE		512
> +
> +enum dln2_handle {
> +	DLN2_HANDLE_EVENT,

This is the only handle that was fixed (0), right? Initialise this one
explicitly in case any one ever tries to reorder them.

> +	DLN2_HANDLE_CTRL,
> +	DLN2_HANDLE_GPIO,
> +	DLN2_HANDLE_I2C,
> +	DLN2_HANDLES
> +};
> +
> +/*
> + * 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 {
> +	/* completion used to wait for a response */
> +	struct completion done;
> +
> +	/* if non-NULL the URB contains the response */
> +	struct urb *urb;
> +
> +	/* if true then this context is used to wait for a response */
> +	bool in_use;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to identify 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 alloc/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_HANDLES];
> +
> +	struct list_head event_cb_list;
> +	spinlock_t event_cb_lock;
> +
> +	bool disconnect;
> +	int active_transfers;
> +	wait_queue_head_t disconnect_wq;
> +	spinlock_t disconnect_lock;
> +};
> +
> +struct dln2_event_cb_entry {
> +	struct list_head list;
> +	u16 id;
> +	struct platform_device *pdev;
> +	dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +			   dln2_event_cb_t rx_cb)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_event_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->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +
> +	if (!ret)
> +		list_add_rcu(&new->list, &dln2->event_cb_list);
> +
> +	spin_unlock_irqrestore(&dln2->event_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_event_cb_entry *i;
> +	unsigned long flags;
> +	bool found = false;
> +
> +	spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			list_del_rcu(&i->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> +	if (found) {
> +		synchronize_rcu();
> +		kfree(i);
> +	}
> +}
> +EXPORT_SYMBOL(dln2_unregister_event_cb);
> +
> +static int dln2_submit_urb(struct dln2_dev *dln2, struct urb *urb, gfp_t gfp)
> +{
> +	int ret;
> +	struct device *dev = &dln2->interface->dev;
> +
> +	ret = usb_submit_urb(urb, gfp);
> +	if (ret < 0)
> +		dev_err(dev, "failed to (re)submit RX URB: %d\n", ret);
> +	return ret;
> +}
> +
> +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;
> +
> +	spin_lock(&rxs->lock);
> +
> +	rxc = &rxs->slots[rx_slot];

This can be done outside the lock, right?

> +
> +	if (rxc->in_use && !rxc->urb) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +		/* URB will be resubmitted in free_rx_slot */
> +	} else {
> +		dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> +		dln2_submit_urb(dln2, urb, GFP_ATOMIC);

And so can the resubmission.

I think this might be easier to follow if you just do this inline in the
completion handler, and always resubmit there before returning (unless
the slot is in use).

> +	}
> +
> +	spin_unlock(&rxs->lock);
> +}
> +
> +static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> +				  void *data, int len)

Rename this one dln2_run_event_callbacks to match your new names (much
better by the way)?

> +{
> +	struct dln2_event_cb_entry *i;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(i, &dln2->event_cb_list, list)
> +		if (i->id == id)
> +			i->callback(i->pdev, echo, data, len);
> +
> +	rcu_read_unlock();
> +}
> +
> +static void dln2_rx(struct urb *urb)
> +{
> +	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_HANDLES) {
> +		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);
> +	} else {
> +		dln2_rx_transfer(dln2, urb, handle, echo);
> +		/* URB will be re-submitted in dln2_rx_transfer */

This comment is a little misleading since the urb will not be
resubmitted if the corresponding slot is in use. See my comment to
dln2_rx_transfer above.

> +		return;
> +	}
> +
> +out:
> +	dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> +}
> +
> +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 bool find_free_slot(struct dln2_dev *dln2, u16 handle, int *slot)
> +{
> +	struct dln2_mod_rx_slots *rxs;
> +	unsigned long flags;
> +

You could still check the global disconnect flag here to speed up
disconnect (locking not needed). Return -ENODEV as I mentioned earlier.

> +	rxs = &dln2->mod_rx_slots[handle];
> +
> +	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->in_use = true;
> +	}
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	return *slot < DLN2_MAX_RX_SLOTS;
> +}
> +
> +static int alloc_rx_slot(struct dln2_dev *dln2, u16 handle)
> +{
> +	int ret;
> +	int slot;
> +
> +	/*
> +	 * No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer.
> +	 */
> +	ret = wait_event_interruptible(dln2->mod_rx_slots[handle].wq,
> +				       find_free_slot(dln2, handle, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, u16 handle, int slot)
> +{
> +	struct dln2_mod_rx_slots *rxs;
> +	struct urb *urb = NULL;
> +	unsigned long flags;
> +	struct dln2_rx_context *rxc;
> +
> +	rxs = &dln2->mod_rx_slots[handle];
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	clear_bit(slot, &rxs->bmap);
> +
> +	rxc = &rxs->slots[slot];
> +	rxc->in_use = false;
> +	urb = rxc->urb;
> +	rxc->urb = NULL;
> +	reinit_completion(&rxc->done);
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	if (urb)
> +		dln2_submit_urb(dln2, urb, GFP_KERNEL);
> +
> +	wake_up_interruptible(&rxs->wq);
> +}
> +
> +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;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> +	spin_lock(&dln2->disconnect_lock);
> +	if (!dln2->disconnect)
> +		dln2->active_transfers++;
> +	else
> +		ret = -ENODEV;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	rx_slot = alloc_rx_slot(dln2, handle);
> +	if (rx_slot < 0) {
> +		ret = rx_slot;
> +		goto out_decr;
> +	}
> +
> +	dev = &dln2->interface->dev;

This can be done when declaring dev.

> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		dev_err(dev, "USB write failed: %d\n", ret);
> +		goto out_free_rx_slot;
> +	}
> +
> +	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 (!rxc->urb) {
> +		ret = -ENODEV;
> +		goto out_free_rx_slot;
> +	}

This can only happen if disconnected, right? Perhaps add a comment
unless you want to the test the disconnected flag instead which would
make it self-explanatory.

> +
> +	/* 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, handle, rx_slot);
> +out_decr:
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->active_transfers--;
> +	spin_unlock(&dln2->disconnect_lock);
> +	if (dln2->disconnect)
> +		wake_up(&dln2->disconnect_wq);
> +
> +	return ret;
> +}
v> +
> +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;
> +
> +	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\n",
> +			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;
> +
> +	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 = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;

Is it really worth having this helper only to save a couple of lines on
a dev_err? If you do all resubmissions on completion inline in the
handler, you only have three places where usb_submit_urb is called.

> +	}
> +
> +	return 0;
> +}
> +
> +static struct dln2_platform_data dln2_pdata_gpio = {
> +	.handle = DLN2_HANDLE_GPIO,
> +};
> +
> +/* Only one I2C port seems to be supported on current hardware */
> +static struct dln2_platform_data dln2_pdata_i2c = {
> +	.handle = DLN2_HANDLE_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);
> +	int i, j;
> +
> +	/* don't allow starting new transfers */
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->disconnect = true;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	/* cancel in progress transfers */
> +	for (i = 0; i < DLN2_HANDLES; i++) {
> +		struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&rxs->lock, flags);
> +
> +		/* cancel all response waiters */
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> +			struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> +			if (rxc->in_use)
> +				complete(&rxc->done);
> +		}
> +
> +		spin_unlock_irqrestore(&rxs->lock, flags);
> +	}
> +
> +	/* wait for transfers to end */
> +	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> +	mfd_remove_devices(&interface->dev);
> +
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret;
> +	int i, j;
> +	int id;
> +
> +	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);
> +	init_waitqueue_head(&dln2->disconnect_wq);
> +
> +	for (i = 0; i < DLN2_HANDLES; 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->event_cb_lock);
> +	INIT_LIST_HEAD(&dln2->event_cb_list);
> +
> +	ret = dln2_setup_rx_urbs(dln2, hostif);
> +	if (ret) {
> +		dln2_free(dln2);

goto out_cleanup as mentioned earlier.

> +		return ret;
> +	}
> +
> +	ret = dln2_hw_init(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize hardware\n");
> +		goto out_cleanup;
> +	}
> +
> +	id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;

After giving this some more thought, I think you should just
use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
other USB MFD drivers and add a convenience helper to register
hotpluggable MFDs here:

        http://marc.info/?l=linux-kernel&m=141172912516884&w=2

> +	ret = mfd_add_devices(dev, id, 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;
> +}

I'll try to take a quick look on the subdrivers on Monday.

Johan

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

[Octavian Purdila]

On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Thu, Sep 25, 2014 at 07:07:31PM +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:
> >

<snip>

> > +
> > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> > +             if (ret < 0)
> > +                     return ret;
>
> Is it really worth having this helper only to save a couple of lines on
> a dev_err? If you do all resubmissions on completion inline in the
> handler, you only have three places where usb_submit_urb is called.
>

I moved the completion in the handler as you suggested. I have kept
the helper, would you prefer to remove it?

<snip>

> > +     id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
>
> After giving this some more thought, I think you should just
> use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
> other USB MFD drivers and add a convenience helper to register
> hotpluggable MFDs here:
>

OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges
first and I have to resubmit mine, I will switch to using
mfd_add_hotplug_devices.

> I'll try to take a quick look on the subdrivers on Monday.
>

Thanks Johan. I have addressed the rest of the comments as well and I
will send another series after your reviews on the sub-drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Octavian Purdila]

On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Sep 25, 2014 at 07:07:31PM +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:
> >

<snip>

> > +
> > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> > +             if (ret < 0)
> > +                     return ret;
>
> Is it really worth having this helper only to save a couple of lines on
> a dev_err? If you do all resubmissions on completion inline in the
> handler, you only have three places where usb_submit_urb is called.
>

I moved the completion in the handler as you suggested. I have kept
the helper, would you prefer to remove it?

<snip>

> > +     id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
>
> After giving this some more thought, I think you should just
> use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
> other USB MFD drivers and add a convenience helper to register
> hotpluggable MFDs here:
>

OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges
first and I have to resubmit mine, I will switch to using
mfd_add_hotplug_devices.

> I'll try to take a quick look on the subdrivers on Monday.
>

Thanks Johan. I have addressed the rest of the comments as well and I
will send another series after your reviews on the sub-drivers.

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---

[...]

> +#define DLN2_I2C_MAX_XFER_SIZE		256
> +#define DLN2_I2C_BUF_SIZE		(DLN2_I2C_MAX_XFER_SIZE + 16)
> +
> +struct dln2_i2c {
> +	struct platform_device *pdev;
> +	struct i2c_adapter adapter;
> +	u32 freq;
> +	u32 min_freq;
> +	u32 max_freq;
> +	u8 port;
> +	/*
> +	 * Buffer to hold the packet for read or write transfers. One
> +	 * is enough since we can't have multiple transfers in
> +	 * parallel on the i2c adapter.
> +	 */
> +	void *buf;
> +};
> +
> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    NULL, NULL);

Don't use the port member of dln2 directly here. Encode the protocol in
the function as you already do for most other messages (and did in
previous versions). You could even declare a
	
	struct {
		u8 port;
	} tx;

for consistency.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
> +{
> +	int ret;
> +	struct tx_data {
> +		u8 port;
> +		__le32 freq;
> +	} __packed tx;
> +
> +	tx.port = dln2->port;
> +	tx.freq = cpu_to_le32(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	dln2->freq = freq;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
> +{
> +	int ret;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    &freq, &len);

Same here.

> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(freq))
> +		return -EPROTO;
> +
> +	*res = le32_to_cpu(freq);
> +
> +	return 0;
> +}
> +

[...]

> +static ssize_t dln2_i2c_freq_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct dln2_i2c *dln2 = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", dln2->freq);
> +}
> +
> +static ssize_t dln2_i2c_freq_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct dln2_i2c *dln2 = dev_get_drvdata(dev);
> +	unsigned long freq;
> +	int ret;
> +
> +	if (kstrtoul(buf, 0, &freq) || freq < dln2->min_freq ||
> +	    freq > dln2->max_freq)
> +		return -EINVAL;
> +
> +	ret = dln2_i2c_enable(dln2, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_set_frequency(dln2, freq);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_enable(dln2, true);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dln2_i2c_freq);

You forgot to add the required documentation of the attribute to
Documentation/ABI as requested.

Johan

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:32PM +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>
> ---

[...]

> +#define DLN2_I2C_MAX_XFER_SIZE		256
> +#define DLN2_I2C_BUF_SIZE		(DLN2_I2C_MAX_XFER_SIZE + 16)
> +
> +struct dln2_i2c {
> +	struct platform_device *pdev;
> +	struct i2c_adapter adapter;
> +	u32 freq;
> +	u32 min_freq;
> +	u32 max_freq;
> +	u8 port;
> +	/*
> +	 * Buffer to hold the packet for read or write transfers. One
> +	 * is enough since we can't have multiple transfers in
> +	 * parallel on the i2c adapter.
> +	 */
> +	void *buf;
> +};
> +
> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +
> +	if (enable)
> +		cmd = DLN2_I2C_ENABLE;
> +	else
> +		cmd = DLN2_I2C_DISABLE;
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    NULL, NULL);

Don't use the port member of dln2 directly here. Encode the protocol in
the function as you already do for most other messages (and did in
previous versions). You could even declare a
	
	struct {
		u8 port;
	} tx;

for consistency.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
> +{
> +	int ret;
> +	struct tx_data {
> +		u8 port;
> +		__le32 freq;
> +	} __packed tx;
> +
> +	tx.port = dln2->port;
> +	tx.freq = cpu_to_le32(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
> +			    NULL, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	dln2->freq = freq;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
> +{
> +	int ret;
> +	__le32 freq;
> +	unsigned len = sizeof(freq);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
> +			    &freq, &len);

Same here.

> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(freq))
> +		return -EPROTO;
> +
> +	*res = le32_to_cpu(freq);
> +
> +	return 0;
> +}
> +

[...]

> +static ssize_t dln2_i2c_freq_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct dln2_i2c *dln2 = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", dln2->freq);
> +}
> +
> +static ssize_t dln2_i2c_freq_store(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct dln2_i2c *dln2 = dev_get_drvdata(dev);
> +	unsigned long freq;
> +	int ret;
> +
> +	if (kstrtoul(buf, 0, &freq) || freq < dln2->min_freq ||
> +	    freq > dln2->max_freq)
> +		return -EINVAL;
> +
> +	ret = dln2_i2c_enable(dln2, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_set_frequency(dln2, freq);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_enable(dln2, true);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dln2_i2c_freq);

You forgot to add the required documentation of the attribute to
Documentation/ABI as requested.

Johan

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:34PM +0300, Octavian Purdila wrote:
> From: Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---

Looks good. I'll provide my reviewed-by tag to the final submission.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:34PM +0300, Octavian Purdila wrote:
> 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>
> ---

Looks good. I'll provide my reviewed-by tag to the final submission.

Thanks,
Johan

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

[Johan Hovold]

On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote:
> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Sep 25, 2014 at 07:07:31PM +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:
> > >
> 
> <snip>
> 
> > > +
> > > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> > > +             if (ret < 0)
> > > +                     return ret;
> >
> > Is it really worth having this helper only to save a couple of lines on
> > a dev_err? If you do all resubmissions on completion inline in the
> > handler, you only have three places where usb_submit_urb is called.
> >
> 
> I moved the completion in the handler as you suggested. I have kept
> the helper, would you prefer to remove it?

Moved the "completion"? I was suggesting that the URB resubmission
should be done inline the URB completion handler.

[ "Completion" may be a little ambiguous. The URB callback is called an
URB completion handler. Not be confused with the completion structures
you use to wait for responses. ]

It's fine to keep the helper as long as it's clear that the urb has been
"cached" and should not be resubmitted (inline) in the completion
handler in that case.
 
> <snip>
> 
> > > +     id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
> >
> > After giving this some more thought, I think you should just
> > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
> > other USB MFD drivers and add a convenience helper to register
> > hotpluggable MFDs here:
> >
> 
> OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges
> first and I have to resubmit mine, I will switch to using
> mfd_add_hotplug_devices.

Lee merged the helper function earlier today.

Johan

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

[Octavian Purdila]

On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Sep 25, 2014 at 07:07:32PM +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>
>> ---
>
> [...]
>
>> +#define DLN2_I2C_MAX_XFER_SIZE               256
>> +#define DLN2_I2C_BUF_SIZE            (DLN2_I2C_MAX_XFER_SIZE + 16)
>> +
>> +struct dln2_i2c {
>> +     struct platform_device *pdev;
>> +     struct i2c_adapter adapter;
>> +     u32 freq;
>> +     u32 min_freq;
>> +     u32 max_freq;
>> +     u8 port;
>> +     /*
>> +      * Buffer to hold the packet for read or write transfers. One
>> +      * is enough since we can't have multiple transfers in
>> +      * parallel on the i2c adapter.
>> +      */
>> +     void *buf;
>> +};
>> +
>> +static int dln2_i2c_enable(struct dln2_i2c *dln2, bool enable)
>> +{
>> +     int ret;
>> +     u16 cmd;
>> +
>> +     if (enable)
>> +             cmd = DLN2_I2C_ENABLE;
>> +     else
>> +             cmd = DLN2_I2C_DISABLE;
>> +
>> +     ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
>> +                         NULL, NULL);
>
> Don't use the port member of dln2 directly here. Encode the protocol in
> the function as you already do for most other messages (and did in
> previous versions). You could even declare a
>
>         struct {
>                 u8 port;
>         } tx;
>
> for consistency.
>

Yes, I think I did this after Arnd's review on the gpio stuff where I
thought he suggested to remove the structure, when in fact he asked to
remove the __packed attribute. I will revert it back.

>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dln2_i2c_set_frequency(struct dln2_i2c *dln2, u32 freq)
>> +{
>> +     int ret;
>> +     struct tx_data {
>> +             u8 port;
>> +             __le32 freq;
>> +     } __packed tx;
>> +
>> +     tx.port = dln2->port;
>> +     tx.freq = cpu_to_le32(freq);
>> +
>> +     ret = dln2_transfer(dln2->pdev, DLN2_I2C_SET_FREQUENCY, &tx, sizeof(tx),
>> +                         NULL, NULL);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     dln2->freq = freq;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dln2_i2c_get_freq(struct dln2_i2c *dln2, u16 cmd, u32 *res)
>> +{
>> +     int ret;
>> +     __le32 freq;
>> +     unsigned len = sizeof(freq);
>> +
>> +     ret = dln2_transfer(dln2->pdev, cmd, &dln2->port, sizeof(dln2->port),
>> +                         &freq, &len);
>
> Same here.
>

OK.

>> +     if (ret < 0)
>> +             return ret;
>> +     if (len < sizeof(freq))
>> +             return -EPROTO;
>> +
>> +     *res = le32_to_cpu(freq);
>> +
>> +     return 0;
>> +}
>> +
>
> [...]
>
>> +static ssize_t dln2_i2c_freq_show(struct device *dev,
>> +                               struct device_attribute *attr, char *buf)
>> +{
>> +     struct dln2_i2c *dln2 = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%d\n", dln2->freq);
>> +}
>> +
>> +static ssize_t dln2_i2c_freq_store(struct device *dev,
>> +                                struct device_attribute *attr,
>> +                                const char *buf, size_t count)
>> +{
>> +     struct dln2_i2c *dln2 = dev_get_drvdata(dev);
>> +     unsigned long freq;
>> +     int ret;
>> +
>> +     if (kstrtoul(buf, 0, &freq) || freq < dln2->min_freq ||
>> +         freq > dln2->max_freq)
>> +             return -EINVAL;
>> +
>> +     ret = dln2_i2c_enable(dln2, false);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = dln2_i2c_set_frequency(dln2, freq);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = dln2_i2c_enable(dln2, true);
>> +
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(dln2_i2c_freq);
>
> You forgot to add the required documentation of the attribute to
> Documentation/ABI as requested.
>

Hmm, I did add a new entry in Documentation/ABI/ at
testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning
at the patch.

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

[Johan Hovold]

On Tue, Oct 07, 2014 at 08:52:35PM +0300, Octavian Purdila wrote:
> On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:

> >> +static DEVICE_ATTR_RW(dln2_i2c_freq);
> >
> > You forgot to add the required documentation of the attribute to
> > Documentation/ABI as requested.
> >
> 
> Hmm, I did add a new entry in Documentation/ABI/ at
> testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning
> at the patch.

Ah, sorry, I missed that. I'll look at it tomorrow.

Johan

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

[Johan Hovold]

On Tue, Oct 07, 2014 at 08:52:35PM +0300, Octavian Purdila wrote:
> On Tue, Oct 7, 2014 at 7:46 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:

> >> +static DEVICE_ATTR_RW(dln2_i2c_freq);
> >
> > You forgot to add the required documentation of the attribute to
> > Documentation/ABI as requested.
> >
> 
> Hmm, I did add a new entry in Documentation/ABI/ at
> testing/sysfs-bus-i2c-busses-dln2. It should be right at the beginning
> at the patch.

Ah, sorry, I missed that. I'll look at it tomorrow.

Johan

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

[Octavian Purdila]

On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote:
>> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> >
>> > On Thu, Sep 25, 2014 at 07:07:31PM +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:
>> > >
>>
>> <snip>
>>
>> > > +
>> > > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
>> > > +             if (ret < 0)
>> > > +                     return ret;
>> >
>> > Is it really worth having this helper only to save a couple of lines on
>> > a dev_err? If you do all resubmissions on completion inline in the
>> > handler, you only have three places where usb_submit_urb is called.
>> >
>>
>> I moved the completion in the handler as you suggested. I have kept
>> the helper, would you prefer to remove it?
>
> Moved the "completion"? I was suggesting that the URB resubmission
> should be done inline the URB completion handler.
>
> [ "Completion" may be a little ambiguous. The URB callback is called an
> URB completion handler. Not be confused with the completion structures
> you use to wait for responses. ]
>

Sorry, I meant to say resubmission instead of completion.

> It's fine to keep the helper as long as it's clear that the urb has been
> "cached" and should not be resubmitted (inline) in the completion
> handler in that case.
>

Not sure I follow you here. I kept the helper and I call it from the
completion handler, from free_rx_slot and from dln2_setup_rx_ubs.

>
>> <snip>
>>
>> > > +     id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
>> >
>> > After giving this some more thought, I think you should just
>> > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
>> > other USB MFD drivers and add a convenience helper to register
>> > hotpluggable MFDs here:
>> >
>>
>> OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges
>> first and I have to resubmit mine, I will switch to using
>> mfd_add_hotplug_devices.
>
> Lee merged the helper function earlier today.
>

Yep, noticed that, I did a rebase earlier.

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

[Octavian Purdila]

On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote:
>> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan@kernel.org> wrote:
>> >
>> > On Thu, Sep 25, 2014 at 07:07:31PM +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:
>> > >
>>
>> <snip>
>>
>> > > +
>> > > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
>> > > +             if (ret < 0)
>> > > +                     return ret;
>> >
>> > Is it really worth having this helper only to save a couple of lines on
>> > a dev_err? If you do all resubmissions on completion inline in the
>> > handler, you only have three places where usb_submit_urb is called.
>> >
>>
>> I moved the completion in the handler as you suggested. I have kept
>> the helper, would you prefer to remove it?
>
> Moved the "completion"? I was suggesting that the URB resubmission
> should be done inline the URB completion handler.
>
> [ "Completion" may be a little ambiguous. The URB callback is called an
> URB completion handler. Not be confused with the completion structures
> you use to wait for responses. ]
>

Sorry, I meant to say resubmission instead of completion.

> It's fine to keep the helper as long as it's clear that the urb has been
> "cached" and should not be resubmitted (inline) in the completion
> handler in that case.
>

Not sure I follow you here. I kept the helper and I call it from the
completion handler, from free_rx_slot and from dln2_setup_rx_ubs.

>
>> <snip>
>>
>> > > +     id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
>> >
>> > After giving this some more thought, I think you should just
>> > use PLATFORM_DEVID_AUTO as id. I have submitted a series to fix the
>> > other USB MFD drivers and add a convenience helper to register
>> > hotpluggable MFDs here:
>> >
>>
>> OK, I will use PLATFORM_DEVID_AUTO for now. If your series merges
>> first and I have to resubmit mine, I will switch to using
>> mfd_add_hotplug_devices.
>
> Lee merged the helper function earlier today.
>

Yep, noticed that, I did a rebase earlier.

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:

> +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;
> +
> +	spin_lock(&rxs->lock);

You must use spin_lock_irqsave here as you call it from the completion
handler.

> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	if (rxc->in_use && !rxc->urb) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +		/* URB will be resubmitted in free_rx_slot */
> +	} else {
> +		dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> +		dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> +	}
> +
> +	spin_unlock(&rxs->lock);
> +}

> +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;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> +	spin_lock(&dln2->disconnect_lock);

How did you test this version? You never initialise disconnect_lock so
lockdep complains loudly when calling _dln2_transfer during probe.

> +	if (!dln2->disconnect)
> +		dln2->active_transfers++;
> +	else
> +		ret = -ENODEV;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	rx_slot = alloc_rx_slot(dln2, handle);
> +	if (rx_slot < 0) {
> +		ret = rx_slot;
> +		goto out_decr;
> +	}
> +
> +	dev = &dln2->interface->dev;
> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		dev_err(dev, "USB write failed: %d\n", ret);
> +		goto out_free_rx_slot;
> +	}
> +
> +	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 (!rxc->urb) {
> +		ret = -ENODEV;
> +		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, handle, rx_slot);
> +out_decr:
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->active_transfers--;
> +	spin_unlock(&dln2->disconnect_lock);
> +	if (dln2->disconnect)
> +		wake_up(&dln2->disconnect_wq);
> +
> +	return ret;
> +}

> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +	int i, j;
> +
> +	/* don't allow starting new transfers */
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->disconnect = true;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	/* cancel in progress transfers */
> +	for (i = 0; i < DLN2_HANDLES; i++) {
> +		struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&rxs->lock, flags);

Just stick to spin_lock in this function.

> +
> +		/* cancel all response waiters */
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> +			struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> +			if (rxc->in_use)
> +				complete(&rxc->done);
> +		}
> +
> +		spin_unlock_irqrestore(&rxs->lock, flags);
> +	}
> +
> +	/* wait for transfers to end */
> +	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> +	mfd_remove_devices(&interface->dev);
> +
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret;
> +	int i, j;
> +	int id;
> +
> +	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);
> +	init_waitqueue_head(&dln2->disconnect_wq);
> +
> +	for (i = 0; i < DLN2_HANDLES; 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->event_cb_lock);
> +	INIT_LIST_HEAD(&dln2->event_cb_list);

The disconnect_lock is never initialised, as mentioned above.

> +
> +	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;
> +	}
> +
> +	id = dln2->usb_dev->bus->busnum << 8 | dln2->usb_dev->devnum;
> +	ret = mfd_add_devices(dev, id, 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 = "usb-dln2",

Just call the usb driver "dln2". 

> +	.probe = dln2_probe,
> +	.disconnect = dln2_disconnect,
> +	.id_table = dln2_table,
> +};

Johan

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

[Johan Hovold]

On Tue, Oct 07, 2014 at 09:01:27PM +0300, Octavian Purdila wrote:
> On Tue, Oct 7, 2014 at 8:10 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Oct 06, 2014 at 03:17:22PM +0300, Octavian Purdila wrote:
> >> On Fri, Oct 3, 2014 at 8:12 PM, Johan Hovold <johan@kernel.org> wrote:
> >> >
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +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:
> >> > >
> >>
> >> <snip>
> >>
> >> > > +
> >> > > +             ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> >> > > +             if (ret < 0)
> >> > > +                     return ret;
> >> >
> >> > Is it really worth having this helper only to save a couple of lines on
> >> > a dev_err? If you do all resubmissions on completion inline in the
> >> > handler, you only have three places where usb_submit_urb is called.
> >>
> >> I moved the completion in the handler as you suggested. I have kept
> >> the helper, would you prefer to remove it?
> >
> > Moved the "completion"? I was suggesting that the URB resubmission
> > should be done inline the URB completion handler.
> >
> > [ "Completion" may be a little ambiguous. The URB callback is called an
> > URB completion handler. Not be confused with the completion structures
> > you use to wait for responses. ]
> >
> 
> Sorry, I meant to say resubmission instead of completion.
> 
> > It's fine to keep the helper as long as it's clear that the urb has been
> > "cached" and should not be resubmitted (inline) in the completion
> > handler in that case.
> 
> Not sure I follow you here. I kept the helper and I call it from the
> completion handler, from free_rx_slot and from dln2_setup_rx_ubs.

Ah sorry, I was referring to your other helper dln2_rx_transfer().

I still think you should do away with the dln2_submit_urb() helper as
it needlessly hides what's on going without any real gain.

Johan

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

[Johan Hovold]

On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:

> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
> new file mode 100644
> index 0000000..ad55af6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2

This is a nonstandard path that does not match the attribute path (or
any other convention followed under ABI/).

I suggest you rename it

	sysfs-class-i2c-adapter-dln2

> @@ -0,0 +1,5 @@
> +What:		/sys/bus/i2c/devices/.../dln2_i2c_freq

I think you should rename the attribute "bus_frequency" and name the
attribute group "dln2". That way the attribute will show up as

	.../<device>/dln2/bus_frequency

Then the "What:" field should read

	What:		/sys/class/i2c-adapter/<device>/dln2/bus_frequency

> +Date:		September 2014
> +Contact:	Octavian Purdila <octavian.purdila@intel.com>
> +Description:
> +		This attribute shows/sets the frequency (in Hz) of the I2C bus.

I'd reword this "Set the frequency...".

Please also describe under what circumstances this may fail, that is,
when setting a frequency less than or greater than the min or max
frequencies reported by the device.

Perhaps you should even consider exporting those two values?

Johan

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

[Octavian Purdila]

On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>
>> +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;
>> +
>> +     spin_lock(&rxs->lock);
>
> You must use spin_lock_irqsave here as you call it from the completion
> handler.

Why? AFAICS the completion handler gets called from the HCD irq handler:

[    2.503945] Call Trace:
[    2.503945]  <IRQ>  [<ffffffff81a73d14>] dump_stack+0x4e/0x7a
[    2.503945]  [<ffffffff81639fcb>] dln2_rx+0x1eb/0x2d0
[    2.503945]  [<ffffffff81742202>] __usb_hcd_giveback_urb+0x72/0x120
[    2.503945]  [<ffffffff817423cf>] usb_hcd_giveback_urb+0x3f/0x120
[    2.503945]  [<ffffffff81786ffa>] uhci_giveback_urb+0xaa/0x290
[    2.503945]  [<ffffffff811d36d7>] ? dma_pool_free+0xa7/0xd0
[    2.503945]  [<ffffffff81788fe3>] uhci_scan_schedule+0x493/0xb20
[    2.503945]  [<ffffffff81789c9e>] uhci_irq+0x9e/0x180
[    2.503945]  [<ffffffff81741546>] usb_hcd_irq+0x26/0x40
[    2.503945]  [<ffffffff8110e46e>] handle_irq_event_percpu+0x3e/0x1e0
[    2.503945]  [<ffffffff8110e64d>] handle_irq_event+0x3d/0x60
[    2.503945]  [<ffffffff811117f9>] handle_fasteoi_irq+0x99/0x160
[    2.503945]  [<ffffffff810492c2>] handle_irq+0x102/0x190
[    2.503945]  [<ffffffff810e493a>] ? atomic_notifier_call_chain+0x3a/0x50
[    2.503945]  [<ffffffff81a7fbcb>] do_IRQ+0x5b/0x100
[    2.503945]  [<ffffffff81a7dd6f>] common_interrupt+0x6f/0x6f
[    2.503945]  <EOI>  [<ffffffff810512ed>] ? default_idle+0x1d/0x100

>
>> +
>> +     rxc = &rxs->slots[rx_slot];
>> +
>> +     if (rxc->in_use && !rxc->urb) {
>> +             rxc->urb = urb;
>> +             complete(&rxc->done);
>> +             /* URB will be resubmitted in free_rx_slot */
>> +     } else {
>> +             dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
>> +             dln2_submit_urb(dln2, urb, GFP_ATOMIC);
>> +     }
>> +
>> +     spin_unlock(&rxs->lock);
>> +}
>
>> +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;
>> +     const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
>> +     struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> +
>> +     spin_lock(&dln2->disconnect_lock);
>
> How did you test this version? You never initialise disconnect_lock so
> lockdep complains loudly when calling _dln2_transfer during probe.
>

Oops, missed that as lockdep was not enable in the lastest test setup.

>> +     if (!dln2->disconnect)
>> +             dln2->active_transfers++;
>> +     else
>> +             ret = -ENODEV;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +
>> +     if (ret)
>> +             return ret;
>> +
>> +     rx_slot = alloc_rx_slot(dln2, handle);
>> +     if (rx_slot < 0) {
>> +             ret = rx_slot;
>> +             goto out_decr;
>> +     }
>> +
>> +     dev = &dln2->interface->dev;
>> +
>> +     ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
>> +     if (ret < 0) {
>> +             dev_err(dev, "USB write failed: %d\n", ret);
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     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 (!rxc->urb) {
>> +             ret = -ENODEV;
>> +             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, handle, rx_slot);
>> +out_decr:
>> +     spin_lock(&dln2->disconnect_lock);
>> +     dln2->active_transfers--;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +     if (dln2->disconnect)
>> +             wake_up(&dln2->disconnect_wq);
>> +
>> +     return ret;
>> +}
>
>> +static void dln2_disconnect(struct usb_interface *interface)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(interface);
>> +     int i, j;
>> +
>> +     /* don't allow starting new transfers */
>> +     spin_lock(&dln2->disconnect_lock);
>> +     dln2->disconnect = true;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +
>> +     /* cancel in progress transfers */
>> +     for (i = 0; i < DLN2_HANDLES; i++) {
>> +             struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
>> +             unsigned long flags;
>> +
>> +             spin_lock_irqsave(&rxs->lock, flags);
>
> Just stick to spin_lock in this function.
>

AFAICS disconnect is called from a kernel thread. Are there guarantees
that we can't get a call to the completion routine while we are
running it?

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

[Octavian Purdila]

On Wed, Oct 8, 2014 at 1:42 PM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Sep 25, 2014 at 07:07:32PM +0300, Octavian Purdila wrote:
>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2 b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
>> new file mode 100644
>> index 0000000..ad55af6
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-busses-dln2
>
> This is a nonstandard path that does not match the attribute path (or
> any other convention followed under ABI/).
>
> I suggest you rename it
>
>         sysfs-class-i2c-adapter-dln2
>
>> @@ -0,0 +1,5 @@
>> +What:                /sys/bus/i2c/devices/.../dln2_i2c_freq
>
> I think you should rename the attribute "bus_frequency" and name the
> attribute group "dln2". That way the attribute will show up as
>
>         .../<device>/dln2/bus_frequency
>
> Then the "What:" field should read
>
>         What:           /sys/class/i2c-adapter/<device>/dln2/bus_frequency
>
>> +Date:                September 2014
>> +Contact:     Octavian Purdila <octavian.purdila@intel.com>
>> +Description:
>> +             This attribute shows/sets the frequency (in Hz) of the I2C bus.
>
> I'd reword this "Set the frequency...".
>
> Please also describe under what circumstances this may fail, that is,
> when setting a frequency less than or greater than the min or max
> frequencies reported by the device.
>
> Perhaps you should even consider exporting those two values?
>

I noticed that a few other USB boards support this. Perhaps it is time
to add these entries to the core i2c layer?

In that case I think is better to remove the setting and update the
drievr when the patches for the i2c layer are ready?

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

[Johan Hovold]

On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >
> >> +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;
> >> +
> >> +     spin_lock(&rxs->lock);
> >
> > You must use spin_lock_irqsave here as you call it from the completion
> > handler.
> 
> Why? AFAICS the completion handler gets called from the HCD irq handler:

The completion handler is currently called with local interrupts
disabled but that is about to change once all drivers have been updated: 

	http://marc.info/?l=linux-usb&m=137353360511003&w=2

In this case you could probably get away with not disabling interrupts
anyway, but using the irqsave versions would make it obvious.

> >> +static void dln2_disconnect(struct usb_interface *interface)
> >> +{
> >> +     struct dln2_dev *dln2 = usb_get_intfdata(interface);
> >> +     int i, j;
> >> +
> >> +     /* don't allow starting new transfers */
> >> +     spin_lock(&dln2->disconnect_lock);
> >> +     dln2->disconnect = true;
> >> +     spin_unlock(&dln2->disconnect_lock);
> >> +
> >> +     /* cancel in progress transfers */
> >> +     for (i = 0; i < DLN2_HANDLES; i++) {
> >> +             struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> >> +             unsigned long flags;
> >> +
> >> +             spin_lock_irqsave(&rxs->lock, flags);
> >
> > Just stick to spin_lock in this function.
> 
> AFAICS disconnect is called from a kernel thread. Are there guarantees
> that we can't get a call to the completion routine while we are
> running it?

Brain fart, nevermind.

Johan

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

[Johan Hovold]

On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >
> >> +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;
> >> +
> >> +     spin_lock(&rxs->lock);
> >
> > You must use spin_lock_irqsave here as you call it from the completion
> > handler.
> 
> Why? AFAICS the completion handler gets called from the HCD irq handler:

The completion handler is currently called with local interrupts
disabled but that is about to change once all drivers have been updated: 

	http://marc.info/?l=linux-usb&m=137353360511003&w=2

In this case you could probably get away with not disabling interrupts
anyway, but using the irqsave versions would make it obvious.

> >> +static void dln2_disconnect(struct usb_interface *interface)
> >> +{
> >> +     struct dln2_dev *dln2 = usb_get_intfdata(interface);
> >> +     int i, j;
> >> +
> >> +     /* don't allow starting new transfers */
> >> +     spin_lock(&dln2->disconnect_lock);
> >> +     dln2->disconnect = true;
> >> +     spin_unlock(&dln2->disconnect_lock);
> >> +
> >> +     /* cancel in progress transfers */
> >> +     for (i = 0; i < DLN2_HANDLES; i++) {
> >> +             struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> >> +             unsigned long flags;
> >> +
> >> +             spin_lock_irqsave(&rxs->lock, flags);
> >
> > Just stick to spin_lock in this function.
> 
> AFAICS disconnect is called from a kernel thread. Are there guarantees
> that we can't get a call to the completion routine while we are
> running it?

Brain fart, nevermind.

Johan

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

[Octavian Purdila]

On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
>> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>> >
>> >> +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;
>> >> +
>> >> +     spin_lock(&rxs->lock);
>> >
>> > You must use spin_lock_irqsave here as you call it from the completion
>> > handler.
>>
>> Why? AFAICS the completion handler gets called from the HCD irq handler:
>
> The completion handler is currently called with local interrupts
> disabled but that is about to change once all drivers have been updated:
>
>         http://marc.info/?l=linux-usb&m=137353360511003&w=2
>
> In this case you could probably get away with not disabling interrupts
> anyway, but using the irqsave versions would make it obvious.
>

I was not assuming that interrupts are disabled while running the
completion handler. Since that spinlock is not touched by any other
interrupt context code I don't think irqsave is necessary.

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

[Octavian Purdila]

On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold <johan@kernel.org> wrote:
> On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
>> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>> >
>> >> +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;
>> >> +
>> >> +     spin_lock(&rxs->lock);
>> >
>> > You must use spin_lock_irqsave here as you call it from the completion
>> > handler.
>>
>> Why? AFAICS the completion handler gets called from the HCD irq handler:
>
> The completion handler is currently called with local interrupts
> disabled but that is about to change once all drivers have been updated:
>
>         http://marc.info/?l=linux-usb&m=137353360511003&w=2
>
> In this case you could probably get away with not disabling interrupts
> anyway, but using the irqsave versions would make it obvious.
>

I was not assuming that interrupts are disabled while running the
completion handler. Since that spinlock is not touched by any other
interrupt context code I don't think irqsave is necessary.

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

[Johan Hovold]

On Wed, Oct 08, 2014 at 03:33:28PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> >> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >> >
> >> >> +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;
> >> >> +
> >> >> +     spin_lock(&rxs->lock);
> >> >
> >> > You must use spin_lock_irqsave here as you call it from the completion
> >> > handler.
> >>
> >> Why? AFAICS the completion handler gets called from the HCD irq handler:
> >
> > The completion handler is currently called with local interrupts
> > disabled but that is about to change once all drivers have been updated:
> >
> >         http://marc.info/?l=linux-usb&m=137353360511003&w=2
> >
> > In this case you could probably get away with not disabling interrupts
> > anyway, but using the irqsave versions would make it obvious.
> >
> 
> I was not assuming that interrupts are disabled while running the
> completion handler. Since that spinlock is not touched by any other
> interrupt context code I don't think irqsave is necessary.

No, it isn't necessary so leave it as it is.

But you are exporting interfaces to other drivers and it may appear to
someone that those could possibly end up indirectly calling a function
taking that lock in IRQ context. We know that isn't the case now, but I
bet someone will post conversion patch for that spinlock at some point.
;)

Johan

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

[Johan Hovold]

On Wed, Oct 08, 2014 at 03:33:28PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> >> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >> >
> >> >> +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;
> >> >> +
> >> >> +     spin_lock(&rxs->lock);
> >> >
> >> > You must use spin_lock_irqsave here as you call it from the completion
> >> > handler.
> >>
> >> Why? AFAICS the completion handler gets called from the HCD irq handler:
> >
> > The completion handler is currently called with local interrupts
> > disabled but that is about to change once all drivers have been updated:
> >
> >         http://marc.info/?l=linux-usb&m=137353360511003&w=2
> >
> > In this case you could probably get away with not disabling interrupts
> > anyway, but using the irqsave versions would make it obvious.
> >
> 
> I was not assuming that interrupts are disabled while running the
> completion handler. Since that spinlock is not touched by any other
> interrupt context code I don't think irqsave is necessary.

No, it isn't necessary so leave it as it is.

But you are exporting interfaces to other drivers and it may appear to
someone that those could possibly end up indirectly calling a function
taking that lock in IRQ context. We know that isn't the case now, but I
bet someone will post conversion patch for that spinlock at some point.
;)

Johan