Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
On Mon, Oct 27, 2014 at 03:21:10PM +0200, Octavian Purdila wrote: > On Thu, Oct 23, 2014 at 6:16 PM, Johan Hovold wrote: > > On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote: > > > +struct dln2_event_cb_entry { > > > + struct list_head list; > > > + u16 id; > > > + struct platform_device *pdev; > > > + dln2_event_cb_t callback; > > > +}; > > > + > > > +int dln2_register_event_cb(struct platform_device *pdev, u16 id, > > > +dln2_event_cb_t rx_cb) > > > +{ > > > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent); > > > + struct dln2_event_cb_entry *i, *new_cb; > > > > Can you use a name which does not have the same suffix as the actual > > callback function (i.e. "_cb"). Just call it "entry" or something. > > > > OK. > > > > + unsigned long flags; > > > + int ret = 0; > > > + > > > + new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL); > > > > Use kzalloc here. > > Why kzalloc? All of the fields are initialized below. It's good practise to clear any data structure at allocation. The cost is negligible. > > > + if (!new_cb) > > > + return -ENOMEM; > > > + > > > + new_cb->id = id; > > > + new_cb->callback = rx_cb; > > > + new_cb->pdev = pdev; > > > + > > > + spin_lock_irqsave(&dln2->event_cb_lock, flags); > > > + > > > + list_for_each_entry(i, &dln2->event_cb_list, list) { > > > + if (i->id == id) { > > > + ret = -EBUSY; > > > + break; > > > + } > > > + } > > > + > > > + if (!ret) > > > + list_add_rcu(&new_cb->list, &dln2->event_cb_list); > > > + > > > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags); > > > + > > > + if (ret) > > > + kfree(new_cb); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL(dln2_register_event_cb); > > > + ret = mfd_add_hotplug_devices(dev, dln2_devs, > > > ARRAY_SIZE(dln2_devs)); > > > > So this now depends on 15bb4d6e8534 ("mfd: core: Add helper function to > > register hotplug devices") in the MFD tree. > > > > Please mention what tree the patch is against in your cover letter (I > > noticed you had rebased when it no longer applied to 3.17). > > > > You should drop the gpiolib patch now that v3.18-rc1 is out as well. > > This series is based against the Lee's for-mfd-next-v3.19 tree that > does not yet contain the gpiolib patch. Ok, but make sure to include that information in the cover letter. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
On Thu, Oct 23, 2014 at 6:16 PM, Johan Hovold wrote: > > On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote: > > Here's some late feedback due to travels. You managed to get two updates > in there so commenting on the diff from v6. > Thanks for the review :) > > +struct dln2_event_cb_entry { > > + struct list_head list; > > + u16 id; > > + struct platform_device *pdev; > > + dln2_event_cb_t callback; > > +}; > > + > > +int dln2_register_event_cb(struct platform_device *pdev, u16 id, > > +dln2_event_cb_t rx_cb) > > +{ > > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent); > > + struct dln2_event_cb_entry *i, *new_cb; > > Can you use a name which does not have the same suffix as the actual > callback function (i.e. "_cb"). Just call it "entry" or something. > OK. > > + unsigned long flags; > > + int ret = 0; > > + > > + new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL); > > Use kzalloc here. Why kzalloc? All of the fields are initialized below. > > > + if (!new_cb) > > + return -ENOMEM; > > + > > + new_cb->id = id; > > + new_cb->callback = rx_cb; > > + new_cb->pdev = pdev; > > + > > + spin_lock_irqsave(&dln2->event_cb_lock, flags); > > + > > + list_for_each_entry(i, &dln2->event_cb_list, list) { > > + if (i->id == id) { > > + ret = -EBUSY; > > + break; > > + } > > + } > > + > > + if (!ret) > > + list_add_rcu(&new_cb->list, &dln2->event_cb_list); > > + > > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags); > > + > > + if (ret) > > + kfree(new_cb); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(dln2_register_event_cb); > > + > > +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id) > > +{ > > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent); > > + struct dln2_event_cb_entry *i; > > + unsigned long flags; > > + bool found = false; > > + > > + spin_lock_irqsave(&dln2->event_cb_lock, flags); > > + > > + list_for_each_entry(i, &dln2->event_cb_list, list) { > > + if (i->id == id) { > > + list_del_rcu(&i->list); > > + found = true; > > + break; > > + } > > + } > > + > > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags); > > + > > + if (found) { > > + synchronize_rcu(); > > + kfree(i); > > + } > > +} > > +EXPORT_SYMBOL(dln2_unregister_event_cb); > > + > > Please add a comment describing the return value (i.e. when the urb had > been saved and shouldn't be resubmitted). > > Also could rename this helper so it doesn't appear to be a variant of > dln2_transfer (e.g. something with "complete" in the name). > Ok, I will use dln2_transfer_complete. > > +static bool 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; > > + bool ret = false; > > + > > + rxc = &rxs->slots[rx_slot]; > > + > > + /* > > + * No need to disable interrupts as this lock is not taken in > > + * interrupt context elsewhere in this driver and this > > + * function (or its callers) are not exported to other > > + * modules. > > Why do you break comment lines already at 70 chars? > Oops, relic in my .emacs, will fix all comments in v9. > > + */ > > + spin_lock(&rxs->lock); > > + if (rxc->in_use && !rxc->urb) { > > + rxc->urb = urb; > > + complete(&rxc->done); > > + ret = true; > > + } > > + spin_unlock(&rxs->lock); > > + > > + return ret; > > +} > > + > > +static void dln2_run_event_callbacks(struct dln2_dev *dln2, u16 id, u16 > > echo, > > + void *data, int len) > > +{ > > + struct dln2_event_cb_entry *i; > > + > > + rcu_read_lock(); > > + > > + list_for_each_entry_rcu(i, &dln2->event_cb_list, list) > > + if (i->id == id) > > + i->callback(i->pdev, echo, data, len); > > + > > + rcu_read_unlock(); > > +} > > + > > +static void dln2_rx(struct urb *urb) > > +{ > > + struct dln2_dev *dln2 = urb->context; > > + struct dln2_header *hdr = urb->transfer_buffer; > > + struct device *dev = &dln2->interface->dev; > > + u16 id, echo, handle, size; > > + u8 *data; > > + int len; > > + int 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; > > +
Re: [PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
On Wed, Oct 15, 2014 at 05:48:08PM +0300, Octavian Purdila wrote: Here's some late feedback due to travels. You managed to get two updates in there so commenting on the diff from v6. > +struct dln2_event_cb_entry { > + struct list_head list; > + u16 id; > + struct platform_device *pdev; > + dln2_event_cb_t callback; > +}; > + > +int dln2_register_event_cb(struct platform_device *pdev, u16 id, > +dln2_event_cb_t rx_cb) > +{ > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent); > + struct dln2_event_cb_entry *i, *new_cb; Can you use a name which does not have the same suffix as the actual callback function (i.e. "_cb"). Just call it "entry" or something. > + unsigned long flags; > + int ret = 0; > + > + new_cb = kmalloc(sizeof(*new_cb), GFP_KERNEL); Use kzalloc here. > + if (!new_cb) > + return -ENOMEM; > + > + new_cb->id = id; > + new_cb->callback = rx_cb; > + new_cb->pdev = pdev; > + > + spin_lock_irqsave(&dln2->event_cb_lock, flags); > + > + list_for_each_entry(i, &dln2->event_cb_list, list) { > + if (i->id == id) { > + ret = -EBUSY; > + break; > + } > + } > + > + if (!ret) > + list_add_rcu(&new_cb->list, &dln2->event_cb_list); > + > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags); > + > + if (ret) > + kfree(new_cb); > + > + return ret; > +} > +EXPORT_SYMBOL(dln2_register_event_cb); > + > +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id) > +{ > + struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent); > + struct dln2_event_cb_entry *i; > + unsigned long flags; > + bool found = false; > + > + spin_lock_irqsave(&dln2->event_cb_lock, flags); > + > + list_for_each_entry(i, &dln2->event_cb_list, list) { > + if (i->id == id) { > + list_del_rcu(&i->list); > + found = true; > + break; > + } > + } > + > + spin_unlock_irqrestore(&dln2->event_cb_lock, flags); > + > + if (found) { > + synchronize_rcu(); > + kfree(i); > + } > +} > +EXPORT_SYMBOL(dln2_unregister_event_cb); > + Please add a comment describing the return value (i.e. when the urb had been saved and shouldn't be resubmitted). Also could rename this helper so it doesn't appear to be a variant of dln2_transfer (e.g. something with "complete" in the name). > +static bool 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; > + bool ret = false; > + > + rxc = &rxs->slots[rx_slot]; > + > + /* > + * No need to disable interrupts as this lock is not taken in > + * interrupt context elsewhere in this driver and this > + * function (or its callers) are not exported to other > + * modules. Why do you break comment lines already at 70 chars? > + */ > + spin_lock(&rxs->lock); > + if (rxc->in_use && !rxc->urb) { > + rxc->urb = urb; > + complete(&rxc->done); > + ret = true; > + } > + spin_unlock(&rxs->lock); > + > + return ret; > +} > + > +static void dln2_run_event_callbacks(struct dln2_dev *dln2, u16 id, u16 echo, > + void *data, int len) > +{ > + struct dln2_event_cb_entry *i; > + > + rcu_read_lock(); > + > + list_for_each_entry_rcu(i, &dln2->event_cb_list, list) > + if (i->id == id) > + i->callback(i->pdev, echo, data, len); > + > + rcu_read_unlock(); > +} > + > +static void dln2_rx(struct urb *urb) > +{ > + struct dln2_dev *dln2 = urb->context; > + struct dln2_header *hdr = urb->transfer_buffer; > + struct device *dev = &dln2->interface->dev; > + u16 id, echo, handle, size; > + u8 *data; > + int len; > + int 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 misma
[PATCH v8 1/4] mfd: add support for Diolan DLN-2 devices
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 --- drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 1 + drivers/mfd/dln2.c | 756 +++ include/linux/mfd/dln2.h | 69 + 4 files changed, 837 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 c9200d3..4538815a 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -189,6 +189,17 @@ 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 such as I2C_DLN2, GPIO_DLN2, + etc. 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 61f8dbf..2cd7e74 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -175,6 +175,7 @@ obj-$(CONFIG_MFD_STW481X) += stw481x.o obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o obj-$(CONFIG_MFD_MENF21BMC)+= menf21bmc.o obj-$(CONFIG_MFD_HI6421_PMIC) += hi6421-pmic-core.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 000..124b262 --- /dev/null +++ b/drivers/mfd/dln2.c @@ -0,0 +1,756 @@ +/* + * 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct dln2_header { + __le16 size; + __le16 id; + __le16 echo; + __le16 handle; +} __packed; + +struct dln2_response { + struct dln2_header hdr; + __le16 result; +} __packed; + +#define DLN2_GENERIC_MODULE_ID 0x00 +#define DLN2_GENERIC_CMD(cmd) DLN2_CMD(cmd, DLN2_GENERIC_MODULE_ID) +#define CMD_GET_DEVICE_VER DLN2_GENERIC_CMD(0x30) +#define CMD_GET_DEVICE_SN DLN2_GENERIC_CMD(0x31) + +#define DLN2_HW_ID 0x200 +#define DLN2_USB_TIMEOUT 200 /* in ms */ +#define DLN2_MAX_RX_SLOTS 16 +#define DLN2_MAX_URBS 16 +#define DLN2_RX_BUF_SIZE 512 + +enum dln2_handle { + DLN2_HANDLE_EVENT = 0, /* don't change, hardware defined */ + DLN2_HANDLE_CTRL, + DLN2_HANDLE_GPIO, + DLN2_HANDLE_I2C, + DLN2_HANDLES +}; + +/* + * Receive context used between the receive demultiplexer and the + * transfer routine. While sending a request the transfer routine + * will look for a free receive context and use it to wait for a + * response and to receive the URB and thus the response data. + */ +struct dln2_rx_context { + /* completion used to wait for a response */ + struct completion done; + + /* if non-NULL the URB contains the response */ + struct urb *urb; + + /* if true then this context is used to wait for a response */ + bool in_use; +}; + +/* + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We + * use the handle header field to identify the module in + * dln2_dev.mod_rx_slots and then the echo header field to index the + * slots field and find the receive context for a particu