Re: [PATCH] Add support for PEAK System PCAN-USB adapter

[Grosjean Stephane <s.grosjean@peak-system.com>]

Hi Sebastian,

Le 23/12/2011 20:38, Sebastian Haas a écrit :
> Hi Stéphane,
>
> find a few comments from me below.
>
> Cheers,
>  Sebastian
>
> Am 22.12.2011 14:13, schrieb Stephane Grosjean:
>> +/*
>> + * read serial number from device
>> + */
>> +static int pcan_usb_get_serial(struct peak_usb_device *dev, u32 
>> *serial_number)
>> +{
>> +    u8 args[PCAN_USB_PARAMETER_LEN];
>> +    int err;
>> +
>> +    err = pcan_usb_wait_rsp(dev, 6, 1, args);
>> +    if (err)
>> +        netdev_err(dev->netdev, "getting serial failure: %d\n", err);
>> +
> Remove empty line an but the initial if() also in brackets.
Ok.
>> +    else if (serial_number) {
>> +        u32 tmp32;
>> +        memcpy(&tmp32, args, 4);
>> +        *serial_number = le32_to_cpu(tmp32);
>> +    }
>> +
>> +    return err;
>> +}
>> +
>> +/*
>> + * read device id from device
>> + */
>> +static int pcan_usb_get_device_id(struct peak_usb_device *dev, u32 
>> *device_id)
>> +{
>> +    u8 args[PCAN_USB_PARAMETER_LEN];
>> +    int err;
>> +
>> +    err = pcan_usb_wait_rsp(dev, 4, 1, args);
>> +    if (err)
>> +        netdev_err(dev->netdev, "getting device id failure: %d\n", 
>> err);
>> +
> Empty line.
Ok.
>> +    else if (device_id)
>> +        *device_id = args[0];
>> +
>> +    return err;
>> +}
>> +
>> +/*
>> + * decode non-data usb message
>> + */
>> +static int pcan_usb_decode_status(struct pcan_usb_msg_context *mc,
>> +    u8 status_len)
>> +{
>> +    u8 rec_len = (status_len&  PCAN_USB_STATUSLEN_DLC);
> Brackets are useless.
Ok.
>> +    struct sk_buff *skb;
>> +    struct can_frame *cf;
>> +    struct timeval tv;
>> +    u8 f, n;
>> +
>> +    /* check whether function and number can be read */
>> +    if ((mc->ptr + 2)>  mc->end)
>> +        return -EINVAL;
>> +
>> +    f = mc->ptr[0];
>> +    n = mc->ptr[1];
>> +    mc->ptr += 2;
>> +
>> +    if (status_len&  PCAN_USB_STATUSLEN_TIMESTAMP) {
>> +        int err = pcan_usb_decode_ts(mc, !mc->rec_idx);
>> +        if (err)
>> +            return err;
>> +    }
>> +
>> +    switch (f) {
>> +    case PCAN_USB_REC_ERROR:
>> +        /* no status flag =>  ignore record */
>> +        if (!n)
>> +            break;
>> +
>> +        /* ignore this error until 1st ts received */
>> +        if (n == PCAN_USB_ERROR_QOVR)
>> +            if (!mc->pdev->time_ref.tick_count)
>> +                break;
>> +
>> +        /* allocate an skb to store the error frame */
>> +        skb = alloc_can_err_skb(mc->netdev,&cf);
>> +        if (!skb)
>> +            return -ENOMEM;
>> +
>> +        if (n&  (PCAN_USB_ERROR_RXQOVR | PCAN_USB_ERROR_QOVR)) {
>> +            cf->can_id |= CAN_ERR_CRTL;
>> +            cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
>> +            mc->netdev->stats.rx_over_errors++;
>> +            mc->netdev->stats.rx_errors++;
>> +        }
>> +        if (n&  PCAN_USB_ERROR_BUS_OFF) {
>> +            cf->can_id |= CAN_ERR_BUSOFF;
>> +            can_bus_off(mc->netdev);
>> +        }
>> +        if (n&  PCAN_USB_ERROR_BUS_HEAVY) {
>> +            cf->can_id |= CAN_ERR_CRTL;
>> +            cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
>> +            mc->pdev->dev.can.can_stats.error_passive++;
>> +        }
>> +        if (n&  PCAN_USB_ERROR_BUS_LIGHT) {
>> +            cf->can_id |= CAN_ERR_CRTL;
>> +            cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
>> +            mc->pdev->dev.can.can_stats.error_warning++;
>> +        }
>> +
>> +        if (cf->can_id != CAN_ERR_FLAG) {
>> +            if (status_len&  PCAN_USB_STATUSLEN_TIMESTAMP) {
>> +                peak_usb_get_ts_tv(&mc->pdev->time_ref,
>> +                    mc->ts16,&tv);
>> +                skb->tstamp = timeval_to_ktime(tv);
>> +            }
>> +            netif_rx(skb);
>> +            mc->netdev->stats.rx_packets++;
>> +        } else {
>> +            kfree_skb(skb);
>> +        }
>> +
>> +        break;
>> +
>> +    case PCAN_USB_REC_ANALOG:
>> +        /* analog values (ignored) */
>> +        rec_len = 2;
>> +        break;
>> +
>> +    case PCAN_USB_REC_BUSLOAD:
>> +        /* bus load (ignored) */
>> +        rec_len = 1;
>> +        break;
>> +
>> +    case PCAN_USB_REC_TS:
>> +        /* only timestamp */
>> +        if (pcan_usb_update_ts(mc))
>> +            return -EINVAL;
>> +        break;
>> +
>> +    case PCAN_USB_REC_BUSEVT:
>> +        /* error frame/bus event */
>> +        if (n&  PCAN_USB_ERROR_TXQFULL)
>> +            netdev_info(mc->netdev, "device Tx queue full)\n");
>> +        break;
>> +
>> +    case PCAN_USB_REC_EXT:
>> +        /* future... */
>> +        break;
>> +
>> +    default:
>> +        netdev_err(mc->netdev, "unexpected function %u\n", f);
>> +        break;
>> +    }
>> +
>> +    if ((mc->ptr + rec_len)>  mc->end)
>> +        return -EINVAL;
>> +
>> +    mc->ptr += rec_len;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * process incoming message
>> + */
>> +static int pcan_usb_decode_msg(struct peak_usb_device *dev,
>> +    u8 *ibuf, u32 lbuf)
>> +{
>> +    struct pcan_usb_msg_context mc = {
>> +        .rec_cnt = ibuf[1],
>> +        .ptr = ibuf + PCAN_USB_MSG_HEADER_LEN,
>> +        .end = ibuf + lbuf,
>> +        .netdev = dev->netdev,
>> +        .pdev = (struct pcan_usb *)dev,
>> +    };
>> +    int err;
>> +
>> +    for (err = 0; mc.rec_idx<  mc.rec_cnt&&  !err; mc.rec_idx++) {
>> +        u8 sl = *mc.ptr++;
>> +
>> +        /* handle status and error frames here */
>> +        if (sl&  PCAN_USB_STATUSLEN_INTERNAL) {
>> +            err = pcan_usb_decode_status(&mc, sl);
>> +
> Remove empty line.
Ok.
>> +        /* handle normal can frames here */
>> +        } else {
>> +            err = pcan_usb_decode_data(&mc, sl);
>> +            mc.rec_data_idx++;
>> +        }
>> +    }
>> +
>> +    return err;
>> +}
>> +
>> +/*
>> + * process any incoming buffer
>> + */
>> +static int pcan_usb_decode_buf(struct peak_usb_device *dev, struct 
>> urb *urb)
>> +{
>> +    int err = 0;
>> +
>> +    if (urb->actual_length>  PCAN_USB_MSG_HEADER_LEN) {
>> +        err = pcan_usb_decode_msg(dev, urb->transfer_buffer,
>> +            urb->actual_length);
>> +
> Dito.
>> +    } else if (urb->actual_length>  0) {
>> +        netdev_err(dev->netdev, "usb message length error (%u)\n",
>> +            urb->actual_length);
>> +        err = -EINVAL;
>> +    }
>> +
>> +    return err;
>> +}
>> +
>> +/*
>> + * start interface
>> + */
>> +static int pcan_usb_start(struct peak_usb_device *dev)
>> +{
>> +    struct pcan_usb *pdev = (struct pcan_usb *)dev;
>> +    int err;
> Initialize err with 0 here.
Why?
>> +
>> +    /* number of bits used in timestamps read from adapter struct */
>> +    peak_usb_init_time_ref(&pdev->time_ref,&pcan_usb);
>> +
>> +    /* if revision greater than 3, can put silent mode on/off */
>> +    if (dev->device_rev>  3) {
>> +        err = pcan_usb_set_silent(dev,
>> +            (dev->can.ctrlmode&  CAN_CTRLMODE_LISTENONLY));
> Remove useless brackets for ctrlmode & LISTENONLY are useless.
Ok.
>> +        if (err)
>> +            goto start_failed;
> Return with err.
Ok.
>> +    }
>> +
>> +    err = pcan_usb_set_ext_vcc(dev, 0);
>> +    if (err)
>> +        goto start_failed;
>> +
>> +    return 0;
>> +
>> +start_failed:
> Remove the 6 lines above.
Ok.
>> +    return err;
>> +}

So, what do you think about that:

/*
  * start interface
  */
static int pcan_usb_start(struct peak_usb_device *dev)
{
         struct pcan_usb *pdev = (struct pcan_usb *)dev;

         /* number of bits used in timestamps read from adapter struct */
         peak_usb_init_time_ref(&pdev->time_ref, &pcan_usb);

         /* if revision greater than 3, can put silent mode on/off */
         if (dev->device_rev > 3) {
                 int err;

                 err = pcan_usb_set_silent(dev,
                                 dev->can.ctrlmode & 
CAN_CTRLMODE_LISTENONLY);
                 if (err)
                         return err;
         }

         return pcan_usb_set_ext_vcc(dev, 0);
}

?

Regards,

Stéphane