Sebastian Haas wrote:
> This patch adds support for the CAN/USB interface CPC-USB/ARM7 from EMS
> Dr. Thomas Wuensche.
Hi Sebastian,
i'm really impressed about this two-days work!
>
> 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(+)
As Wolfgang stated creating a separate drivers/net/can/usb directory would
make sense. But this is something we can do on the SVN later.
> +/* 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
Can you fix these whitespace issues? There are many of them.
> +/* 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
e.g. this one ...
> +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));
Please remove the memset.
> +
> + cf->can_id = msg->msg.canmsg.id;
> + cf->can_dlc = msg->msg.canmsg.length > 8 ? 8 : msg->msg.canmsg.length;
> +
> + 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 */
And this also (which is also doubled regarding the memset() above).
can_dlc gives the information about valid data[] and the rest may be
uninitialized.
@Wolfgang: Is the leakage of kernel memory a problem in this case?
Like here:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e84b90ae5eb3c112d1f208964df1d8156a538289
> +
> +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);
> +}
I always wondered how ems_usb_set_bittiming() would look like in the end.
And it is slim and easy. Awesome ;-)
Finally besides some nitpicking the driver looks really good.
Best regards,
Oliver
_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core
