Message-ID: <20141003171207.GC2394@localhost>
Date:	Fri, 3 Oct 2014 19:12:07 +0200
From:	Johan Hovold <johan@...nel.org>
To:	Octavian Purdila <octavian.purdila@...el.com>
Cc:	gregkh@...uxfoundation.org, linus.walleij@...aro.org,
	gnurou@...il.com, wsa@...-dreams.de, sameo@...ux.intel.com,
	lee.jones@...aro.org, arnd@...db.de, johan@...nel.org,
	daniel.baluta@...el.com, laurentiu.palcu@...el.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices

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

[...]

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

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

> +	DLN2_HANDLE_CTRL,
> +	DLN2_HANDLE_GPIO,
> +	DLN2_HANDLE_I2C,
> +	DLN2_HANDLES
> +};
> +
> +/*
> + * Receive context used between the receive demultiplexer and the
> + * transfer routine. While sending a request the transfer routine
> + * will look for a free receive context and use it to wait for a
> + * response and to receive the URB and thus the response data.
> + */
> +struct dln2_rx_context {
> +	/* completion used to wait for a response */
> +	struct completion done;
> +
> +	/* if non-NULL the URB contains the response */
> +	struct urb *urb;
> +
> +	/* if true then this context is used to wait for a response */
> +	bool in_use;
> +};
> +
> +/*
> + * Receive contexts for a particular DLN2 module (i2c, gpio, etc.). We
> + * use the handle header field to identify the module in
> + * dln2_dev.mod_rx_slots and then the echo header field to index the
> + * slots field and find the receive context for a particular request.
> + */
> +struct dln2_mod_rx_slots {
> +	/* RX slots bitmap */
> +	unsigned long bmap;
> +
> +	/* used to wait for a free RX slot */
> +	wait_queue_head_t wq;
> +
> +	/* used to wait for an RX operation to complete */
> +	struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
> +
> +	/* avoid races between alloc/free_rx_slot and dln2_rx_transfer */
> +	spinlock_t lock;
> +};
> +
> +struct dln2_dev {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *interface;
> +	u8 ep_in;
> +	u8 ep_out;
> +
> +	struct urb *rx_urb[DLN2_MAX_URBS];
> +	void *rx_buf[DLN2_MAX_URBS];
> +
> +	struct dln2_mod_rx_slots mod_rx_slots[DLN2_HANDLES];
> +
> +	struct list_head event_cb_list;
> +	spinlock_t event_cb_lock;
> +
> +	bool disconnect;
> +	int active_transfers;
> +	wait_queue_head_t disconnect_wq;
> +	spinlock_t disconnect_lock;
> +};
> +
> +struct dln2_event_cb_entry {
> +	struct list_head list;
> +	u16 id;
> +	struct platform_device *pdev;
> +	dln2_event_cb_t callback;
> +};
> +
> +int dln2_register_event_cb(struct platform_device *pdev, u16 id,
> +			   dln2_event_cb_t rx_cb)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_event_cb_entry *i, *new;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	new = kmalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new->id = id;
> +	new->callback = rx_cb;
> +	new->pdev = pdev;
> +
> +	spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +
> +	if (!ret)
> +		list_add_rcu(&new->list, &dln2->event_cb_list);
> +
> +	spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> +	if (ret)
> +		kfree(new);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(dln2_register_event_cb);
> +
> +void dln2_unregister_event_cb(struct platform_device *pdev, u16 id)
> +{
> +	struct dln2_dev *dln2 = dev_get_drvdata(pdev->dev.parent);
> +	struct dln2_event_cb_entry *i;
> +	unsigned long flags;
> +	bool found = false;
> +
> +	spin_lock_irqsave(&dln2->event_cb_lock, flags);
> +
> +	list_for_each_entry(i, &dln2->event_cb_list, list) {
> +		if (i->id == id) {
> +			list_del_rcu(&i->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&dln2->event_cb_lock, flags);
> +
> +	if (found) {
> +		synchronize_rcu();
> +		kfree(i);
> +	}
> +}
> +EXPORT_SYMBOL(dln2_unregister_event_cb);
> +
> +static int dln2_submit_urb(struct dln2_dev *dln2, struct urb *urb, gfp_t gfp)
> +{
> +	int ret;
> +	struct device *dev = &dln2->interface->dev;
> +
> +	ret = usb_submit_urb(urb, gfp);
> +	if (ret < 0)
> +		dev_err(dev, "failed to (re)submit RX URB: %d\n", ret);
> +	return ret;
> +}
> +
> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> +			     u16 handle, u16 rx_slot)
> +{
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +
> +	spin_lock(&rxs->lock);
> +
> +	rxc = &rxs->slots[rx_slot];

This can be done outside the lock, right?

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

And so can the resubmission.

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

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

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

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

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

> +		return;
> +	}
> +
> +out:
> +	dln2_submit_urb(dln2, urb, GFP_ATOMIC);
> +}
> +
> +static void *dln2_prep_buf(u16 handle, u16 cmd, u16 echo, const void *obuf,
> +			   int *obuf_len, gfp_t gfp)
> +{
> +	int len;
> +	void *buf;
> +	struct dln2_header *hdr;
> +
> +	len = *obuf_len + sizeof(*hdr);
> +	buf = kmalloc(len, gfp);
> +	if (!buf)
> +		return NULL;
> +
> +	hdr = (struct dln2_header *)buf;
> +	hdr->id = cpu_to_le16(cmd);
> +	hdr->size = cpu_to_le16(len);
> +	hdr->echo = cpu_to_le16(echo);
> +	hdr->handle = cpu_to_le16(handle);
> +
> +	memcpy(buf + sizeof(*hdr), obuf, *obuf_len);
> +
> +	*obuf_len = len;
> +
> +	return buf;
> +}
> +
> +static int dln2_send_wait(struct dln2_dev *dln2, u16 handle, u16 cmd, u16 echo,
> +			  const void *obuf, int obuf_len)
> +{
> +	int ret = 0;
> +	int len = obuf_len;
> +	void *buf;
> +	int actual;
> +
> +	buf = dln2_prep_buf(handle, cmd, echo, obuf, &len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = usb_bulk_msg(dln2->usb_dev,
> +			   usb_sndbulkpipe(dln2->usb_dev, dln2->ep_out),
> +			   buf, len, &actual, DLN2_USB_TIMEOUT);
> +
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static bool find_free_slot(struct dln2_dev *dln2, u16 handle, int *slot)
> +{
> +	struct dln2_mod_rx_slots *rxs;
> +	unsigned long flags;
> +

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

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

This can be done when declaring dev.

> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		dev_err(dev, "USB write failed: %d\n", ret);
> +		goto out_free_rx_slot;
> +	}
> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (!rxc->urb) {
> +		ret = -ENODEV;
> +		goto out_free_rx_slot;
> +	}

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

> +
> +	/* if we got here we know that the response header has been checked */
> +	rsp = rxc->urb->transfer_buffer;
> +
> +	if (rsp->hdr.size < sizeof(*rsp)) {
> +		ret = -EPROTO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	result = le16_to_cpu(rsp->result);
> +	if (result) {
> +		dev_dbg(dev, "%d received response with error %d\n",
> +			handle, result);
> +		ret = -EREMOTEIO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (!ibuf) {
> +		ret = 0;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
> +		*ibuf_len = rsp->hdr.size - sizeof(*rsp);
> +
> +	memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> +	free_rx_slot(dln2, handle, rx_slot);
> +out_decr:
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->active_transfers--;
> +	spin_unlock(&dln2->disconnect_lock);
> +	if (dln2->disconnect)
> +		wake_up(&dln2->disconnect_wq);
> +
> +	return ret;
> +}
v> +
> +int dln2_transfer(struct platform_device *pdev, u16 cmd,
> +		  const void *obuf, unsigned obuf_len,
> +		  void *ibuf, unsigned *ibuf_len)
> +{
> +	struct dln2_platform_data *dln2_pdata;
> +	struct dln2_dev *dln2;
> +	u16 h;
> +
> +	dln2 = dev_get_drvdata(pdev->dev.parent);
> +	dln2_pdata = dev_get_platdata(&pdev->dev);
> +	h = dln2_pdata->handle;
> +
> +	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
> +}
> +EXPORT_SYMBOL(dln2_transfer);
> +
> +static int dln2_check_hw(struct dln2_dev *dln2)
> +{
> +	int ret;
> +	__le32 hw_type;
> +	int len = sizeof(hw_type);
> +
> +	ret = _dln2_transfer(dln2, DLN2_HANDLE_CTRL, CMD_GET_DEVICE_VER,
> +			     NULL, 0, &hw_type, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(hw_type))
> +		return -EREMOTEIO;
> +
> +	if (le32_to_cpu(hw_type) != DLN2_HW_ID) {
> +		dev_err(&dln2->interface->dev, "Device ID 0x%x not supported\n",
> +			le32_to_cpu(hw_type));
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dln2_print_serialno(struct dln2_dev *dln2)
> +{
> +	int ret;
> +	__le32 serial_no;
> +	int len = sizeof(serial_no);
> +	struct device *dev = &dln2->interface->dev;
> +
> +	ret = _dln2_transfer(dln2, DLN2_HANDLE_CTRL, CMD_GET_DEVICE_SN, NULL, 0,
> +			     &serial_no, &len);
> +	if (ret < 0)
> +		return ret;
> +	if (len < sizeof(serial_no))
> +		return -EREMOTEIO;
> +
> +	dev_info(dev, "Diolan DLN2 serial 0x%x\n", le32_to_cpu(serial_no));
> +
> +	return 0;
> +}
> +
> +static int dln2_hw_init(struct dln2_dev *dln2)
> +{
> +	int ret;
> +
> +	ret = dln2_check_hw(dln2);
> +	if (ret < 0)
> +		return ret;
> +
> +	return dln2_print_serialno(dln2);
> +}
> +
> +static void dln2_free_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		usb_kill_urb(dln2->rx_urb[i]);
> +		usb_free_urb(dln2->rx_urb[i]);
> +		kfree(dln2->rx_buf[i]);
> +	}
> +}
> +
> +static void dln2_free(struct dln2_dev *dln2)
> +{
> +	dln2_free_rx_urbs(dln2);
> +	usb_put_dev(dln2->usb_dev);
> +	kfree(dln2);
> +}
> +
> +static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
> +			      struct usb_host_interface *hostif)
> +{
> +	int i;
> +	const int rx_max_size = DLN2_RX_BUF_SIZE;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		int ret;
> +
> +		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> +		if (!dln2->rx_buf[i])
> +			return -ENOMEM;
> +
> +		dln2->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!dln2->rx_urb[i])
> +			return -ENOMEM;
> +
> +		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
> +				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
> +				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
> +
> +		ret = dln2_submit_urb(dln2, dln2->rx_urb[i], GFP_KERNEL);
> +		if (ret < 0)
> +			return ret;

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

> +	}
> +
> +	return 0;
> +}
> +
> +static struct dln2_platform_data dln2_pdata_gpio = {
> +	.handle = DLN2_HANDLE_GPIO,
> +};
> +
> +/* Only one I2C port seems to be supported on current hardware */
> +static struct dln2_platform_data dln2_pdata_i2c = {
> +	.handle = DLN2_HANDLE_I2C,
> +	.port = 0,
> +};
> +
> +static const struct mfd_cell dln2_devs[] = {
> +	{
> +		.name = "dln2-gpio",
> +		.platform_data = &dln2_pdata_gpio,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +	{
> +		.name = "dln2-i2c",
> +		.platform_data = &dln2_pdata_i2c,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
> +};
> +
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +	int i, j;
> +
> +	/* don't allow starting new transfers */
> +	spin_lock(&dln2->disconnect_lock);
> +	dln2->disconnect = true;
> +	spin_unlock(&dln2->disconnect_lock);
> +
> +	/* cancel in progress transfers */
> +	for (i = 0; i < DLN2_HANDLES; i++) {
> +		struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&rxs->lock, flags);
> +
> +		/* cancel all response waiters */
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
> +			struct dln2_rx_context *rxc = &rxs->slots[j];
> +
> +			if (rxc->in_use)
> +				complete(&rxc->done);
> +		}
> +
> +		spin_unlock_irqrestore(&rxs->lock, flags);
> +	}
> +
> +	/* wait for transfers to end */
> +	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
> +
> +	mfd_remove_devices(&interface->dev);
> +
> +	dln2_free(dln2);
> +}
> +
> +static int dln2_probe(struct usb_interface *interface,
> +		      const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_interface *hostif = interface->cur_altsetting;
> +	struct device *dev = &interface->dev;
> +	struct dln2_dev *dln2;
> +	int ret;
> +	int i, j;
> +	int id;
> +
> +	if (hostif->desc.bInterfaceNumber != 0 ||
> +	    hostif->desc.bNumEndpoints < 2)
> +		return -ENODEV;
> +
> +	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	dln2->ep_out = hostif->endpoint[0].desc.bEndpointAddress;
> +	dln2->ep_in = hostif->endpoint[1].desc.bEndpointAddress;
> +	dln2->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	dln2->interface = interface;
> +	usb_set_intfdata(interface, dln2);
> +	init_waitqueue_head(&dln2->disconnect_wq);
> +
> +	for (i = 0; i < DLN2_HANDLES; i++) {
> +		init_waitqueue_head(&dln2->mod_rx_slots[i].wq);
> +		spin_lock_init(&dln2->mod_rx_slots[i].lock);
> +		for (j = 0; j < DLN2_MAX_RX_SLOTS; j++)
> +			init_completion(&dln2->mod_rx_slots[i].slots[j].done);
> +	}
> +
> +	spin_lock_init(&dln2->event_cb_lock);
> +	INIT_LIST_HEAD(&dln2->event_cb_list);
> +
> +	ret = dln2_setup_rx_urbs(dln2, hostif);
> +	if (ret) {
> +		dln2_free(dln2);

goto out_cleanup as mentioned earlier.

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

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

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

> +	ret = mfd_add_devices(dev, id, dln2_devs, ARRAY_SIZE(dln2_devs),
> +			      NULL, 0, NULL);
> +	if (ret != 0) {
> +		dev_err(dev, "failed to add mfd devices to core\n");
> +		goto out_cleanup;
> +	}
> +
> +	return 0;
> +
> +out_cleanup:
> +	dln2_free(dln2);
> +
> +	return ret;
> +}

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

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists