Re: [PATCH] Add PEAK System USB adapters core driver

[Wolfgang Grandegger <wg@grandegger.com>]

I finally found some time to review these patches...

Please use a patch version for subsequent versions of the series. Also a
cover letter would be nice.

And please send the patch also to the relevant USB mailing list.

On 12/22/2011 02:11 PM, Stephane Grosjean wrote:
>>From 377720ccf260b1df75cddd7a23bef658079a22aa Mon Sep 17 00:00:00 2001
> From: Stephane Grosjean <s.grosjean@peak-system.com>
> Date: Thu, 22 Dec 2011 13:54:47 +0100
> Subject: [PATCH] Add PEAK System USB adapters core driver

Adding a commit message would be nice describing what device are supported.

> 
> ---
>  drivers/net/can/usb/Kconfig                  |    1 +
>  drivers/net/can/usb/Makefile                 |    1 +
>  drivers/net/can/usb/peak_usb/Kconfig         |   19 +
>  drivers/net/can/usb/peak_usb/Makefile        |   10 +
>  drivers/net/can/usb/peak_usb/pcan_usb_core.c |  893 ++++++++++++++++++++++++++
>  drivers/net/can/usb/peak_usb/peak_usb.h      |  149 +++++
>  6 files changed, 1073 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/usb/peak_usb/Kconfig
>  create mode 100644 drivers/net/can/usb/peak_usb/Makefile
>  create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_core.c

Why not naming the file peak_usb.c? You already use "peak_usb" for the
header file as function prefix inside!

>  create mode 100644 drivers/net/can/usb/peak_usb/peak_usb.h
> 
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index 0452549..6cbe40e 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -13,4 +13,5 @@ config CAN_ESD_USB2
>            This driver supports the CAN-USB/2 interface
>            from esd electronic system design gmbh (http://www.esd.eu).
>  
> +source "drivers/net/can/usb/peak_usb/Kconfig"
>  endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index fce3cf1..da6d1d3 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -4,5 +4,6 @@
>  
>  obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
>  obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
> +obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/usb/peak_usb/Kconfig b/drivers/net/can/usb/peak_usb/Kconfig
> new file mode 100644
> index 0000000..fd583a1
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/Kconfig
> @@ -0,0 +1,19 @@
> +menuconfig CAN_PEAK_USB
> +	tristate "PEAK-System USB adapters"
> +	---help---
> +	  This driver supports PEAK-System GmbH CAN interfaces to USB
> +	  (http://www.peak-system.com).
> +
> +config CAN_PCAN_USB
> +	tristate "PEAK PCAN-USB adapter"
> +	depends on CAN_PEAK_USB
> +	---help---
> +	  This driver supports the one channel PCAN-USB interface
> +	  from PEAK-System GmbH (http://www.peak-system.com).
> +
> +config CAN_PCAN_USB_PRO
> +	tristate "PEAK PCAN-USB Pro adapter"
> +	depends on CAN_PEAK_USB
> +	---help---
> +	  This driver supports the two channels PCAN-USB Pro interface
> +	  from PEAK-System GmbH (http://www.peak-system.com).

Do we really need different configs? More on that later...

> diff --git a/drivers/net/can/usb/peak_usb/Makefile b/drivers/net/can/usb/peak_usb/Makefile
> new file mode 100644
> index 0000000..8af81a5
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/Makefile
> @@ -0,0 +1,10 @@
> +obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
> +peak_usb-y = pcan_usb_core.o
> +
> +ifneq ($(CONFIG_CAN_PCAN_USB),)
> +peak_usb-y += pcan_usb.o
> +endif
> +
> +ifneq ($(CONFIG_CAN_PCAN_USB_PRO),)
> +peak_usb-y += pcan_usb_pro.o
> +endif
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> new file mode 100644
> index 0000000..d753da0
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -0,0 +1,893 @@
> +/*
> + * CAN driver for PEAK System USB adapters
> + *
> + * Copyright (C) 2011-2012 PEAK-System GmbH
> + *
> + * 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 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +#include <linux/init.h>
> +#include <linux/signal.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/stringify.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +
> +#include "peak_usb.h"
> +
> +#define PCAN_USB_VERSION_STRING	__stringify(PCAN_USB_VERSION_MAJOR)"."\
> +	__stringify(PCAN_USB_VERSION_MINOR)"."\
> +	__stringify(PCAN_USB_VERSION_SUBMINOR)

Please drop this extra old-fashined versioning stuff. I'm quite sure
that it will never be updated.

> +MODULE_AUTHOR("Stephane Grosjean <s.grosjean@peak-system.com>");
> +MODULE_DESCRIPTION("CAN driver for PEAK-System USB adapters");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(PCAN_USB_VERSION_STRING);
> +
> +/*
> + * Table of devices that work with this driver
> + */
> +static struct usb_device_id peak_usb_table[] = {
> +#ifdef PCAN_USB_PRODUCT_ID
> +	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USB_PRODUCT_ID)},
> +#endif
> +#ifdef PCAN_USBPRO_PRODUCT_ID
> +	{USB_DEVICE(PCAN_USB_VENDOR_ID, PCAN_USBPRO_PRODUCT_ID)},
> +#endif
> +	{} /* Terminating entry */
> +};

Ifdef's a ugly and should be avoided. It's common to support both
devices together, unconditionally.

> +MODULE_DEVICE_TABLE(usb, peak_usb_table);
> +
> +/* List of supported PCAN-USB adapters (NULL terminated list) */
> +static struct peak_usb_adapter *peak_usb_adapters_list[] = {
> +#ifdef PCAN_USB_PRODUCT_ID
> +	&pcan_usb,
> +#endif
> +#ifdef PCAN_USBPRO_PRODUCT_ID
> +	&pcan_usb_pro,
> +#endif
> +	NULL,
> +};


This construction is not really nice (no proper encapsulation) but I see
similar code in other USB driver code :(.

> +/*
> + * Dump memory
> + */
> +#define DUMP_WIDTH	16
> +void dump_mem(char *prompt, void *p, int l)
> +{
> +	char tmp[(3*DUMP_WIDTH)+1];
> +	u8 *pc = (u8 *)p;

Cast?

> +	int i, ltmp;
> +
> +	pr_info("%s dumping %s (%d bytes):\n",
> +		PCAN_USB_DRIVER_NAME, prompt ? prompt : "memory", l);
> +	for (i = ltmp = 0; i < l; ) {
> +		ltmp += sprintf(tmp+ltmp, "%02X ", *pc++);

Spaces arount "+"

> +		if ((++i % DUMP_WIDTH) == 0) {
> +			pr_info("%s %s\n", PCAN_USB_DRIVER_NAME, tmp);
> +			ltmp = 0;
> +		}
> +	}
> +	if (i % DUMP_WIDTH)
> +		pr_info("%s %s\n", PCAN_USB_DRIVER_NAME, tmp);
> +}
> +
> +/*
> + * used to simulate no device
> + */
> +int peak_usb_no_dev(struct usb_interface *intf)
> +{
> +	return -ENODEV;
> +}
> +
> +/*
> + * initialize a time_ref object with usb adapter own settings
> + */
> +void peak_usb_init_time_ref(struct peak_time_ref *time_ref,
> +	struct peak_usb_adapter *adapter)
> +{
> +	if (time_ref) {
> +		memset(time_ref, '\0', sizeof(struct peak_time_ref));

s/\0/0/ ?

> +		time_ref->adapter = adapter;
> +	}
> +}
> +
> +static void peak_usb_add_us(struct timeval *tv, u32 delta_us)
> +{
> +	/* number of s. to add to final time */
> +	u32 delta_s = delta_us / 1000000;
> +	delta_us -= (delta_s * 1000000);

"()" are not needed.

> +
> +	tv->tv_usec += delta_us;
> +	if (tv->tv_usec >= 1000000) {
> +		tv->tv_usec -= 1000000;
> +		delta_s++;
> +	}
> +	tv->tv_sec += delta_s;
> +}
> +
> +/*
> + * sometimes, another now may be  more recent than current one...
> + */
> +void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
> +{
> +	time_ref->ts_dev_2 = ts_now;
> +
> +	/* should wait at least two passes before computing */
> +	if (time_ref->tv_host.tv_sec > 0) {
> +		u32 delta_ts = time_ref->ts_dev_2 - time_ref->ts_dev_1;
> +		if (time_ref->ts_dev_2 < time_ref->ts_dev_1)
> +			delta_ts &= (1 << time_ref->adapter->ts_used_bits) - 1;
> +
> +		time_ref->ts_total += delta_ts;
> +	}
> +}
> +
> +/*
> + * register device timestamp as now
> + */
> +void peak_usb_set_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
> +{
> +	if (time_ref->tv_host_0.tv_sec == 0) {
> +		do_gettimeofday(&time_ref->tv_host_0);
> +		time_ref->tv_host.tv_sec = 0;
> +	} else {
> +		/*
> +		 * delta_us should not be >= 2^32 => delta_s should be < 4294
> +		 * handle 32-bits wrapping here: if count of s. reaches 4200,
> +		 * reset counters and change time base
> +		 */
> +		if (time_ref->tv_host.tv_sec != 0) {
> +			u32 delta_s = time_ref->tv_host.tv_sec \

"\" ?

> +				- time_ref->tv_host_0.tv_sec;
> +			if (delta_s > 4200) {
> +				time_ref->tv_host_0 = time_ref->tv_host;
> +				time_ref->ts_total = 0;
> +			}
> +		}
> +
> +		do_gettimeofday(&time_ref->tv_host);
> +		time_ref->tick_count++;
> +	}
> +
> +	time_ref->ts_dev_1 = time_ref->ts_dev_2;
> +	peak_usb_update_ts_now(time_ref, ts_now);
> +}
> +
> +/*
> + * compute timeval according to current ts and time_ref data
> + */
> +void peak_usb_get_ts_tv(struct peak_time_ref *time_ref, u32 ts,
> +	struct timeval *tv)
> +{
> +	/* protect from getting timeval before setting now */
> +	if (time_ref->tv_host.tv_sec > 0) {
> +		u64 delta_us;
> +
> +		delta_us = ts - time_ref->ts_dev_2;
> +		if (ts < time_ref->ts_dev_2)
> +			delta_us &= (1 << time_ref->adapter->ts_used_bits) - 1;
> +
> +		delta_us += time_ref->ts_total;
> +
> +		delta_us *= time_ref->adapter->us_per_ts_scale;
> +		delta_us >>= time_ref->adapter->us_per_ts_shift;
> +
> +		*tv = time_ref->tv_host_0;
> +		peak_usb_add_us(tv, (u32)delta_us);
> +	} else {
> +		do_gettimeofday(tv);
> +	}
> +}
> +
> +/*
> + * callback for bulk IN urb
> + */
> +static void peak_usb_read_bulk_callback(struct urb *urb)
> +{
> +	struct peak_usb_device *dev = urb->context;
> +	struct net_device *netdev;
> +	int err;
> +
> +	netdev = dev->netdev;
> +
> +	if (!netif_device_present(netdev))
> +		return;
> +
> +	switch (urb->status) {
> +	case 0: /* success */
> +		break;
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		return;
> +	default:
> +		netdev_info(netdev, "Rx URB aborted (%d)\n", urb->status);
> +		goto resubmit_urb;
> +	}
> +
> +	/* protect from any incoming empty msgs */
> +	if ((urb->actual_length > 0) && (dev->adapter->dev_decode_buf))
> +		/* handle these kinds of msgs only is _start callback called */
> +		if (dev->state & PCAN_USB_STATE_STARTED) {
> +			err = dev->adapter->dev_decode_buf(dev, urb);
> +			if (err)
> +				dump_mem("received usb message",
> +					urb->transfer_buffer,
> +					urb->transfer_buffer_length);
> +		}
> +
> +resubmit_urb:
> +	usb_fill_bulk_urb(urb, dev->udev,
> +		usb_rcvbulkpipe(dev->udev, dev->ep_msg_in),
> +		urb->transfer_buffer, dev->adapter->rx_buffer_size,
> +		peak_usb_read_bulk_callback, dev);
> +
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +
> +	if (err == -ENODEV)
> +		netif_device_detach(netdev);
> +	else if (err)
> +		netdev_err(netdev, "failed resubmitting read bulk urb: %d\n",
> +			err);

Does it fit one one line?

> +}
> +
> +/*
> + * callback for bulk OUT urb
> + */
> +static void peak_usb_write_bulk_callback(struct urb *urb)
> +{
> +	struct peak_tx_urb_context *context = urb->context;
> +	struct peak_usb_device *dev;
> +	struct net_device *netdev;
> +
> +	BUG_ON(!context);
> +
> +	dev = context->dev;
> +	netdev = dev->netdev;
> +
> +	atomic_dec(&dev->active_tx_urbs);
> +
> +	if (!netif_device_present(netdev))
> +		return;
> +
> +	if (urb->status)
> +		netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);

Is this an error condition? netdev_dbg instead?

> +	netdev->trans_start = jiffies;
> +
> +	/* transmission complete interrupt */
> +	netdev->stats.tx_packets++;
> +	netdev->stats.tx_bytes += context->dlc;
> +
> +	can_get_echo_skb(netdev, context->echo_index);
> +
> +	/* Release context */
> +	context->echo_index = PCAN_USB_MAX_TX_URBS;
> +
> +	if (netif_queue_stopped(netdev))

Can be removed!

> +		netif_wake_queue(netdev);
> +}
> +
> +static void peak_usb_unlink_all_urbs(struct peak_usb_device *dev)
> +{
> +	int i;
> +
> +	usb_kill_anchored_urbs(&dev->rx_submitted);
> +
> +	usb_kill_anchored_urbs(&dev->tx_submitted);
> +	atomic_set(&dev->active_tx_urbs, 0);
> +
> +	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++) {
> +		struct urb *urb = dev->tx_contexts[i].urb;
> +
> +		if (urb) {
> +			if (urb->transfer_buffer) {
> +				usb_free_coherent(urb->dev,
> +					urb->transfer_buffer_length,
> +					urb->transfer_buffer,
> +					urb->transfer_dma);
> +			}
> +			usb_free_urb(urb);
> +			dev->tx_contexts[i].urb = NULL;
> +		}
> +		dev->tx_contexts[i].echo_index = PCAN_USB_MAX_TX_URBS;
> +	}
> +}
> +
> +/*
> + * Start interface
> + */
> +static int peak_usb_start(struct peak_usb_device *dev)
> +{
> +	struct net_device *netdev = dev->netdev;
> +	int err, i;
> +
> +	for (i = 0; i < PCAN_USB_MAX_RX_URBS; i++) {
> +		struct urb *urb;
> +		u8 *buf;
> +
> +		/* create a URB, and a buffer for it, to receive usb messages */
> +		urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!urb) {
> +			netdev_err(netdev, "No memory left for URBs\n");
> +			err = -ENOMEM;
> +			break;
> +		}
> +
> +		buf = usb_alloc_coherent(dev->udev,
> +			dev->adapter->rx_buffer_size, GFP_KERNEL,
> +			&urb->transfer_dma);
> +		if (!buf) {
> +			netdev_err(netdev, "No memory left for USB buffer\n");
> +			usb_free_urb(urb);
> +			err = -ENOMEM;
> +			break;
> +		}
> +
> +		usb_fill_bulk_urb(urb, dev->udev,
> +			usb_rcvbulkpipe(dev->udev, dev->ep_msg_in),
> +			buf, dev->adapter->rx_buffer_size,
> +			peak_usb_read_bulk_callback, dev);
> +		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +		usb_anchor_urb(urb, &dev->rx_submitted);
> +
> +		err = usb_submit_urb(urb, GFP_KERNEL);
> +		if (err) {
> +			if (err == -ENODEV)
> +				netif_device_detach(dev->netdev);
> +
> +			usb_unanchor_urb(urb);
> +			usb_free_coherent(dev->udev,
> +				dev->adapter->rx_buffer_size, buf,
> +				urb->transfer_dma);
> +			usb_free_urb(urb);
> +			break;
> +		}
> +
> +		/* drop reference, USB core will take care of freeing it */
> +		usb_free_urb(urb);
> +	}
> +
> +	/* did we submit any URBs? Warn if we was not able to submit all urbs */
> +	if (i < PCAN_USB_MAX_RX_URBS) {
> +		if (i == 0) {
> +			netdev_err(netdev, "couldn't setup iany rx URB\n");

Typo?

> +			return err;
> +		}
> +
> +		netdev_warn(netdev, "rx performance may be slow\n");

Can this message come frequently? If yes, ratelimiting would be nice.

> +	}
> +
> +	/* pre-alloc tx buffers and corresponding urbs */
> +	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++) {
> +		struct peak_tx_urb_context *context;
> +		struct urb *urb;
> +		u8 *buf;
> +
> +		/* create a URB and a buffer for it, to transmit usb messages */
> +		urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!urb) {
> +			netdev_err(netdev, "No memory left for URBs\n");
> +			err = -ENOMEM;
> +			break;
> +		}
> +
> +		buf = usb_alloc_coherent(dev->udev,
> +			dev->adapter->tx_buffer_size, GFP_KERNEL,
> +			&urb->transfer_dma);
> +		if (!buf) {
> +			netdev_err(netdev, "No memory left for USB buffer\n");
> +			usb_free_urb(urb);
> +			err = -ENOMEM;
> +			break;
> +		}
> +
> +		context = dev->tx_contexts + i;
> +		context->dev = dev;
> +		context->urb = urb;
> +
> +		usb_fill_bulk_urb(urb, dev->udev,
> +			usb_sndbulkpipe(dev->udev, dev->ep_msg_out),
> +			buf, dev->adapter->tx_buffer_size,
> +			peak_usb_write_bulk_callback, context);
> +		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +	}
> +
> +	/* warn if we were able to allocate enough tx contexts */
> +	if (i < PCAN_USB_MAX_TX_URBS) {
> +		if (i == 0) {
> +			netdev_err(netdev, "couldn't setup any tx URB\n");
> +			return err;
> +		}
> +
> +		netdev_warn(netdev, "tx performance may be slow\n");

Ditto...

> +	}
> +
> +	if (dev->adapter->dev_start) {
> +		err = dev->adapter->dev_start(dev);
> +		if (err)
> +			goto failed;
> +	}
> +
> +	dev->state |= PCAN_USB_STATE_STARTED;
> +
> +	/* can set bus on now */
> +	if (dev->adapter->dev_set_bus) {
> +		err = dev->adapter->dev_set_bus(dev, 1);
> +		if (err)
> +			goto failed;
> +	}
> +
> +	dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	return 0;
> +
> +failed:
> +	if (err == -ENODEV)
> +		netif_device_detach(dev->netdev);
> +
> +	netdev_warn(netdev, "couldn't submit control: %d\n", err);

Is this a warning or an error? You are very verbose with your
info/warn/err messages. Hope it will not result in kernel log flooding
in case of problems.

> +
> +	return err;
> +}
> +
> +static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,
> +	struct net_device *netdev)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	struct peak_tx_urb_context *context = NULL;
> +	struct net_device_stats *stats = &netdev->stats;
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	struct urb *urb;
> +	u8 *obuf;
> +	int i, err;
> +	size_t size = dev->adapter->tx_buffer_size;
> +
> +	if (can_dropped_invalid_skb(netdev, skb))
> +		return NETDEV_TX_OK;
> +
> +	if (!dev->adapter->dev_encode_msg)
> +		return NETDEV_TX_OK;
> +
> +	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++)
> +		if (dev->tx_contexts[i].echo_index == PCAN_USB_MAX_TX_URBS) {
> +			context = dev->tx_contexts + i;
> +			break;
> +		}
> +
> +	if (!context) {
> +		netdev_warn(netdev, "couldn't find free context\n");
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	urb = context->urb;
> +	obuf = urb->transfer_buffer;
> +	context->echo_index = i;
> +	context->dlc = cf->can_dlc;
> +
> +	err = dev->adapter->dev_encode_msg(dev, skb, obuf, &size);
> +	if (err)
> +		goto nomem;
> +
> +	usb_anchor_urb(urb, &dev->tx_submitted);
> +
> +	can_put_echo_skb(skb, netdev, context->echo_index);
> +
> +	atomic_inc(&dev->active_tx_urbs);
> +
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (unlikely(err)) {

This is the only place where you use "(un)likely". Either use in
consitantly or remove it.

> +		can_free_echo_skb(netdev, context->echo_index);
> +
> +		usb_unanchor_urb(urb);
> +		dev_kfree_skb(skb);
> +
> +		atomic_dec(&dev->active_tx_urbs);
> +
> +		if (err == -ENODEV) {
> +			netif_device_detach(netdev);
> +		} else {
> +			netdev_warn(netdev, "failed tx_urb %d\n", err);
> +			stats->tx_dropped++;
> +		}
> +	} else {
> +		netdev->trans_start = jiffies;
> +
> +		/* Slow down tx path */
> +		if (atomic_read(&dev->active_tx_urbs) >= PCAN_USB_MAX_TX_URBS)
> +			netif_stop_queue(netdev);
> +	}
> +
> +	return NETDEV_TX_OK;
> +
> +nomem:
> +	netdev_err(netdev, "Packet dropped\n");
> +	dev_kfree_skb(skb);
> +	stats->tx_dropped++;

I do not see any need for a label. Please move the code to the if block
with a proper error message.

> +	return NETDEV_TX_OK;
> +}
> +
> +static int peak_usb_ndo_open(struct net_device *netdev)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	int err;
> +
> +	/* common open */
> +	err = open_candev(netdev);
> +	if (err)
> +		return err;
> +
> +	/* finally start device */
> +	err = peak_usb_start(dev);
> +	if (err) {
> +		netdev_warn(netdev, "couldn't start device: %d\n", err);

warning or error ?

> +		close_candev(netdev);
> +		return err;
> +	}
> +
> +	dev->open_time = jiffies;

The open_time member can be removed, IIRC.

> +	netif_start_queue(netdev);
> +
> +	return 0;
> +}
> +
> +static int peak_usb_ndo_stop(struct net_device *netdev)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +
> +	/* Stop polling */
> +	peak_usb_unlink_all_urbs(dev);
> +
> +	netif_stop_queue(netdev);
> +
> +	dev->state &= ~PCAN_USB_STATE_STARTED;
> +
> +	if (dev->adapter->dev_stop)
> +		dev->adapter->dev_stop(dev);
> +
> +	close_candev(netdev);
> +
> +	dev->open_time = 0;
> +
> +	dev->can.state = CAN_STATE_STOPPED;
> +
> +	/* can set bus off now */
> +	if (dev->adapter->dev_set_bus) {
> +		int err = dev->adapter->dev_set_bus(dev, 0);

Empty line please.

> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops peak_usb_netdev_ops = {
> +	.ndo_open = peak_usb_ndo_open,
> +	.ndo_stop = peak_usb_ndo_stop,
> +	.ndo_start_xmit = peak_usb_ndo_start_xmit,
> +};
> +
> +static int peak_usb_set_mode(struct net_device *netdev, enum can_mode mode)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +
> +	if (!dev->open_time)
> +		return -EINVAL;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		if (dev->adapter->dev_set_bus) {
> +			int err = dev->adapter->dev_set_bus(dev, 1);
> +			if (err)
> +				netdev_warn(netdev,
> +					"couldn't start device (err %d)", err);

Warning or error?

> +		}
> +		if (netif_queue_stopped(netdev))

Not needed.

> +			netif_wake_queue(netdev);
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int peak_usb_set_bittiming(struct net_device *netdev)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	struct can_bittiming *bt = &dev->can.bittiming;
> +
> +	netdev_info(netdev, "setting bitrate to %u Kbps\n", bt->bitrate);
> +	netdev_dbg(netdev, "sam=%u phase_seg2=%u phase_seg1=%u\n",
> +		bt->sample_point, bt->phase_seg2, bt->phase_seg1);
> +	netdev_dbg(netdev, "prop_seg=%u sjw=%d brp=%u\n",
> +		bt->prop_seg, bt->sjw, bt->brp);

We don't need that. The info is available via ip tool.

> +	if (dev->adapter->dev_set_bittiming) {
> +		int err = dev->adapter->dev_set_bittiming(dev, bt);

Empty line please.

> +		if (err)
> +			netdev_info(netdev, "couldn't set bitrate (err %d)",
> +				err);

Does it fit on one line?

> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * called to create one device, atached to USB adapter's CAN controller
> + * number 'ctrl_idx'
> + */
> +static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
> +	struct usb_interface *intf, int ctrl_idx)
> +{
> +	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	int sizeof_candev = peak_usb_adapter->sizeof_dev_private;
> +	struct peak_usb_device *dev;
> +	struct net_device *netdev;
> +	int i, err;
> +	u16 tmp16;
> +
> +	if (sizeof_candev < sizeof(struct peak_usb_device))
> +		sizeof_candev = sizeof(struct peak_usb_device);
> +
> +	netdev = alloc_candev(sizeof_candev, PCAN_USB_MAX_TX_URBS);
> +	if (!netdev) {
> +		dev_err(&intf->dev, "%s: Couldn't alloc candev\n",
> +			PCAN_USB_DRIVER_NAME);
> +		return -ENOMEM;
> +	}
> +
> +	dev = netdev_priv(netdev);
> +
> +	dev->udev = usb_dev;
> +	dev->netdev = netdev;
> +	dev->adapter = peak_usb_adapter;
> +	dev->ctrl_idx = ctrl_idx;
> +	dev->state = PCAN_USB_STATE_CONNECTED;
> +
> +	dev->ep_msg_in = peak_usb_adapter->ep_msg_in;
> +	dev->ep_msg_out = peak_usb_adapter->ep_msg_out[ctrl_idx];
> +
> +	dev->can.clock = peak_usb_adapter->clock;
> +	dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
> +	dev->can.do_set_bittiming = peak_usb_set_bittiming;
> +	dev->can.do_set_mode = peak_usb_set_mode;
> +	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES \
> +		| CAN_CTRLMODE_LISTENONLY;
> +
> +	netdev->netdev_ops = &peak_usb_netdev_ops;
> +
> +	netdev->flags |= IFF_ECHO; /* we support local echo */
> +
> +	init_usb_anchor(&dev->rx_submitted);
> +
> +	init_usb_anchor(&dev->tx_submitted);
> +	atomic_set(&dev->active_tx_urbs, 0);
> +
> +	for (i = 0; i < PCAN_USB_MAX_TX_URBS; i++)
> +		dev->tx_contexts[i].echo_index = PCAN_USB_MAX_TX_URBS;
> +
> +	dev->prev_siblings = usb_get_intfdata(intf);
> +	usb_set_intfdata(intf, dev);
> +
> +	SET_NETDEV_DEV(netdev, &intf->dev);
> +
> +	err = register_candev(netdev);
> +	if (err) {
> +		dev_err(&intf->dev,
> +			"couldn't register CAN device: %d\n", err);
> +		goto lbl_set_intf_data;
> +	}
> +
> +	if (dev->prev_siblings)
> +		(dev->prev_siblings)->next_siblings = dev;
> +
> +	/* read some info from PCAN-USB device */
> +	tmp16 = le16_to_cpu(usb_dev->descriptor.bcdDevice);
> +	dev->device_num = (u8)(tmp16 & 0xff);
> +	dev->device_rev = (u8)(tmp16 >> 8);

Are the casts needed?

> +	if (dev->adapter->dev_init) {
> +		err = dev->adapter->dev_init(dev);
> +		if (err)
> +			goto lbl_set_intf_data;
> +	}
> +
> +	/* set bus off */
> +	if (dev->adapter->dev_set_bus) {
> +		err = dev->adapter->dev_set_bus(dev, 0);
> +		if (err)
> +			goto lbl_set_intf_data;
> +	}
> +
> +	/* get device number early */
> +	if (dev->adapter->dev_get_device_id)
> +		dev->adapter->dev_get_device_id(dev, &dev->device_number);
> +
> +	dev_info(&intf->dev,
> +		"%s attached to %s can controller %u (device %u)\n",
> +		netdev->name, peak_usb_adapter->name, ctrl_idx,
> +		dev->device_number);
> +
> +	return 0;
> +
> +lbl_set_intf_data:
> +	usb_set_intfdata(intf, dev->prev_siblings);
> +	free_candev(netdev);
> +
> +	return err;
> +}
> +
> +/*
> + * called by the usb core when the device is removed from the system
> + */
> +static void peak_usb_disconnect(struct usb_interface *intf)
> +{
> +	struct peak_usb_device *dev;
> +
> +	/* unregister as netdev devices as siblings */
> +	for (dev = usb_get_intfdata(intf); dev; dev = dev->prev_siblings) {
> +		struct net_device *netdev = dev->netdev;
> +		char name[IFNAMSIZ];
> +
> +		dev->state &= ~PCAN_USB_STATE_CONNECTED;
> +		strcpy(name, netdev->name);
> +
> +		unregister_netdev(netdev);
> +		free_candev(netdev);
> +
> +		peak_usb_unlink_all_urbs(dev);
> +
> +		dev->next_siblings = NULL;
> +		if (dev->adapter->dev_free)
> +			dev->adapter->dev_free(dev);
> +
> +		dev_info(&intf->dev, "%s removed\n", name);
> +	}
> +
> +	usb_set_intfdata(intf, NULL);
> +}
> +
> +/*
> + * probe function for new PEAK-System devices
> + */
> +static int peak_usb_probe(struct usb_interface *intf,
> +	const struct usb_device_id *id)
> +{
> +	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct peak_usb_adapter *peak_usb_adapter, **pp;
> +	int i, err = -ENOMEM;
> +
> +	usb_dev = interface_to_usbdev(intf);
> +
> +	/* get corresponding PCAN-USB adapter */
> +	for (pp = peak_usb_adapters_list; *pp; pp++)
> +		if ((*pp)->device_id == usb_dev->descriptor.idProduct)
> +			break;
> +
> +	peak_usb_adapter = *pp;
> +	if (!peak_usb_adapter) {
> +		/* should never come except device_id bad usage in this file */
> +		pr_info("%s: didn't find device id. 0x%x in devices list\n",
> +			PCAN_USB_DRIVER_NAME, usb_dev->descriptor.idProduct);

info or error?

> +		return -ENODEV;
> +	}
> +
> +	/* got corresponding adapter: check if it handles current interface */
> +	if (peak_usb_adapter->intf_probe) {
> +		err = peak_usb_adapter->intf_probe(intf);
> +		if (err)
> +			return err;
> +	}
> +
> +	dev_info(&intf->dev,
> +		"new PEAK-System usb adapter with %u CAN detected:\n",
> +		peak_usb_adapter->ctrl_count);
> +
> +	dev_info(&intf->dev, "%s %s\n",
> +		(usb_dev->manufacturer) ? usb_dev->manufacturer : "PEAK-System",
> +		peak_usb_adapter->name);
> +
> +	if (usb_dev->product) {
> +		/* remove some non-printable chars from the product string */
> +		char *pc;
> +		for (pc = usb_dev->product; *pc != 0; pc++)
> +			if (*pc < 32 || *pc > 127)
> +				*pc = '.';
> +		dev_info(&intf->dev, "%s\n", usb_dev->product);
> +	}
> +
> +	if (usb_dev->serial)
> +		dev_info(&intf->dev, "Serial: %s\n", usb_dev->serial);
> +
> +	for (i = 0; i < peak_usb_adapter->ctrl_count; i++) {
> +		err = peak_usb_create_dev(peak_usb_adapter, intf, i);
> +		if (err) {
> +			/* deregister already created devices */
> +			peak_usb_disconnect(intf);
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +/* usb specific object needed to register this driver with the usb subsystem */
> +static struct usb_driver peak_usb_driver = {
> +	.name = PCAN_USB_DRIVER_NAME,
> +	.disconnect = peak_usb_disconnect,
> +	.probe = peak_usb_probe,
> +	.id_table = peak_usb_table,
> +};
> +
> +static int __init peak_usb_init(void)
> +{
> +	int err;
> +
> +	pr_info("%s: PCAN-USB kernel driver v%s loaded\n",
> +		PCAN_USB_DRIVER_NAME, PCAN_USB_VERSION_STRING);
> +
> +	/* check whether at least ONE device is supported! */
> +	if (peak_usb_table[0].idVendor != PCAN_USB_VENDOR_ID)
> +		pr_warn("%s: adapters list empty (check config options)!\n",
> +			PCAN_USB_DRIVER_NAME);
> +
> +	/* register this driver with the USB subsystem */
> +	err = usb_register(&peak_usb_driver);
> +	if (err)
> +		pr_info("%s: usb_register failed (err %d)\n",
> +			PCAN_USB_DRIVER_NAME, err);

Info or error?

> +	return err;
> +}
> +
> +static int peak_usb_do_device_exit(struct device *d, void *arg)
> +{
> +	struct usb_interface *intf = to_usb_interface(d);
> +	struct peak_usb_device *dev;
> +
> +	/* stop as netdev devices as siblings */
> +	for (dev = usb_get_intfdata(intf); dev; dev = dev->prev_siblings) {
> +		struct net_device *netdev = dev->netdev;
> +
> +		if (netif_device_present(netdev))
> +			if (dev->adapter->dev_exit)
> +				dev->adapter->dev_exit(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit peak_usb_exit(void)
> +{
> +	int err;
> +
> +	/* Last chance do send some synchronous commands here */
> +	err = driver_for_each_device(&peak_usb_driver.drvwrap.driver, NULL,
> +		NULL, peak_usb_do_device_exit);
> +	if (err)
> +		err = 0;

What's that good for?

> +
> +	/* deregister this driver with the USB subsystem */
> +	usb_deregister(&peak_usb_driver);
> +
> +	pr_info("%s: PCAN-USB kernel driver unloaded\n", PCAN_USB_DRIVER_NAME);
> +}
> +
> +module_init(peak_usb_init);
> +module_exit(peak_usb_exit);
> diff --git a/drivers/net/can/usb/peak_usb/peak_usb.h b/drivers/net/can/usb/peak_usb/peak_usb.h
> new file mode 100644
> index 0000000..35ccc2e
> --- /dev/null
> +++ b/drivers/net/can/usb/peak_usb/peak_usb.h
> @@ -0,0 +1,149 @@
> +/*
> + * CAN driver for PEAK System USB adapters
> + *
> + * Copyright (C) 2011-2012 PEAK-System GmbH
> + *
> + * 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 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +#ifndef __peak_usb_h__
> +#define __peak_usb_h__
> +
> +/* PEAK-System USB driver version */
> +#define PCAN_USB_VERSION_MAJOR	0
> +#define PCAN_USB_VERSION_MINOR	4
> +#define PCAN_USB_VERSION_SUBMINOR	4
> +
> +/* PEAK-System vendor id. */
> +#define PCAN_USB_VENDOR_ID	0x0c72
> +
> +/* Driver name */
> +#define PCAN_USB_DRIVER_NAME	"peak_usb"
> +
> +/* Number of urbs that are submitted for rx/tx per channel */
> +#define PCAN_USB_MAX_RX_URBS	4
> +#define PCAN_USB_MAX_TX_URBS	10
> +
> +/* PEAK-System usb adapters maximum channels per usb interface */
> +#define PCAN_USB_MAX_CHANNEL	2
> +
> +struct peak_usb_adapter;
> +struct peak_usb_device;
> +
> +/* PEAK-System USB adapter descriptor */
> +struct peak_usb_adapter {
> +	char *name;
> +	u32 device_id;
> +	struct can_clock clock;
> +	struct can_bittiming_const bittiming_const;
> +	unsigned int ctrl_count;
> +
> +	int (*intf_probe)(struct usb_interface *intf);
> +
> +	int (*dev_init)(struct peak_usb_device *);

Please add a name as you do one line above.

> +	void (*dev_exit)(struct peak_usb_device *);

Ditto here and in may lines below.

> +	void (*dev_free)(struct peak_usb_device *);
> +	int (*dev_open)(struct peak_usb_device *);
> +	int (*dev_close)(struct peak_usb_device *);
> +	int (*dev_set_bittiming)(struct peak_usb_device *,
> +		struct can_bittiming *bt);
> +	int (*dev_set_bus)(struct peak_usb_device *, u8 onoff);
> +	int (*dev_get_device_id)(struct peak_usb_device *, u32 *device_id);
> +	int (*dev_decode_buf)(struct peak_usb_device *dev, struct urb *);
> +	int (*dev_encode_msg)(struct peak_usb_device *dev, struct sk_buff *,
> +		u8 *obuf, size_t *size);
> +	int (*dev_start)(struct peak_usb_device *dev);
> +	int (*dev_stop)(struct peak_usb_device *dev);
> +
> +	u8 ep_msg_in;
> +	u8 ep_msg_out[PCAN_USB_MAX_CHANNEL];
> +	u8 ts_used_bits;
> +	u32 ts_period;
> +	u8 us_per_ts_shift;
> +	u32 us_per_ts_scale;
> +
> +	int rx_buffer_size;
> +	int tx_buffer_size;
> +	int sizeof_dev_private;
> +};
> +
> +struct peak_time_ref {
> +	struct timeval tv_host_0, tv_host;
> +	u32 ts_dev_1, ts_dev_2;
> +	u64 ts_total;
> +	u32 tick_count;
> +	struct peak_usb_adapter *adapter;
> +};
> +
> +struct peak_tx_urb_context {
> +	struct peak_usb_device *dev;
> +	u32 echo_index;
> +	u8 dlc;
> +	struct urb *urb;
> +};
> +
> +#define PCAN_USB_STATE_CONNECTED	0x00000001
> +#define PCAN_USB_STATE_STARTED	0x00000002
> +
> +/* PCAN-USB device */
> +struct peak_usb_device {
> +	struct can_priv can;
> +	struct peak_usb_adapter *adapter;
> +	unsigned int ctrl_idx;
> +	int open_time;
> +	u32 state;
> +
> +	struct sk_buff *echo_skb[PCAN_USB_MAX_TX_URBS];
> +
> +	struct usb_device *udev;
> +	struct net_device *netdev;
> +
> +	atomic_t active_tx_urbs;
> +	struct usb_anchor tx_submitted;
> +	struct peak_tx_urb_context tx_contexts[PCAN_USB_MAX_TX_URBS];
> +
> +	struct usb_anchor rx_submitted;
> +
> +	u32 device_number;
> +	u8 device_num;
> +	u8 device_rev;
> +
> +	u8 ep_msg_in;
> +	u8 ep_msg_out;
> +
> +	u16 bus_load;
> +
> +	struct peak_usb_device *prev_siblings;
> +	struct peak_usb_device *next_siblings;
> +};
> +
> +/* supported device ids. */
> +#if defined(CONFIG_CAN_PCAN_USB) || defined(CONFIG_CAN_PCAN_USB_MODULE)
> +#define PCAN_USB_PRODUCT_ID	0x000c
> +extern struct peak_usb_adapter pcan_usb;
> +#endif
> +
> +#if defined(CONFIG_CAN_PCAN_USB_PRO) || defined(CONFIG_CAN_PCAN_USB_PRO_MODULE)
> +#define PCAN_USBPRO_PRODUCT_ID	0x000d
> +extern struct peak_usb_adapter pcan_usb_pro;
> +#endif
> +
> +void dump_mem(char *prompt, void *p, int l);
> +
> +/* reject device usb interface */
> +int peak_usb_no_dev(struct usb_interface *intf);
> +
> +/* common timestamp management */
> +void peak_usb_init_time_ref(struct peak_time_ref *time_ref,
> +	struct peak_usb_adapter *adapter);
> +void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now);
> +void peak_usb_set_ts_now(struct peak_time_ref *time_ref, u32 ts_now);
> +void peak_usb_get_ts_tv(struct peak_time_ref *time_ref, u32 ts,
> +	struct timeval *tv);
> +#endif

Wolfgang.