Also please send a *series* of patches next time, e.g.:

[PATCH 1/3 v2] ....

On 12/22/2011 02:13 PM, Stephane Grosjean wrote:

No need for "\"

+		| (((bt->phase_seg2 - 1) & 0x7) << 4);
+	if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		btr1 |= 0x80;
+
+	args[0] = btr1;
+	args[1] = btr0;
+
+	netdev_dbg(dev->netdev, "btr0=0x%02x btr1=0x%02x\n", btr0, btr1);

Please use netdev_info here, like all other drivers.

+
+	return pcan_usb_send_cmd(dev, 1, 2, args);
+}
+
+/*
+ * init/reset can
+ */
+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);
+
+	return err;
+}
+
+/*
+ * read serial number from device
+ */
+static int pcan_usb_get_serial(struct peak_usb_device *dev, u32 *serial_number)
+{
+	u8 args[PCAN_USB_PARAMETER_LEN];
+	int err;
+
+	err = pcan_usb_wait_rsp(dev, 6, 1, args);
+	if (err)
+		netdev_err(dev->netdev, "getting serial failure: %d\n", err);
+

Remove empty line.

+	else if (serial_number) {
+		u32 tmp32;

Add empty line.

+		memcpy(&tmp32, args, 4);
+		*serial_number = le32_to_cpu(tmp32);
+	}
+
+	return err;
+}
+
+/*
+ * read device id from device
+ */
+static int pcan_usb_get_device_id(struct peak_usb_device *dev, u32 *device_id)
+{
+	u8 args[PCAN_USB_PARAMETER_LEN];
+	int err;
+
+	err = pcan_usb_wait_rsp(dev, 4, 1, args);
+	if (err)
+		netdev_err(dev->netdev, "getting device id failure: %d\n", err);
+
+	else if (device_id)
+		*device_id = args[0];
+
+	return err;
+}
+
+/*
+ * update current time ref with received timestamp
+ */
+static int pcan_usb_update_ts(struct pcan_usb_msg_context *mc)
+{
+	u16 tmp16;
+
+	if ((mc->ptr+2) > mc->end)
+		return -EINVAL;
+
+	memcpy(&tmp16, mc->ptr, 2);
+
+	mc->ts16 = le16_to_cpu(tmp16);
+
+	if (mc->rec_idx > 0)
+		peak_usb_update_ts_now(&mc->pdev->time_ref, mc->ts16);
+	else
+		peak_usb_set_ts_now(&mc->pdev->time_ref, mc->ts16);
+
+	return 0;
+}
+
+/*
+ * decode received timestamp
+ */
+static int pcan_usb_decode_ts(struct pcan_usb_msg_context *mc, u8 first_packet)
+{
+	/* only 1st packet supplies a word timestamp */
+	if (first_packet) {
+		u16 tmp16;
+
+		if ((mc->ptr+2) > mc->end)

Spaces around "+"

+			return -EINVAL;
+
+		memcpy(&tmp16, mc->ptr, 2);
+		mc->ptr += 2;
+
+		mc->ts16 = le16_to_cpu(tmp16);
+		mc->prev_ts8 = mc->ts16 & 0x00ff;
+	} else {
+		u8 ts8;
+
+		if ((mc->ptr+1) > mc->end)

Ditto

+			return -EINVAL;
+
+		ts8 = *mc->ptr++;
+
+		if (ts8 < mc->prev_ts8)
+			mc->ts16 += 0x100;
+
+		mc->ts16 &= 0xff00;
+		mc->ts16 |= ts8;
+		mc->prev_ts8 = ts8;
+	}
+
+	return 0;
+}
+
+/*
+ * decode non-data usb message
+ */
+static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
+	u8 status_len)
+{
+	u8 rec_len = (status_len & PCAN_USB_STATUSLEN_DLC);
+	struct sk_buff *skb;
+	struct can_frame *cf;
+	struct timeval tv;
+	u8 f, n;
+
+	/* check whether function and number can be read */
+	if ((mc->ptr + 2) > mc->end)
+		return -EINVAL;
+
+	f = mc->ptr[0];
+	n = mc->ptr[1];
+	mc->ptr += 2;
+
+	if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
+		int err = pcan_usb_decode_ts(mc, !mc->rec_idx);

Add empty line .

+		if (err)
+			return err;
+	}
+
+	switch (f) {
+	case PCAN_USB_REC_ERROR:
+		/* no status flag => ignore record */
+		if (!n)
+			break;
+
+		/* ignore this error until 1st ts received */
+		if (n == PCAN_USB_ERROR_QOVR)
+			if (!mc->pdev->time_ref.tick_count)
+				break;
+
+		/* allocate an skb to store the error frame */
+		skb = alloc_can_err_skb(mc->netdev, &cf);
+		if (!skb)
+			return -ENOMEM;
+
+		if (n & (PCAN_USB_ERROR_RXQOVR | PCAN_USB_ERROR_QOVR)) {
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
+			mc->netdev->stats.rx_over_errors++;
+			mc->netdev->stats.rx_errors++;
+		}
+		if (n & PCAN_USB_ERROR_BUS_OFF) {
+			cf->can_id |= CAN_ERR_BUSOFF;
+			can_bus_off(mc->netdev);
+		}
+		if (n & PCAN_USB_ERROR_BUS_HEAVY) {
+			cf->can_id |= CAN_ERR_CRTL;
+			cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;

As you don't know the direction. Please add "| CAN_ERR_CRTL_TX_PASSIVE"

+			mc->pdev->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;
+			mc->pdev->dev.can.can_stats.error_warning++;

Similar here.

+		}
+
+		if (cf->can_id != CAN_ERR_FLAG) {
+			if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
+				peak_usb_get_ts_tv(&mc->pdev->time_ref,
+					mc->ts16, &tv);
+				skb->tstamp = timeval_to_ktime(tv);
+			}
+			netif_rx(skb);
+			mc->netdev->stats.rx_packets++;

What about rx_bytes?

+		} else {
+			kfree_skb(skb);
+		}
+
+		break;
+
+	case PCAN_USB_REC_ANALOG:
+		/* analog values (ignored) */
+		rec_len = 2;
+		break;
+
+	case PCAN_USB_REC_BUSLOAD:
+		/* bus load (ignored) */
+		rec_len = 1;
+		break;
+
+	case PCAN_USB_REC_TS:
+		/* only timestamp */
+		if (pcan_usb_update_ts(mc))
+			return -EINVAL;
+		break;
+
+	case PCAN_USB_REC_BUSEVT:
+		/* error frame/bus event */
+		if (n & PCAN_USB_ERROR_TXQFULL)
+			netdev_info(mc->netdev, "device Tx queue full)\n");

Can this happen frequently? netdev_dbg() or ratelimiting?

+		break;
+
+	case PCAN_USB_REC_EXT:
+		/* future... */
+		break;

Please remove if it's not really needed!

+	default:
+		netdev_err(mc->netdev, "unexpected function %u\n", f);
+		break;
+	}
+
+	if ((mc->ptr + rec_len) > mc->end)
+		return -EINVAL;
+
+	mc->ptr += rec_len;
+
+	return 0;
+}
+
+/*
+ * decode data usb message
+ */
+static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
+{
+	u8 rec_len = (status_len & PCAN_USB_STATUSLEN_DLC);

"()" not needed.

+	struct sk_buff *skb;
+	struct can_frame *cf;
+	struct timeval tv;
+
+	skb = alloc_can_skb(mc->netdev, &cf);
+	if (!skb)
+		return -ENOMEM;
+
+	if (status_len & PCAN_USB_STATUSLEN_EXT_ID) {
+		u32 tmp32;
+
+		if ((mc->ptr + 4) > mc->end)
+			goto decode_failed;
+
+		memcpy(&tmp32, mc->ptr, 4);
+		mc->ptr += 4;
+
+		cf->can_id = le32_to_cpu(tmp32 >> 3) | CAN_EFF_FLAG;
+	} else {
+		u16 tmp16;
+
+		if ((mc->ptr + 2) > mc->end)
+			goto decode_failed;
+
+		memcpy(&tmp16, mc->ptr, 2);
+		mc->ptr += 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 (pcan_usb_decode_ts(mc, !mc->rec_data_idx))
+		goto decode_failed;
+
+	/* read data */
+	memset(cf->data, 0x0, sizeof(cf->data));
+	if (status_len & PCAN_USB_STATUSLEN_RTR) {
+		cf->can_id |= CAN_RTR_FLAG;
+	} else {
+		if ((mc->ptr + rec_len) > mc->end)
+			goto decode_failed;
+
+		memcpy(cf->data, mc->ptr, rec_len);
+		mc->ptr += rec_len;
+	}
+
+	/* convert timestamp into kernel time */
+	peak_usb_get_ts_tv(&mc->pdev->time_ref, mc->ts16, &tv);
+	skb->tstamp = timeval_to_ktime(tv);
+
+	/* push the skb */
+	netif_rx(skb);
+
+	/* update statistics */
+	mc->netdev->stats.rx_packets++;
+	mc->netdev->stats.rx_bytes += cf->can_dlc;
+
+	return 0;
+
+decode_failed:
+	kfree_skb(skb);
+	return -EINVAL;
+}
+
+/*
+ * process incoming message
+ */
+static int pcan_usb_decode_msg(struct peak_usb_device *dev,
+	u8 *ibuf, u32 lbuf)
+{
+	struct pcan_usb_msg_context mc = {
+		.rec_cnt = ibuf[1],
+		.ptr = ibuf + PCAN_USB_MSG_HEADER_LEN,
+		.end = ibuf + lbuf,
+		.netdev = dev->netdev,
+		.pdev = (struct pcan_usb *)dev,

Is the cast really necessary?

+	};
+	int err;
+
+	for (err = 0; mc.rec_idx < mc.rec_cnt && !err; mc.rec_idx++) {
+		u8 sl = *mc.ptr++;
+
+		/* handle status and error frames here */
+		if (sl & PCAN_USB_STATUSLEN_INTERNAL) {
+			err = pcan_usb_decode_status(&mc, sl);
+

Remove empty line.

+		/* handle normal can frames here */
+		} else {
+			err = pcan_usb_decode_data(&mc, sl);
+			mc.rec_data_idx++;
+		}
+	}
+
+	return err;
+}
+
+/*
+ * process any incoming buffer
+ */
+static int pcan_usb_decode_buf(struct peak_usb_device *dev, struct urb *urb)
+{
+	int err = 0;
+
+	if (urb->actual_length > PCAN_USB_MSG_HEADER_LEN) {
+		err = pcan_usb_decode_msg(dev, urb->transfer_buffer,
+			urb->actual_length);
+
+	} else if (urb->actual_length > 0) {
+		netdev_err(dev->netdev, "usb message length error (%u)\n",
+			urb->actual_length);
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
+/*
+ * process outgoing packet
+ */
+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) {
+		__le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);

Empty line.

+		tmp32 <<= 3;
+		*pc |= PCAN_USB_STATUSLEN_EXT_ID;
+		memcpy(++pc, &tmp32, 4);
+		pc += 4;
+	} else {
+		__le16 tmp16 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);

Ditto.

+		tmp16 <<= 5;
+		memcpy(++pc, &tmp16, 2);
+		pc += 2;
+	}
+
+	/* can data */
+	if (!(cf->can_id & CAN_RTR_FLAG)) {
+		memcpy(pc, cf->data, cf->can_dlc);
+		pc += cf->can_dlc;
+	}
+
+	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));

"()" not needed!

+		if (err)
+			goto start_failed;
+	}
+
+	err = pcan_usb_set_ext_vcc(dev, 0);
+	if (err)
+		goto start_failed;
+
+	return 0;
+
+start_failed:
+	return err;

This label is not really useful.

+}
+
+static int pcan_usb_init(struct peak_usb_device *dev)
+{
+	u32 serial_number;
+	int err;
+
+	err = pcan_usb_get_serial(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 *if_desc;
+	int i;
+
+	if_desc = intf->altsetting;
+
+	/* check interface endpoint addresses */
+	for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
+		struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
+
+		switch (ep->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,

Spaces around "/"

+	},
+	.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) */
+	.us_per_ts_shift = PCAN_USB_TS_DIV_SHIFTER, /*  >> shift     */
+
+	/* 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,
+	.dev_init = pcan_usb_init,
+	.dev_set_bus = pcan_usb_write_mode,
+	.dev_set_bittiming = pcan_usb_set_bittiming,
+	.dev_get_device_id = pcan_usb_get_device_id,
+	.dev_decode_buf = pcan_usb_decode_buf,
+	.dev_encode_msg = pcan_usb_encode_msg,
+	.dev_start = pcan_usb_start,
+};

Wolfgang.