[PATCH] Add PEAK System USB adapters core driver

[PATCH] Add PEAK System USB adapters core driver

[Stephane Grosjean]

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

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

---
 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
 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).
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)
+
+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 */
+};
+
+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,
+};
+
+/*
+ * 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;
+	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++);
+		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));
+		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);
+
+	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);
+}
+
+/*
+ * 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);
+
+	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))
+		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");
+			return err;
+		}
+
+		netdev_warn(netdev, "rx performance may be slow\n");
+	}
+
+	/* 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");
+	}
+
+	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);
+
+	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)) {
+		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++;
+
+	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);
+		close_candev(netdev);
+		return err;
+	}
+
+	dev->open_time = jiffies;
+	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);
+		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);
+		}
+		if (netif_queue_stopped(netdev))
+			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);
+
+	if (dev->adapter->dev_set_bittiming) {
+		int err = dev->adapter->dev_set_bittiming(dev, bt);
+		if (err)
+			netdev_info(netdev, "couldn't set bitrate (err %d)",
+				err);
+		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);
+
+	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);
+		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);
+
+	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;
+
+	/* 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 *);
+	void (*dev_exit)(struct peak_usb_device *);
+	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
-- 
1.7.1

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

[Sebastian Haas]

Hi again,

some nitpicking this time. ;-)

You are mixing pr_*, dev_* and netdev_* please check if it is possible 
to harmonize the usage.

Cheers,
  Sebastian

Am 22.12.2011 14:11, schrieb Stephane Grosjean:
> +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);
Missing \n and I wonder if you should return \err\ or another error code 
if dev_set_bus failed.

> +		}
> +		if (netif_queue_stopped(netdev))
> +			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;
                                  ^^^ remove whitespace
> +
> +	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);
> +
> +	if (dev->adapter->dev_set_bittiming) {
> +		int err = dev->adapter->dev_set_bittiming(dev, bt);
> +		if (err)
> +			netdev_info(netdev, "couldn't set bitrate (err %d)",
> +				err);
Missing \n.
> +		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))
                         ^^^^^^ cleanup
> +		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;
                            ^^^^ whitespace
> +
> +	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);
> +
> +	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);
Use strncpy with IFNAMSIZ.
> +
> +		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);
> +		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;
> +}
...
> +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;
\err\ is neither checked nor printed, why not removing it?
> +
> +	/* 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);
> +}

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

[Grosjean Stephane]

Hi Sebastian,

Le 22/12/2011 22:41, Sebastian Haas a écrit :
> Hi again,
>
> some nitpicking this time. ;-)
>
... I certainly agree with you! But it's good news anyway.

Generally speaking:

- ok for the two missing ending \n
- ok for strncpy instead of strcpy()
- but what about the followings?

> +    struct can_bittiming *bt =&dev->can.bittiming;
                                  ^^^ remove whitespace
...

> +    if (sizeof_candev<  sizeof(struct peak_usb_device))
                         ^^^^^^ cleanup

...

> +    netdev->netdev_ops =&peak_usb_netdev_ops;
                            ^^^^ whitespace
...

>> +
>> +    SET_NETDEV_DEV(netdev,&intf->dev);
>                              ^^^
>> +
>> +        dev->state&= ~PCAN_USB_STATE_CONNECTED;
>                          ^^^^
>> +            if (*pc<  32 || *pc>  127)
>                               ^^^^         ^^
It looks like missing whitespaces sometimes, and sometimes there were 
too in your text... which is different from the patch I sent yesterday. 
Or do I misunderstand your comments?

>> +
>> +    /* 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;
> \err\ is neither checked nor printed, why not removing it?
>

Because of that (see <linux/device.h>):

extern int __must_check driver_for_each_device(struct device_driver *drv,
            ~~~~~~~~~~~~

... Is there any other way to get around that?

> You are mixing pr_*, dev_* and netdev_* please check if it is possible 
> to harmonize the usage.
>
Ok I'll try do that, whenever it is possible.

> Cheers,
>  Sebastian +

Thanks and regards,

Stéphane

> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

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

[dev]

Am 23.12.2011 10:33, schrieb Grosjean Stephane:
> - but what about the followings?
>
>> +    struct can_bittiming *bt =&dev->can.bittiming;
>                                  ^^^ remove whitespace
Add whitespace between = and &
> ...
>
>> +    if (sizeof_candev<  sizeof(struct peak_usb_device))
>                         ^^^^^^ cleanup
Add whitespace between candev and <
>
> ...
>
>> +    netdev->netdev_ops =&peak_usb_netdev_ops;
>                            ^^^^ whitespace
Add whitespace between = and &
> ...
>
>>> +
>>> +    SET_NETDEV_DEV(netdev,&intf->dev);
>>                              ^^^
Add whitespace between , and &
>>> +
>>> +        dev->state&= ~PCAN_USB_STATE_CONNECTED;
>>                          ^^^^
>>> +            if (*pc<  32 || *pc>  127)
>>                               ^^^^         ^^
Change whitespaces to pc < 32 and pc > 127
> It looks like missing whitespaces sometimes, and sometimes there were
> too in your text... which is different from the patch I sent
> yesterday. Or do I misunderstand your comments?
Yes, this is what I meant. On some places you have too many whitespaces 
when using operators, and sometimes they are completely missing.

>>> +
>>> +    /* 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;
>> \err\ is neither checked nor printed, why not removing it?
>>
>
> Because of that (see <linux/device.h>):
>
> extern int __must_check driver_for_each_device(struct device_driver 
> *drv,
>            ~~~~~~~~~~~~
>
> ... Is there any other way to get around that?
Then I suggest to log it.

I hope to find time to review the other patches on evening.

Cheers,
  Sebastian

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

[Wolfgang Grandegger]

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.

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

[Marc Kleine-Budde]

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

On 12/22/2011 10:41 PM, Sebastian Haas wrote:
> Hi again,
> 
> some nitpicking this time. ;-)
> 
> You are mixing pr_*, dev_* and netdev_* please check if it is possible
> to harmonize the usage.

I think, that you should use netdev_* whenever you have a netdev
pointer, dev_* until no netdev has been registered and pr_* if not even
a device pointer is available.

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 --]

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

[Oliver Hartkopp]

On 10.01.2012 11:17, Wolfgang Grandegger wrote:

>>  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!


AFAIR the driver built results in peak_usb.ko

And the driver contains the pcan_usb.c and pcan_usb_pro.c

If it's possible from the build process pcan_usb_core.c should be renamed to
peak_usb.c - that's right.

Regards,
Oliver

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

[Wolfgang Grandegger]

On 01/10/2012 04:22 PM, Oliver Hartkopp wrote:
> On 10.01.2012 11:17, Wolfgang Grandegger wrote:
> 
>>>  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!
> 
> 
> AFAIR the driver built results in peak_usb.ko
> 
> And the driver contains the pcan_usb.c and pcan_usb_pro.c
> 
> If it's possible from the build process pcan_usb_core.c should be renamed to
> peak_usb.c - that's right.

We should remove the device specific Kconfigs including the related
#ifdefs. It's then *one* driver which always supports the two USB
devices. Anything else does not really make sense. Maybe just for very
low end devices where any byte counts. Have a look to other USB drivers.
They supports tons of devices without any #ifdef.

Wolfgang.

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

[Grosjean Stephane]

Le 10/01/2012 16:35, Wolfgang Grandegger a écrit :
> On 01/10/2012 04:22 PM, Oliver Hartkopp wrote:
>> On 10.01.2012 11:17, Wolfgang Grandegger wrote:
>>
>>>>   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!
>>
>> AFAIR the driver built results in peak_usb.ko
>>
>> And the driver contains the pcan_usb.c and pcan_usb_pro.c
>>
>> If it's possible from the build process pcan_usb_core.c should be renamed to
>> peak_usb.c - that's right.

What I know from the build process doesn't enable to do that (that is, 
building module.ko from module.c **and** file.c:

obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o

linux-can-next$
   CHK     include/linux/version.h
   CHK     include/generated/utsrelease.h
   CALL    scripts/checksyscalls.sh
   CHK     include/generated/compile.h
   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb.o
   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb_pro.o
Kernel: arch/x86/boot/bzImage is ready  (#3)
   Building modules, stage 2.
   MODPOST 3012 modules
ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
ERROR: "peak_usb_set_ts_now" 
[drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
ERROR: "peak_usb_get_ts_tv" 
[drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
ERROR: "peak_usb_init_time_ref" 
[drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko] 
undefined!
ERROR: "peak_usb_update_ts_now" 
[drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko] 
undefined!
ERROR: "peak_usb_init_time_ref" 
[drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
WARNING: modpost: Found 23 section mismatch(es).
To see full details build your kernel with:
'make CONFIG_DEBUG_SECTION_MISMATCH=y'
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
linux-can-next$

... but I'm not an expert in the mainline kernel. Is there another way 
to do that?

> We should remove the device specific Kconfigs including the related
> #ifdefs. It's then *one* driver which always supports the two USB
> devices. Anything else does not really make sense. Maybe just for very
> low end devices where any byte counts.
Ok I'll do a driver which supports the two usb adapters without any #ifdef.

> Have a look to other USB drivers.
> They supports tons of devices without any #ifdef.
... had a look to driver/net/usb  but all of these are single file 
module drivers, so does not help. Where are the other please?
> Wolfgang.
>

Stéphane.

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

[Marc Kleine-Budde]

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

On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
>>>>>   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!
>>>
>>> AFAIR the driver built results in peak_usb.ko
>>>
>>> And the driver contains the pcan_usb.c and pcan_usb_pro.c
>>>
>>> If it's possible from the build process pcan_usb_core.c should be
>>> renamed to
>>> peak_usb.c - that's right.
> 
> What I know from the build process doesn't enable to do that (that is,
> building module.ko from module.c **and** file.c:
> 
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o
> 
> linux-can-next$
>   CHK     include/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CALL    scripts/checksyscalls.sh
>   CHK     include/generated/compile.h
>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb.o
>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb_pro.o
> Kernel: arch/x86/boot/bzImage is ready  (#3)
>   Building modules, stage 2.
>   MODPOST 3012 modules
> ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
> ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
> ERROR: "peak_usb_set_ts_now"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_get_ts_tv"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_init_time_ref"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
> undefined!
> ERROR: "peak_usb_update_ts_now"
> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
> ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
> undefined!
> ERROR: "peak_usb_init_time_ref"
> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
> WARNING: modpost: Found 23 section mismatch(es).
> To see full details build your kernel with:
> 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> linux-can-next$
> 
> ... but I'm not an expert in the mainline kernel. Is there another way
> to do that?

Documentation/kbuild/makefiles.txt has this example:

	obj-$(CONFIG_EXT2_FS) += ext2.o
	ext2-y := balloc.o dir.o file.o ialloc.o inode.o ioctl.o \
		namei.o super.o symlink.o
	ext2-$(CONFIG_EXT2_FS_XATTR) += xattr.o xattr_user.o \
		xattr_trusted.o

Which translates into (untested, though):

	obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
	peak_usb-y := peak_usb_core.o pcan_usb.o pcan_usb_pro.o

With a peak_usb_core.c file.

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 --]

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

[Wolfgang Grandegger]

On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
> 
> 
> Le 10/01/2012 16:35, Wolfgang Grandegger a écrit :
>> On 01/10/2012 04:22 PM, Oliver Hartkopp wrote:
>>> On 10.01.2012 11:17, Wolfgang Grandegger wrote:
>>>
>>>>>   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!
>>>
>>> AFAIR the driver built results in peak_usb.ko
>>>
>>> And the driver contains the pcan_usb.c and pcan_usb_pro.c
>>>
>>> If it's possible from the build process pcan_usb_core.c should be
>>> renamed to
>>> peak_usb.c - that's right.
> 
> What I know from the build process doesn't enable to do that (that is,
> building module.ko from module.c **and** file.c:
> 
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o
> 
> linux-can-next$
>   CHK     include/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CALL    scripts/checksyscalls.sh
>   CHK     include/generated/compile.h
>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb.o
>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb_pro.o
> Kernel: arch/x86/boot/bzImage is ready  (#3)
>   Building modules, stage 2.
>   MODPOST 3012 modules
> ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
> ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
> ERROR: "peak_usb_set_ts_now"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_get_ts_tv"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_init_time_ref"
> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
> ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
> undefined!
> ERROR: "peak_usb_update_ts_now"
> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
> ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
> undefined!
> ERROR: "peak_usb_init_time_ref"
> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
> WARNING: modpost: Found 23 section mismatch(es).
> To see full details build your kernel with:
> 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> linux-can-next$
> 
> ... but I'm not an expert in the mainline kernel. Is there another way
> to do that?
> 
>> We should remove the device specific Kconfigs including the related
>> #ifdefs. It's then *one* driver which always supports the two USB
>> devices. Anything else does not really make sense. Maybe just for very
>> low end devices where any byte counts.
> Ok I'll do a driver which supports the two usb adapters without any #ifdef.
> 
>> Have a look to other USB drivers.
>> They supports tons of devices without any #ifdef.
> ... had a look to driver/net/usb  but all of these are single file
> module drivers, so does not help. Where are the other please?

Search for drivers calling usb_register (they are not in
"driver/net/usb"). I was looking in
"drivers/media/dvb/dvb-usb/dib0700_devices.c".

Anyway, I dislike the way you provide support for the two devices. If
you want to separate them, you should register a driver for each using
"usb_register" using a common infrastructure. You currently provide one
driver for both using a homebrewn interface. Have a look to
"drivers/media/dvb/dvb-usb". But I'm not sure if it's worth the effort.

What would be the use-case for creating a module for just one device? If
you provide a distribution, you need to enable both anyway.

Wolfgang.

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

[Grosjean Stephane]

Le 11/01/2012 10:50, Marc Kleine-Budde a écrit :
> On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
>>>>>>    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!
>>>> AFAIR the driver built results in peak_usb.ko
>>>>
>>>> And the driver contains the pcan_usb.c and pcan_usb_pro.c
>>>>
>>>> If it's possible from the build process pcan_usb_core.c should be
>>>> renamed to
>>>> peak_usb.c - that's right.
>> What I know from the build process doesn't enable to do that (that is,
>> building module.ko from module.c **and** file.c:
>>
>> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o
>>
>> linux-can-next$
>>    CHK     include/linux/version.h
>>    CHK     include/generated/utsrelease.h
>>    CALL    scripts/checksyscalls.sh
>>    CHK     include/generated/compile.h
>>    CC [M]  drivers/net/can/usb/peak_usb/pcan_usb.o
>>    CC [M]  drivers/net/can/usb/peak_usb/pcan_usb_pro.o
>> Kernel: arch/x86/boot/bzImage is ready  (#3)
>>    Building modules, stage 2.
>>    MODPOST 3012 modules
>> ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
>> ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
>> ERROR: "peak_usb_set_ts_now"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_get_ts_tv"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_init_time_ref"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
>> undefined!
>> ERROR: "peak_usb_update_ts_now"
>> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
>> ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
>> undefined!
>> ERROR: "peak_usb_init_time_ref"
>> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
>> WARNING: modpost: Found 23 section mismatch(es).
>> To see full details build your kernel with:
>> 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
>> make[1]: *** [__modpost] Error 1
>> make: *** [modules] Error 2
>> linux-can-next$
>>
>> ... but I'm not an expert in the mainline kernel. Is there another way
>> to do that?
> Documentation/kbuild/makefiles.txt has this example:
>
> 	obj-$(CONFIG_EXT2_FS) += ext2.o
> 	ext2-y := balloc.o dir.o file.o ialloc.o inode.o ioctl.o \
> 		namei.o super.o symlink.o
> 	ext2-$(CONFIG_EXT2_FS_XATTR) += xattr.o xattr_user.o \
> 		xattr_trusted.o
>
> Which translates into (untested, though):
>
> 	obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
> 	peak_usb-y := peak_usb_core.o pcan_usb.o pcan_usb_pro.o
>
> With a peak_usb_core.c file.
Yes of course this works... I already tested that but if everybody 
agrees this workaround, I'll use it  (but it is cheating ;-))
> Marc

Stéphane.

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

[Wolfgang Grandegger]

On 01/11/2012 10:50 AM, Marc Kleine-Budde wrote:
> On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
>>>>>>   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!
>>>>
>>>> AFAIR the driver built results in peak_usb.ko
>>>>
>>>> And the driver contains the pcan_usb.c and pcan_usb_pro.c
>>>>
>>>> If it's possible from the build process pcan_usb_core.c should be
>>>> renamed to
>>>> peak_usb.c - that's right.
>>
>> What I know from the build process doesn't enable to do that (that is,
>> building module.ko from module.c **and** file.c:
>>
>> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o
>>
>> linux-can-next$
>>   CHK     include/linux/version.h
>>   CHK     include/generated/utsrelease.h
>>   CALL    scripts/checksyscalls.sh
>>   CHK     include/generated/compile.h
>>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb.o
>>   CC [M]  drivers/net/can/usb/peak_usb/pcan_usb_pro.o
>> Kernel: arch/x86/boot/bzImage is ready  (#3)
>>   Building modules, stage 2.
>>   MODPOST 3012 modules
>> ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
>> ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined!
>> ERROR: "peak_usb_set_ts_now"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_get_ts_tv"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_init_time_ref"
>> [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined!
>> ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
>> undefined!
>> ERROR: "peak_usb_update_ts_now"
>> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
>> ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko]
>> undefined!
>> ERROR: "peak_usb_init_time_ref"
>> [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined!
>> WARNING: modpost: Found 23 section mismatch(es).
>> To see full details build your kernel with:
>> 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
>> make[1]: *** [__modpost] Error 1
>> make: *** [modules] Error 2
>> linux-can-next$
>>
>> ... but I'm not an expert in the mainline kernel. Is there another way
>> to do that?
> 
> Documentation/kbuild/makefiles.txt has this example:
> 
> 	obj-$(CONFIG_EXT2_FS) += ext2.o
> 	ext2-y := balloc.o dir.o file.o ialloc.o inode.o ioctl.o \
> 		namei.o super.o symlink.o
> 	ext2-$(CONFIG_EXT2_FS_XATTR) += xattr.o xattr_user.o \
> 		xattr_trusted.o
> 
> Which translates into (untested, though):
> 
> 	obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
> 	peak_usb-y := peak_usb_core.o pcan_usb.o pcan_usb_pro.o
> 
> With a peak_usb_core.c file.

I suggested to use peak_usb.c (instead of pcan_core.c) to have a
consistent naming (header file and prefix). This seems not to be
possible. I was not aware of that. Naming the files peak_usb_core.c and
peak_usb_core.h seems a good alternative.

Wolfgang.

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

[Oliver Hartkopp]

On 11.01.2012 11:12, Wolfgang Grandegger wrote:

> On 01/11/2012 10:50 AM, Marc Kleine-Budde wrote:
>> On 01/11/2012 10:23 AM, Grosjean Stephane wrote:

>> Documentation/kbuild/makefiles.txt has this example:
>>
>> 	obj-$(CONFIG_EXT2_FS) += ext2.o
>> 	ext2-y := balloc.o dir.o file.o ialloc.o inode.o ioctl.o \
>> 		namei.o super.o symlink.o
>> 	ext2-$(CONFIG_EXT2_FS_XATTR) += xattr.o xattr_user.o \
>> 		xattr_trusted.o
>>
>> Which translates into (untested, though):
>>
>> 	obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
>> 	peak_usb-y := peak_usb_core.o pcan_usb.o pcan_usb_pro.o
>>
>> With a peak_usb_core.c file.
> 
> I suggested to use peak_usb.c (instead of pcan_core.c) to have a
> consistent naming (header file and prefix). This seems not to be
> possible. I was not aware of that. Naming the files peak_usb_core.c and
> peak_usb_core.h seems a good alternative.



pcan is the adapter model name and peak is the manufacturer name.

Then i would suggest to name all building source files with pcan_*

like:

pcan_usb_core.c
pcan_usb_core.h
pcan_usb.c
pcan_usb_pro.c

resulting in a peak_usb.ko


Regards,
Oliver

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

[Wolfgang Grandegger]

On 01/11/2012 11:29 AM, Oliver Hartkopp wrote:
> On 11.01.2012 11:12, Wolfgang Grandegger wrote:
> 
>> On 01/11/2012 10:50 AM, Marc Kleine-Budde wrote:
>>> On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
> 
>>> Documentation/kbuild/makefiles.txt has this example:
>>>
>>> 	obj-$(CONFIG_EXT2_FS) += ext2.o
>>> 	ext2-y := balloc.o dir.o file.o ialloc.o inode.o ioctl.o \
>>> 		namei.o super.o symlink.o
>>> 	ext2-$(CONFIG_EXT2_FS_XATTR) += xattr.o xattr_user.o \
>>> 		xattr_trusted.o
>>>
>>> Which translates into (untested, though):
>>>
>>> 	obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
>>> 	peak_usb-y := peak_usb_core.o pcan_usb.o pcan_usb_pro.o
>>>
>>> With a peak_usb_core.c file.
>>
>> I suggested to use peak_usb.c (instead of pcan_core.c) to have a
>> consistent naming (header file and prefix). This seems not to be
>> possible. I was not aware of that. Naming the files peak_usb_core.c and
>> peak_usb_core.h seems a good alternative.
> 
> 
> 
> pcan is the adapter model name and peak is the manufacturer name.
> 
> Then i would suggest to name all building source files with pcan_*
> 
> like:
> 
> pcan_usb_core.c
> pcan_usb_core.h
> pcan_usb.c
> pcan_usb_pro.c
> 
> resulting in a peak_usb.ko

I don't care about the name but it should be used consistently, for the
sake of readability.

Wolfgang.