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

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

[Octavian Purdila]

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

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

Changes since v2:

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

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

 * I2C driver: add I2C_FUNC_SMBUS_I2C_BLOCK support

Changes since v1:

 * rewrite the drivers as an MFD

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

 * cleanup the I/O interface

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

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

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

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

 drivers/gpio/Kconfig          |  12 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-dln2.c      | 537 ++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig    |  10 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-dln2.c | 301 +++++++++++++++++++
 drivers/mfd/Kconfig           |   9 +
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/dln2.c            | 653 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/dln2.h      |  61 ++++
 10 files changed, 1586 insertions(+)
 create mode 100644 drivers/gpio/gpio-dln2.c
 create mode 100644 drivers/i2c/busses/i2c-dln2.c
 create mode 100644 drivers/mfd/dln2.c
 create mode 100644 include/linux/mfd/dln2.h

-- 
1.9.1

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

[Octavian Purdila]

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

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

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

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

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

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

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

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/mfd/Kconfig      |   9 +
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/dln2.c       | 653 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/dln2.h |  61 +++++
 4 files changed, 724 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..81ff58e
--- /dev/null
+++ b/drivers/mfd/dln2.c
@@ -0,0 +1,653 @@
+/*
+ * Driver for the Diolan DLN-2 USB adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * Derived from:
+ *  i2c-diolan-u2c.c
+ *  Copyright (c) 2010-2011 Ericsson AB
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/dln2.h>
+
+#define DRIVER_NAME			"usb-dln2"
+
+struct dln2_header {
+	__le16 size;
+	__le16 id;
+	__le16 echo;
+	__le16 handle;
+} __packed;
+
+struct dln2_response {
+	struct dln2_header hdr;
+	__le16 result;
+} __packed;
+
+#define DLN2_GENERIC_MODULE_ID		0x00
+#define DLN2_GENERIC_CMD(cmd)		DLN2_CMD(cmd, DLN2_GENERIC_MODULE_ID)
+#define CMD_GET_DEVICE_VER		DLN2_GENERIC_CMD(0x30)
+#define CMD_GET_DEVICE_SN		DLN2_GENERIC_CMD(0x31)
+
+#define DLN2_HW_ID			0x200
+#define DLN2_USB_TIMEOUT		200	/* in ms */
+#define DLN2_MAX_RX_SLOTS		16
+#define DLN2_MAX_MODULES		5
+#define DLN2_MAX_URBS			16
+
+#define DLN2_HANDLE_GPIO_EVENT		0
+#define DLN2_HANDLE_CTRL		1
+#define DLN2_HANDLE_GPIO		2
+#define DLN2_HANDLE_I2C			3
+
+/* Receive context used between the receive demultiplexer and the
+ * transfer routine. While sending a request the transfer routine
+ * will look for a free receive context and use it to wait for a
+ * response and to receive the URB and thus the response data. */
+struct dln2_rx_context {
+	struct completion done;
+	struct urb *urb;
+	bool connected;
+};
+
+/* Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
+ * use the handle header field to indentify the module in
+ * dln2_dev.mod_rx_slots and then the echo header field to index the
+ * slots field and find the receive context for a particular
+ * request. */
+struct dln2_mod_rx_slots {
+	/* RX slots bitmap */
+	unsigned long bmap;
+
+	/* used to wait for a free RX slot */
+	wait_queue_head_t wq;
+
+	/* used to wait for an RX operation to complete */
+	struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
+
+	/* avoid races between free_rx_slot and dln2_rx_transfer_cb */
+	spinlock_t lock;
+};
+
+struct dln2_dev {
+	struct usb_device *usb_dev;
+	struct usb_interface *interface;
+	u8 ep_in;
+	u8 ep_out;
+
+	struct urb *rx_urb[DLN2_MAX_URBS];
+	void *rx_buf[DLN2_MAX_URBS];
+
+	struct dln2_mod_rx_slots mod_rx_slots[DLN2_MAX_MODULES];
+
+	struct list_head rx_cb_list;
+	spinlock_t rx_cb_lock;
+};
+
+static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&rxs->lock, flags);
+
+	*slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
+
+	if (*slot < DLN2_MAX_RX_SLOTS) {
+		struct dln2_rx_context *rxc = &rxs->slots[*slot];
+
+		init_completion(&rxc->done);
+		set_bit(*slot, &rxs->bmap);
+		rxc->connected = true;
+	}
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	return *slot < DLN2_MAX_RX_SLOTS;
+}
+
+static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
+{
+	int slot, ret;
+
+	/* No need to timeout here, the wait is bounded by the timeout
+	 * in _dln2_transfer */
+	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
+	if (ret < 0)
+		return ret;
+
+	return slot;
+}
+
+static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots *rxs,
+			 int slot)
+{
+	struct urb *urb = NULL;
+	unsigned long flags;
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+	int ret;
+
+	spin_lock_irqsave(&rxs->lock, flags);
+
+	clear_bit(slot, &rxs->bmap);
+
+	rxc = &rxs->slots[slot];
+	rxc->connected = false;
+	urb = rxc->urb;
+	rxc->urb = NULL;
+
+	spin_unlock_irqrestore(&rxs->lock, flags);
+
+	if (urb)  {
+		ret = usb_submit_urb(urb, GFP_KERNEL);
+		if (ret < 0)
+			dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
+	}
+
+	wake_up_interruptible(&rxs->wq);
+}
+
+struct dln2_rx_cb_entry {
+	struct list_head list;
+	u16 id;
+	struct platform_device *pdev;
+	dln2_rx_cb_t callback;
+};
+
+int dln2_register_event_cb(struct platform_device *pdev, u16 id,
+			   dln2_rx_cb_t rx_cb)
+{
+	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
+	struct dln2_rx_cb_entry *i, *new;
+	unsigned long flags;
+	int ret = 0;
+
+	new = kmalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	new->id = id;
+	new->callback = rx_cb;
+	new->pdev = pdev;
+
+	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
+
+	list_for_each_entry(i, &dln2->rx_cb_list, list) {
+		if (i->id == id) {
+			ret = -EBUSY;
+			break;
+		}
+	}
+
+	if (!ret)
+		list_add(&new->list, &dln2->rx_cb_list);
+
+	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
+
+	if (ret)
+		kfree(new);
+
+	return ret;
+}
+EXPORT_SYMBOL(dln2_register_event_cb);
+
+void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
+{
+	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
+	struct dln2_rx_cb_entry *i, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
+
+	list_for_each_entry_safe(i, tmp, &dln2->rx_cb_list, list) {
+		if (i->id == id) {
+			list_del(&i->list);
+			kfree(i);
+		}
+	}
+
+	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
+}
+EXPORT_SYMBOL(dln2_unregister_event_cb);
+
+static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
+			     u16 handle, u16 rx_slot)
+{
+	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+	int err;
+
+	spin_lock(&rxs->lock);
+	rxc = &rxs->slots[rx_slot];
+	if (rxc->connected) {
+		rxc->urb = urb;
+		complete(&rxc->done);
+	} else {
+		dev_warn(dev, "droping response %d/%d", handle, rx_slot);
+		err = usb_submit_urb(urb, GFP_ATOMIC);
+		if (err < 0)
+			dev_err(dev, "failed to re-submit RX URB: %d\n", err);
+	}
+	spin_unlock(&rxs->lock);
+}
+
+static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
+				  void *data, int len)
+{
+	struct dln2_rx_cb_entry *i;
+
+	spin_lock(&dln2->rx_cb_lock);
+
+	list_for_each_entry(i, &dln2->rx_cb_list, list)
+		if (i->id == id)
+			i->callback(i->pdev, echo, data, len);
+
+	spin_unlock(&dln2->rx_cb_lock);
+}
+
+static void dln2_rx(struct urb *urb)
+{
+	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, err;
+
+	switch (urb->status) {
+	case 0:
+		/* success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+	case -EPIPE:
+		/* this urb is terminated, clean up */
+		dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
+		return;
+	default:
+		dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
+		goto out;
+	}
+
+	if (urb->actual_length < sizeof(struct dln2_header)) {
+		dev_err(dev, "short response: %d\n", urb->actual_length);
+		goto out;
+	}
+
+	handle = le16_to_cpu(hdr->handle);
+	id = le16_to_cpu(hdr->id);
+	echo = le16_to_cpu(hdr->echo);
+	size = le16_to_cpu(hdr->size);
+
+	if (size != urb->actual_length) {
+		dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
+			handle, id, echo, size, urb->actual_length);
+		goto out;
+	}
+
+	if (handle > DLN2_MAX_MODULES) {
+		dev_warn(dev, "RX: invalid handle %d\n", handle);
+		goto out;
+	}
+
+	data = urb->transfer_buffer + sizeof(struct dln2_header);
+	len = urb->actual_length - sizeof(struct dln2_header);
+
+	if (!handle) {
+		dln2_run_rx_callbacks(dln2, id, echo, data, len);
+		err = usb_submit_urb(urb, GFP_ATOMIC);
+		if (err < 0)
+			goto out_submit_failed;
+	} else {
+		dln2_rx_transfer(dln2, urb, handle, echo);
+	}
+
+	return;
+
+out:
+	err = usb_submit_urb(urb, GFP_ATOMIC);
+out_submit_failed:
+	if (err < 0)
+		dev_err(dev, "failed to re-submit RX URB: %d\n", err);
+}
+
+static void *dln2_prep_buf(u16 handle, u16 cmd, u16 echo, void *obuf,
+			   int *obuf_len, gfp_t gfp)
+{
+	void *buf;
+	int len;
+	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,
+			  void *obuf, int obuf_len)
+{
+	int len = obuf_len, ret = 0, actual;
+	void *buf;
+
+	buf = dln2_prep_buf(handle, cmd, echo, obuf, &len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = usb_bulk_msg(dln2->usb_dev,
+			   usb_sndbulkpipe(dln2->usb_dev, dln2->ep_out),
+			   buf, len, &actual, DLN2_USB_TIMEOUT);
+
+	kfree(buf);
+
+	return ret;
+}
+
+static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
+			  void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
+{
+	u16 result, rx_slot;
+	struct dln2_response *rsp;
+	int ret = 0;
+	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
+	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
+	struct dln2_rx_context *rxc;
+	struct device *dev = &dln2->interface->dev;
+
+	rx_slot = alloc_rx_slot(rxs);
+	if (rx_slot < 0)
+		return rx_slot;
+
+	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
+	if (ret < 0) {
+		free_rx_slot(dln2, rxs, rx_slot);
+		dev_err(dev, "USB write failed: %d", ret);
+		return ret;
+	}
+
+	rxc = &rxs->slots[rx_slot];
+
+	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
+	if (ret <= 0) {
+		ret = !ret ? -ETIMEDOUT : ret;
+		goto out_free_rx_slot;
+	}
+
+	/* if we got here we know that the response header has been checked */
+	rsp = rxc->urb->transfer_buffer;
+	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 > rxc->urb->actual_length - sizeof(*rsp))
+		*ibuf_len = rxc->urb->actual_length - sizeof(*rsp);
+
+	memcpy(ibuf, rsp + 1, *ibuf_len);
+
+out_free_rx_slot:
+	free_rx_slot(dln2, rxs, rx_slot);
+
+	return ret;
+}
+
+struct dln2_platform_data {
+	u16 handle;
+};
+
+int dln2_transfer(struct platform_device *pdev, u16 cmd,
+		  void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
+{
+	struct dln2_platform_data *dln2_pdata;
+	struct dln2_dev *dln2;
+	u16 h;
+
+	/* USB device has been disconnected, bail out */
+	if (!pdev)
+		return -ENODEV;
+
+	dln2 = dev_get_drvdata(pdev->dev.parent);
+	dln2_pdata = dev_get_platdata(&pdev->dev);
+	h = dln2_pdata->handle;
+
+	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
+}
+EXPORT_SYMBOL(dln2_transfer);
+
+static int dln2_check_hw(struct dln2_dev *dln2)
+{
+	__le32 hw_type;
+	int ret, 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 (le32_to_cpu(hw_type) != DLN2_HW_ID) {
+		dev_err(&dln2->interface->dev, "Device ID 0x%x not supported!",
+			le32_to_cpu(hw_type));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int dln2_print_serialno(struct dln2_dev *dln2)
+{
+	__le32 serial_no;
+	int ret, 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;
+
+	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 = 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, ret;
+	int rx_max_size = le16_to_cpu(hostif->endpoint[1].desc.wMaxPacketSize);
+	struct device *dev = &dln2->interface->dev;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
+		if (!dln2->rx_buf[i])
+			return -ENOMEM;
+
+		dln2->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
+		if (!dln2->rx_urb[i])
+			return -ENOMEM;
+
+		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
+				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
+				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
+
+		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
+		if (ret < 0) {
+			dev_err(dev, "failed to submit RX URB: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static struct dln2_platform_data dln2_pdata_gpio = {
+	.handle = DLN2_HANDLE_GPIO,
+};
+
+static struct dln2_platform_data dln2_pdata_i2c = {
+	.handle = DLN2_HANDLE_I2C,
+};
+
+static const struct mfd_cell dln2_devs[] = {
+	{
+		.name	= "dln2-gpio",
+		.platform_data = &dln2_pdata_gpio,
+		.pdata_size = sizeof(struct dln2_platform_data),
+	},
+	{
+		.name	= "dln2-i2c",
+		.platform_data = &dln2_pdata_i2c,
+		.pdata_size = sizeof(struct dln2_platform_data),
+	},
+};
+
+static void dln2_disconnect(struct usb_interface *interface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(interface);
+
+	mfd_remove_devices(&interface->dev);
+	dln2_free(dln2);
+}
+
+static int dln2_probe(struct usb_interface *interface,
+		      const struct usb_device_id *id)
+{
+	struct usb_host_interface *hostif = interface->cur_altsetting;
+	struct device *dev = &interface->dev;
+	struct dln2_dev *dln2;
+	int ret, i;
+
+	if (hostif->desc.bInterfaceNumber != 0 ||
+	    hostif->desc.bNumEndpoints < 2)
+		return -ENODEV;
+
+	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
+	if (!dln2) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
+	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
+	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
+	dln2->interface = interface;
+	usb_set_intfdata(interface, dln2);
+
+	for (i = 0; i < DLN2_MAX_MODULES; i++) {
+		init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
+		spin_lock_init(&dln2->mod_rx_slots[i].lock);
+	}
+
+	spin_lock_init(&dln2->rx_cb_lock);
+	INIT_LIST_HEAD(&dln2->rx_cb_list);
+
+	ret = dln2_setup_rx_urbs(dln2, hostif);
+	if (ret) {
+		dln2_disconnect(interface);
+		return ret;
+	}
+
+	ret = dln2_hw_init(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize hardware\n");
+		goto out_cleanup;
+	}
+
+	ret = mfd_add_devices(dev, -1, dln2_devs, ARRAY_SIZE(dln2_devs),
+			      NULL, 0, NULL);
+	if (ret != 0) {
+		dev_err(dev, "Failed to add mfd devices to core.\n");
+		goto out_cleanup;
+	}
+
+	return 0;
+
+out_cleanup:
+	dln2_free(dln2);
+out:
+	return ret;
+}
+
+static const struct usb_device_id dln2_table[] = {
+	{ USB_DEVICE(0xa257, 0x2013) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(usb, dln2_table);
+
+static struct usb_driver dln2_driver = {
+	.name = DRIVER_NAME,
+	.probe = dln2_probe,
+	.disconnect = dln2_disconnect,
+	.id_table = dln2_table,
+};
+
+module_usb_driver(dln2_driver);
+
+MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
new file mode 100644
index 0000000..731bf32
--- /dev/null
+++ b/include/linux/mfd/dln2.h
@@ -0,0 +1,61 @@
+#ifndef __LINUX_USB_DLN2_H
+#define __LINUX_USB_DLN2_H
+
+#define DLN2_CMD(cmd, id)		((cmd) | ((id) << 8))
+
+/**
+ * dln2_rx_cb - event callback function signature
+ *
+ * @pdev - the sub-device that registered this callback
+ * @echo - the echo header field received in the message
+ * @data - the data payload
+ * @len  - the data playload length
+ *
+ * The callback function is called in interrupt context and the data
+ * payload is only valid during the call. If the user needs later
+ * access of the data, it must copy it.
+ */
+
+typedef void (*dln2_rx_cb_t)(struct platform_device *pdev, u16 echo,
+			     const void *data, int len);
+
+/**
+ * dl2n_register_event_cb - register a callback function for an event
+ *
+ * @pdev - the sub-device that registers the callback
+ * @event - the event for which to register a callback
+ * @rx_cb - the callback function
+ *
+ * @return 0 in case of success, negative value in case of error
+ */
+int dln2_register_event_cb(struct platform_device *pdev, u16 event,
+			   dln2_rx_cb_t rx_cb);
+
+/**
+ * dln2_unregister_event_cb - unregister the callback function for an event
+ *
+ * @pdev - the sub-device that registered the callback
+ * @event - the event for which to register a callback
+ */
+void dln2_unregister_event_cb(struct platform_device *pdev, u16 event);
+
+/**
+ * dln2_transfer - issue a DLN2 command and wait for a response and
+ * the associated data
+ *
+ * @pdev - the sub-device which is issueing 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,
+		  void *obuf, int obuf_len, void *ibuf, int *ibuf_len);
+
+#endif
-- 
1.9.1

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 06:17:57PM +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       | 653 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h |  61 +++++
>  4 files changed, 724 insertions(+)
>  create mode 100644 drivers/mfd/dln2.c
>  create mode 100644 include/linux/mfd/dln2.h

First of all, this is much cleaner than the first non-MFD version. I'll
have a look at the subdrivers shortly as well.

>
> 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..81ff58e
> --- /dev/null
> +++ b/drivers/mfd/dln2.c
> @@ -0,0 +1,653 @@
> +/*
> + * Driver for the Diolan DLN-2 USB adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + *  i2c-diolan-u2c.c
> + *  Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME			"usb-dln2"
> +
> +struct dln2_header {
> +	__le16 size;
> +	__le16 id;
> +	__le16 echo;
> +	__le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> +	struct dln2_header hdr;
> +	__le16 result;
> +} __packed;
> +
> +#define DLN2_GENERIC_MODULE_ID		0x00
> +#define DLN2_GENERIC_CMD(cmd)		DLN2_CMD(cmd, DLN2_GENERIC_MODULE_ID)
> +#define CMD_GET_DEVICE_VER		DLN2_GENERIC_CMD(0x30)
> +#define CMD_GET_DEVICE_SN		DLN2_GENERIC_CMD(0x31)
> +
> +#define DLN2_HW_ID			0x200
> +#define DLN2_USB_TIMEOUT		200	/* in ms */
> +#define DLN2_MAX_RX_SLOTS		16
> +#define DLN2_MAX_MODULES		5
> +#define DLN2_MAX_URBS			16
> +
> +#define DLN2_HANDLE_GPIO_EVENT		0

Just DLN2_HANDLE_EVENT?

> +#define DLN2_HANDLE_CTRL		1
> +#define DLN2_HANDLE_GPIO		2
> +#define DLN2_HANDLE_I2C			3
> +
> +/* Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data. */

The recommended style for multi-line comments is

	/*
	 * ...
	 */

This applies to all three patches.

> +struct dln2_rx_context {
> +	struct completion done;
> +	struct urb *urb;
> +	bool connected;
> +};
> +
> +/* Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to indentify the module in
> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> + * slots field and find the receive context for a particular
> + * request. */
> +struct dln2_mod_rx_slots {
> +	/* RX slots bitmap */
> +	unsigned long bmap;
> +
> +	/* used to wait for a free RX slot */
> +	wait_queue_head_t wq;
> +
> +	/* used to wait for an RX operation to complete */
> +	struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> +	/* avoid races between free_rx_slot and dln2_rx_transfer_cb */
> +	spinlock_t lock;
> +};
> +
> +struct dln2_dev {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *interface;
> +	u8 ep_in;
> +	u8 ep_out;
> +
> +	struct urb *rx_urb[DLN2_MAX_URBS];
> +	void *rx_buf[DLN2_MAX_URBS];
> +
> +	struct dln2_mod_rx_slots mod_rx_slots[DLN2_MAX_MODULES];
> +
> +	struct list_head rx_cb_list;
> +	spinlock_t rx_cb_lock;
> +};
> +
> +static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	*slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
> +
> +	if (*slot < DLN2_MAX_RX_SLOTS) {
> +		struct dln2_rx_context *rxc = &rxs->slots[*slot];
> +
> +		init_completion(&rxc->done);

Initialise the completions when you allocate them (and there's no need
to re-init here).

> +		set_bit(*slot, &rxs->bmap);
> +		rxc->connected = true;
> +	}
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	return *slot < DLN2_MAX_RX_SLOTS;
> +}
> +
> +static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
> +{
> +	int slot, ret;
> +
> +	/* No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer */
> +	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots *rxs,
> +			 int slot)
> +{
> +	struct urb *urb = NULL;
> +	unsigned long flags;
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int ret;
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	clear_bit(slot, &rxs->bmap);
> +
> +	rxc = &rxs->slots[slot];
> +	rxc->connected = false;
> +	urb = rxc->urb;
> +	rxc->urb = NULL;
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	if (urb)  {
> +		ret = usb_submit_urb(urb, GFP_KERNEL);
> +		if (ret < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
> +	}
> +
> +	wake_up_interruptible(&rxs->wq);
> +}
> +
> +struct dln2_rx_cb_entry {
> +	struct list_head list;
> +	u16 id;
> +	struct platform_device *pdev;
> +	dln2_rx_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +			   dln2_rx_cb_t rx_cb)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_rx_cb_entry *i, *new;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	new = kmalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new->id = id;
> +	new->callback = rx_cb;
> +	new->pdev = pdev;
> +
> +	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->rx_cb_list, list) {
> +		if (i->id == id) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +
> +	if (!ret)
> +		list_add(&new->list, &dln2->rx_cb_list);
> +
> +	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
> +
> +	if (ret)
> +		kfree(new);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(dln2_register_event_cb);
> +
> +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_rx_cb_entry *i, *tmp;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dln2->rx_cb_lock, flags);
> +
> +	list_for_each_entry_safe(i, tmp, &dln2->rx_cb_list, list) {
> +		if (i->id == id) {
> +			list_del(&i->list);
> +			kfree(i);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&dln2->rx_cb_lock, flags);
> +}
> +EXPORT_SYMBOL(dln2_unregister_event_cb);
> +
> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> +			     u16 handle, u16 rx_slot)
> +{
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int err;
> +
> +	spin_lock(&rxs->lock);
> +	rxc = &rxs->slots[rx_slot];
> +	if (rxc->connected) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +	} else {
> +		dev_warn(dev, "droping response %d/%d", handle, rx_slot);

s/droping/dropping/

> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +	}
> +	spin_unlock(&rxs->lock);
> +}
> +
> +static void dln2_run_rx_callbacks(struct dln2_dev *dln2, u16 id, u16 echo,
> +				  void *data, int len)
> +{
> +	struct dln2_rx_cb_entry *i;
> +
> +	spin_lock(&dln2->rx_cb_lock);
> +
> +	list_for_each_entry(i, &dln2->rx_cb_list, list)
> +		if (i->id == id)
> +			i->callback(i->pdev, echo, data, len);
> +
> +	spin_unlock(&dln2->rx_cb_lock);
> +}
> +
> +static void dln2_rx(struct urb *urb)
> +{
> +	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, err;
> +
> +	switch (urb->status) {
> +	case 0:
> +		/* success */
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +	case -EPIPE:
> +		/* this urb is terminated, clean up */
> +		dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> +		return;
> +	default:
> +		dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> +		goto out;
> +	}
> +
> +	if (urb->actual_length < sizeof(struct dln2_header)) {
> +		dev_err(dev, "short response: %d\n", urb->actual_length);
> +		goto out;
> +	}
> +
> +	handle = le16_to_cpu(hdr->handle);
> +	id = le16_to_cpu(hdr->id);
> +	echo = le16_to_cpu(hdr->echo);
> +	size = le16_to_cpu(hdr->size);
> +
> +	if (size != urb->actual_length) {
> +		dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
> +			handle, id, echo, size, urb->actual_length);
> +		goto out;
> +	}
> +
> +	if (handle > DLN2_MAX_MODULES) {

You need (handle >= DLN2_MAX_MODULES) or you might end up with a buffer
overflow in dln2_run_rx_callbacks.

> +		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) {

Test if (handle == DLN2_HANDLE_EVENT) instead.

> +		dln2_run_rx_callbacks(dln2, id, echo, data, len);
> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			goto out_submit_failed;
> +	} else {
> +		dln2_rx_transfer(dln2, urb, handle, echo);
> +	}
> +
> +	return;
> +
> +out:
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +out_submit_failed:
> +	if (err < 0)
> +		dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +}
> +
> +static void *dln2_prep_buf(u16 handle, u16 cmd, u16 echo, void *obuf,
> +			   int *obuf_len, gfp_t gfp)
> +{
> +	void *buf;
> +	int len;
> +	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,
> +			  void *obuf, int obuf_len)
> +{
> +	int len = obuf_len, ret = 0, actual;
> +	void *buf;
> +
> +	buf = dln2_prep_buf(handle, cmd, echo, obuf, &len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = usb_bulk_msg(dln2->usb_dev,
> +			   usb_sndbulkpipe(dln2->usb_dev, dln2->ep_out),
> +			   buf, len, &actual, DLN2_USB_TIMEOUT);
> +
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> +			  void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
> +{
> +	u16 result, rx_slot;
> +	struct dln2_response *rsp;
> +	int ret = 0;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +
> +	rx_slot = alloc_rx_slot(rxs);
> +	if (rx_slot < 0)
> +		return rx_slot;
> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		free_rx_slot(dln2, rxs, rx_slot);
> +		dev_err(dev, "USB write failed: %d", ret);
> +		return ret;
> +	}
> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> +	if (ret <= 0) {
> +		ret = !ret ? -ETIMEDOUT : ret;

Don't use use ?: constructs.

Setting ret to -ETIMEDOUT if 0 is much more readable.

> +		goto out_free_rx_slot;
> +	}
> +
> +	/* if we got here we know that the response header has been checked */
> +	rsp = rxc->urb->transfer_buffer;
> +	result = le16_to_cpu(rsp->result);

Yes, but you haven't verified that rsp->hdr.size > 0, so you may still
be reading stale data.

> +
> +	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 > rxc->urb->actual_length - sizeof(*rsp))
> +		*ibuf_len = rxc->urb->actual_length - sizeof(*rsp);

And then you get an underflow here, although that doesn't seem to cause
any troubles in this case.

But why isn't ibuf_len unsigned?

> +
> +	memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> +	free_rx_slot(dln2, rxs, rx_slot);
> +
> +	return ret;
> +}
> +
> +struct dln2_platform_data {
> +	u16 handle;
> +};
> +
> +int dln2_transfer(struct platform_device *pdev, u16 cmd,
> +		  void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
> +{
> +	struct dln2_platform_data *dln2_pdata;
> +	struct dln2_dev *dln2;
> +	u16 h;
> +
> +	/* USB device has been disconnected, bail out */
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	dln2 = dev_get_drvdata(pdev->dev.parent);
> +	dln2_pdata = dev_get_platdata(&pdev->dev);
> +	h = dln2_pdata->handle;
> +
> +	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
> +}
> +EXPORT_SYMBOL(dln2_transfer);
> +
> +static int dln2_check_hw(struct dln2_dev *dln2)
> +{
> +	__le32 hw_type;
> +	int ret, len = sizeof(hw_type);

Again, please use one declaration per line (and especially when
initialising).

> +
> +	ret = _dln2_transfer(dln2, DLN2_HANDLE_CTRL, CMD_GET_DEVICE_VER,
> +			     NULL, 0, &hw_type, &len);
> +	if (ret < 0)
> +		return ret;
> +

You never check the returned buffer length.

> +	if (le32_to_cpu(hw_type) != DLN2_HW_ID) {
> +		dev_err(&dln2->interface->dev, "Device ID 0x%x not supported!",

Exclamation mark really warranted?

> +			le32_to_cpu(hw_type));
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dln2_print_serialno(struct dln2_dev *dln2)
> +{
> +	__le32 serial_no;
> +	int ret, 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;

Verify buffer length.

> +
> +	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 = dln2_check_hw(dln2);

Split declaration and non-trivial initialisation.

> +
> +	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, ret;
> +	int rx_max_size = le16_to_cpu(hostif->endpoint[1].desc.wMaxPacketSize);
> +	struct device *dev = &dln2->interface->dev;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> +		if (!dln2->rx_buf[i])
> +			return -ENOMEM;
> +
> +		dln2->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!dln2->rx_urb[i])
> +			return -ENOMEM;
> +
> +		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
> +				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
> +				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
> +
> +		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to submit RX URB: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct dln2_platform_data dln2_pdata_gpio = {
> +	.handle = DLN2_HANDLE_GPIO,
> +};
> +
> +static struct dln2_platform_data dln2_pdata_i2c = {
> +	.handle = DLN2_HANDLE_I2C,
> +};
> +
> +static const struct mfd_cell dln2_devs[] = {
> +	{
> +		.name	= "dln2-gpio",
> +		.platform_data = &dln2_pdata_gpio,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +	{
> +		.name	= "dln2-i2c",
> +		.platform_data = &dln2_pdata_i2c,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +};
> +
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	mfd_remove_devices(&interface->dev);
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret, i;
> +
> +	if (hostif->desc.bInterfaceNumber != 0 ||
> +	    hostif->desc.bNumEndpoints < 2)
> +		return -ENODEV;
> +
> +	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2) {
> +		ret = -ENOMEM;
> +		goto out;

Why not simply return -ENOMEM here as well?

> +	}
> +
> +	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
> +	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
> +	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	dln2->interface = interface;
> +	usb_set_intfdata(interface, dln2);
> +
> +	for (i = 0; i < DLN2_MAX_MODULES; i++) {
> +		init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
> +		spin_lock_init(&dln2->mod_rx_slots[i].lock);
> +	}
> +
> +	spin_lock_init(&dln2->rx_cb_lock);
> +	INIT_LIST_HEAD(&dln2->rx_cb_list);
> +
> +	ret = dln2_setup_rx_urbs(dln2, hostif);
> +	if (ret) {
> +		dln2_disconnect(interface);

Call dln2_free directly here instead, and only let USB core use the USB
callbacks directly (i.e. disconnect should only be called after a
successful probe).

> +		return ret;
> +	}
> +
> +	ret = dln2_hw_init(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize hardware\n");
> +		goto out_cleanup;
> +	}
> +
> +	ret = mfd_add_devices(dev, -1, dln2_devs, ARRAY_SIZE(dln2_devs),
> +			      NULL, 0, NULL);
> +	if (ret != 0) {
> +		dev_err(dev, "Failed to add mfd devices to core.\n");
> +		goto out_cleanup;
> +	}
> +
> +	return 0;
> +
> +out_cleanup:
> +	dln2_free(dln2);
> +out:
> +	return ret;
> +}
> +
> +static const struct usb_device_id dln2_table[] = {
> +	{ USB_DEVICE(0xa257, 0x2013) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, dln2_table);
> +
> +static struct usb_driver dln2_driver = {
> +	.name = DRIVER_NAME,
> +	.probe = dln2_probe,
> +	.disconnect = dln2_disconnect,
> +	.id_table = dln2_table,
> +};
> +
> +module_usb_driver(dln2_driver);
> +
> +MODULE_AUTHOR("Octavian Purdila <octavian.purdila@intel.com>");
> +MODULE_DESCRIPTION(DRIVER_NAME " driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/dln2.h b/include/linux/mfd/dln2.h
> new file mode 100644
> index 0000000..731bf32
> --- /dev/null
> +++ b/include/linux/mfd/dln2.h
> @@ -0,0 +1,61 @@
> +#ifndef __LINUX_USB_DLN2_H
> +#define __LINUX_USB_DLN2_H
> +
> +#define DLN2_CMD(cmd, id)		((cmd) | ((id) << 8))
> +
> +/**
> + * dln2_rx_cb - event callback function signature
> + *
> + * @pdev - the sub-device that registered this callback
> + * @echo - the echo header field received in the message
> + * @data - the data payload
> + * @len  - the data playload length

s/playload/payload/

> + *
> + * The callback function is called in interrupt context and the data
> + * payload is only valid during the call. If the user needs later
> + * access of the data, it must copy it.
> + */
> +
> +typedef void (*dln2_rx_cb_t)(struct platform_device *pdev, u16 echo,
> +			     const void *data, int len);
> +
> +/**
> + * dl2n_register_event_cb - register a callback function for an event
> + *
> + * @pdev - the sub-device that registers the callback
> + * @event - the event for which to register a callback
> + * @rx_cb - the callback function
> + *
> + * @return 0 in case of success, negative value in case of error
> + */
> +int dln2_register_event_cb(struct platform_device *pdev, u16 event,
> +			   dln2_rx_cb_t rx_cb);
> +
> +/**
> + * dln2_unregister_event_cb - unregister the callback function for an event
> + *
> + * @pdev - the sub-device that registered the callback
> + * @event - the event for which to register a callback
> + */
> +void dln2_unregister_event_cb(struct platform_device *pdev, u16 event);
> +
> +/**
> + * dln2_transfer - issue a DLN2 command and wait for a response and
> + * the associated data
> + *
> + * @pdev - the sub-device which is issueing this transfer

s/issueing/issuing/

> + * @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,
> +		  void *obuf, int obuf_len, void *ibuf, int *ibuf_len);
> +
> +#endif

Johan

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

[Johan Hovold]

On Mon, Sep 08, 2014 at 01:32:33PM +0200, Johan Hovold wrote:
> On Fri, Sep 05, 2014 at 06:17:57PM +0300, Octavian Purdila wrote:

> > +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> > +			  void *obuf, int obuf_len, void *ibuf, int *ibuf_len)
> > +{

<snip>

> > +	/* if we got here we know that the response header has been checked */
> > +	rsp = rxc->urb->transfer_buffer;
> > +	result = le16_to_cpu(rsp->result);
> 
> Yes, but you haven't verified that rsp->hdr.size > 0, so you may still
> be reading stale data.

I meant that you haven't verified that the payload size > 1 (the header
size is included in rsp->hdr.size and result is two byte wide).

> > +
> > +	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 > rxc->urb->actual_length - sizeof(*rsp))
> > +		*ibuf_len = rxc->urb->actual_length - sizeof(*rsp);
> 
> And then you get an underflow here, although that doesn't seem to cause
> any troubles in this case.

Unless ibuf_len is -1... 

> But why isn't ibuf_len unsigned?
>
> > +
> > +	memcpy(ibuf, rsp + 1, *ibuf_len);
> > +
> > +out_free_rx_slot:
> > +	free_rx_slot(dln2, rxs, rx_slot);
> > +
> > +	return ret;
> > +}

Johan

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

[Octavian Purdila]

On Mon, Sep 8, 2014 at 2:32 PM, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Sep 05, 2014 at 06:17:57PM +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>
>
> First of all, this is much cleaner than the first non-MFD version. I'll
> have a look at the subdrivers shortly as well.
>

Hi Johan,

I have addressed all you comments except one, please see bellow. I
will wait for the review on the subdrivers before submitting a new
version of the patch set. Thanks for for review !

<snip>
> > +static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&rxs->lock, flags);
> > +
> > +     *slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
> > +
> > +     if (*slot < DLN2_MAX_RX_SLOTS) {
> > +             struct dln2_rx_context *rxc = &rxs->slots[*slot];
> > +
> > +             init_completion(&rxc->done);
>
> Initialise the completions when you allocate them (and there's no need
> to re-init here).
>

You are right, but I think we need a reinit_completion(). Technically
we don't, as we don't use complete_all(), but I feel it is cleaner
that way. I will move the initialization in probe and add the
reinit_completion() in free_rx_slot. Is that OK with you?

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

[Lee Jones]

On Mon, 08 Sep 2014, Octavian Purdila wrote:
> On Mon, Sep 8, 2014 at 2:32 PM, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Sep 05, 2014 at 06:17:57PM +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>
> >
> > First of all, this is much cleaner than the first non-MFD version. I'll
> > have a look at the subdrivers shortly as well.
> >
> 
> Hi Johan,
> 
> I have addressed all you comments except one, please see bellow. I
> will wait for the review on the subdrivers before submitting a new
> version of the patch set. Thanks for for review !

No, please don't.  Fix-up and resubmit.

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

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

[Johan Hovold]

On Mon, Sep 08, 2014 at 04:20:34PM +0300, Octavian Purdila wrote:
> On Mon, Sep 8, 2014 at 2:32 PM, Johan Hovold <johan@kernel.org> wrote:

> > > +static bool find_free_slot(struct dln2_mod_rx_slots *rxs, int *slot)
> > > +{
> > > +     unsigned long flags;
> > > +
> > > +     spin_lock_irqsave(&rxs->lock, flags);
> > > +
> > > +     *slot = find_first_zero_bit(&rxs->bmap, DLN2_MAX_RX_SLOTS);
> > > +
> > > +     if (*slot < DLN2_MAX_RX_SLOTS) {
> > > +             struct dln2_rx_context *rxc = &rxs->slots[*slot];
> > > +
> > > +             init_completion(&rxc->done);
> >
> > Initialise the completions when you allocate them (and there's no need
> > to re-init here).
> 
> You are right, but I think we need a reinit_completion(). Technically
> we don't, as we don't use complete_all(), but I feel it is cleaner
> that way. I will move the initialization in probe and add the
> reinit_completion() in free_rx_slot. Is that OK with you?

Sure, that's fine. 

Johan

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

[Octavian Purdila]

From: Laurentiu Palcu <laurentiu.palcu@intel.com>

This patch adds support for the Diolan DLN-2 I2C master module. Due
to hardware limitations it does not support SMBUS quick commands.

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

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

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

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..4873161 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 USB && 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..93e85ff
--- /dev/null
+++ b/drivers/i2c/busses/i2c-dln2.c
@@ -0,0 +1,301 @@
+/*
+ * Driver for the Diolan DLN-2 USB-I2C adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * Derived from:
+ *  i2c-diolan-u2c.c
+ *  Copyright (c) 2010-2011 Ericsson AB
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+
+#define DRIVER_NAME			"dln2-i2c"
+
+#define DLN2_I2C_MODULE_ID		0x03
+#define DLN2_I2C_CMD(cmd)		DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
+
+/* I2C commands */
+#define DLN2_I2C_GET_PORT_COUNT		DLN2_I2C_CMD(0x00)
+#define DLN2_I2C_ENABLE			DLN2_I2C_CMD(0x01)
+#define DLN2_I2C_DISABLE			DLN2_I2C_CMD(0x02)
+#define DLN2_I2C_IS_ENABLED		DLN2_I2C_CMD(0x03)
+#define DLN2_I2C_SET_FREQUENCY		DLN2_I2C_CMD(0x04)
+#define DLN2_I2C_GET_FREQUENCY		DLN2_I2C_CMD(0x05)
+#define DLN2_I2C_WRITE			DLN2_I2C_CMD(0x06)
+#define DLN2_I2C_READ			DLN2_I2C_CMD(0x07)
+#define DLN2_I2C_SCAN_DEVICES		DLN2_I2C_CMD(0x08)
+#define DLN2_I2C_PULLUP_ENABLE		DLN2_I2C_CMD(0x09)
+#define DLN2_I2C_PULLUP_DISABLE		DLN2_I2C_CMD(0x0A)
+#define DLN2_I2C_PULLUP_IS_ENABLED	DLN2_I2C_CMD(0x0B)
+#define DLN2_I2C_TRANSFER		DLN2_I2C_CMD(0x0C)
+#define DLN2_I2C_SET_MAX_REPLY_COUNT	DLN2_I2C_CMD(0x0D)
+#define DLN2_I2C_GET_MAX_REPLY_COUNT	DLN2_I2C_CMD(0x0E)
+#define DLN2_I2C_GET_MIN_FREQUENCY	DLN2_I2C_CMD(0x40)
+#define DLN2_I2C_GET_MAX_FREQUENCY	DLN2_I2C_CMD(0x41)
+
+#define DLN2_I2C_FREQ_FAST		400000
+#define DLN2_I2C_FREQ_STD		100000
+
+#define DLN2_I2C_MAX_XFER_SIZE		256
+
+struct dln2_i2c {
+	struct platform_device *pdev;
+	struct i2c_adapter adapter;
+};
+
+static uint frequency = DLN2_I2C_FREQ_STD;	/* I2C clock frequency in Hz */
+
+module_param(frequency, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(frequency, "I2C clock frequency in hertz");
+
+static int dln2_i2c_set_state(struct dln2_i2c *dln2, u8 state)
+{
+	int ret;
+	u8 port = 0;
+
+	ret = dln2_transfer(dln2->pdev,
+			    state ? DLN2_I2C_ENABLE : DLN2_I2C_DISABLE,
+			    &port, sizeof(port), NULL, NULL);
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+#define dln2_i2c_enable(dln2)	dln2_i2c_set_state(dln2, 1)
+#define dln2_i2c_disable(dln2)	dln2_i2c_set_state(dln2, 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 = 0;
+	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;
+
+	return 0;
+}
+
+static int dln2_i2c_write(struct dln2_i2c *dln2, u8 addr,
+			  u8 *data, u16 data_len)
+{
+	int ret, len;
+	struct tx_data {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed tx;
+
+	if (data_len > DLN2_I2C_MAX_XFER_SIZE)
+		return -ENOSPC;
+
+	tx.port = 0;
+	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)
+{
+	struct tx_data {
+		u8 port;
+		u8 addr;
+		u8 mem_addr_len;
+		__le32 mem_addr;
+		__le16 buf_len;
+	} __packed tx;
+	struct rx_data {
+		__le16 buf_len;
+		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
+	} __packed rx;
+	int ret, buf_len, rx_len = sizeof(rx);
+
+	tx.port = 0;
+	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 < 2)
+		return -EPROTO;
+
+	buf_len = le16_to_cpu(rx.buf_len);
+	if (rx_len + sizeof(__le16) <  buf_len)
+		return -EPROTO;
+
+	memcpy(data, rx.buf, buf_len);
+
+	return buf_len;
+}
+
+static int dln2_i2c_setup(struct dln2_i2c *dln2)
+{
+	int ret;
+
+	ret = dln2_i2c_disable(dln2);
+	if (ret < 0)
+		return ret;
+
+	/* Set I2C frequency */
+	ret = dln2_i2c_set_frequency(dln2, frequency);
+	if (ret < 0)
+		return ret;
+
+	ret = dln2_i2c_enable(dln2);
+
+	return ret;
+}
+
+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;
+	int ret = 0;
+
+	for (i = 0; i < num; i++) {
+		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;
+}
+
+/*
+ * Return list of supported functionality.
+ */
+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,
+};
+
+/* device layer */
+
+static int dln2_i2c_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct dln2_i2c *dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
+
+	if (!dln2)
+		return -ENOMEM;
+
+	dln2->pdev = pdev;
+
+	/* setup i2c adapter description */
+	dln2->adapter.owner = THIS_MODULE;
+	dln2->adapter.class = I2C_CLASS_HWMON;
+	dln2->adapter.algo = &dln2_i2c_usb_algorithm;
+	dln2->adapter.dev.parent = dev;
+	i2c_set_adapdata(&dln2->adapter, dln2);
+	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), DRIVER_NAME);
+
+	/* initialize the i2c interface */
+	ret = dln2_i2c_setup(dln2);
+	if (ret < 0) {
+		dev_err(dev, "failed to initialize adapter\n");
+		goto error_free;
+	}
+
+	/* and finally attach to i2c layer */
+	ret = i2c_add_adapter(&dln2->adapter);
+	if (ret < 0) {
+		dev_err(dev, "failed to add I2C adapter\n");
+		goto error_free;
+	}
+
+	platform_set_drvdata(pdev, dln2);
+
+	return 0;
+
+error_free:
+	kfree(dln2);
+
+	return ret;
+}
+
+static int dln2_i2c_remove(struct platform_device *pdev)
+{
+	struct dln2_i2c *dln2 = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dln2->adapter);
+
+	return 0;
+}
+
+static struct platform_driver dln2_i2c_driver = {
+	.driver.name	= DRIVER_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= dln2_i2c_probe,
+	.remove		= dln2_i2c_remove,
+};
+
+module_platform_driver(dln2_i2c_driver);
+
+MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 06:17:58PM +0300, Octavian Purdila wrote:
> From: Laurentiu Palcu <laurentiu.palcu@intel.com>
> 
> This patch adds support for the Diolan DLN-2 I2C master module. Due
> to hardware limitations it does not support SMBUS quick commands.
> 
> Information about the USB protocol interface can be found in the
> Programmer's Reference Manual [1], see section 6.2.2 for the I2C
> master module commands and responses.
> 
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/i2c/busses/Kconfig    |  10 ++
>  drivers/i2c/busses/Makefile   |   1 +
>  drivers/i2c/busses/i2c-dln2.c | 301 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 312 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-dln2.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..4873161 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 USB && MFD_DLN2

MFD_DLN2 should be sufficient.

> +       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..93e85ff
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dln2.c
> @@ -0,0 +1,301 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-I2C adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Derived from:
> + *  i2c-diolan-u2c.c
> + *  Copyright (c) 2010-2011 Ericsson AB
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +

No newline.

> +#include <linux/platform_device.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME			"dln2-i2c"
> +
> +#define DLN2_I2C_MODULE_ID		0x03
> +#define DLN2_I2C_CMD(cmd)		DLN2_CMD(cmd, DLN2_I2C_MODULE_ID)
> +
> +/* I2C commands */
> +#define DLN2_I2C_GET_PORT_COUNT		DLN2_I2C_CMD(0x00)
> +#define DLN2_I2C_ENABLE			DLN2_I2C_CMD(0x01)
> +#define DLN2_I2C_DISABLE			DLN2_I2C_CMD(0x02)
> +#define DLN2_I2C_IS_ENABLED		DLN2_I2C_CMD(0x03)
> +#define DLN2_I2C_SET_FREQUENCY		DLN2_I2C_CMD(0x04)
> +#define DLN2_I2C_GET_FREQUENCY		DLN2_I2C_CMD(0x05)
> +#define DLN2_I2C_WRITE			DLN2_I2C_CMD(0x06)
> +#define DLN2_I2C_READ			DLN2_I2C_CMD(0x07)
> +#define DLN2_I2C_SCAN_DEVICES		DLN2_I2C_CMD(0x08)
> +#define DLN2_I2C_PULLUP_ENABLE		DLN2_I2C_CMD(0x09)
> +#define DLN2_I2C_PULLUP_DISABLE		DLN2_I2C_CMD(0x0A)
> +#define DLN2_I2C_PULLUP_IS_ENABLED	DLN2_I2C_CMD(0x0B)
> +#define DLN2_I2C_TRANSFER		DLN2_I2C_CMD(0x0C)
> +#define DLN2_I2C_SET_MAX_REPLY_COUNT	DLN2_I2C_CMD(0x0D)
> +#define DLN2_I2C_GET_MAX_REPLY_COUNT	DLN2_I2C_CMD(0x0E)
> +#define DLN2_I2C_GET_MIN_FREQUENCY	DLN2_I2C_CMD(0x40)
> +#define DLN2_I2C_GET_MAX_FREQUENCY	DLN2_I2C_CMD(0x41)
> +
> +#define DLN2_I2C_FREQ_FAST		400000
> +#define DLN2_I2C_FREQ_STD		100000
> +
> +#define DLN2_I2C_MAX_XFER_SIZE		256
> +
> +struct dln2_i2c {
> +	struct platform_device *pdev;
> +	struct i2c_adapter adapter;
> +};
> +
> +static uint frequency = DLN2_I2C_FREQ_STD;	/* I2C clock frequency in Hz */
> +
> +module_param(frequency, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(frequency, "I2C clock frequency in hertz");

These seems like a very bad idea. Why set one common frequency for all
connected USB-I2C devices using a module parameter? That might have made
sense a long time ago with embedded i2c-controller, but certainly does
not with usb-i2c controllers.

This should probably be set through sysfs on a per-device basis.

> +
> +static int dln2_i2c_set_state(struct dln2_i2c *dln2, u8 state)
> +{
> +	int ret;
> +	u8 port = 0;

So these devices can apparently have more than one i2c "master port".
You could query the device from the core MFD driver and add one i2c-dln2
platform device per master.

Either way, you shouldn't hard-code the port number throughout the driver.

> +
> +	ret = dln2_transfer(dln2->pdev,
> +			    state ? DLN2_I2C_ENABLE : DLN2_I2C_DISABLE,

Please try to avoid using ?: constructs.

> +			    &port, sizeof(port), NULL, NULL);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +#define dln2_i2c_enable(dln2)	dln2_i2c_set_state(dln2, 1)
> +#define dln2_i2c_disable(dln2)	dln2_i2c_set_state(dln2, 0)

Skip the macros and simply call one appropriately renamed function with
a boolean argument.

> +
> +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 = 0;
> +	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;
> +
> +	return 0;
> +}
> +
> +static int dln2_i2c_write(struct dln2_i2c *dln2, u8 addr,
> +			  u8 *data, u16 data_len)
> +{
> +	int ret, len;
> +	struct tx_data {
> +		u8 port;
> +		u8 addr;
> +		u8 mem_addr_len;
> +		__le32 mem_addr;
> +		__le16 buf_len;
> +		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> +	} __packed tx;

Allocate these buffers dynamically (possibly at probe).

> +
> +	if (data_len > DLN2_I2C_MAX_XFER_SIZE)
> +		return -ENOSPC;

-EINVAL

> +
> +	tx.port = 0;
> +	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)
> +{
> +	struct tx_data {
> +		u8 port;
> +		u8 addr;
> +		u8 mem_addr_len;
> +		__le32 mem_addr;
> +		__le16 buf_len;
> +	} __packed tx;
> +	struct rx_data {
> +		__le16 buf_len;
> +		u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> +	} __packed rx;
> +	int ret, buf_len, rx_len = sizeof(rx);

Again, one declaration per line.

> +
> +	tx.port = 0;
> +	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 < 2)
> +		return -EPROTO;
> +
> +	buf_len = le16_to_cpu(rx.buf_len);
> +	if (rx_len + sizeof(__le16) <  buf_len)

Aren't you counting sizeof(rx.buf_len) twice here?

> +		return -EPROTO;
> +

Either way, you are not verifying that the returned data does not
overflow the supplied buffer (if you received more data than you asked
for).

> +	memcpy(data, rx.buf, buf_len);
> +
> +	return buf_len;
> +}
> +
> +static int dln2_i2c_setup(struct dln2_i2c *dln2)
> +{
> +	int ret;
> +
> +	ret = dln2_i2c_disable(dln2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set I2C frequency */
> +	ret = dln2_i2c_set_frequency(dln2, frequency);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dln2_i2c_enable(dln2);
> +
> +	return ret;
> +}
> +
> +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;
> +	int ret = 0;

No need to initialise.

> +
> +	for (i = 0; i < num; i++) {
> +		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;
> +}
> +
> +/*
> + * Return list of supported functionality.
> + */
> +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,
> +};
> +
> +/* device layer */
> +
> +static int dln2_i2c_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct dln2_i2c *dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);

Split declaration and non-trivial initialisation as I already mentioned
in my comments to patch 1/3. Generally, any review-comment regarding
style applies to the whole series.  

> +
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	dln2->pdev = pdev;
> +
> +	/* setup i2c adapter description */
> +	dln2->adapter.owner = THIS_MODULE;
> +	dln2->adapter.class = I2C_CLASS_HWMON;
> +	dln2->adapter.algo = &dln2_i2c_usb_algorithm;
> +	dln2->adapter.dev.parent = dev;
> +	i2c_set_adapdata(&dln2->adapter, dln2);
> +	snprintf(dln2->adapter.name, sizeof(dln2->adapter.name), DRIVER_NAME);

This probably needs to include the USB bus and device number since you
can have more than of these devices connected.

> +
> +	/* initialize the i2c interface */
> +	ret = dln2_i2c_setup(dln2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to initialize adapter\n");
> +		goto error_free;
> +	}
> +
> +	/* and finally attach to i2c layer */
> +	ret = i2c_add_adapter(&dln2->adapter);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add I2C adapter\n");

Shouldn't you disable the i2c master in the device?

> +		goto error_free;
> +	}
> +
> +	platform_set_drvdata(pdev, dln2);
> +
> +	return 0;
> +
> +error_free:
> +	kfree(dln2);
> +
> +	return ret;
> +}
> +
> +static int dln2_i2c_remove(struct platform_device *pdev)
> +{
> +	struct dln2_i2c *dln2 = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&dln2->adapter);

Same here.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver dln2_i2c_driver = {
> +	.driver.name	= DRIVER_NAME,
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= dln2_i2c_probe,
> +	.remove		= dln2_i2c_remove,
> +};
> +
> +module_platform_driver(dln2_i2c_driver);
> +
> +MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
> +MODULE_DESCRIPTION(DRIVER_NAME " driver");
> +MODULE_LICENSE("GPL");

Johan

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

[Octavian Purdila]

On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold <johan@kernel.org> wrote:

<snip>

Hi Johan,

Again, thanks for the detailed review, I am addressing your review
comments as we speak. Some questions below.

<snip>

> > +     int ret, len;
> > +     struct tx_data {
> > +             u8 port;
> > +             u8 addr;
> > +             u8 mem_addr_len;
> > +             __le32 mem_addr;
> > +             __le16 buf_len;
> > +             u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> > +     } __packed tx;
>
> Allocate these buffers dynamically (possibly at probe).
>

I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be <
64 as the USB endpoint configuration max packet size is 64. In this
case, can we keep it on the stack?

<snip>

> > +     int ret, buf_len, rx_len = sizeof(rx);
>
> Again, one declaration per line.
>

AFAICS there are many places where declaration on the same line
(initialization included) are used. When did this became a coding
style issue?


Thanks,
Tavi

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

[Johan Hovold]

On Mon, Sep 08, 2014 at 06:57:29PM +0300, Octavian Purdila wrote:
> On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> <snip>
> 
> Hi Johan,
> 
> Again, thanks for the detailed review, I am addressing your review
> comments as we speak. Some questions below.
> 
> <snip>
> 
> > > +     int ret, len;
> > > +     struct tx_data {
> > > +             u8 port;
> > > +             u8 addr;
> > > +             u8 mem_addr_len;
> > > +             __le32 mem_addr;
> > > +             __le16 buf_len;
> > > +             u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> > > +     } __packed tx;
> >
> > Allocate these buffers dynamically (possibly at probe).
> >
> 
> I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be <
> 64 as the USB endpoint configuration max packet size is 64. In this
> case, can we keep it on the stack?

It's better to lift that restriction and allocate it dynamically. Using
larger buffers (> EP size) is also more efficient.

> <snip>
> 
> > > +     int ret, buf_len, rx_len = sizeof(rx);
> >
> > Again, one declaration per line.
> 
> AFAICS there are many places where declaration on the same line
> (initialization included) are used. When did this became a coding
> style issue?

It's ugly, hurts readability, and can also obfuscate the fact that your
function really needs to be refactored.

And it's in the CodingStyle:

	"To this end, use just one data declaration per line (no commas
	for multiple data declarations)."

Johan

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

[Octavian Purdila]

On Mon, Sep 8, 2014 at 7:30 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Sep 08, 2014 at 06:57:29PM +0300, Octavian Purdila wrote:
>> On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold <johan@kernel.org> wrote:
>>
>> <snip>
>>
>> Hi Johan,
>>
>> Again, thanks for the detailed review, I am addressing your review
>> comments as we speak. Some questions below.
>>
>> <snip>
>>
>> > > +     int ret, len;
>> > > +     struct tx_data {
>> > > +             u8 port;
>> > > +             u8 addr;
>> > > +             u8 mem_addr_len;
>> > > +             __le32 mem_addr;
>> > > +             __le16 buf_len;
>> > > +             u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> > > +     } __packed tx;
>> >
>> > Allocate these buffers dynamically (possibly at probe).
>> >
>>
>> I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be <
>> 64 as the USB endpoint configuration max packet size is 64. In this
>> case, can we keep it on the stack?
>
> It's better to lift that restriction and allocate it dynamically. Using
> larger buffers (> EP size) is also more efficient.
>
>> <snip>
>>
>> > > +     int ret, buf_len, rx_len = sizeof(rx);
>> >
>> > Again, one declaration per line.
>>
>> AFAICS there are many places where declaration on the same line
>> (initialization included) are used. When did this became a coding
>> style issue?
>
> It's ugly, hurts readability, and can also obfuscate the fact that your
> function really needs to be refactored.
>
> And it's in the CodingStyle:
>
>         "To this end, use just one data declaration per line (no commas
>         for multiple data declarations)."
>

OK, I always thought that was for when declaring structures/unions.
Just one more question on this subject: is the following allowed:

int ret, len;

or should it be:

int ret;
int len;

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

[Johan Hovold]

On Mon, Sep 08, 2014 at 08:15:07PM +0300, Octavian Purdila wrote:

> Just one more question on this subject: is the following allowed:
> 
> int ret, len;
> 
> or should it be:
> 
> int ret;
> int len;

I try to avoid it, at least unless obviously related such as min/max or
x/y.

Johan

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

[Lee Jones]

On Mon, 08 Sep 2014, Octavian Purdila wrote:

> On Mon, Sep 8, 2014 at 7:30 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Sep 08, 2014 at 06:57:29PM +0300, Octavian Purdila wrote:
> >> On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold <johan@kernel.org> wrote:
> >>
> >> <snip>
> >>
> >> Hi Johan,
> >>
> >> Again, thanks for the detailed review, I am addressing your review
> >> comments as we speak. Some questions below.
> >>
> >> <snip>
> >>
> >> > > +     int ret, len;
> >> > > +     struct tx_data {
> >> > > +             u8 port;
> >> > > +             u8 addr;
> >> > > +             u8 mem_addr_len;
> >> > > +             __le32 mem_addr;
> >> > > +             __le16 buf_len;
> >> > > +             u8 buf[DLN2_I2C_MAX_XFER_SIZE];
> >> > > +     } __packed tx;
> >> >
> >> > Allocate these buffers dynamically (possibly at probe).
> >> >
> >>
> >> I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be <
> >> 64 as the USB endpoint configuration max packet size is 64. In this
> >> case, can we keep it on the stack?
> >
> > It's better to lift that restriction and allocate it dynamically. Using
> > larger buffers (> EP size) is also more efficient.
> >
> >> <snip>
> >>
> >> > > +     int ret, buf_len, rx_len = sizeof(rx);
> >> >
> >> > Again, one declaration per line.
> >>
> >> AFAICS there are many places where declaration on the same line
> >> (initialization included) are used. When did this became a coding
> >> style issue?
> >
> > It's ugly, hurts readability, and can also obfuscate the fact that your
> > function really needs to be refactored.
> >
> > And it's in the CodingStyle:
> >
> >         "To this end, use just one data declaration per line (no commas
> >         for multiple data declarations)."
> >
> 
> OK, I always thought that was for when declaring structures/unions.
> Just one more question on this subject: is the following allowed:
> 
> int ret, len;
> 
> or should it be:
> 
> int ret;
> int len;

Common sense usually prevails here.  I sometimes bunch up the really
simple/obvious/throw-away variables like; i, j, k, ret, val etc,
especially when there are a lot of variables in use, but it's nicer to
see the less used, more complex ones on separate lines.

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

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

[Octavian Purdila]

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

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

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

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

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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..6a9e352 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 USB && 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..f8c0bcb
--- /dev/null
+++ b/drivers/gpio/gpio-dln2.c
@@ -0,0 +1,537 @@
+/*
+ * Driver for the Diolan DLN-2 USB-GPIO adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/mutex.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/usb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/ptrace.h>
+#include <linux/wait.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+
+#define DRIVER_NAME "dln2-gpio"
+
+#define DLN2_GPIO_ID			0x01
+
+#define DLN2_GPIO_GET_PORT_COUNT	DLN2_CMD(0x00, DLN2_GPIO_ID)
+#define DLN2_GPIO_GET_PIN_COUNT		DLN2_CMD(0x01, DLN2_GPIO_ID)
+#define DLN2_GPIO_SET_DEBOUNCE		DLN2_CMD(0x04, DLN2_GPIO_ID)
+#define DLN2_GPIO_GET_DEBOUNCE		DLN2_CMD(0x05, DLN2_GPIO_ID)
+#define DLN2_GPIO_PORT_GET_VAL		DLN2_CMD(0x06, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_VAL		DLN2_CMD(0x0B, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_OUT_VAL	DLN2_CMD(0x0C, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_OUT_VAL	DLN2_CMD(0x0D, DLN2_GPIO_ID)
+#define DLN2_GPIO_CONDITION_MET_EV	DLN2_CMD(0x0F, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_ENABLE		DLN2_CMD(0x10, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_DISABLE		DLN2_CMD(0x11, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_DIRECTION	DLN2_CMD(0x13, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_DIRECTION	DLN2_CMD(0x14, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_EVENT_CFG	DLN2_CMD(0x1E, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_EVENT_CFG	DLN2_CMD(0x1F, DLN2_GPIO_ID)
+
+#define DLN2_GPIO_EVENT_NONE		0
+#define DLN2_GPIO_EVENT_CHANGE		1
+#define DLN2_GPIO_EVENT_LVL_HIGH	2
+#define DLN2_GPIO_EVENT_LVL_LOW		3
+#define DLN2_GPIO_EVENT_CHANGE_RISING	0x11
+#define DLN2_GPIO_EVENT_CHANGE_FALLING  0x21
+#define DLN2_GPIO_EVENT_MASK		0x0F
+
+#define DLN2_GPIO_MAX_PINS 32
+
+struct dln2_irq_work {
+	struct work_struct work;
+	struct dln2_gpio *dln2;
+	int pin, type;
+};
+
+struct dln2_remove_work {
+	struct delayed_work work;
+	struct dln2_gpio *dln2;
+};
+
+struct dln2_gpio {
+	struct platform_device *pdev;
+	struct gpio_chip gpio;
+
+	DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
+	DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
+	DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
+	struct dln2_irq_work irq_work[DLN2_GPIO_MAX_PINS];
+	struct dln2_remove_work remove_work;
+};
+
+struct dln2_gpio_pin {
+	__le16 pin;
+} __packed;
+
+struct dln2_gpio_pin_val {
+	__le16 pin;
+	u8 value;
+} __packed;
+
+static int dln2_gpio_get_pin_count(struct platform_device *pdev)
+{
+	__le16 count;
+	int ret, 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)
+{
+	struct dln2_gpio_pin req = {
+		.pin = cpu_to_le16(pin),
+	};
+	struct dln2_gpio_pin_val rsp;
+	int ret, 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 = 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 = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_OUT_VAL, pin);
+
+	if (ret < 0)
+		return ret;
+	return !!ret;
+}
+
+static void dln2_gpio_pin_set_out_val(struct dln2_gpio *dln2,
+				      unsigned int pin, int value)
+{
+	struct dln2_gpio_pin_val req = {
+		.pin = cpu_to_le16(pin),
+		.value = cpu_to_le16(value),
+	};
+
+	dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_OUT_VAL, &req, sizeof(req),
+		      NULL, NULL);
+}
+
+static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	return dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
+}
+
+static void dln2_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
+}
+
+#define DLN2_GPIO_DIRECTION_IN		0
+#define DLN2_GPIO_DIRECTION_OUT		1
+
+static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	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 ret, len = sizeof(rsp);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
+			    &req, sizeof(req), &rsp, &len);
+	if (ret < 0)
+		return ret;
+
+	if (len < sizeof(rsp) || req.pin != rsp.pin)
+		return -EPROTO;
+
+	switch (rsp.value) {
+	case DLN2_GPIO_DIRECTION_IN:
+		return 1;
+	case DLN2_GPIO_DIRECTION_OUT:
+		return 0;
+	default:
+		return -EPROTO;
+	}
+}
+
+static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	int dir = dln2_gpio_get_direction(chip, offset);
+
+	if (dir < 0)
+		return dir;
+
+	if (dir)
+		return dln2_gpio_pin_get_in_val(dln2, offset);
+
+	return dln2_gpio_pin_get_out_val(dln2, offset);
+}
+
+static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+
+	dln2_gpio_pin_set_out_val(dln2, offset, value);
+}
+
+static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
+				   unsigned dir)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	struct dln2_gpio_pin_val req = {
+		.pin = cpu_to_le16(offset),
+		.value = cpu_to_le16(dir),
+	};
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static int dln2_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_IN);
+}
+
+static int dln2_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+				      int value)
+{
+	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_OUT);
+}
+
+static int dln2_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
+				  unsigned debounce)
+{
+	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
+	struct {
+		__le32 duration;
+	} __packed req = {
+		.duration = cpu_to_le32(debounce),
+	};
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
+				   unsigned type, unsigned period)
+{
+	struct {
+		__le16 pin;
+		u8 type;
+		__le16 period;
+	} __packed req = {
+		.pin = cpu_to_le16(pin),
+		.type = type,
+		.period = cpu_to_le16(period),
+	};
+
+	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
+			     &req, sizeof(req), NULL, NULL);
+}
+
+static void dln2_irq_work(struct work_struct *w)
+{
+	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
+	struct dln2_gpio *dln2 = iw->dln2;
+	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
+
+	if (test_bit(iw->pin, dln2->irqs_enabled))
+		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
+	else
+		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
+}
+
+static void dln2_irq_enable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	set_bit(pin, dln2->irqs_enabled);
+	schedule_work(&dln2->irq_work[pin].work);
+}
+
+static void dln2_irq_disable(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	clear_bit(pin, dln2->irqs_enabled);
+	schedule_work(&dln2->irq_work[pin].work);
+}
+
+static void dln2_irq_mask(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	set_bit(pin, dln2->irqs_masked);
+}
+
+static void dln2_irq_unmask(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	if (test_and_clear_bit(pin, dln2->irqs_pending))
+		generic_handle_irq(pin);
+}
+
+static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_LOW;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_FALLING;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct irq_chip dln2_gpio_irqchip = {
+	.name = "dln2-irq",
+	.irq_enable = dln2_irq_enable,
+	.irq_disable = dln2_irq_disable,
+	.irq_mask = dln2_irq_mask,
+	.irq_unmask = dln2_irq_unmask,
+	.irq_set_type = dln2_irq_set_type,
+};
+
+static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
+			    const void *data, int len)
+{
+	int pin, irq;
+	const struct {
+		__le16 count;
+		__u8 type;
+		__le16 pin;
+		__u8 value;
+	} __packed *event = data;
+	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
+
+	pin = le16_to_cpu(event->pin);
+	irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
+
+	if (!irq) {
+		dev_err(dln2->gpio.dev, "pin %d not mapped to IRQ\n", pin);
+		return;
+	}
+
+	if (!test_bit(pin, dln2->irqs_enabled))
+		return;
+	if (test_bit(pin, dln2->irqs_masked)) {
+		set_bit(pin, dln2->irqs_pending);
+		return;
+	}
+
+	switch (dln2->irq_work[pin].type) {
+	case DLN2_GPIO_EVENT_CHANGE_RISING:
+		if (event->value)
+			generic_handle_irq(irq);
+		break;
+	case DLN2_GPIO_EVENT_CHANGE_FALLING:
+		if (!event->value)
+			generic_handle_irq(irq);
+		break;
+	default:
+		generic_handle_irq(irq);
+	}
+}
+
+static int dln2_do_remove(struct dln2_gpio *dln2)
+{
+	/* When removing the DLN2 USB device, gpiochip_remove may fail
+	 * due to i2c drivers holding a GPIO pin. However, the i2c bus
+	 * will eventually be removed triggering an i2c driver remove
+	 * which will release the GPIO pin. So retrying the operation
+	 * later should succeed. */
+	int ret = gpiochip_remove(&dln2->gpio);
+	struct device *dev = dln2->gpio.dev;
+
+	if (ret < 0) {
+		if (ret == -EBUSY)
+			schedule_delayed_work(&dln2->remove_work.work, HZ/10);
+		else
+			dev_warn(dev, "error removing gpio chip: %d\n", ret);
+	} else {
+		kfree(dln2);
+	}
+
+	return ret;
+}
+
+static void dln2_remove_work(struct work_struct *w)
+{
+	struct delayed_work *dw = to_delayed_work(w);
+	struct dln2_remove_work *rw = container_of(dw, struct dln2_remove_work,
+						   work);
+	struct dln2_gpio *dln2 = rw->dln2;
+
+	dln2_do_remove(dln2);
+}
+
+static int dln2_gpio_probe(struct platform_device *pdev)
+{
+	struct dln2_gpio *dln2;
+	struct device *dev = &pdev->dev;
+	int pins = dln2_gpio_get_pin_count(pdev), i, ret;
+
+	if (pins < 0) {
+		dev_err(dev, "failed to get pin count: %d\n", pins);
+		return pins;
+	}
+	if (pins > DLN2_GPIO_MAX_PINS)
+		dev_warn(dev, "clamping pins to %d\n", DLN2_GPIO_MAX_PINS);
+
+	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
+	if (!dln2)
+		return -ENOMEM;
+
+	for (i = 0; i < DLN2_GPIO_MAX_PINS; i++) {
+		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
+		dln2->irq_work[i].pin = i;
+		dln2->irq_work[i].dln2 = dln2;
+	}
+	INIT_DELAYED_WORK(&dln2->remove_work.work, dln2_remove_work);
+	dln2->remove_work.dln2 = dln2;
+
+	ret = dln2_register_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV,
+				     dln2_gpio_event);
+	if (ret) {
+		kfree(dln2);
+		return ret;
+	}
+
+	dln2->pdev = pdev;
+
+	dln2->gpio.label = "dln2";
+	dln2->gpio.dev = dev;
+	dln2->gpio.owner = THIS_MODULE;
+	dln2->gpio.base = -1;
+	dln2->gpio.ngpio = pins;
+	dln2->gpio.exported = 1;
+	dln2->gpio.set = dln2_gpio_set;
+	dln2->gpio.get = dln2_gpio_get;
+	dln2->gpio.request = dln2_gpio_request;
+	dln2->gpio.free = dln2_gpio_free;
+	dln2->gpio.get_direction = dln2_gpio_get_direction;
+	dln2->gpio.direction_input = dln2_gpio_direction_input;
+	dln2->gpio.direction_output = dln2_gpio_direction_output;
+	dln2->gpio.set_debounce = dln2_gpio_set_debounce;
+
+	ret = gpiochip_add(&dln2->gpio);
+	if (ret < 0) {
+		dev_err(dev, "failed to add gpio chip: %d\n", ret);
+		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
+		kfree(dln2);
+		return ret;
+	}
+
+	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);
+		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
+		gpiochip_remove(&dln2->gpio);
+		kfree(dln2);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, dln2);
+
+	return 0;
+}
+
+static int dln2_gpio_remove(struct platform_device *pdev)
+{
+	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
+
+	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
+	dln2->pdev = NULL;
+
+	return dln2_do_remove(dln2);
+}
+
+static struct platform_driver dln2_gpio_driver = {
+	.driver.name	= DRIVER_NAME,
+	.driver.owner	= THIS_MODULE,
+	.probe		= dln2_gpio_probe,
+	.remove		= dln2_gpio_remove,
+};
+
+module_platform_driver(dln2_gpio_driver);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
+MODULE_DESCRIPTION(DRIVER_NAME "driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:

> +static int dln2_do_remove(struct dln2_gpio *dln2)
> +{
> +	/* When removing the DLN2 USB device, gpiochip_remove may fail
> +	 * due to i2c drivers holding a GPIO pin. However, the i2c bus
> +	 * will eventually be removed triggering an i2c driver remove
> +	 * which will release the GPIO pin. So retrying the operation
> +	 * later should succeed. */
> +	int ret = gpiochip_remove(&dln2->gpio);
> +	struct device *dev = dln2->gpio.dev;
> +
> +	if (ret < 0) {
> +		if (ret == -EBUSY)
> +			schedule_delayed_work(&dln2->remove_work.work, HZ/10);
> +		else
> +			dev_warn(dev, "error removing gpio chip: %d\n", ret);
> +	} else {
> +		kfree(dln2);
> +	}
> +
> +	return ret;
> +}

Apparently, the return value from gpiochip_remove is going away:

	"Start to kill off the return value from gpiochip_remove() by
	removing the __must_check attribute and removing all checks
	inside the drivers/gpio directory. The rationale is: well what
	were we supposed to do if there is an error code? Not much:
	print an error message. And gpiolib already does that. So make
	this function return void eventually."

	https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg03468.html

Also, have you considered what happens if there are gpios exported
through sysfs? These may never be released.

In general, how well have these patches been tested with disconnect
events? At least gpiolib is known to blow up (sooner or later) when a
gpiochip is removed when having requested gpios.

Johan

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

[Octavian Purdila]

On Fri, Sep 5, 2014 at 6:38 PM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:
>
>> +static int dln2_do_remove(struct dln2_gpio *dln2)
>> +{
>> +     /* When removing the DLN2 USB device, gpiochip_remove may fail
>> +      * due to i2c drivers holding a GPIO pin. However, the i2c bus
>> +      * will eventually be removed triggering an i2c driver remove
>> +      * which will release the GPIO pin. So retrying the operation
>> +      * later should succeed. */
>> +     int ret = gpiochip_remove(&dln2->gpio);
>> +     struct device *dev = dln2->gpio.dev;
>> +
>> +     if (ret < 0) {
>> +             if (ret == -EBUSY)
>> +                     schedule_delayed_work(&dln2->remove_work.work, HZ/10);
>> +             else
>> +                     dev_warn(dev, "error removing gpio chip: %d\n", ret);
>> +     } else {
>> +             kfree(dln2);
>> +     }
>> +
>> +     return ret;
>> +}
>
> Apparently, the return value from gpiochip_remove is going away:
>
>         "Start to kill off the return value from gpiochip_remove() by
>         removing the __must_check attribute and removing all checks
>         inside the drivers/gpio directory. The rationale is: well what
>         were we supposed to do if there is an error code? Not much:
>         print an error message. And gpiolib already does that. So make
>         this function return void eventually."
>
>         https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg03468.html
>

Oh, I missed this, thanks for pointing it out.

> Also, have you considered what happens if there are gpios exported
> through sysfs? These may never be released.
>
> In general, how well have these patches been tested with disconnect
> events? At least gpiolib is known to blow up (sooner or later) when a
> gpiochip is removed when having requested gpios.
>

I do disconnect tests regularly. Since switching to the new irq
interface the following patch is needed:

https://lkml.org/lkml/2014/9/5/408

With it and the current patch sets things seems to work well.

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 07:04:51PM +0300, Octavian Purdila wrote:
> On Fri, Sep 5, 2014 at 6:38 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:

> > In general, how well have these patches been tested with disconnect
> > events? At least gpiolib is known to blow up (sooner or later) when a
> > gpiochip is removed when having requested gpios.
> 
> I do disconnect tests regularly. Since switching to the new irq
> interface the following patch is needed:
> 
> https://lkml.org/lkml/2014/9/5/408
> 
> With it and the current patch sets things seems to work well.

I see no comments from Linus W on that patch?

And I can confirm that things do blow up.

After disconnecting while having a gpio exported, I get the familiar
OOPS below when reconnecting the device.

This has also been reported here:

	https://lkml.org/lkml/2014/8/4/303

[  711.232574] gpiochip_find_base: found new base at 234
[  711.232696] Unable to handle kernel NULL pointer dereference at virtual address 00000030
[  711.232727] pgd = c0004000
[  711.232757] [00000030] *pgd=00000000
[  711.232849] Internal error: Oops: 17 [#1] PREEMPT ARM
[  711.232879] Modules linked in: i2c_dln2 gpio_dln2 dln2 netconsole [last unloaded: i2c_dln2]
[  711.233032] CPU: 0 PID: 16 Comm: khubd Tainted: G        W      3.17.0-rc3 #1
[  711.233093] task: df2b5480 ti: df2b6000 task.ti: df2b6000
[  711.233123] PC is at gpiochip_add+0x7c/0x378
[  711.233184] LR is at vprintk_emit+0x284/0x628
[  711.233215] pc : [<c023aee8>]    lr : [<c0079034>]    psr: 000f0093
[  711.233215] sp : df2b7900  ip : df2b77f8  fp : df2b792c
[  711.233276] r10: df4dd800  r9 : ffffffe0  r8 : 000000ea
[  711.233306] r7 : c06ba8b0  r6 : a00f0013  r5 : c06ba8d0  r4 : df452804
[  711.233337] r3 : 00000000  r2 : 00000000  r1 : 000000ec  r0 : 000000ea
[  711.233367] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[  711.233398] Control: 10c5387d  Table: 9f4c0019  DAC: 00000015
[  711.233459] Process khubd (pid: 16, stack limit = 0xdf2b6240)
[  711.233551] Stack: (0xdf2b7900 to 0xdf2b8000)
[  711.233581] 7900: c0055f6c df452e08 00000020 00000000 df4dd810 df452800 bf033640 ffffffe0
[  711.233642] 7920: df2b795c df2b7930 bf0334b0 c023ae78 bf0332e8 df4dd810 bf033978 c06de2c8
[  711.233673] 7940: 00000000 bf033978 c06c081c 0000000e df2b7974 df2b7960 c02893e0 bf0332f4
[  711.233703] 7960: c0eb2cf0 df4dd810 df2b79ac df2b7978 c0287314 c02893b0 df2b799c c02895f4
[  711.233764] 7980: df2b79ac bf033978 df4dd810 c0287574 df4acc20 00000000 c06c081c df41f480
[  711.233795] 79a0: df2b79c4 df2b79b0 c02875c4 c02871d4 00000000 df4dd810 df2b79ec df2b79c8
[  711.233856] 79c0: c02853e8 c0287580 df0414d8 df4cf094 df4acc20 df4dd810 df4dd844 c06bded0
[  711.233886] 79e0: df2b7a0c df2b79f0 c0287154 c028538c df041400 df4dd818 df4dd810 c06bded0
[  711.233917] 7a00: df2b7a2c df2b7a10 c02865c0 c02870d8 df2b5480 df4dd818 df4dd810 00000000
[  711.233978] 7a20: df2b7a64 df2b7a30 c02844fc c0286534 df2b7a58 df2b7a40 c0282b8c c02160a0
[  711.234008] 7a40: 00000000 df4dd800 df4dd810 00000010 00000000 bf02ec18 df2b7a84 df2b7a68
[  711.234069] 7a60: c02890c8 c02840a4 df4acc20 df4dd800 00000000 00000010 df2b7ac4 df2b7a88
[  711.234100] 7a80: c02a571c c0288ffc 00000000 c010c048 df0000c0 df4dd810 000000d0 df41f484
[  711.234130] 7aa0: bf02ec54 00000000 df4acc20 ffffffff 00000002 00000000 df2b7b0c df2b7ac8
[  711.234191] 7ac0: c02a5918 c02a54ec 00000000 00000000 00000000 00000040 bf02e380 df41f480
[  711.234222] 7ae0: df4acc20 df4e4000 00000000 00000040 bf02e380 df4acc00 df4acc20 df4e4040
[  711.234283] 7b00: df2b7b54 df2b7b10 bf02e7bc c02a586c 00000000 00000000 00000000 df4dbac8
[  711.234313] 7b20: df2b7b3c df4acc00 c0430dc8 df41a868 df41a800 bf02efc0 df4acc20 df4dbac8
[  711.234344] 7b40: 00000000 00000000 df2b7b8c df2b7b58 c030bc38 bf02e604 c016f220 df4acc00
[  711.234405] 7b60: df2b7b8c c0eb2cf0 df4acc20 c06de2c8 00000000 bf02efc0 c06c6c88 0000000e
[  711.234436] 7b80: df2b7bc4 df2b7b90 c0287314 c030ba88 df4acc00 c0287574 df41a868 bf02efc0
[  711.234497] 7ba0: df4acc20 c0287574 df41a868 00000000 c06c6c88 00000001 df2b7bdc df2b7bc8
[  711.234527] 7bc0: c02875c4 c02871d4 00000000 df4acc20 df2b7c04 df2b7be0 c02853e8 c0287580
[  711.234588] 7be0: df2936d8 df4cf194 df41a868 df4acc20 df4acc54 c06c6ca0 df2b7c24 df2b7c08
[  711.234619] 7c00: c0287154 c028538c df293600 df4acc28 df4acc20 c06c6ca0 df2b7c44 df2b7c28
[  711.234649] 7c20: c02865c0 c02870d8 df2b5480 df4acc28 df4acc20 00000000 df2b7c7c df2b7c48
[  711.234710] 7c40: c02844fc c0286534 df2b7c64 df2b7c58 c0430dc8 c0309a04 00000000 c06e0d88
[  711.234741] 7c60: df41a868 df4acc00 df41a800 df407650 df2b7d04 df2b7c80 c0309a74 c02840a4
[  711.234802] 7c80: 00000001 00000000 00000000 00000000 00001388 df499090 df2b7cbc c016f194
[  711.234832] 7ca0: c06d7dbd df347000 00000001 df477b40 df41a804 00000001 df407600 c06c6e3c
[  711.234924] 7cc0: c06c6ca0 c03087f8 00000001 df477b40 df41a868 df407650 c016f194 df41a800
[  711.234985] 7ce0: 00000001 c06de2c8 00000000 c06c76ec c06c6994 0000000e df2b7d1c df2b7d08
[  711.235015] 7d00: c0313d88 c03094c4 c06c76ec df41a800 df2b7d34 df2b7d20 c030ba48 c0313d58
[  711.235046] 7d20: c0eb2cf0 df41a868 df2b7d6c df2b7d38 c0287314 c030ba08 df2b7d5c df2b7d48
[  711.235107] 7d40: c0432ae0 c06c76ec df41a868 c0287574 df40ac68 00000000 c06c6994 00000000
[  711.235137] 7d60: df2b7d84 df2b7d70 c02875c4 c02871d4 00000000 df41a868 df2b7dac df2b7d88
[  711.235198] 7d80: c02853e8 c0287580 df2936d8 df294b14 df40ac68 df41a868 df41a89c c06c6ca0
[  711.235229] 7da0: df2b7dcc df2b7db0 c0287154 c028538c df293600 df41a870 df41a868 c06c6ca0
[  711.235290] 7dc0: df2b7dec df2b7dd0 c02865c0 c02870d8 df2b5480 df41a870 df41a868 00000000
[  711.235321] 7de0: df2b7e24 df2b7df0 c02844fc c0286534 39383160 c000343a df407868 0006463a
[  711.235351] 7e00: df41a800 df41a868 df4773c0 df41a800 df40ac00 df407868 df2b7e64 df2b7e28
[  711.235412] 7e20: c02ff3b8 c02840a4 00000000 c006e374 00000007 00000000 00000001 00000000
[  711.235443] 7e40: 00000001 df40a644 df41a800 df40ac00 df407868 00000000 df2b7f24 df2b7e68
[  711.235504] 7e60: c0300c84 c02ff11c 00000000 c042d9d8 df2b7f14 df2b7e80 c042d9d8 00000005
[  711.235534] 7e80: 00000000 c0ea95fc c0eb4008 df347000 00000001 df40a644 df407808 c06c69bc
[  711.235565] 7ea0: df40787c 00000064 df407874 df407870 df407800 df40a820 df40a644 df40a408
[  711.235626] 7ec0: c06e03b0 df40a800 df40ac9c df407800 df40a400 df40ac00 df083dfc 01010113
[  711.235656] 7ee0: df2b0001 00000000 df2b5480 c0069ebc df2b7ef0 df2b7ef0 c0300360 df295ac0
[  711.235717] 7f00: 00000000 00000000 c0300360 00000000 00000000 00000000 df2b7fac df2b7f28
[  711.235748] 7f20: c005bdf4 c030036c df2b7f44 00000000 c006e37c 00000000 00000000 00000001
[  711.235778] 7f40: dead4ead ffffffff ffffffff c06e5a34 00000000 00000000 c053dfec df2b7f5c
[  711.235839] 7f60: df2b7f5c 00000000 00000001 dead4ead ffffffff ffffffff c06e5a34 00000000
[  711.235870] 7f80: 00000000 c053dfec df2b7f88 df2b7f88 df295ac0 c005bd0c 00000000 00000000
[  711.235931] 7fa0: 00000000 df2b7fb0 c000f9c8 c005bd18 00000000 00000000 00000000 00000000
[  711.235961] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  711.236022] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 0843a044 40201100
[  711.236083] [<c023aee8>] (gpiochip_add) from [<bf0334b0>] (dln2_gpio_probe+0x1c8/0x22c [gpio_dln2])
[  711.236145] [<bf0334b0>] (dln2_gpio_probe [gpio_dln2]) from [<c02893e0>] (platform_drv_probe+0x3c/0x6c)
[  711.236206] [<c02893e0>] (platform_drv_probe) from [<c0287314>] (driver_probe_device+0x14c/0x3ac)
[  711.236267] [<c0287314>] (driver_probe_device) from [<c02875c4>] (__device_attach+0x50/0x54)
[  711.236328] [<c02875c4>] (__device_attach) from [<c02853e8>] (bus_for_each_drv+0x68/0x9c)
[  711.236419] [<c02853e8>] (bus_for_each_drv) from [<c0287154>] (device_attach+0x88/0x9c)
[  711.236480] [<c0287154>] (device_attach) from [<c02865c0>] (bus_probe_device+0x98/0xbc)
[  711.236511] [<c02865c0>] (bus_probe_device) from [<c02844fc>] (device_add+0x464/0x584)
[  711.236572] [<c02844fc>] (device_add) from [<c02890c8>] (platform_device_add+0xd8/0x26c)
[  711.236633] [<c02890c8>] (platform_device_add) from [<c02a571c>] (mfd_add_device+0x23c/0x340)
[  711.236663] [<c02a571c>] (mfd_add_device) from [<c02a5918>] (mfd_add_devices+0xb8/0x110)
[  711.236724] [<c02a5918>] (mfd_add_devices) from [<bf02e7bc>] (dln2_probe+0x1c4/0x26c [dln2])
[  711.236816] [<bf02e7bc>] (dln2_probe [dln2]) from [<c030bc38>] (usb_probe_interface+0x1bc/0x2a8)
[  711.236846] [<c030bc38>] (usb_probe_interface) from [<c0287314>] (driver_probe_device+0x14c/0x3ac)
[  711.236907] [<c0287314>] (driver_probe_device) from [<c02875c4>] (__device_attach+0x50/0x54)
[  711.236968] [<c02875c4>] (__device_attach) from [<c02853e8>] (bus_for_each_drv+0x68/0x9c)
[  711.236999] [<c02853e8>] (bus_for_each_drv) from [<c0287154>] (device_attach+0x88/0x9c)
[  711.237060] [<c0287154>] (device_attach) from [<c02865c0>] (bus_probe_device+0x98/0xbc)
[  711.237091] [<c02865c0>] (bus_probe_device) from [<c02844fc>] (device_add+0x464/0x584)
[  711.237152] [<c02844fc>] (device_add) from [<c0309a74>] (usb_set_configuration+0x5bc/0x7f8)
[  711.237213] [<c0309a74>] (usb_set_configuration) from [<c0313d88>] (generic_probe+0x3c/0x88)
[  711.237274] [<c0313d88>] (generic_probe) from [<c030ba48>] (usb_probe_device+0x4c/0x80)
[  711.237304] [<c030ba48>] (usb_probe_device) from [<c0287314>] (driver_probe_device+0x14c/0x3ac)
[  711.237365] [<c0287314>] (driver_probe_device) from [<c02875c4>] (__device_attach+0x50/0x54)
[  711.237396] [<c02875c4>] (__device_attach) from [<c02853e8>] (bus_for_each_drv+0x68/0x9c)
[  711.237457] [<c02853e8>] (bus_for_each_drv) from [<c0287154>] (device_attach+0x88/0x9c)
[  711.237518] [<c0287154>] (device_attach) from [<c02865c0>] (bus_probe_device+0x98/0xbc)
[  711.237548] [<c02865c0>] (bus_probe_device) from [<c02844fc>] (device_add+0x464/0x584)
[  711.237609] [<c02844fc>] (device_add) from [<c02ff3b8>] (usb_new_device+0x2a8/0x474)
[  711.237640] [<c02ff3b8>] (usb_new_device) from [<c0300c84>] (hub_thread+0x924/0x161c)
[  711.237701] [<c0300c84>] (hub_thread) from [<c005bdf4>] (kthread+0xe8/0xfc)
[  711.237762] [<c005bdf4>] (kthread) from [<c000f9c8>] (ret_from_fork+0x14/0x20)
[  711.237823] Code: e0801001 e1510002 ca000003 ea00006e (e5932030) 
[  711.237854] ---[ end trace c3415a06381032c8 ]---
[  711.237915] note: khubd[16] exited with preempt_count 1


And with no exported gpios I also get the following BUG on disconnect just
like Octavian reported in the link above:

[  206.720703] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/kernel/locking/mutex.c:583
[  206.720825] in_atomic(): 1, irqs_disabled(): 128, pid: 188, name: modprobe
[  206.720855] 3 locks held by modprobe/188:
[  206.720886]  #0:  (&dev->mutex){......}, at: [<c02876c4>] driver_detach+0x54/0xc8
[  206.721069]  #1:  (&dev->mutex){......}, at: [<c02876d0>] driver_detach+0x60/0xc8
[  206.721221]  #2:  (gpio_lock){......}, at: [<c023b720>] gpiochip_remove+0x24/0x160
[  206.721435] irq event stamp: 3994
[  206.721466] hardirqs last  enabled at (3993): [<c0432bcc>] _raw_spin_unlock_irqrestore+0x7c/0x84
[  206.721527] hardirqs last disabled at (3994): [<c0432998>] _raw_spin_lock_irqsave+0x2c/0x6c
[  206.721588] softirqs last  enabled at (3574): [<c0044084>] __do_softirq+0x230/0x3b8
[  206.721679] softirqs last disabled at (3569): [<c0044574>] irq_exit+0xd8/0x114
[  206.721740] Preemption disabled at:[<  (null)>]   (null)
[  206.721801] 
[  206.721832] CPU: 0 PID: 188 Comm: modprobe Tainted: G        W      3.17.0-rc3 #1
[  206.721893] [<c0016bec>] (unwind_backtrace) from [<c0013850>] (show_stack+0x20/0x24)
[  206.721954] [<c0013850>] (show_stack) from [<c042cb14>] (dump_stack+0x24/0x28)
[  206.722015] [<c042cb14>] (dump_stack) from [<c0061fa0>] (__might_sleep+0x144/0x1a0)
[  206.722076] [<c0061fa0>] (__might_sleep) from [<c042ec6c>] (mutex_lock_nested+0x40/0x3d0)
[  206.722137] [<c042ec6c>] (mutex_lock_nested) from [<c007a978>] (free_desc+0x4c/0x74)
[  206.722198] [<c007a978>] (free_desc) from [<c007ab14>] (irq_free_descs+0x58/0x94)
[  206.722229] [<c007ab14>] (irq_free_descs) from [<c0080984>] (irq_dispose_mapping+0x48/0x60)
[  206.722290] [<c0080984>] (irq_dispose_mapping) from [<c023b750>] (gpiochip_remove+0x54/0x160)
[  206.722351] [<c023b750>] (gpiochip_remove) from [<bf0090fc>] (dln2_gpio_remove+0x30/0x44 [gpio_dln2])
[  206.722473] [<bf0090fc>] (dln2_gpio_remove [gpio_dln2]) from [<c0288c2c>] (platform_drv_remove+0x28/0x2c)
[  206.722534] [<c0288c2c>] (platform_drv_remove) from [<c0286d3c>] (__device_release_driver+0x80/0xd4)
[  206.722595] [<c0286d3c>] (__device_release_driver) from [<c0287734>] (driver_detach+0xc4/0xc8)
[  206.722656] [<c0287734>] (driver_detach) from [<c02869dc>] (bus_remove_driver+0x70/0xe4)
[  206.722686] [<c02869dc>] (bus_remove_driver) from [<c028804c>] (driver_unregister+0x38/0x58)
[  206.722747] [<c028804c>] (driver_unregister) from [<c028942c>] (platform_driver_unregister+0x1c/0x20)
[  206.722808] [<c028942c>] (platform_driver_unregister) from [<bf0099a4>] (dln2_gpio_driver_exit+0x14/0x1c [gpio_dln2])
[  206.722930] [<bf0099a4>] (dln2_gpio_driver_exit [gpio_dln2]) from [<c00a11b0>] (SyS_delete_module+0x15c/0x1d4)
[  206.722991] [<c00a11b0>] (SyS_delete_module) from [<c000f900>] (ret_fast_syscall+0x0/0x48)

Johan

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

[Octavian Purdila]

On Tue, Sep 9, 2014 at 12:36 PM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Sep 05, 2014 at 07:04:51PM +0300, Octavian Purdila wrote:
>> On Fri, Sep 5, 2014 at 6:38 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:
>
>> > In general, how well have these patches been tested with disconnect
>> > events? At least gpiolib is known to blow up (sooner or later) when a
>> > gpiochip is removed when having requested gpios.
>>
>> I do disconnect tests regularly. Since switching to the new irq
>> interface the following patch is needed:
>>
>> https://lkml.org/lkml/2014/9/5/408
>>
>> With it and the current patch sets things seems to work well.
>
> I see no comments from Linus W on that patch?
>
> And I can confirm that things do blow up.
>
> After disconnecting while having a gpio exported, I get the familiar
> OOPS below when reconnecting the device.
>
> This has also been reported here:
>
>         https://lkml.org/lkml/2014/8/4/303
>

Hi Johan,

I did not test with gpio exported via sysfs, only with an i2c driver
that requests a gpio irq. That works because when the i2c bus gets
removed the i2c device gets removed as well and that drops the
requested irq which frees the gpio.

I do see your point with exporting a gpio via sysfs. So I will drop
the remove retry from the patch.

However, I think the above mentioned patch is worth merging as it is
simple enough and it fixes a couple of issues in the gpio remove path.
But I guess we can discuss this in that thread, when Linus W gets to
it.

And thanks a lot for spending so much time on reviewing these patches.

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 06:17:59PM +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>
> ---
>  drivers/gpio/Kconfig     |  12 ++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-dln2.c | 537 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 550 insertions(+)
>  create mode 100644 drivers/gpio/gpio-dln2.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..6a9e352 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 USB && MFD_DLN2

Just 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..f8c0bcb
> --- /dev/null
> +++ b/drivers/gpio/gpio-dln2.c
> @@ -0,0 +1,537 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-GPIO adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/usb.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/ptrace.h>
> +#include <linux/wait.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME "dln2-gpio"
> +
> +#define DLN2_GPIO_ID			0x01
> +
> +#define DLN2_GPIO_GET_PORT_COUNT	DLN2_CMD(0x00, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_PIN_COUNT		DLN2_CMD(0x01, DLN2_GPIO_ID)
> +#define DLN2_GPIO_SET_DEBOUNCE		DLN2_CMD(0x04, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_DEBOUNCE		DLN2_CMD(0x05, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PORT_GET_VAL		DLN2_CMD(0x06, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_VAL		DLN2_CMD(0x0B, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_OUT_VAL	DLN2_CMD(0x0C, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_OUT_VAL	DLN2_CMD(0x0D, DLN2_GPIO_ID)
> +#define DLN2_GPIO_CONDITION_MET_EV	DLN2_CMD(0x0F, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_ENABLE		DLN2_CMD(0x10, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_DISABLE		DLN2_CMD(0x11, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_DIRECTION	DLN2_CMD(0x13, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_DIRECTION	DLN2_CMD(0x14, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_EVENT_CFG	DLN2_CMD(0x1E, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_EVENT_CFG	DLN2_CMD(0x1F, DLN2_GPIO_ID)
> +
> +#define DLN2_GPIO_EVENT_NONE		0
> +#define DLN2_GPIO_EVENT_CHANGE		1
> +#define DLN2_GPIO_EVENT_LVL_HIGH	2
> +#define DLN2_GPIO_EVENT_LVL_LOW		3
> +#define DLN2_GPIO_EVENT_CHANGE_RISING	0x11
> +#define DLN2_GPIO_EVENT_CHANGE_FALLING  0x21
> +#define DLN2_GPIO_EVENT_MASK		0x0F
> +
> +#define DLN2_GPIO_MAX_PINS 32
> +
> +struct dln2_irq_work {
> +	struct work_struct work;
> +	struct dln2_gpio *dln2;
> +	int pin, type;

One declaration per line.

Please consider my previous style comments also for this patch.

> +};
> +
> +struct dln2_remove_work {
> +	struct delayed_work work;
> +	struct dln2_gpio *dln2;
> +};
> +
> +struct dln2_gpio {
> +	struct platform_device *pdev;
> +	struct gpio_chip gpio;
> +
> +	DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
> +	DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
> +	DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
> +	struct dln2_irq_work irq_work[DLN2_GPIO_MAX_PINS];
> +	struct dln2_remove_work remove_work;
> +};
> +
> +struct dln2_gpio_pin {
> +	__le16 pin;
> +} __packed;
> +
> +struct dln2_gpio_pin_val {
> +	__le16 pin;
> +	u8 value;
> +} __packed;
> +
> +static int dln2_gpio_get_pin_count(struct platform_device *pdev)
> +{
> +	__le16 count;
> +	int ret, len = sizeof(count);

Dito.

> +
> +	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)
> +{
> +	struct dln2_gpio_pin req = {
> +		.pin = cpu_to_le16(pin),
> +	};
> +	struct dln2_gpio_pin_val rsp;
> +	int ret, 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 = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_VAL, pin);

Dito.

> +
> +	if (ret < 0)
> +		return ret;
> +	return !!ret;
> +}
> +
> +static int dln2_gpio_pin_get_out_val(struct dln2_gpio *dln2, unsigned int pin)
> +{
> +	int ret = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_OUT_VAL, pin);
> +
> +	if (ret < 0)
> +		return ret;
> +	return !!ret;
> +}
> +
> +static void dln2_gpio_pin_set_out_val(struct dln2_gpio *dln2,
> +				      unsigned int pin, int value)
> +{
> +	struct dln2_gpio_pin_val req = {
> +		.pin = cpu_to_le16(pin),
> +		.value = cpu_to_le16(value),
> +	};
> +
> +	dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_OUT_VAL, &req, sizeof(req),
> +		      NULL, NULL);
> +}
> +
> +static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	return dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
> +}
> +
> +static void dln2_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
> +}
> +
> +#define DLN2_GPIO_DIRECTION_IN		0
> +#define DLN2_GPIO_DIRECTION_OUT		1
> +
> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +	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 ret, len = sizeof(rsp);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> +			    &req, sizeof(req), &rsp, &len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (len < sizeof(rsp) || req.pin != rsp.pin)
> +		return -EPROTO;
> +
> +	switch (rsp.value) {
> +	case DLN2_GPIO_DIRECTION_IN:
> +		return 1;
> +	case DLN2_GPIO_DIRECTION_OUT:
> +		return 0;
> +	default:
> +		return -EPROTO;
> +	}
> +}
> +
> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	int dir = dln2_gpio_get_direction(chip, offset);

Do you really want the overhead of this call for every get?

> +
> +	if (dir < 0)
> +		return dir;
> +
> +	if (dir)
> +		return dln2_gpio_pin_get_in_val(dln2, offset);
> +
> +	return dln2_gpio_pin_get_out_val(dln2, offset);
> +}
> +
> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	dln2_gpio_pin_set_out_val(dln2, offset, value);
> +}
> +
> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
> +				   unsigned dir)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct dln2_gpio_pin_val req = {
> +		.pin = cpu_to_le16(offset),
> +		.value = cpu_to_le16(dir),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static int dln2_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_IN);
> +}
> +
> +static int dln2_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> +				      int value)
> +{
> +	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_OUT);
> +}
> +
> +static int dln2_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
> +				  unsigned debounce)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct {
> +		__le32 duration;
> +	} __packed req = {
> +		.duration = cpu_to_le32(debounce),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
> +				   unsigned type, unsigned period)
> +{
> +	struct {
> +		__le16 pin;
> +		u8 type;
> +		__le16 period;
> +	} __packed req = {
> +		.pin = cpu_to_le16(pin),
> +		.type = type,
> +		.period = cpu_to_le16(period),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static void dln2_irq_work(struct work_struct *w)
> +{
> +	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
> +	struct dln2_gpio *dln2 = iw->dln2;
> +	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
> +
> +	if (test_bit(iw->pin, dln2->irqs_enabled))
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
> +	else
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
> +}
> +
> +static void dln2_irq_enable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	set_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_disable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	clear_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_mask(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	set_bit(pin, dln2->irqs_masked);
> +}
> +
> +static void dln2_irq_unmask(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	if (test_and_clear_bit(pin, dln2->irqs_pending))
> +		generic_handle_irq(pin);
> +}
> +
> +static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_LOW;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_FALLING;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct irq_chip dln2_gpio_irqchip = {
> +	.name = "dln2-irq",
> +	.irq_enable = dln2_irq_enable,
> +	.irq_disable = dln2_irq_disable,
> +	.irq_mask = dln2_irq_mask,
> +	.irq_unmask = dln2_irq_unmask,
> +	.irq_set_type = dln2_irq_set_type,
> +};
> +
> +static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
> +			    const void *data, int len)
> +{
> +	int pin, irq;
> +	const struct {
> +		__le16 count;
> +		__u8 type;
> +		__le16 pin;
> +		__u8 value;
> +	} __packed *event = data;

You never verify the length of the received data before parsing it.

> +	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
> +
> +	pin = le16_to_cpu(event->pin);
> +	irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
> +
> +	if (!irq) {
> +		dev_err(dln2->gpio.dev, "pin %d not mapped to IRQ\n", pin);
> +		return;
> +	}
> +
> +	if (!test_bit(pin, dln2->irqs_enabled))
> +		return;
> +	if (test_bit(pin, dln2->irqs_masked)) {
> +		set_bit(pin, dln2->irqs_pending);
> +		return;
> +	}
> +
> +	switch (dln2->irq_work[pin].type) {
> +	case DLN2_GPIO_EVENT_CHANGE_RISING:
> +		if (event->value)
> +			generic_handle_irq(irq);
> +		break;
> +	case DLN2_GPIO_EVENT_CHANGE_FALLING:
> +		if (!event->value)
> +			generic_handle_irq(irq);
> +		break;
> +	default:
> +		generic_handle_irq(irq);
> +	}
> +}
> +
> +static int dln2_do_remove(struct dln2_gpio *dln2)
> +{
> +	/* When removing the DLN2 USB device, gpiochip_remove may fail
> +	 * due to i2c drivers holding a GPIO pin. However, the i2c bus
> +	 * will eventually be removed triggering an i2c driver remove
> +	 * which will release the GPIO pin. So retrying the operation
> +	 * later should succeed. */
> +	int ret = gpiochip_remove(&dln2->gpio);
> +	struct device *dev = dln2->gpio.dev;
> +
> +	if (ret < 0) {
> +		if (ret == -EBUSY)
> +			schedule_delayed_work(&dln2->remove_work.work, HZ/10);
> +		else
> +			dev_warn(dev, "error removing gpio chip: %d\n", ret);
> +	} else {
> +		kfree(dln2);
> +	}
> +
> +	return ret;
> +}

Wow. You really need to get rid of this hack.

As I already mentioned when you first posted this, the return value of
gpiochip_remove is going away. Furthermore, this is not a problem
specific to this driver, so if anything this would belong in gpiolib.

And finally, as I also mentioned, a gpio may stay requested
indefinitely so you could be looping here forever.

> +
> +static void dln2_remove_work(struct work_struct *w)
> +{
> +	struct delayed_work *dw = to_delayed_work(w);
> +	struct dln2_remove_work *rw = container_of(dw, struct dln2_remove_work,
> +						   work);
> +	struct dln2_gpio *dln2 = rw->dln2;
> +
> +	dln2_do_remove(dln2);
> +}
> +
> +static int dln2_gpio_probe(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2;
> +	struct device *dev = &pdev->dev;
> +	int pins = dln2_gpio_get_pin_count(pdev), i, ret;
> +
> +	if (pins < 0) {
> +		dev_err(dev, "failed to get pin count: %d\n", pins);
> +		return pins;
> +	}
> +	if (pins > DLN2_GPIO_MAX_PINS)
> +		dev_warn(dev, "clamping pins to %d\n", DLN2_GPIO_MAX_PINS);

You never seem to actually clamp the number of pins, as you set set
ngpio bases on pins below. This is bound to lead to crashes eventually.

> +
> +	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DLN2_GPIO_MAX_PINS; i++) {
> +		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
> +		dln2->irq_work[i].pin = i;
> +		dln2->irq_work[i].dln2 = dln2;
> +	}
> +	INIT_DELAYED_WORK(&dln2->remove_work.work, dln2_remove_work);
> +	dln2->remove_work.dln2 = dln2;
> +
> +	ret = dln2_register_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV,
> +				     dln2_gpio_event);
> +	if (ret) {
> +		kfree(dln2);
> +		return ret;
> +	}
> +
> +	dln2->pdev = pdev;
> +
> +	dln2->gpio.label = "dln2";
> +	dln2->gpio.dev = dev;
> +	dln2->gpio.owner = THIS_MODULE;
> +	dln2->gpio.base = -1;
> +	dln2->gpio.ngpio = pins;
> +	dln2->gpio.exported = 1;
> +	dln2->gpio.set = dln2_gpio_set;
> +	dln2->gpio.get = dln2_gpio_get;
> +	dln2->gpio.request = dln2_gpio_request;
> +	dln2->gpio.free = dln2_gpio_free;
> +	dln2->gpio.get_direction = dln2_gpio_get_direction;
> +	dln2->gpio.direction_input = dln2_gpio_direction_input;
> +	dln2->gpio.direction_output = dln2_gpio_direction_output;
> +	dln2->gpio.set_debounce = dln2_gpio_set_debounce;
> +
> +	ret = gpiochip_add(&dln2->gpio);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add gpio chip: %d\n", ret);
> +		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +		kfree(dln2);

You should probably add a common error path for this.

> +		return ret;
> +	}
> +
> +	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);
> +		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +		gpiochip_remove(&dln2->gpio);
> +		kfree(dln2);
> +		return ret;
> +	}

You dereference the platform data in dln2_gpio_event, which could
possibly be NULL if you get a callback here.

> +
> +	platform_set_drvdata(pdev, dln2);
> +
> +	return 0;
> +}
> +
> +static int dln2_gpio_remove(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
> +
> +	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +	dln2->pdev = NULL;
> +
> +	return dln2_do_remove(dln2);
> +}
> +
> +static struct platform_driver dln2_gpio_driver = {
> +	.driver.name	= DRIVER_NAME,
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= dln2_gpio_probe,
> +	.remove		= dln2_gpio_remove,
> +};
> +
> +module_platform_driver(dln2_gpio_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
> +MODULE_DESCRIPTION(DRIVER_NAME "driver");
> +MODULE_LICENSE("GPL");

Johan

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

[Johan Hovold]

On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:

> --- /dev/null
> +++ b/drivers/gpio/gpio-dln2.c
> @@ -0,0 +1,537 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-GPIO adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/usb.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/ptrace.h>
> +#include <linux/wait.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/dln2.h>

It seems you don't need all these includes (usb, ptrace, wait...). Only
include what you actually use.

Johan