Date: Wed, 5 Nov 2014 16:48:27 +0100
From: Johan Hovold <johan@kernel.org>
To: Octavian Purdila <octavian.purdila@intel.com>
Cc: gregkh@linuxfoundation.org, linus.walleij@linaro.org, gnurou@gmail.com,
        wsa@the-dreams.de, sameo@linux.intel.com, lee.jones@linaro.org,
        arnd@arndb.de, johan@kernel.org, daniel.baluta@intel.com,
        laurentiu.palcu@intel.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-i2c@vger.kernel.org
Subject: Re: [PATCH v9 1/4] mfd: add support for Diolan DLN-2 devices
Message-ID: <20141105154827.GU31358@localhost>
References: <1414427472-4100-1-git-send-email-octavian.purdila@intel.com>
 <1414427472-4100-2-git-send-email-octavian.purdila@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1414427472-4100-2-git-send-email-octavian.purdila@intel.com>
User-Agent: Mutt/1.5.22 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org

On Mon, Oct 27, 2014 at 06:31:09PM +0200, 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      |  11 +
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/dln2.c       | 761 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/dln2.h | 103 +++++++
>  4 files changed, 876 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 0000000..b3946ef
> --- /dev/null
> +++ b/drivers/mfd/dln2.c

[...]

> +struct dln2_header {
> +	__le16 size;
> +	__le16 id;
> +	__le16 echo;
> +	__le16 handle;
> +} __packed;
> +
> +struct dln2_response {
> +	struct dln2_header hdr;
> +	__le16 result;
> +} __packed;
> +

__packed not needed on either struct above.

[...]

> +/*
> + * Returns true if a valid transfer slot is found. In this case the URB must not
> + * be resubmitted imediately in dln2_rx as we need the data when dln2_transfer

s/imediately/immediately/

> + * is woke up. It will be resubmitted there.
> + */
> +static bool dln2_transfer_complete(struct dln2_dev *dln2, struct urb *urb,
> +				   u16 handle, u16 rx_slot)
> +{
> +	struct device *dev = &dln2->interface->dev;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	bool valid_slot = false;
> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	/*
> +	 * No need to disable interrupts as this lock is not taken in interrupt
> +	 * context elsewhere in this driver. This function (or its callers) are
> +	 * also not exported to other modules.
> +	 */
> +	spin_lock(&rxs->lock);
> +	if (rxc->in_use && !rxc->urb) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +		valid_slot = true;
> +	}
> +	spin_unlock(&rxs->lock);
> +
> +	if (!valid_slot)
> +		dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
> +
> +	return valid_slot;
> +}
> +
> +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);

No need to continue the search if id is found as it will be unique in
the list.

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

Drop the RX: prefix for consistency.

> +		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_event_callbacks(dln2, id, echo, data, len);
> +	} else {
> +		/* URB will be re-submitted in _dln2_transfer (free_rx_slot) */
> +		if (dln2_transfer_complete(dln2, urb, handle, echo))
> +			return;
> +	}
> +
> +out:
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (err < 0)
> +		dev_err(dev, "failed to resubmit RX URB: %d\n", err);
> +}

[...]

> +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;
> +		struct device *dev = &dln2->interface->dev;

Again, no need to keep these inside the for-loop.

> +
> +		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;
> +}

Looks good otherwise.

I'll respond with my reviewed by tag after you have addressed the above
and Joe's comments.

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