Re: [PATCH v4 3/4] can: kvaser_usb: Add support for the Usbcan-II family

Hi Marc,

On Mon, Jan 12, 2015 at 12:43:56PM +0100, Marc Kleine-Budde wrote:

On 01/11/2015 09:36 PM, Ahmed S. Darwish wrote:

[...]

quoted

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 0eb870b..da47d17 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c

@@ -6,10 +6,12 @@
  * Parts of this driver are based on the following:
  *  - Kvaser linux leaf driver (version 4.78)
  *  - CAN driver for esd CAN-USB/2
+ *  - Kvaser linux usbcanII driver (version 5.3)
  *
  * Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
  * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
  * Copyright (C) 2012 Olivier Sobrie <olivier@sobrie.be>
+ * Copyright (C) 2015 Valeo Corporation
  */
 
 #include <linux/completion.h>

@@ -21,6 +23,15 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
+/* Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+	KVASER_LEAF,
+	KVASER_USBCAN,
+};

Nitpick: please move after the #defines

Will do.

[...]

quoted

@@ -616,53 +810,24 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
 }
 
 static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
-				const struct kvaser_msg *msg)
+				struct kvaser_error_summary *es)

Can you make "struct kvaser_error_summary *es" const?

Sure.

[...]

quoted

+/* For USBCAN, report error to userspace iff the channels's errors counter
+ * has increased, or we're the only channel seeing a bus error state.
+ */
+static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,
+						 struct kvaser_error_summary *es)

const struct kvaser_error_summary *es?

Ditto.

[...]

quoted

+
+		/* The USBCAN firmware does not support more than 2 channels.

Does USBCAN support always 2 channels or are there models with 1
channels, too. I'd rephrase ..."does support up to 2 channels."

Yup, that will be more accurate.

[...]

quoted

+
+	switch (dev->family) {
+	case KVASER_LEAF:
+		rx_msg = msg->u.leaf.rx_can.msg;
+		break;
+	case KVASER_USBCAN:
+		rx_msg = msg->u.usbcan.rx_can.msg;
+		break;
+	default:
+		dev_err(dev->udev->dev.parent,
+			"Invalid device family (%d)\n", dev->family);
 		return;

should not happen.

Yes, but otherwise I get GCC warnings of 'rx_msg' possibly
being unused. I can add __maybe_unused to rx_msg of course,
but such annotation may hide possible errors in the future.

quoted

+	switch (dev->family) {
+	case KVASER_LEAF:
+		msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
+		break;
+	case KVASER_USBCAN:
+		msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
+		break;
+	default:
+		dev_err(dev->udev->dev.parent,
+			"Invalid device family (%d)\n", dev->family);
+		goto releasebuf;

should not happen, please remove

Like the 'rx_msg' case above.

Thanks,
Darwish