From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [PATCH] Add PEAK System USB adapters core driver Date: Tue, 10 Jan 2012 11:17:17 +0100 Message-ID: <4F0C102D.5060304@grandegger.com> References: <871799.098418222-sendEmail@ubuntu-i386> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:45470 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754929Ab2AJKRW (ORCPT ); Tue, 10 Jan 2012 05:17:22 -0500 In-Reply-To: <871799.098418222-sendEmail@ubuntu-i386> Sender: linux-can-owner@vger.kernel.org List-ID: To: Stephane Grosjean Cc: Oliver Hartkopp , Linux CAN mailing list 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 > 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#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 "); > +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.