Re: "[PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2) "

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 24906 bytes --]

On 12/16/2011 11:19 AM, s.grosjean@peak-system.com wrote:
> From 1cee3be3875f27a2ee3942b00d611450f4369325 Mon Sep 17 00:00:00 2001
> From: Stephane Grosjean <s.grosjean@peak-system.com>
> Date: Fri, 16 Dec 2011 11:11:37 +0100
> Subject: [PATCH] Add support for PEAK-System GmbH CAN USB adapters (v2)

The driver has some unusal coding style, please use checkpatch and do
sparse checking (compile the drivers with C=2). Make use of C99
initialisers. Use foo->bar not &foo->bar[0] to get the pointer for the
first array element

more comments inline.

Marc

> 
> v2 includes:
>  change dev_xxx() into netdev_xxx() macros
>  add missing include linux/module.h
>  pre-allocate urbs and buffers for tx path at _open() and free them at _close()
>   rather than into _start_xmit()
>  remove some unused code and (boring) #ifdef/#endif
>  use "menuconfig" in Kconfig rather than "config" entry
> ---
>  drivers/net/can/usb/Kconfig         |   20 +
>  drivers/net/can/usb/Makefile        |   12 +
>  drivers/net/can/usb/pcan_usb.c      |  654 +++++++++++++++++++
>  drivers/net/can/usb/pcan_usb_core.c |  895 ++++++++++++++++++++++++++
>  drivers/net/can/usb/pcan_usb_pro.c  | 1207 +++++++++++++++++++++++++++++++++++
>  drivers/net/can/usb/pcan_usb_pro.h  |  468 ++++++++++++++
>  drivers/net/can/usb/peak_usb.h      |  148 +++++
>  7 files changed, 3404 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/usb/pcan_usb.c
>  create mode 100644 drivers/net/can/usb/pcan_usb_core.c
>  create mode 100644 drivers/net/can/usb/pcan_usb_pro.c
>  create mode 100644 drivers/net/can/usb/pcan_usb_pro.h
>  create mode 100644 drivers/net/can/usb/peak_usb.h
> 
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index 0452549..cb5f91c 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -13,4 +13,24 @@ config CAN_ESD_USB2
>            This driver supports the CAN-USB/2 interface
>            from esd electronic system design gmbh (http://www.esd.eu).
>  
> +menuconfig CAN_PEAK_USB
> +	tristate "PEAK-System USB adapters"
> +	---help---
> +	  This driver is for PCAN USB interfaces from PEAK-System 
> +	  (http://www.peak-system.com).
> +
> +config CAN_PCAN_USB
> +	tristate "PEAK PCAN-USB adapter"
> +	depends on CAN_PEAK_USB
> +	---help---
> +	  This driver is for the one channel PCAN-USB interface
> +	  from PEAK-System (http://www.peak-system.com).
> +
> +config CAN_PCAN_USB_PRO
> +	tristate "PEAK PCAN-USB Pro adapter"
> +	depends on CAN_PEAK_USB
> +	---help---
> +	  This driver is for the two channels PCAN-USB Pro interface
> +	  from PEAK-System (http://www.peak-system.com).
> +
>  endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index fce3cf1..a6f1cf6 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -5,4 +5,16 @@
>  obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
>  obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
>  
> +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
> +
> +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG

^^^^^^^^^^

one ccflag should be enough :)

> diff --git a/drivers/net/can/usb/pcan_usb.c b/drivers/net/can/usb/pcan_usb.c
> new file mode 100644
> index 0000000..ec46da6
> --- /dev/null
> +++ b/drivers/net/can/usb/pcan_usb.c
> @@ -0,0 +1,654 @@
> +/*
> + * CAN driver for PEAK System PCAN-USB adapter
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

please remove the address

> + */
> +#include <linux/netdevice.h>
> +#include <linux/usb.h>
> +#include <linux/module.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +
> +#include "peak_usb.h"
> +
> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB adapter");
> +
> +/* PCAN-USB Endpoints */
> +#define PCAN_USB_EP_CMDOUT   1
> +#define PCAN_USB_EP_CMDIN    (PCAN_USB_EP_CMDOUT|USB_DIR_IN)
> +#define PCAN_USB_EP_MSGOUT   2
> +#define PCAN_USB_EP_MSGIN    (PCAN_USB_EP_MSGOUT|USB_DIR_IN)
                                                  ^^^
please add spaces around the |

> +
> +/* PCAN-USB parameter command */
> +#define PCAN_USB_PARAMETER_LEN   14
> +struct __packed pcan_usb_parameter {
> +	u8 function;
> +	u8	number;
         ^^^
please use one space
> +	u8 parameters[PCAN_USB_PARAMETER_LEN];
> +};
> +
> +/* PCAN-USB command timeout (ms.) */
> +#define PCAN_USB_COMMAND_TIMEOUT 1000
> +
> +/* PCAN-USB rx/tx buffers size */
> +#define PCAN_USB_RX_BUFFER_SIZE  64
> +#define PCAN_USB_TX_BUFFER_SIZE  64
> +
> +#define PCAN_USB_MSG_HEADER_LEN  2 /* Packet type information (1xbyte) 
> +                                    + count of messages (1xbyte) */
> +
> +/* PCAN-USB adapter internal clock (MHz) */
> +#define PCAN_USB_CRYSTAL_HZ      16000000
> +
> +/* PCAN-USB USB message record status/len field */
> +#define PCAN_USB_STATUSLEN_TIMESTAMP (1 << 7)
> +#define PCAN_USB_STATUSLEN_INTERNAL  (1 << 6)
> +#define PCAN_USB_STATUSLEN_EXT_ID    (1 << 5)
> +#define PCAN_USB_STATUSLEN_RTR       (1 << 4)
> +#define PCAN_USB_STATUSLEN_DLC       (0xf)
> +
> +/* PCAN-USB error flags */
> +#define PCAN_USB_ERROR_TXFULL    0x01
> +#define PCAN_USB_ERROR_RXQOVR    0x02
> +#define PCAN_USB_ERROR_BUS_LIGHT 0x04
> +#define PCAN_USB_ERROR_BUS_HEAVY 0x08
> +#define PCAN_USB_ERROR_BUS_OFF   0x10
> +#define PCAN_USB_ERROR_RXQEMPTY  0x20
> +#define PCAN_USB_ERROR_QOVR      0x40
> +#define PCAN_USB_ERROR_TXQFULL   0x80
> +
> +/* SJA1000 modes */
> +#define SJA1000_MODE_NORMAL 0x00
> +#define SJA1000_MODE_INIT   0x01
> +
> +/* tick duration = 42.666 us => 
> + * (tick_number * 44739243) >> 20 ~ (tick_number * 42666) / 1000
> + *  accuracy = 10^-7
> + */
> +#define PCAN_USB_TS_DIV_SHIFTER        20
> +#define PCAN_USB_TS_US_PER_TICK        44739243

If you want to align the #defines please use tab between symbol and value

> +
> +struct pcan_usb {
> +	struct peak_usb_device pcandev; /* must be the first member */
> +	struct peak_time_ref time_ref;
> +};
> +
> +/*
> + * Send the given PCAN-USB command synchronously
> + */
> +static int pcan_usb_send_command(struct peak_usb_device *dev, u8 f, u8 n, u8 *p)
> +{
> +	int actual_length;
> +	struct pcan_usb_parameter cmd;
> +	int err;
> +
> +	/* usb device unregistered? */
> +	if (!(dev->state & PCAN_USB_STATE_CONNECTED)) return 0;
> +
> +	cmd.function = f;
> +	cmd.number = n;

you can use C99 initialisers instead:

	struct pcan_usb_parameter cmd = {
		.function = f,
		.number = n,
	};

then parameters in automatically set with 0x0....
> +
> +	if (p != NULL)

if (p)

> +		memcpy(&cmd.parameters[0], p, PCAN_USB_PARAMETER_LEN);

I'd write memcpy(cmd.parameters, p, ARRAY_SIZE(cmd.parameters))

> +	else
> +		memset(&cmd.parameters[0], '\0', PCAN_USB_PARAMETER_LEN);

...and you can remove the memset here.

> +
> +	err = usb_bulk_msg(dev->udev, 
> +	                   usb_sndbulkpipe(dev->udev, PCAN_USB_EP_CMDOUT),
> +	                   &cmd, sizeof(struct pcan_usb_parameter),
> +	                   &actual_length, PCAN_USB_COMMAND_TIMEOUT);
> +	if (err) 
> +		netdev_err(dev->netdev, 
> +		           "sending command function=0x%x number=0x%x failure: %d\n",
> +		           f, n, err);
> +
> +	return err;
> +}
> +
> +static int pcan_usb_wait_response(struct peak_usb_device *dev, u8 f, u8 n, u8 *p)
> +{
> +	int actual_length;
> +	struct pcan_usb_parameter cmd;
> +	int err;
> +
> +	/* usb device unregistered? */
> +	if (!(dev->state & PCAN_USB_STATE_CONNECTED)) return 0;

please put the return in a seperate line.

> +
> +	/* first, send command */
> +	err = pcan_usb_send_command(dev, f, n, NULL);
> +	if (err) {
> +		return err;
> +	}

please remove the { }

> +
> +	cmd.function = f;
> +	cmd.number = n;
> +	memset(&cmd.parameters[0], '\0', PCAN_USB_PARAMETER_LEN);

please use C99 initialisers

> +
> +	/* then, wait for the response */
> +	mdelay(5);
> +	err = usb_bulk_msg(dev->udev, 
> +	                   usb_rcvbulkpipe(dev->udev, PCAN_USB_EP_CMDIN),
> +	                   &cmd, sizeof(struct pcan_usb_parameter),
> +	                   &actual_length, PCAN_USB_COMMAND_TIMEOUT);
> +	if (err)
> +		netdev_err(dev->netdev, 
> +		           "waiting response function=0x%x number=0x%x failure: %d\n",
> +		           f, n, err);
> +	else if (p) memcpy(p, &cmd.parameters[0], PCAN_USB_PARAMETER_LEN);

please put the memcpy in a seperate line. I'd use cmd.parameters instead
of &cmd.parameters[0], please use ARRAY_SIZE.

> +
> +	return err;
> +}
> +
> +static int pcan_usb_set_sja1000(struct peak_usb_device *dev, u8 mode)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +
> +	args[0] = 0;
> +	args[1] = mode;

the array can be initializes in C99 style, too:

	u8 args[PCAN_USB_PARAMETER_LEN] = {
		[0] = 0,
		[1] = mode,
	};

The [0] can be skipped, by you might want to include it for
documentation purpose.

Please add a newline in before return

> +	return pcan_usb_send_command(dev, 9, 2, args);
> +}
> +
> +static int pcan_usb_set_bus(struct peak_usb_device *dev, u8 onoff)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +
> +	args[0] = onoff ? 1 : 0;
> +	return pcan_usb_send_command(dev, 3, 2, args);

please use/add C99 initialisers + newline, too

> +}
> +
> +static int pcan_usb_set_silent(struct peak_usb_device *dev, u8 onoff)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +
> +	args[0] = onoff ? 1 : 0;
> +	return pcan_usb_send_command(dev, 3, 3, args);

dito

> +}
> +
> +static int pcan_usb_set_ext_vcc(struct peak_usb_device *dev, u8 onoff)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +
> +	args[0] = onoff ? 1 : 0;
> +	return pcan_usb_send_command(dev, 10, 2, args);

dito

I see some magic numbers for the command here, does it make sense to
define an enum for them?

> +}
> +
> +/*
> + * This routine was stolen from drivers/net/can/sja1000/sja1000.c
> + */

Nice for crediting this, but IMHO no need to write it into the code.

> +static int pcan_usb_set_bittiming(struct peak_usb_device *dev, struct can_bittiming *bt)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +	u8 btr0, btr1;
> +
> +	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
> +	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) \
> +	     | (((bt->phase_seg2 - 1) & 0x7) << 4);
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> +		btr1 |= 0x80;
> +
> +	args[0] = btr1;
> +	args[1] = btr0;
> +
> +	printk("btr0=0x%02x btr1=0x%02x", btr0, btr1);

Please use netdev_LEVEL() instead of printk
> +
> +	return pcan_usb_send_command(dev, 1, 2, args);
> +}
> +
> +static int pcan_usb_write_mode(struct peak_usb_device *dev, u8 onoff)
> +{
> +	int err;
> +
> +	err = pcan_usb_set_bus(dev, onoff);
> +	if (err) 
> +		return err;
> +
> +	if (!onoff) {
> +		err = pcan_usb_set_sja1000(dev, SJA1000_MODE_INIT);
> +	}

please remove the { }
> +
> +	return err;
> +}
> +
> +static int pcan_usb_get_serial_number(struct peak_usb_device *dev, u32 *serial_number)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +	int err;
> +
> +	err = pcan_usb_wait_response(dev, 6, 1, args);
> +	if (err) 
> +		netdev_err(dev->netdev, "getting serial number failure: %d\n", err);
> +	else {
> +		if (serial_number) memcpy(serial_number, &args[0], 4);
> +	}

please remove the { }, no command after the if, please.

Hmmm that serial number to u32 memcpy looks fishy, I smell endianess
problem. Can you test your driver on an big endian machine, like powerpc?

> +
> +	return err;
> +}
> +
> +static int pcan_usb_get_device_number(struct peak_usb_device *dev, u32 *device_number)
> +{
> +	u8 args[PCAN_USB_PARAMETER_LEN];
> +	int err;
> +
> +	err = pcan_usb_wait_response(dev, 4, 1, args);
> +	if (err) 
> +		netdev_err(dev->netdev, "getting device number failure: %d\n", err);
> +	else {
> +		if (device_number) *device_number = args[0];

please remove the { }, no command in the same line as the "if"

> +	}
> +
> +	return err;
> +}
> +
> +static void pcan_usb_update_time_word(struct pcan_usb *pdev, u16 ts16, u8 s)
> +{
> +
> +	if (s == 0)
> +		peak_usb_set_timestamp_now(&pdev->time_ref, ts16);
> +	else 
> +		peak_usb_update_timestamp_now(&pdev->time_ref, ts16);
> +}
> +
> +/*
> + * callback for bulk IN urb
> + */
> +static int pcan_usb_decode_msg(struct peak_usb_device *dev, struct urb *urb)
> +{
> +	struct pcan_usb *pdev = (struct pcan_usb *)dev;
> +	struct net_device *netdev = dev->netdev;
> +	int err = 0;
> +
> +	if (urb->actual_length > PCAN_USB_MSG_HEADER_LEN) {
> +		struct net_device_stats *stats = &netdev->stats;
> +
> +		u8 *ibuf = urb->transfer_buffer;
> +		const u8 msg_count = ibuf[1];
> +		int frm_count = 0;
> +		u8 i;
> +
> +		ibuf += PCAN_USB_MSG_HEADER_LEN;
> +
> +		for (i = 0; i < msg_count && !err; i++) {
> +

please remove that extra blame line

> +			struct sk_buff *skb;
> +			struct can_frame *cf;
> +			struct timeval tv;
> +			u16 tmp16, ts16=0, dts=0, prev_ts8=0;

please add spaces around =

> +			u8 sl = *ibuf++;
> +			u8 rec_len = (sl & PCAN_USB_STATUSLEN_DLC);
> +
> +			/* handle error frames here */

I suggest to put the error handling in a sub function. If you have

> +			if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
> +				u8 f = *ibuf++;
> +				u8 n = *ibuf++;
> +
> +				if (sl & PCAN_USB_STATUSLEN_TIMESTAMP) {
> +					/* only the first packet supplies a word timestamp */
> +					if (i == 0) {
> +						memcpy(&tmp16, ibuf, 2);
> +						ibuf += 2;
> +						ts16 = le16_to_cpu(tmp16);
> +						dts = 0;
> +					}
> +					else {
> +						u8 ts8 = *ibuf++;
> +
> +						if (dts) {
> +							dts &= 0xff00;
> +							if (ts8 < prev_ts8)
> +								dts += 0x100;
> +						}
> +
> +						dts |= ts8;
> +						prev_ts8 = ts8;
> +
> +					}
> +				}
> +
> +				switch (f) {
> +
> +				case 1:
> +
please reove these two empty lines. I think you should define an enum
for the functions.

> +					/* allocate an skb to store the error frame */
> +					skb = alloc_can_err_skb(netdev, &cf);
> +					if (skb == NULL) {
!skb

please talk to Wolfgang for the error handling, he is the expert now :)

> +						err = -ENOMEM;
> +						break;
> +					}
> +
> +					if (n & (PCAN_USB_ERROR_RXQOVR|PCAN_USB_ERROR_QOVR)) {
> +						cf->can_id |= CAN_ERR_CRTL;
> +						cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> +						stats->rx_over_errors++;
> +						stats->rx_errors++;
> +					}
> +					if (n & PCAN_USB_ERROR_BUS_OFF) {
> +						cf->can_id |= CAN_ERR_BUSOFF;
> +						can_bus_off(netdev);
> +					}
> +					if (n & PCAN_USB_ERROR_BUS_HEAVY) {
> +						cf->can_id |= CAN_ERR_CRTL;
> +						cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> +						dev->can.can_stats.error_passive++;
> +					}
> +					if (n & PCAN_USB_ERROR_BUS_LIGHT) {
> +						cf->can_id |= CAN_ERR_CRTL;
> +						cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> +						dev->can.can_stats.error_warning++;
> +					}
> +
> +					if (cf->can_id != CAN_ERR_FLAG) {
> +						if (sl & PCAN_USB_STATUSLEN_TIMESTAMP) {
> +							peak_usb_get_timestamp_tv(&pdev->time_ref, ts16+dts, &tv);
> +							skb->tstamp = timeval_to_ktime(tv);
> +						}
> +						netif_rx(skb);
> +						stats->rx_packets++;
> +					}
> +
> +					/* v3: sometimes the telegram carries 3 additional data */
> +					/* without note in ucStatusLen. */
> +					ibuf += rec_len;
> +					break;
> +
> +				case 2:
> +					/* analog values (ignored) */
> +					ibuf += 2;
> +					break;
> +
> +				case 3:
> +					/* bus load (ignored) */
> +					ibuf++;
> +					break;
> +
> +				case 4:
> +					/* only timestamp */
> +					memcpy(&tmp16, ibuf, 2);
> +					ibuf += 2;
> +					ts16 = le16_to_cpu(tmp16);
> +					pcan_usb_update_time_word(pdev, ts16, i);
> +					break;
> +
> +				case 5:
> +					/* error frame/bus event */
> +					if (n & PCAN_USB_ERROR_TXQFULL) {
> +						netdev_info(netdev, "device Tx queue full)\n");
> +					}

remove { }
> +					ibuf += rec_len;
> +					break;
> +
> +				case 10:
> +					/* future... */
> +					break;
> +
> +				default:
> +					netdev_err(netdev, "unexpected function %u\n", f);
> +					break;
> +				}
> +			}
> +
> +			/* handle normal can frames here */

please use an extra fucntion for normal frames

> +			else {
> +
> +				u8 d = 0;
> +
> +				skb = alloc_can_skb(netdev, &cf);
> +				if (skb == NULL) {
> +					err = -ENOMEM;
> +					break;
> +				}
> +
> +				if (sl & PCAN_USB_STATUSLEN_EXT_ID) {
> +					u32 tmp32;
> +					memcpy(&tmp32, ibuf, 4);
> +					ibuf += 4;
> +					cf->can_id = le32_to_cpu(tmp32 >> 3) | CAN_EFF_FLAG;
> +				}
> +				else {
> +					memcpy(&tmp16, ibuf, 2);
> +					ibuf += 2;
> +					cf->can_id = le16_to_cpu(tmp16 >> 5);
> +				}
> +
> +				cf->can_dlc = get_can_dlc(rec_len);
> +
> +				/* first data packet timestamp is a word */
> +				if (!frm_count++) {
> +					memcpy(&tmp16, ibuf, 2);
> +					ibuf += 2;
> +					ts16 = le16_to_cpu(tmp16);
> +					dts = 0;
> +				}
> +				else {

the preferred coding style is
} else {
> +					u8 ts8 = *ibuf++;
> +
> +					if (dts) {
> +						dts &= 0xff00;
> +						if (ts8 < prev_ts8)
> +							dts += 0x100;
> +					}
> +
> +					dts |= ts8;
> +					prev_ts8 = ts8;

I think I've seen this code before, please add a function for it.

> +
> +				}
> +
> +				/* read data */
> +				if (sl & PCAN_USB_STATUSLEN_RTR) {
> +					cf->can_id |= CAN_RTR_FLAG;
> +				}
> +				else {

} else { - or better get remove the { }

> +					for (; d < rec_len; d++)
> +						cf->data[d] = *ibuf++;
> +				}
> +
> +				for (; d < 8; d++) {
> +					cf->data[d] = 0;
> +				}
memset?

> +
> +				peak_usb_get_timestamp_tv(&pdev->time_ref, ts16+dts, &tv);
> +				skb->tstamp = timeval_to_ktime(tv);
> +				netif_rx(skb);
> +
> +				stats->rx_packets++;
> +				stats->rx_bytes += cf->can_dlc;
> +			}
> +
> +			if ((ibuf - (u8 *)urb->transfer_buffer) > urb->transfer_buffer_length) {

What does this check do? You should do bounds checking of ibuf before
accessing it.

> +				netdev_err(netdev, "usb message format error\n");
> +				err = -EINVAL;
> +				break;
> +			}
> +		}
> +	}
> +	else if (urb->actual_length > 0) {
> +		netdev_err(netdev, "usb message length error (%u)\n", urb->actual_length);
> +		err = -EINVAL;
> +	}

I'd move the error checking to the beginning of the function. Return
early if you detect an error, so that you can get rid of (at least) one
indention level.

> +
> +	return err;
> +}
> +
> +static int pcan_usb_encode_msg(struct peak_usb_device *dev, struct sk_buff *skb, u8 *obuf, size_t *size)
> +{
> +	struct net_device *netdev = dev->netdev;
> +	struct net_device_stats *stats = &netdev->stats;
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	u8 *pc;
> +
> +	obuf[0] = 2;
> +	obuf[1] = 1;
> +
> +	pc = &obuf[PCAN_USB_MSG_HEADER_LEN];
> +
> +	/* status/len byte */
> +	*pc = cf->can_dlc;
> +	if (cf->can_id & CAN_RTR_FLAG) 
> +		*pc |= PCAN_USB_STATUSLEN_RTR;
> +	
> +	/* can id */
> +	if (cf->can_id & CAN_EFF_FLAG) {
> +		u32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
                ^^^ should be __le32 then
> +		tmp32 <<= 3;
> +		*pc |= PCAN_USB_STATUSLEN_EXT_ID;
> +		memcpy(++pc, &tmp32, 4);
> +		pc += 4;
> +	}
> +	else {
} else {
> +		u16 tmp16 = (u16 )cpu_to_le32(cf->can_id & CAN_ERR_MASK);
                            ^^^^^^^^^^^^^^^^^
why convert to le32 and then cast to u16?

> +		tmp16 <<= 5;
> +		memcpy(++pc, &tmp16, 2);
> +		pc += 2;
> +	}
> +
> +	/* can data */
> +	if ((cf->can_id & CAN_RTR_FLAG) == 0) {

if (cf->can_id & CAN_RTR_FLAG)

> +		memcpy(pc, &cf->data[0], cf->can_dlc);

		memcpy(pc, cf->data, cf->can_dlc);

> +		pc += cf->can_dlc;
> +	}
> +
> +	/* Hmm... pcan driver sets the count of messages sent in the last byte... */
> +	obuf[(*size)-1] = (u8 )(stats->tx_packets & 0xff);
> +
> +	return 0;
> +}
> +
> +/*
> + * Start interface
> + */
> +static int pcan_usb_start(struct peak_usb_device *dev)
> +{
> +	struct pcan_usb *pdev = (struct pcan_usb *)dev; 
> +	int err;
> +
> +	/* number of bits used in timestamps read from adapter struct */
> +	peak_usb_init_time_ref(&pdev->time_ref, &pcan_usb);
> +
> +	/* If revision greater than 3, can put silent mode on/off*/
> +	if (dev->device_rev > 3) {
> +	
> +		err = pcan_usb_set_silent(dev, 
> +		                          (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY));
> +		if (err)
> +			goto failed;
> +	}
> +
> +	err = pcan_usb_set_ext_vcc(dev, 0);
> +	if (err)
> +		goto failed;
> +
> +	return 0;
> +
> +failed:
> +
> +	return err;
> +}
> +
> +static int pcan_usb_init(struct peak_usb_device *dev)
> +{
> +	u32 serial_number;
> +	int err;
> +
> +	err = pcan_usb_get_serial_number(dev, &serial_number);
> +	if (!err)
> +		netdev_info(dev->netdev, "serial %08X\n", serial_number);
> +
> +	return err;
> +}
> +
> +/*
> + * probe function for new PCAN-USB usb interface
> + */
> +static int pcan_usb_probe(struct usb_interface *intf)
> +{
> +	struct usb_host_interface *iface_desc;
> +	int i;
> +
> +	/* check endpoint addresses (numbers) and associated max data length */
> +	/* (only from setting 0) */
> +	iface_desc = &intf->altsetting[0];
> +
> +	/* check interface endpoint addresses */
> +	for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
> +		struct usb_endpoint_descriptor *endpoint = &iface_desc->endpoint[i].desc;
> +
> +		/* Below is the list of valid ep addreses. All other ep address */
> +		/* is considered as not-CAN interface address => no dev created */
> +		switch (endpoint->bEndpointAddress) {
> +		case PCAN_USB_EP_CMDOUT:
> +		case PCAN_USB_EP_CMDIN:
> +		case PCAN_USB_EP_MSGOUT:
> +		case PCAN_USB_EP_MSGIN:
> +			break;
> +		default:
> +			return -ENODEV;
> +		}
> +	}
> + 
> +	return 0;
> +}
> +
> +/* 
> + * Describe the PCAN-USB adapter 
> + */
> +struct peak_usb_adapter pcan_usb = {
> +	.name = "PCAN-USB",
> +	.device_id = PCAN_USB_PRODUCT_ID,
> +	.ctrl_count = 1,
> +	.clock = {
> +		.freq = PCAN_USB_CRYSTAL_HZ/2,
> +	},
> +	.bittiming_const = {
> +		.name = "pcan_usb",
> +		.tseg1_min = 1,
> +		.tseg1_max = 16,
> +		.tseg2_min = 1,
> +		.tseg2_max = 8,
> +		.sjw_max = 4,
> +		.brp_min = 1,
> +		.brp_max = 64,
> +		.brp_inc = 1,
> +	},
> +
> +	/* size of device private data */
> +	.sizeof_dev_private = sizeof(struct pcan_usb),
> +	
> +	/* timestamps usage */
> +	.ts_used_bits = 16,
> +	.ts_period = 24575, /* calibration period in ts. */
> +	.us_per_ts_scale = PCAN_USB_TS_US_PER_TICK, /* us = (ts * scale) >> shift */
> +	.us_per_ts_shift = PCAN_USB_TS_DIV_SHIFTER,
> +
> +	/* give here commands/messages in/out endpoints */
> +	.ep_msg_in = PCAN_USB_EP_MSGIN,
> +	.ep_msg_out = {PCAN_USB_EP_MSGOUT},
> +
> +	/* size of rx/tx usb buffers */
> +	.rx_buffer_size = PCAN_USB_RX_BUFFER_SIZE,
> +	.tx_buffer_size = PCAN_USB_TX_BUFFER_SIZE,
> +
> +	/* device callbacks */
> +	.intf_probe = pcan_usb_probe,
> +	.device_init = pcan_usb_init,
> +	.device_set_bus = pcan_usb_write_mode,
> +	.device_set_bittiming = pcan_usb_set_bittiming,
> +	.device_get_device_number = pcan_usb_get_device_number,
> +	.device_decode_msg = pcan_usb_decode_msg,
> +	.device_encode_msg = pcan_usb_encode_msg,
> +	.device_start = pcan_usb_start,
> +};
> +

that's it for now

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]