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

["Ahmed S. Darwish" <darwish.07@gmail.com>]

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:
[...]
> > 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.

[...]
> > @@ -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.

[...]
> > +/* 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.

[...]
> > +
> > +		/* 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.

[...]
> > +
> > +	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.

> > +	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