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 <[email protected]>, esd gmbh
> > * Copyright (C) 2012 Olivier Sobrie <[email protected]>
> > + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
