-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Wolfgang,
thanks for your review. Wolfgang Grandegger schrieb: > Hi Sebastian, > > you just fix the lines I pointed to. There are more similar issues. > For consitancy reasons, could you please do a global: > > s/canstate/can_state/g > s/canmsg/canmsg/g > s/CANSTATE/CAN_STATE/g > s/CANERROR/CAN_ERROR/g > s/canparams/can_params/g > > Or the other way round if you prefer. I took the first one. > More issues inline: > > On 09/09/2009 12:37 PM, 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<[email protected]> >> >> Makefile | 1 >> drivers/net/can/Kconfig | 7 >> drivers/net/can/Makefile | 1 >> drivers/net/can/ems_usb.c | 1119 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 1128 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,13 @@ >> 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.com). >> + >> 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,1119 @@ >> +/* >> + * 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> >> + >> +MODULE_AUTHOR("Sebastian Haas<[email protected]>"); >> +MODULE_DESCRIPTION("CAN driver for EMS Dr. Thomas Wuensche CAN/USB >> interfaces"); >> +MODULE_LICENSE("GPL v2"); >> + >> +/* Control-Values for CPC_Control() Command Subject Selection */ >> +#define CONTR_CAN_MESSAGE 0x04 >> +#define CONTR_CAN_STATE 0x0C >> +#define CONTR_BUS_ERROR 0x1C >> + >> +/* 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_TYPE_CAN 1 /* CAN data frame */ > > This name is not significant enough. > >> +#define CPC_MSG_TYPE_RTR 8 /* CAN remote frame */ > > What about: > > #define CPC_MSG_TYPE_DATA_FRAME 1 /* CAN data frame */ > #define CPC_MSG_TYPE_RTR_FRAME 8 /* CAN remote frame */ Okay. >> +#define CPC_MSG_TYPE_CAN_PRMS 12 /* Actual CAN parameters */ > > #define CPC_MSG_TYPE_CAN_PARAMS 12 /* Actual CAN parameters */ > >> +#define CPC_MSG_TYPE_CANSTATE 14 /* CAN state message */ > > #define CPC_MSG_TYPE_CAN_STATE 14 /* CAN state message */ > >> +#define CPC_MSG_TYPE_XCAN 16 /* XCAN data frame */ >> +#define CPC_MSG_TYPE_XRTR 17 /* XCAN remote frame */ > > #define CPC_MSG_TYPE_DATA_FRAME 1 /* CAN data frame */ > #define CPC_MSG_TYPE_RTR_FRAME 8 /* CAN remote frame */ > >> +#define CPC_MSG_TYPE_CONTROL 19 /* used for control of interface >> behavior*/ >> +#define CPC_MSG_TYPE_CONFIRM 20 /* response type for confirmed >> requests */ >> +#define CPC_MSG_TYPE_OVERRUN 21 /* response type for overrun >> conditions */ >> +#define CPC_MSG_TYPE_CANERROR 23 /* response type for bus error >> conditions*/ > > CAN_ERROR? Okay. >> +#define CPC_MSG_TYPE_ERR_COUNTER 25 /* RX/TX error counter of CAN >> controller */ >> + >> +/* Messages from the PC to the CPC interface contain a command field > > Fix comment style. Hopefully fixed. >> + * 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_TYPE_CAN 1 /* CAN data frame */ >> +#define CPC_CMD_TYPE_CONTROL 3 /* control of interface >> behavior */ >> +#define CPC_CMD_TYPE_CAN_PRMS 6 /* set CAN parameters */ >> +#define CPC_CMD_TYPE_INQ_CAN_PARMS 11 /* inquire actual CAN >> parameters */ > > You use three different abbreviation for "parameters" in this file: > prms, parms and params :-( I know, that is legacy code from our old cpc.h. Everything fixed. > > #define CPC_CMD_TYPE_CAN_PARAMS 6 /* set CAN parameters */ > #define CPC_CMD_TYPE_INQ_CAN_PARAMS 11 /* inquire actual CAN parameters */ > >> +#define CPC_CMD_TYPE_RTR 13 /* CAN remote frame */ >> +#define CPC_CMD_TYPE_CANSTATE 14 /* CAN state message */ >> +#define CPC_CMD_TYPE_XCAN 15 /* XCAN data frame */ >> +#define CPC_CMD_TYPE_XRTR 16 /* XCAN remote frame */ > > See above. > >> +#define CPC_CMD_TYPE_CAN_EXIT 200 /* exit the CAN */ >> + >> +#define CPC_CMD_TYPE_INQ_ERR_COUNTER 25 /* request the CAN error >> counters */ >> +#define CPC_CMD_TYPE_CLEAR_MSG_QUEUE 8 /* clear CPC_MSG queue */ >> +#define CPC_CMD_TYPE_CLEAR_CMD_QUEUE 28 /* clear CPC_CMD queue */ >> + >> +#define CPC_CC_TYPE_SJA1000 2 /* Philips basic CAN controller */ >> + >> +#define CPC_CAN_ECODE_ERRFRAME 0x01 /* Ecode type */ >> + >> +/* Overrun types */ >> +#define CPC_OVR_EVENT_CAN 0x01 >> +#define CPC_OVR_EVENT_CANSTATE 0x02 >> +#define CPC_OVR_EVENT_BUSERROR 0x04 >> + >> +/* If the CAN controller lost a message > > Fix comment style. Fixed. >> + * we indicate it with the highest bit >> + * set in the count field. >> + */ >> +#define CPC_OVR_HW 0x80 >> + >> +/* size of the "struct ems_cpc_msg" without the union */ >> +#define CPC_MSG_HEADER_LEN 11 >> + >> +/* Define these values to match your devices */ >> +#define USB_CPCUSB_VENDOR_ID 0x12D6 >> + >> +#define USB_CPCUSB_M16C_PRODUCT_ID 0x0888 >> +#define USB_CPCUSB_ARM7_PRODUCT_ID 0x0444 >> + >> +/* Mode register NXP LPC2119/SJA1000 CAN Controller */ >> +#define SJA1000_MOD_NORMAL 0x00 >> +#define SJA1000_MOD_RM 0x01 >> + >> +/* ECC register NXP LPC2119/SJA1000 CAN Controller */ >> +#define SJA1000_ECC_SEG 0x1F >> +#define SJA1000_ECC_DIR 0x20 >> +#define SJA1000_ECC_ERR 0x06 >> +#define SJA1000_ECC_BIT 0x00 >> +#define SJA1000_ECC_FORM 0x40 >> +#define SJA1000_ECC_STUFF 0x80 >> +#define SJA1000_ECC_MASK 0xc0 >> + >> +/* status register content */ >> +#define SJA1000_SR_BS 0x80 >> +#define SJA1000_SR_ES 0x40 >> + >> +/* CAN-Message representation in a CPC_MSG > > Fix comment style. Fixed. >> + * Message object type is CPC_MSG_TYPE_CAN or CPC_MSG_TYPE_RTR >> + * or CPC_MSG_TYPE_XCAN or CPC_MSG_TYPE_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; >> + } cc_params; >> +}; >> + >> +/* Structure for confirmed message handling */ >> +struct cpc_confirm { >> + u8 result; /* error code */ >> +}; > > s/result/error ? No. it isn't an >> +/* 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 */ >> +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. > > Comment style! Fixed. >> + * 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 */ > > Remove spaces, please. Fixed. >> + >> + 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; >> + } msg; >> +}; >> + >> +/* 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)},*/ > > Please remove. Fixed for now. Will reintroduce if I have time for M16C. >> + {USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_ARM7_PRODUCT_ID)}, >> + {} /* Terminating entry */ >> +}; >> + >> +MODULE_DEVICE_TABLE(usb, ems_usb_table); >> + >> +#define RX_BUFFER_SIZE 64 >> +#define HEADER_SIZE 4 > > HEADER_SIZE is very general and the risk of a name clash sooner than > later is rather high. Please us a resonable prefix for them. CPC_ added. >> + >> +#define MAX_RX_URBS 10 >> +#define MAX_TX_URBS 8 >> + >> +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; > > Remove spaces, please. Fixed. >> + 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 */ >> +}; >> + >> +static void init_params_sja1000(struct ems_cpc_msg *msg, u8 btr0, u8 >> btr1, >> + u8 outp, u8 mod) >> +{ >> + struct cpc_sja1000_params *sja1000 = >> + &msg->msg.canparams.cc_params.sja1000; >> + >> + msg->type = CPC_CMD_TYPE_CAN_PRMS; >> + msg->length = sizeof(struct cpc_can_params); >> + msg->msgid = 0; >> + >> + msg->msg.canparams.cc_type = CPC_CC_TYPE_SJA1000; >> + >> + /* Acceptance filter open */ >> + sja1000->acc_code0 = 0x00; >> + sja1000->acc_code1 = 0x00; >> + sja1000->acc_code2 = 0x00; >> + sja1000->acc_code3 = 0x00; >> + >> + /* Acceptance filter open */ >> + sja1000->acc_mask0 = 0xFF; >> + sja1000->acc_mask1 = 0xFF; >> + sja1000->acc_mask2 = 0xFF; >> + sja1000->acc_mask3 = 0xFF; >> + >> + sja1000->btr0 = btr0; >> + sja1000->btr1 = btr1; >> + >> + sja1000->outp_contr = outp; >> + sja1000->mode = mod; >> +} >> + >> +static void ems_usb_read_interrupt_callback(struct urb *urb) >> +{ >> + struct ems_usb *dev = urb->context; >> + struct net_device *netdev; >> + >> + int retval; > > Remove empty line above. Fixed. >> + >> + 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)); >> + >> + cf->can_id = msg->msg.canmsg.id; > > Remove spaces, please Fixed. >> + cf->can_dlc = clamp_t(u8, msg->msg.canmsg.length, 0, 8); >> + >> + if (msg->type == CPC_MSG_TYPE_XCAN || msg->type == >> CPC_MSG_TYPE_XRTR) >> + cf->can_id |= CAN_EFF_FLAG; >> + >> + if (msg->type == CPC_MSG_TYPE_RTR || msg->type == >> CPC_MSG_TYPE_XRTR) { >> + cf->can_id |= CAN_RTR_FLAG; >> + } else { >> + 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; > > Remove empty line above. Fixed. >> + 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; > > Remove spaces, please > >> + cf->can_dlc = CAN_ERR_DLC; >> + >> + if (msg->type == CPC_MSG_TYPE_CANSTATE) { >> + u8 status = msg->msg.canstate; > > Hm, status != state !? Fixed. >> + >> + if (status& SJA1000_SR_BS) { >> + dev->can.state = CAN_STATE_BUS_OFF; >> + cf->can_id |= CAN_ERR_BUSOFF; >> + >> + can_bus_off(dev->netdev); >> + } else if (status& SJA1000_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_TYPE_CANERROR) { >> + u8 ecc = msg->msg.error.cc.regs.sja1000.ecc; > > Remove spaces, please Fixed. >> + 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& SJA1000_ECC_MASK) { >> + case SJA1000_ECC_BIT: >> + cf->data[2] |= CAN_ERR_PROT_BIT; >> + break; >> + case SJA1000_ECC_FORM: >> + cf->data[2] |= CAN_ERR_PROT_FORM; >> + break; >> + case SJA1000_ECC_STUFF: >> + cf->data[2] |= CAN_ERR_PROT_STUFF; >> + break; >> + default: >> + cf->data[2] |= CAN_ERR_PROT_UNSPEC; >> + cf->data[3] = ecc& SJA1000_ECC_SEG; > > Space after ecc ! Mh, looks fine in my editor. >> + break; >> + } >> + >> + /* Error occured during transmission? */ >> + if ((ecc& SJA1000_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) ? > > Space after txerr ! Mh, looks fine in my editor. Is their any option in Thunderbird which can cause this? >> + CAN_ERR_CRTL_TX_PASSIVE : >> + CAN_ERR_CRTL_RX_PASSIVE; >> + } >> + } else if (msg->type == CPC_MSG_TYPE_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; > > Remove empty line above, please. > s/retval/err ? Fixed. Fixed. > >> + >> + 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); >> + goto resubmit_urb; >> + } >> + >> + if (urb->actual_length> HEADER_SIZE) { >> + struct ems_cpc_msg *msg; >> + u8 *ibuf = urb->transfer_buffer; >> + u8 msg_count, again, start; >> + >> + msg_count = ibuf[0]& ~0x80; > > *One* space before and after & ! > >> + again = ibuf[0]& 0x80; > > Ditto. Same problem, looks fine in VIM and Eclipse. >> + >> + start = HEADER_SIZE; >> + >> + while (msg_count) { >> + msg = (struct ems_cpc_msg *)&ibuf[start]; >> + >> + switch (msg->type) { >> + case CPC_MSG_TYPE_CANSTATE: >> + /* Process CAN state changes */ >> + ems_usb_rx_err(dev, msg); >> + break; >> + >> + case CPC_MSG_TYPE_CAN: >> + case CPC_MSG_TYPE_XCAN: >> + case CPC_MSG_TYPE_RTR: >> + case CPC_MSG_TYPE_XRTR: >> + ems_usb_rx_canmsg(dev, msg); >> + break; >> + >> + case CPC_MSG_TYPE_CANERROR: >> + /* Process errorframe */ >> + ems_usb_rx_err(dev, msg); >> + break; >> + >> + case CPC_MSG_TYPE_OVERRUN: >> + /* Message lost while receiving */ >> + ems_usb_rx_err(dev, msg); >> + break; >> + } >> + >> + start += CPC_MSG_HEADER_LEN + msg->length; >> + msg_count--; >> + >> + if (start> urb->transfer_buffer_length) { > > *One* space before and after > ! > >> + dev_err(ND2D(netdev), "%d> %d\n", start, >> + urb->transfer_buffer_length); > > *One* space before and after > ! The error message is missing? > >> + 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 >> + */ >> +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? Fixed. >> +} >> + >> +/* >> + * Change CAN controllers' mode register >> + */ >> +static int ems_usb_write_mode(struct ems_usb *dev, u8 mode) >> +{ >> + dev->active_params.msg.canparams.cc_params.sja1000.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_TYPE_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; > > Check spaces? Removed. >> + int err, i; >> + >> + 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 */ >> + 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); > > Indention? Fixed. >> + if (!buf) { > > err = -ENOMEM; ???? Okay. >> + 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); >> + >> + err = usb_submit_urb(urb, GFP_KERNEL); >> + if (err) { >> + if (err == -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 err ? err : -ENOMEM; >> + } >> + >> + /* Warn if we've couldn't transmit all the URBs */ >> + if (i< MAX_RX_URBS) > > Check spaces? Same as before. >> + 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); > > Indention? Whats wrong? >> + >> + err = usb_submit_urb(dev->intr_urb, GFP_KERNEL); >> + if (err) { >> + if (err == -ENODEV) >> + netif_device_detach(dev->netdev); >> + >> + dev_warn(ND2D(netdev), "intr urb submit failed: %d\n", err); >> + >> + return err; >> + } >> + >> + /* CPC-USB will transfer received message to host */ >> + err = ems_usb_control_cmd(dev, CONTR_CAN_MESSAGE | CONTR_CONT_ON); >> + if (err) >> + goto failed; >> + >> + /* CPC-USB will transfer CAN state changes to host */ >> + err = ems_usb_control_cmd(dev, CONTR_CAN_STATE | CONTR_CONT_ON); >> + if (err) >> + goto failed; >> + >> + /* CPC-USB will transfer bus errors to host */ >> + err = ems_usb_control_cmd(dev, CONTR_BUS_ERROR | CONTR_CONT_ON); >> + if (err) >> + goto failed; >> + >> + err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL); >> + if (err) >> + goto failed; >> + >> + dev->can.state = CAN_STATE_ERROR_ACTIVE; >> + >> + return 0; >> + >> +failed: >> + if (err == -ENODEV) >> + netif_device_detach(dev->netdev); >> + >> + dev_warn(ND2D(netdev), "couldn't submit control: %d\n", err); >> + >> + return err; >> +} >> + >> +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 err; >> + >> + err = ems_usb_write_mode(dev, SJA1000_MOD_RM); >> + if (err) >> + return err; >> + >> + /* common open */ >> + err = open_candev(netdev); >> + if (err) >> + return err; >> + >> + /* finally start device */ >> + err = ems_usb_start(dev); >> + if (err) { >> + if (err == -ENODEV) >> + netif_device_detach(dev->netdev); >> + >> + dev_warn(ND2D(netdev), "couldn't start device: %d\n", err); > > close_candev(); ? Okay. >> + return err; >> + } >> + >> + dev->open_time = jiffies; >> + >> + 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; > > Remove empty lines above. Fixed. >> + /* 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); > > Indention? Fixed. >> + 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; > > Check spaces! Mh. >> + msg->msg.canmsg.length = cf->can_dlc; >> + >> + if (cf->can_id& CAN_RTR_FLAG) { >> + msg->type = cf->can_id& CAN_EFF_FLAG ? >> + CPC_CMD_TYPE_XRTR : CPC_CMD_TYPE_RTR; >> + >> + msg->length = 4 + 1; > > Please use defines here. Okay. >> + } else { >> + msg->type = cf->can_id& CAN_EFF_FLAG ? >> + CPC_CMD_TYPE_XCAN : CPC_CMD_TYPE_CAN; >> + >> + for (i = 0; i< cf->can_dlc; i++) >> + msg->msg.canmsg.msg[i] = cf->data[i]; >> + >> + msg->length = 4 + 1 + cf->can_dlc; > > Ditto. > >> + } >> + >> + 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) { > > Check spaces ... puh. Fixed. >> + netif_stop_queue(netdev); >> + return 0; >> + } >> + } >> + >> + /* release our reference to this urb, the > > Comment style. Fixed. >> + * 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, SJA1000_MOD_RM)) >> + dev_warn(ND2D(dev), "couldn't stop device"); >> + >> + 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, > > Remove spaces, please. Fixed. >> +}; >> +#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, SJA1000_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 *netdev) >> +{ >> + struct ems_usb *dev = netdev_priv(netdev); >> + struct can_bittiming *bt =&dev->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); > > Check spaces. Fixed. >> + if (dev->can.ctrlmode& CAN_CTRLMODE_3_SAMPLES) >> + btr1 |= 0x80; >> + >> + dev_info(ND2D(netdev), "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, >> + btr1); >> + >> + dev->active_params.msg.canparams.cc_params.sja1000.btr0 = btr0; >> + dev->active_params.msg.canparams.cc_params.sja1000.btr1 = btr1; >> + >> + return ems_usb_command_msg(dev,&dev->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 *netdev; >> + struct ems_usb *dev; >> + int err; >> + >> + netdev = alloc_candev(sizeof(struct ems_usb)); >> + if (!netdev) { >> + dev_err(ND2D(netdev), "Out of memory"); >> + return -ENOMEM; >> + } >> + >> + dev = netdev_priv(netdev); >> + >> + dev->udev = interface_to_usbdev(intf); >> + dev->netdev = netdev; >> + >> + dev->can.state = CAN_STATE_STOPPED; >> + dev->can.bittiming_const =&ems_usb_bittiming_const; >> + dev->can.do_set_bittiming = ems_usb_set_bittiming; >> + dev->can.do_set_mode = ems_usb_set_mode; >> + >> + netdev->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). >> + */ >> + dev->can.clock.freq = 8000000; >> + >> +#if LINUX_VERSION_CODE> KERNEL_VERSION(2,6,28) >> + netdev->netdev_ops =&ems_usb_netdev_ops; >> +#else >> + netdev->open = ems_usb_open; >> + netdev->stop = ems_usb_close; >> + netdev->hard_start_xmit = ems_usb_start_xmit; >> +#endif >> + >> + 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); >> + >> + dev->intr_urb = usb_alloc_urb(0, GFP_KERNEL); >> + if (!dev->intr_urb) { >> + dev_err(ND2D(netdev), "Out of memory"); >> + free_candev(netdev); >> + return -ENOMEM; >> + } >> + >> + dev->tx_msg_buffer = kzalloc(HEADER_SIZE + sizeof(struct >> ems_cpc_msg), >> + GFP_KERNEL); >> + if (!dev->tx_msg_buffer) { >> + dev_err(ND2D(netdev), "Out of memory"); >> + free_candev(netdev); >> + usb_free_urb(dev->intr_urb); >> + return -ENOMEM; >> + } >> + >> + usb_set_intfdata(intf, dev); >> + >> + SET_NETDEV_DEV(netdev,&intf->dev); >> + >> + init_params_sja1000(&dev->active_params, 0, 0, 0xDA, >> SJA1000_MOD_RM); >> + err = ems_usb_command_msg(dev,&dev->active_params); >> + >> + if (err) { >> + dev_err(ND2D(netdev), "couldn't initialize controller: %d\n", >> + err); >> + return err; >> + } >> + >> + return register_candev(netdev); >> +} >> + >> +/* >> + * 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); >> + >> + usb_free_urb(dev->intr_urb); >> + } >> +} >> + >> +/* 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, > > Check spaces. Fixed. >> + >> +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); > > Puh, likely I have missed various coding style issues. Please use > *Lindent* to fix them and resubmit. I've also search the whole code according the coding style. I can't understand why there are so many whitespace errors, even in the mail sent folder the patch looks fine (no whitespace errors) I've also found a bug at the echoing code, will send patch V3 with coding style issues fixed and echoing bugfix. Cheers, Sebastian -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkqn1AEACgkQpqRB8PJG7XzsZgCdHk4a1ipH80JzumF9K4+3rtWg LBAAn3AY2QO9rm3Z9BkDb0InJljSRNR4 =G/Lr -----END PGP SIGNATURE----- -- EMS Dr. Thomas Wuensche e.K. Sonnenhang 3 85304 Ilmmuenster HRA Neuburg a.d. Donau, HR-Nr. 70.106 Phone: +49-8441-490260 Fax : +49-8441-81860 http://www.ems-wuensche.com _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
