Hi Sebastian,
chekcpatch.pl reports various errors like DOS endings :-( and lines too
long. More comments inline. I suggest various improvements mainly to
improve the readability. There are a few patterns:
> + union {
> + struct cpc_m16c_basic_params m16c_basic;
> + struct cpc_sja1000_params sja1000;
Please don't align the names, especially because you do not do it
consequently.
> + struct can_frame *cf;
> + struct sk_buff *skb;
> +
> + struct net_device_stats *stats =&dev->netdev->stats;
Don't use emtpy lines in the function/block header.
> +#define CPC_MSG_T_CAN 1 /* CAN data frame */
> +#define CPC_MSG_T_RTR 8 /* CAN remote frame */
The names of some macro definition could be improved for better readability.
> + msg->msg.canparams.cc_params.sja1000.mode = mod;
This is ugly. When it's used often, it could be replaced by:
struct cpc_sja1000_params *sja =
&msg->msg.canparams.cc_params.sja1000;
sja->acc_code0 = 0x00;
...
Often you use the variable name "status" for the function return value.
Please check if "err" might not be more appropriate.
Also, please put defines in the file header.
More comments inline...
On 09/09/2009 08:27 AM, Sebastian Haas wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> This patch adds support for the CAN/USB interface CPC-USB/ARM7 from EMS
> Dr. Thomas Wuensche.
>
> Signed-off-by: Sebastian Haas<h...@ems-wuensche.com>
>
> Makefile | 1
> drivers/net/can/Kconfig | 8
> drivers/net/can/Makefile | 1
> drivers/net/can/ems_usb.c | 1114
> ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1124 insertions(+)
>
> Index: Makefile
> ===================================================================
> - --- Makefile (Revision 1048)
> +++ Makefile (Arbeitskopie)
> @@ -19,6 +19,7 @@
> export CONFIG_CAN_PEAK_PCI=m
> export CONFIG_CAN_KVASER_PCI=m
> export CONFIG_CAN_EMS_PCI=m
> +export CONFIG_CAN_EMS_USB=m
> export CONFIG_CAN_EMS_PCMCIA=m
> export CONFIG_CAN_EMS_104M=m
> export CONFIG_CAN_ESD_PCI=m
> Index: drivers/net/can/Kconfig
> ===================================================================
> - --- drivers/net/can/Kconfig (Revision 1048)
> +++ drivers/net/can/Kconfig (Arbeitskopie)
> @@ -127,6 +127,14 @@
> OpenFirmware bindings, e.g. if you have a PowerPC based system
> you may want to enable this option.
>
> +config CAN_EMS_USB
> + tristate "EMS CPC-USB/ARM7"
> + depends on USB&& CAN_DEV
> + ---help---
> + This driver is for the one channel CAN/USB interface CPC-USB
> + from EMS Dr. Thomas Wuensche.
> + (http://www.ems-wuensche.de).
Does fits on one line.
> config CAN_EMS_PCI
> tristate "EMS CPC-PCI, CPC-PCIe and CPC-104P Card"
> depends on PCI&& CAN_SJA1000
> Index: drivers/net/can/Makefile
> ===================================================================
> - --- drivers/net/can/Makefile (Revision 1048)
> +++ drivers/net/can/Makefile (Arbeitskopie)
> @@ -49,6 +49,7 @@
> obj-$(CONFIG_CAN_MSCAN_OLD) += old/mscan/
> obj-$(CONFIG_CAN_CCAN_OLD) += old/ccan/
> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> +obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
>
> ifeq ($(CONFIG_CAN_DEBUG_DEVICES),y)
> EXTRA_CFLAGS += -DDEBUG
> Index: drivers/net/can/ems_usb.c
> ===================================================================
> - --- drivers/net/can/ems_usb.c (Revision 0)
> +++ drivers/net/can/ems_usb.c (Revision 0)
> @@ -0,0 +1,1114 @@
> +/*
> + * CAN driver for EMS Dr. Thomas Wuensche CPC-USB/ARM7
> + *
> + * Copyright (C) 2004-2009 EMS Dr. Thomas Wuensche
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published
> + * by the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +#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<asm/uaccess.h>
> +
> +#include<socketcan/can.h>
> +#include<socketcan/can/dev.h>
> +#include<socketcan/can/error.h>
> +#include<socketcan/can/dev.h>
Already included above.
> +
> +MODULE_AUTHOR("Sebastian Haas<h...@ems-wuensche.com>");
> +MODULE_DESCRIPTION("CAN driver for EMS Dr. Thomas Wuensche CAN/USB
> interfaces");
> +MODULE_VERSION("v0.9");
I'm not happy about the version number. Git does a better job.
> +MODULE_LICENSE("GPL v2");
> +
> +/* Control-Values for CPC_Control() Command Subject Selection */
> +#define CONTR_CAN_Message 0x04
> +#define CONTR_CAN_State 0x0C
> +#define CONTR_CmdQueue 0x18
> +#define CONTR_BusError 0x1C
Please use uppercase letters and underscores for defines.
> +/* Control Command Actions */
> +#define CONTR_CONT_OFF 0
> +#define CONTR_CONT_ON 1
> +#define CONTR_SING_ON 2
> +
> +/* Messages from CPC to PC contain a message object type field.
> + * The following message types are sent by CPC and can be used in
> + * handlers, others should be ignored.
> + */
> +#define CPC_MSG_T_CAN 1 /* CAN data frame */
What is the meaning of "_T_"? I would either remove it or use "_TYPE_
instead to improve readability.
> +#define CPC_MSG_T_RTR 8 /* CAN remote frame */
> +#define CPC_MSG_T_CAN_PRMS 12 /* Actual CAN parameters */
> +#define CPC_MSG_T_CANSTATE 14 /* CAN state message */
Your naming here and in other places is not always logical to me and. I
think the following is more readable:
#define CPC_MSG_T_CAN_DATA 1 /* CAN data frame */
#define CPC_MSG_T_CAN_RTR 8 /* CAN remote frame */
#define CPC_MSG_T_CAN_PARAMS 12 /* Actual CAN parameters */
#define CPC_MSG_T_CAN_STATE 14 /* CAN state message */
Would be more readable and consistent.
> +#define CPC_MSG_T_XCAN 16 /* XCAN data frame */
Indention? Why do you use tabs here?
> +#define CPC_MSG_T_XRTR 17 /* XCAN remote frame */
CPC_MSG_T_CAN_XRTR or CPC_MSG_T_CAN_EXT_RTR? Please explain the meaning
of "X":
/* Extended CAN remote frame */
/* Standard CAN remote frame */
> +#define CPC_MSG_T_CONTROL 19 /* used for control of interface behavior*/
> +#define CPC_MSG_T_CONFIRM 20 /* response type for confirmed requests */
> +#define CPC_MSG_T_OVERRUN 21 /* response type for overrun conditions */
> +#define CPC_MSG_T_CANERROR 23 /* response type for bus error conditions*/
> +#define CPC_MSG_T_ERR_COUNTER 25 /* RX/TX error counter of CAN controller */
> +
> +/* Messages from the PC to the CPC interface contain a command field
> + * Most of the command types are wrapped by the library functions and have
> + * therefore normally not to be used.
> + * However, programmers who wish to circumvent the library and talk directly
> + * to the drivers (mainly Linux programmers) can use the following
> + *
> + * Command types:
> + */
> +#define CPC_CMD_T_CAN 1 /* CAN data frame */
> +#define CPC_CMD_T_CONTROL 3 /* used for control of interface
> behavior */
> +#define CPC_CMD_T_CAN_PRMS 6 /* set CAN parameters */
> +#define CPC_CMD_T_INQ_CAN_PARMS 11 /* inquire actual CAN parameters */
> +#define CPC_CMD_T_RTR 13 /* CAN remote frame */
> +#define CPC_CMD_T_CANSTATE 14 /* CAN state message */
> +#define CPC_CMD_T_XCAN 15 /* XCAN data frame */
See above.
> +#define CPC_CMD_T_XRTR 16 /* XCAN remote frame */
> +#define CPC_CMD_T_CAN_EXIT 200 /* exit the CAN */
> +
> +#define CPC_CMD_T_INQ_ERR_COUNTER 25 /* request the CAN error counters */
> +#define CPC_CMD_T_CLEAR_MSG_QUEUE 8 /* clear CPC_MSG queue */
> +#define CPC_CMD_T_CLEAR_CMD_QUEUE 28 /* clear CPC_CMD queue */
> +
> +#define SJA1000 2 /* Philips basic CAN controller */
Please be more specific, e.g. CPC_CC_SJA1000 or CPC_CC_TYPE_SJA1000.
> +
> +/* CAN-Message representation in a CPC_MSG
> + * Message object type is CPC_MSG_T_CAN or CPC_MSG_T_RTR
> + * or CPC_MSG_T_XCAN or CPC_MSG_T_XRTR
> + */
> +struct cpc_can_msg {
> + u32 id;
> + u8 length;
> + u8 msg[8];
> +};
> +
> +/* representation of the CAN parameters for the SJA1000 controller */
> +struct cpc_sja1000_params {
> + u8 mode;
> + u8 acc_code0;
> + u8 acc_code1;
> + u8 acc_code2;
> + u8 acc_code3;
> + u8 acc_mask0;
> + u8 acc_mask1;
> + u8 acc_mask2;
> + u8 acc_mask3;
> + u8 btr0;
> + u8 btr1;
> + u8 outp_contr;
> +};
> +
> +/* Representation of the CAN parameters for the M16C controller
> + * in basic CAN mode (means no full CAN)
> + */
> +struct cpc_m16c_basic_params {
> + u8 con0;
> + u8 con1;
> + u8 ctlr0;
> + u8 ctlr1;
> + u8 clk;
> + u8 acc_std_code0;
> + u8 acc_std_code1;
> + u8 acc_ext_code0;
> + u8 acc_ext_code1;
> + u8 acc_ext_code2;
> + u8 acc_ext_code3;
> + u8 acc_std_mask0;
> + u8 acc_std_mask1;
> + u8 acc_ext_mask0;
> + u8 acc_ext_mask1;
> + u8 acc_ext_mask2;
> + u8 acc_ext_mask3;
> +};
> +
> +/* CAN params message representation */
> +struct cpc_can_params {
> + u8 cc_type;
> + union {
> + struct cpc_m16c_basic_params m16c_basic;
> + struct cpc_sja1000_params sja1000;
Please avoid alignment of names also because you do not use it consequently.
> + } cc_params;
> +};
> +
> +/* Structure for confirmed message handling */
> +struct cpc_confirm {
> + u8 result; /* error code */
> +};
> +
> +/* Overrun types */
> +#define CPC_OVR_EVENT_CAN 0x01
> +#define CPC_OVR_EVENT_CANSTATE 0x02
> +#define CPC_OVR_EVENT_BUSERROR 0x04
/s/_EVENT// ?
> +/* If the CAN controller lost a message
> + * we indicate it with the highest bit
> + * set in the count field.
> + */
> +#define CPC_OVR_HW 0x80
> +
> +/* Structure for overrun conditions */
> +struct cpc_overrun {
> + u8 event;
> + u8 count;
> +};
> +
> +/* SJA1000 CAN errors (compatible to NXP LPC2119) */
> +struct cpc_sja1000_can_error {
> + u8 ecc;
> + u8 rxerr;
> + u8 txerr;
> +};
> +
> +/* structure for CAN error conditions */
> +#define CPC_CAN_ECODE_ERRFRAME 0x01
Please put the defines in the file header.,
> +struct cpc_can_error {
> + u8 ecode;
> + struct {
> + u8 cc_type;
> +
> + union {
> + struct cpc_sja1000_can_error sja1000;
> + } regs;
> + } cc;
> +};
> +
> +/* Structure containing RX/TX error counter.
> + * This structure is used to request the
> + * values of the CAN controllers TX and RX
> + * error counter.
> + */
> +struct cpc_can_err_counter {
> + u8 rx;
> + u8 tx;
> +};
> +
> +/* Main message type used between library and application */
> +struct __attribute__ ((packed)) ems_cpc_msg {
> + u8 type; /* type of message */
> + u8 length; /* length of data within union 'msg' */
> + u8 msgid; /* confirmation handle */
> + u32 ts_sec; /* timestamp in seconds */
> + u32 ts_nsec; /* timestamp in nano seconds */
Comment alignment?
> + union {
> + u8 generic[64];
> + struct cpc_can_msg canmsg;
> + struct cpc_can_params canparams;
> + struct cpc_confirm confirmation;
> + struct cpc_overrun overrun;
> + struct cpc_can_error error;
> + struct cpc_can_err_counter err_counter;
> + u8 canstate;
See above.
> + } msg;
> +};
> +
> +/* Define these values to match your devices */
> +#define USB_CPCUSB_VENDOR_ID 0x12D6
> +
> +#define USB_CPCUSB_M16C_PRODUCT_ID 0x0888
> +#define USB_CPCUSB_LPC2119_PRODUCT_ID 0x0444
Please put defines in the file header.
> +/* Table of devices that work with this driver
> + * NOTE: This driver supports only CPC-USB/ARM7 (LPC2119) yet.
> + */
> +static struct usb_device_id ems_usb_table[] = {
> + //{USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_M16C_PRODUCT_ID)},
> + {USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_LPC2119_PRODUCT_ID)},
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, ems_usb_table);
> +
> +#define CPC_MSG_HEADER_LEN 11
> +
> +#define RX_BUFFER_SIZE 64
> +#define HEADER_SIZE 4
> +
> +#define MAX_RX_URBS 10
> +#define MAX_TX_URBS 8
Please put defines in the file header. Also a prefix, e.g. CPC_ might be
logical.
> +struct ems_usb {
> + struct can_priv can; /* must be the first member */
> + int open_time;
> +
> + struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX];
> +
> + struct usb_device *udev;
> + struct net_device *netdev;
> +
> + atomic_t active_tx_urbs;
> + struct usb_anchor tx_submitted;
> +
> + struct usb_anchor rx_submitted;
> +
> + struct urb *intr_urb;
> +
> + u8 *tx_msg_buffer;
> +
> + u8 intr_in_buffer[4];
> + unsigned int free_slots; /* Remember number of freely available slots */
> +
> + struct ems_cpc_msg active_params; /* Active controller parameters */
> +};
> +
> +#define GET_PARAMS(d) (d->active_params.msg.canparams.cc_params.sja1000)
Rarely used. Consider removing it for better readability.
> +/* Mode register NXP LPC2119/SJA1000 CAN Controller */
> +#define MOD_NORMAL 0x00
> +#define MOD_RM 0x01
> +
> +/* ECC register NXP LPC2119/SJA1000 CAN Controller */
> +#define ECC_SEG 0x1F
> +#define ECC_DIR 0x20
> +#define ECC_ERR 6
> +#define ECC_BIT 0x00
> +#define ECC_FORM 0x40
> +#define ECC_STUFF 0x80
> +#define ECC_MASK 0xc0
> +
> +/* status register content */
> +#define SR_BS 0x80
> +#define SR_ES 0x40
Please put defines in the file header. Consider using a proper prefix,
e.g. SJA_ or CPC_.
> +static void init_params_sja1000(struct ems_cpc_msg *msg, u8 btr0, u8 btr1,
> + u8 outp, u8 mod)
> +{
> + msg->type = CPC_CMD_T_CAN_PRMS;
> + msg->length = sizeof(struct cpc_can_params);
> + msg->msgid = 0;
> +
> + msg->msg.canparams.cc_type = SJA1000;
> +
> + msg->msg.canparams.cc_params.sja1000.acc_code0 = 0x00;
> + msg->msg.canparams.cc_params.sja1000.acc_code1 = 0x00;
> + msg->msg.canparams.cc_params.sja1000.acc_code2 = 0x00;
> + msg->msg.canparams.cc_params.sja1000.acc_code3 = 0x00;
> +
> + msg->msg.canparams.cc_params.sja1000.acc_mask0 = 0xFF;
> + msg->msg.canparams.cc_params.sja1000.acc_mask1 = 0xFF;
> + msg->msg.canparams.cc_params.sja1000.acc_mask2 = 0xFF;
> + msg->msg.canparams.cc_params.sja1000.acc_mask3 = 0xFF;
> +
> + msg->msg.canparams.cc_params.sja1000.btr0 = btr0;
> + msg->msg.canparams.cc_params.sja1000.btr1 = btr1;
> +
> + msg->msg.canparams.cc_params.sja1000.outp_contr = outp;
> + msg->msg.canparams.cc_params.sja1000.mode = mod;
Ugly. Maybe:
struct cpc_sja1000_params *sja =
&msg->msg.canparams.cc_params.sja1000;
sja->acc_code0 = 0x00;
...
> +}
> +
> +static void ems_usb_read_interrupt_callback(struct urb *urb)
> +{
> + struct ems_usb *dev = urb->context;
> + struct net_device *netdev;
> +
> + int retval;
> +
> + netdev = dev->netdev;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + switch (urb->status) {
> + case 0:
> + dev->free_slots = dev->intr_in_buffer[1];
> + break;
> +
> + case -ECONNRESET:/* unlink */
> + case -ENOENT:
> + case -ESHUTDOWN:
> + return;
> +
> + default:
> + dev_info(ND2D(netdev), "nonzero urb status %d\n", urb->status);
> + break;
> + }
> +
> + retval = usb_submit_urb(urb, GFP_ATOMIC);
> +
> + if (retval == -ENODEV)
> + netif_device_detach(netdev);
> + else if (retval)
> + dev_err(ND2D(netdev), "failed resubmitting intr urb: %d\n",
> retval);
> +
> + return;
> +}
> +
> +static void ems_usb_rx_canmsg(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +{
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + int i;
> +
> + struct net_device_stats *stats =&dev->netdev->stats;
> +
> + skb = dev_alloc_skb(sizeof(struct can_frame));
> + if (skb == NULL)
> + return;
> +
> + skb->dev = dev->netdev;
> + skb->protocol = htons(ETH_P_CAN);
> +
> + cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> + memset(cf, 0, sizeof(struct can_frame));
> +
> + cf->can_id = msg->msg.canmsg.id;
> + cf->can_dlc = msg->msg.canmsg.length> 8 ? 8 : msg->msg.canmsg.length;
Coding style.
> +
> + if (msg->type == CPC_MSG_T_XCAN || msg->type == CPC_MSG_T_XRTR)
> + cf->can_id |= CAN_EFF_FLAG;
> +
> + if (msg->type == CPC_MSG_T_RTR || msg->type == CPC_MSG_T_XRTR) {
> + cf->can_id |= CAN_RTR_FLAG;
> + } else {
> + *(u64 *)(&cf->data) = 0; /* clear payload */
> +
> + for (i = 0; i< cf->can_dlc; i++)
> + cf->data[i] = msg->msg.canmsg.msg[i];
> + }
> +
> + netif_rx(skb);
> +
> + dev->netdev->last_rx = jiffies;
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> +}
> +
> +static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +{
> + struct can_frame *cf;
> + struct sk_buff *skb;
> +
> + struct net_device_stats *stats =&dev->netdev->stats;
> +
> + skb = dev_alloc_skb(sizeof(struct can_frame));
> + if (skb == NULL)
> + return;
> +
> + skb->dev = dev->netdev;
> + skb->protocol = htons(ETH_P_CAN);
> +
> + cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> + memset(cf, 0, sizeof(struct can_frame));
> +
> + cf->can_id = CAN_ERR_FLAG;
> + cf->can_dlc = CAN_ERR_DLC;
> +
> + if (msg->type == CPC_MSG_T_CANSTATE) {
> + u8 status = msg->msg.canstate;
> +
> + if (status& SR_BS) {
> + dev->can.state = CAN_STATE_BUS_OFF;
> + cf->can_id |= CAN_ERR_BUSOFF;
> +
> + can_bus_off(dev->netdev);
> + } else if (status& SR_ES) {
> + dev->can.state = CAN_STATE_ERROR_WARNING;
> + dev->can.can_stats.error_warning++;
> + } else {
> + dev->can.state = CAN_STATE_ERROR_ACTIVE;
> + dev->can.can_stats.error_passive++;
> + }
> + } else if (msg->type == CPC_MSG_T_CANERROR) {
> + u8 ecc = msg->msg.error.cc.regs.sja1000.ecc;
> + u8 txerr = msg->msg.error.cc.regs.sja1000.txerr;
> + u8 rxerr = msg->msg.error.cc.regs.sja1000.rxerr;
> +
> + /* bus error interrupt */
> + dev->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + switch (ecc& ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> + cf->data[3] = ecc& ECC_SEG;
> + break;
> + }
> +
> + /* Error occured during transmission? */
> + if ((ecc& ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> +
> + if (dev->can.state == CAN_STATE_ERROR_WARNING ||
> + dev->can.state == CAN_STATE_ERROR_PASSIVE) {
> + cf->data[1] = (txerr> rxerr) ?
cf->data[1] = (txerr> rxerr) ?
> + CAN_ERR_CRTL_TX_PASSIVE :
> CAN_ERR_CRTL_RX_PASSIVE;
> + }
> + } else if (msg->type == CPC_MSG_T_OVERRUN) {
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> + }
> +
> + netif_rx(skb);
> +
> + dev->netdev->last_rx = jiffies;
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> +}
> +
> +/*
> + * callback for bulk IN urb
> + */
> +static void ems_usb_read_bulk_callback(struct urb *urb)
> +{
> + struct ems_usb *dev = urb->context;
> + struct net_device *netdev;
> +
> + int retval;
> +
> + netdev = dev->netdev;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + switch (urb->status) {
> + case 0: /* success */
> + break;
> +
> + case -ENOENT:
> + return;
> +
> + default:
> + dev_warn(ND2D(netdev), "nonzero URB status %d\n", urb->status);
Error?
> + goto resubmit_urb;
> + }
> +
> + if (urb->actual_length> HEADER_SIZE) {
> + struct ems_cpc_msg *msg;
> +
> + u8 *ibuf = urb->transfer_buffer;
> +
> + u8 msgCnt, again, start;
Remove empty lines above.
> + msgCnt = ibuf[0]& ~0x80;
> + again = ibuf[0]& 0x80;
> +
> + start = HEADER_SIZE;
> +
> + while (msgCnt) {
msg_cnt or msg_count!?
> + msg = (struct ems_cpc_msg *)&ibuf[start];
> +
> + switch (msg->type) {
> + case CPC_MSG_T_CANSTATE:
> + /* Process CAN state changes */
> + ems_usb_rx_err(dev, msg);
> + break;
> +
> + case CPC_MSG_T_CAN:
> + case CPC_MSG_T_XCAN:
> + case CPC_MSG_T_RTR:
> + case CPC_MSG_T_XRTR:
> + ems_usb_rx_canmsg(dev, msg);
> + break;
> +
> + case CPC_MSG_T_CANERROR:
> + /* Process errorframe */
> + ems_usb_rx_err(dev, msg);
> + break;
> +
> + case CPC_MSG_T_OVERRUN:
> + /* Message lost while receiving */
> + ems_usb_rx_err(dev, msg);
> + break;
> + }
> +
> + start += CPC_MSG_HEADER_LEN + msg->length;
> + msgCnt--;
> +
> + if (start> urb->transfer_buffer_length) {
Coding style.
> + err("%d> %d\n", start,
> urb->transfer_buffer_length);
dev_err()?
> + break;
> + }
> + }
> + }
> +
> +resubmit_urb:
> + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
> + urb->transfer_buffer, RX_BUFFER_SIZE,
> ems_usb_read_bulk_callback, dev);
> +
> + retval = usb_submit_urb(urb, GFP_ATOMIC);
> +
> + if (retval == -ENODEV)
> + netif_device_detach(netdev);
> + else if (retval)
> + dev_err(ND2D(netdev), "failed resubmitting bulk urb: %d\n",
> retval);
> +
> + return;
> +}
> +
> +/*
> + * callback for bulk IN urb
> + */
> +static void ems_usb_write_bulk_callback(struct urb *urb)
> +{
> + struct ems_usb *dev = urb->context;
> + struct net_device *netdev;
> +
> + if (!dev)
> + return;
> +
> + netdev = dev->netdev;
> +
> + if (!netif_device_present(netdev))
> + return;
> +
> + if (urb->status)
> + dev_info(ND2D(netdev), "%s: Tx status %d\n", netdev->name,
> urb->status);
> +
> + /* free up our allocated buffer */
> + usb_buffer_free(urb->dev, urb->transfer_buffer_length,
> + urb->transfer_buffer, urb->transfer_dma);
> +
> + netdev->trans_start = jiffies;
> +
> + /* transmission complete interrupt */
> + netdev->stats.tx_packets++;
> + can_get_echo_skb(netdev, 0);
> +
> + atomic_dec(&dev->active_tx_urbs);
> +
> + if (netif_queue_stopped(netdev))
> + netif_wake_queue(netdev);
> +}
> +
> +/**
> + * Send the given CPC command synchronously
> + */
Docbook comment?
> +static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +{
> + int actual_length;
> +
> + /* Copy payload */
> + memcpy(&dev->tx_msg_buffer[HEADER_SIZE], msg,
> + msg->length + CPC_MSG_HEADER_LEN);
> +
> + /* Clear header */
> + memset(&dev->tx_msg_buffer[0], 0, HEADER_SIZE);
> +
> + return usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 2),
> + &dev->tx_msg_buffer[0],
> + msg->length +
> CPC_MSG_HEADER_LEN + HEADER_SIZE,
> + &actual_length, 1000);
> +}
Alignment? Line length?
> +
> +/*
> + * Change CAN controllers' mode register
> + */
> +static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
> +{
> + GET_PARAMS(dev).mode = mode;
> +
> + return ems_usb_command_msg(dev,&dev->active_params);
> +}
> +
> +/*
> + * Send a CPC_Control command to change behaviour when interface receives a
> CAN
> + * message, bus error or CAN state changed notifications.
> + */
> +static int ems_usb_control_cmd(struct ems_usb *dev, u8 val)
> +{
> + struct ems_cpc_msg cmd;
> +
> + cmd.type = CPC_CMD_T_CONTROL;
> + cmd.length = CPC_MSG_HEADER_LEN + 1;
> +
> + cmd.msgid = 0;
> +
> + cmd.msg.generic[0] = val;
> +
> + return ems_usb_command_msg(dev,&cmd);
> +}
> +
> +/*
> + * Start interface
> + */
> +static int ems_usb_start(struct ems_usb *dev)
> +{
> + struct net_device *netdev = dev->netdev;
> +
> + int result, i;
Please remove empty line above. Also s/result/err/ might be more
appropriate. Please check.
> +
> + dev->intr_in_buffer[0] = 0;
> + dev->free_slots = 15; /* initial size */
> +
> + for (i = 0; i< MAX_RX_URBS; i++) {
> + struct urb *urb = NULL;
> + u8 *buf = NULL;
> +
> + /* create a urb, and a buffer for it, and copy the data to the
> urb */
> + urb = usb_alloc_urb(0, GFP_ATOMIC);
> + if (!urb) {
> + dev_err(ND2D(netdev), "No memory left for URBs\n");
> + return -ENOMEM;
> + }
> +
> + buf = usb_buffer_alloc(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
> + &urb->transfer_dma);
> + if (!buf) {
> + dev_err(ND2D(netdev), "No memory left for USB
> buffer\n");
> + usb_free_urb(urb);
> + break;
> + }
> +
> + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev,
> 2), buf,
> + RX_BUFFER_SIZE, ems_usb_read_bulk_callback,
> dev);
> + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> + usb_anchor_urb(urb,&dev->rx_submitted);
> +
> + result = usb_submit_urb(urb, GFP_KERNEL);
> + if (result) {
> + if (result == -ENODEV)
> + netif_device_detach(dev->netdev);
> +
> + usb_unanchor_urb(urb);
> + usb_buffer_free(dev->udev, RX_BUFFER_SIZE, buf,
> urb->transfer_dma);
> + break;
> + }
> +
> + /* Drop reference, USB core will take care of freeing it */
> + usb_free_urb(urb);
> + }
> +
> + /* Do we submitted any URBs */
> + if (i == 0) {
> + dev_warn(ND2D(netdev), "couldn't setup read URBs\n");
> + return result ? result : -ENOMEM;
> + }
> +
> + /* Warn if we've couldn't transmit all the URBs */
> + if (i< MAX_RX_URBS)
Coding style.
> + dev_warn(ND2D(netdev), "rx performance may be slow\n");
> +
> + /* Setup and start interrupt URB */
> + usb_fill_int_urb(dev->intr_urb, dev->udev,
> + usb_rcvintpipe(dev->udev, 1),
> + dev->intr_in_buffer,
> sizeof(dev->intr_in_buffer),
> + ems_usb_read_interrupt_callback, dev,
> 1);
> +
> + result = usb_submit_urb(dev->intr_urb, GFP_KERNEL);
> + if (result) {
> + if (result == -ENODEV)
> + netif_device_detach(dev->netdev);
> +
> + dev_warn(ND2D(netdev), "intr urb submit failed: %d\n", result);
> +
> + return result;
> + }
> +
> + /* CPC-USB will transfer received message to host */
> + result = ems_usb_control_cmd(dev, CONTR_CAN_Message | CONTR_CONT_ON);
> + if (result)
> + goto failed;
> +
> + /* CPC-USB will transfer CAN state changes to host */
> + result = ems_usb_control_cmd(dev, CONTR_CAN_State | CONTR_CONT_ON);
> + if (result)
> + goto failed;
> +
> + /* CPC-USB will transfer bus errors to host */
> + result = ems_usb_control_cmd(dev, CONTR_BusError | CONTR_CONT_ON);
> + if (result)
> + goto failed;
> +
> + result = ems_usb_write_mode(dev, MOD_NORMAL);
> + if (result)
> + goto failed;
> +
> + return 0;
> +
> +failed:
> + if (result == -ENODEV)
> + netif_device_detach(dev->netdev);
> +
> + dev_warn(ND2D(netdev), "couldn't submit control: %d\n", result);
> +
> + return result;
> +}
> +
> +static int alloc_all_urbs(struct ems_usb *dev)
> +{
> + init_usb_anchor(&dev->rx_submitted);
> +
> + init_usb_anchor(&dev->tx_submitted);
> + atomic_set(&dev->active_tx_urbs, 0);
> +
> + dev->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!dev->intr_urb)
> + return 0;
> +
> + return 1;
> +}
Unusual return values for error and success.
> +static void free_all_urbs(struct ems_usb *dev)
> +{
> + usb_free_urb(dev->intr_urb);
> +}
> +
> +static void unlink_all_urbs(struct ems_usb *dev)
> +{
> + usb_unlink_urb(dev->intr_urb);
> +
> + usb_kill_anchored_urbs(&dev->rx_submitted);
> +
> + usb_kill_anchored_urbs(&dev->tx_submitted);
> + atomic_set(&dev->active_tx_urbs, 0);
> +}
> +
> +static int ems_usb_open(struct net_device *netdev)
> +{
> + struct ems_usb *dev = netdev_priv(netdev);
> + int result;
s/result/err/ ? See above.
> +
> + result = ems_usb_write_mode(dev, MOD_RM);
> + if (result)
> + return result;
> +
> + /* common open */
> + result = open_candev(netdev);
> + if (result)
> + return result;
> +
> + /* finally start device */
> + result = ems_usb_start(dev);
> + if (result) {
> + if (result == -ENODEV)
> + netif_device_detach(dev->netdev);
> +
> + dev_warn(ND2D(netdev), "couldn't start device: %d\n", result);
> +
> + return result;
> + }
> +
> + dev->open_time = jiffies;
> + dev->can.state = CAN_STATE_ERROR_ACTIVE;
Should go to usb_start.
> +
> + netif_start_queue(netdev);
> +
> + return 0;
> +}
> +
> +static int ems_usb_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct ems_usb *dev = netdev_priv(netdev);
> + struct net_device_stats *stats =&netdev->stats;
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + struct ems_cpc_msg *msg = NULL;
> + struct urb *urb = NULL;
> + u8 *buf = NULL;
> +
> + size_t size = HEADER_SIZE + CPC_MSG_HEADER_LEN + sizeof(struct
> cpc_can_msg);
> +
> + int i, result;
> +
> + /* create a urb, and a buffer for it, and copy the data to the urb */
> + urb = usb_alloc_urb(0, GFP_ATOMIC);
> + if (!urb) {
> + dev_err(ND2D(netdev), "No memory left for URBs\n");
> + return -ENOMEM;
> + }
> +
> + buf = usb_buffer_alloc(dev->udev, size, GFP_KERNEL,&urb->transfer_dma);
> + if (!buf) {
> + dev_err(ND2D(netdev), "No memory left for USB buffer\n");
> + usb_free_urb(urb);
> + return -ENOMEM;
> + }
> +
> + msg = (struct ems_cpc_msg *)&buf[HEADER_SIZE];
> +
> + msg->msg.canmsg.id = cf->can_id& 0x1FFFFFFFU;
> + msg->msg.canmsg.length = cf->can_dlc;
> +
> + if (cf->can_id& CAN_RTR_FLAG) {
> + msg->type = cf->can_id& CAN_EFF_FLAG ? CPC_CMD_T_XRTR :
> CPC_CMD_T_RTR;
> +
> + msg->length = 4 + 1;
> + } else {
> + msg->type = cf->can_id& CAN_EFF_FLAG ? CPC_CMD_T_XCAN :
> CPC_CMD_T_CAN;
> +
> + for (i = 0; i< cf->can_dlc; i++)
> + msg->msg.canmsg.msg[i] = cf->data[i];
> +
> + msg->length = 4 + 1 + cf->can_dlc;
> + }
> +
> + usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
> + size, ems_usb_write_bulk_callback, dev);
> + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> + usb_anchor_urb(urb,&dev->tx_submitted);
> +
> + can_put_echo_skb(skb, netdev, 0);
> +
> + result = usb_submit_urb(urb, GFP_ATOMIC);
> + if (result) {
> + can_free_echo_skb(netdev, 0);
> +
> + usb_unanchor_urb(urb);
> + usb_buffer_free(dev->udev, size, buf, urb->transfer_dma);
> +
> + if (result == -ENODEV) {
> + netif_device_detach(netdev);
> + } else {
> + dev_warn(ND2D(netdev), "failed tx_urb %d\n", result);
> +
> + stats->tx_errors++;
> + netif_start_queue(netdev);
> + }
> + } else {
> + atomic_inc(&dev->active_tx_urbs);
> +
> + stats->tx_bytes += cf->can_dlc;
> + netdev->trans_start = jiffies;
> +
> + stats->tx_packets++;
> + stats->tx_bytes += skb->len;
> +
> + netdev->trans_start = jiffies;
> +
> + /* Slow down tx path */
> + if (atomic_read(&dev->active_tx_urbs)> MAX_TX_URBS ||
> + dev->free_slots< 5) {
> + netif_stop_queue(netdev);
> + return 0;
> + }
> + }
> +
> + /* release our reference to this urb, the
> + * USB core will eventually free it entirely
> + */
> + usb_free_urb(urb);
> +
> + return 0;
> +}
> +
> +static int ems_usb_close(struct net_device *dev)
> +{
> + struct ems_usb *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> +
> + /* Stop polling */
> + unlink_all_urbs(priv);
> +
> + /* Set CAN controller to reset mode */
> + if (ems_usb_write_mode(priv, MOD_RM))
> + dev_warn(ND2D(dev), "couldn't stop device");
Hm, it's that an error?
> +
> + close_candev(dev);
> +
> + priv->open_time = 0;
> +
> + return 0;
> +}
> +
> +#if LINUX_VERSION_CODE> KERNEL_VERSION(2,6,28)
> +static const struct net_device_ops ems_usb_netdev_ops = {
> + .ndo_open = ems_usb_open,
> + .ndo_stop = ems_usb_close,
> + .ndo_start_xmit = ems_usb_start_xmit,
> +};
> +#endif
> +
> +static struct can_bittiming_const ems_usb_bittiming_const = {
> + .name = "ems_usb",
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 64,
> + .brp_inc = 1,
> +};
> +
> +static int ems_usb_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> + struct ems_usb *priv = netdev_priv(dev);
> +
> + if (!priv->open_time)
> + return -EINVAL;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + if (ems_usb_write_mode(priv, MOD_NORMAL))
> + dev_warn(ND2D(dev), "couldn't start device");
> +
> + if (netif_queue_stopped(dev))
> + netif_wake_queue(dev);
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int ems_usb_set_bittiming(struct net_device *dev)
> +{
> + struct ems_usb *priv = netdev_priv(dev);
> + struct can_bittiming *bt =&priv->can.bittiming;
> + u8 btr0, btr1;
> +
> + btr0 = ((bt->brp - 1)& 0x3f) | (((bt->sjw - 1)& 0x3)<< 6);
> + btr1 = ((bt->prop_seg + bt->phase_seg1 - 1)& 0xf) |
> + (((bt->phase_seg2 - 1)& 0x7)<< 4);
> + if (priv->can.ctrlmode& CAN_CTRLMODE_3_SAMPLES)
> + btr1 |= 0x80;
> +
> + dev_info(ND2D(dev), "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
> +
> + GET_PARAMS(priv).btr0 = btr0;
> + GET_PARAMS(priv).btr1 = btr1;
> +
> + return ems_usb_command_msg(priv,&priv->active_params);
> +}
> +
> +/*
> + * probe function for new CPC-USB devices
> + */
> +static int ems_usb_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct net_device *dev;
> + struct ems_usb *priv;
> + int result;
s/result/err/ ? See above.
> +
> + dev = alloc_candev(sizeof(struct ems_usb));
> + if (!dev) {
> + err("Out of memory");
dev_err()? There are more similar lines below.
> + return -ENOMEM;
> + }
> +
> + priv = netdev_priv(dev);
> +
> + priv->udev = interface_to_usbdev(intf);
> + priv->netdev = dev;
> +
> + priv->can.state = CAN_STATE_STOPPED;
> + priv->can.bittiming_const =&ems_usb_bittiming_const;
> + priv->can.do_set_bittiming = ems_usb_set_bittiming;
> + priv->can.do_set_mode = ems_usb_set_mode;
> +
> + dev->flags |= IFF_ECHO; /* we support local echo */
> +
> + /* The device actually uses a 16MHz clock to generate the CAN clock,
> but it
> + * expects SJA1000 bit settings based on 8MHz (is internally converted).
> + */
> + priv->can.clock.freq = 8000000;
> +
> +#if LINUX_VERSION_CODE> KERNEL_VERSION(2,6,28)
> + dev->netdev_ops =&ems_usb_netdev_ops;
> +#else
> + dev->open = ems_usb_open;
> + dev->stop = ems_usb_close;
> + dev->hard_start_xmit = ems_usb_start_xmit;
> +#endif
> +
> + dev->flags |= IFF_ECHO; /* we support local echo */
> +
> + if (!alloc_all_urbs(priv)) {
> + err("Out of memory");
> + free_candev(dev);
> + return -ENOMEM;
> + }
> +
> + priv->tx_msg_buffer = kzalloc(HEADER_SIZE + sizeof(struct ems_cpc_msg),
> + GFP_KERNEL);
Alignment of second line.
> + if (!priv->tx_msg_buffer) {
> + err("Out of memory");
> + free_candev(dev);
> + free_all_urbs(priv);
> + return -ENOMEM;
> + }
> +
> + usb_set_intfdata(intf, priv);
> +
> + SET_NETDEV_DEV(dev,&intf->dev);
> +
> + init_params_sja1000(&priv->active_params, 0, 0, 0xDA, MOD_RM);
> + result = ems_usb_command_msg(priv,&priv->active_params);
> +
> + if (result)
> + dev_warn(ND2D(dev), "couldn't initialize controller: %d\n",
> result);
That's an error condition, isn't it?
> + return register_candev(dev);
> +}
> +
> +/*
> + * called by the usb core when the device is removed from the system
> + */
> +static void ems_usb_disconnect(struct usb_interface *intf)
> +{
> + struct ems_usb *dev = usb_get_intfdata(intf);
> +
> + usb_set_intfdata(intf, NULL);
> +
> + if (dev) {
> + unregister_netdev(dev->netdev);
> + free_candev(dev->netdev);
> +
> + unlink_all_urbs(dev);
> + free_all_urbs(dev);
> + }
> +}
> +
> +/* usb specific object needed to register this driver with the usb subsystem
> */
> +static struct usb_driver ems_usb_driver = {
> + .name = "ems_usb",
> + .probe = ems_usb_probe,
> + .disconnect = ems_usb_disconnect,
> + .id_table = ems_usb_table,
> +};
> +
> +static int __init ems_usb_init(void)
> +{
> + int result;
> +
> + printk(KERN_INFO "CPC-USB kernel driver loaded\n");
> +
> + /* register this driver with the USB subsystem */
> + result = usb_register(&ems_usb_driver);
> +
> + if (result) {
> + err("usb_register failed. Error number %d\n", result);
> + return result;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit ems_usb_exit(void)
> +{
> + /* deregister this driver with the USB subsystem */
> + usb_deregister(&ems_usb_driver);
> +}
> +
> +module_init(ems_usb_init);
> +module_exit(ems_usb_exit);
I have not really reviewed the driver from the functional point of view.
Will do later. Apart from that, the patch looks already quite good.
Thanks for your contribution.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core
