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

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

Hello Sebastian,

Le 23/12/2011 20:57, Sebastian Haas a écrit :
> Hi Stéphane,
>
> just some minor annotations.
>
> Cheers and Merry Christmas,
>  Sebastian
>
> Am 22.12.2011 14:14, schrieb Stephane Grosjean:
>> +static int pcan_usb_pro_sizeof_rec(u8 data_type)
>> +{
>> +    switch (data_type) {
>> +    case PCAN_USBPRO_RXMSG8:
>> +        return sizeof(struct pcan_usb_pro_rxmsg);
>> +    case PCAN_USBPRO_RXMSG4:
>> +        return sizeof(struct pcan_usb_pro_rxmsg) - 4;
>> +    case PCAN_USBPRO_RXMSG0:
>> +    case PCAN_USBPRO_RXRTR:
>> +        return sizeof(struct pcan_usb_pro_rxmsg) - 8;
>> +    case PCAN_USBPRO_RXSTATUS:
>> +        return sizeof(struct pcan_usb_pro_rxstatus);
> ...
> I wonder if it is possible to use some kind of a table here to improve 
> readability and reduce code.
> ...
Yes it is... You all are right: I changed that ugly function with 
something like that:

/* record size array */
static u16 pcan_usb_pro_sizeof_rec[256] = {
         [PCAN_USBPRO_RXMSG8] = sizeof(struct pcan_usb_pro_rxmsg),
         [PCAN_USBPRO_RXMSG4] = sizeof(struct pcan_usb_pro_rxmsg) - 4,
         [PCAN_USBPRO_RXMSG0] = sizeof(struct pcan_usb_pro_rxmsg) - 8,
         [PCAN_USBPRO_RXRTR] = sizeof(struct pcan_usb_pro_rxmsg) - 8,
         [PCAN_USBPRO_RXSTATUS] = sizeof(struct pcan_usb_pro_rxstatus),
...

and changed the (2) function calls from:

rec_len = pcan_usb_pro_sizeof_rec(x);
if (rec_len <= 0) {

to:

rec_len = pcan_usb_pro_sizeof_rec[x];
if (!rec_len) {

(knowing that "x" is always a BYTE value)

>> +    case PCAN_USBPRO_SETLED:
>> +        return sizeof(struct pcan_usb_pro_setled);
>> +    default:
>> +        pr_info("%s: %s(%d): unsupported data type\n",
>> +            PCAN_USB_DRIVER_NAME, __func__, data_type);
>> +        break;
>> +    }
>> +
>> +    return -1;
>> +}
>> +static int pcan_usb_pro_set_bittiming(struct peak_usb_device *dev,
>> +    struct can_bittiming *bt)
>> +{
>> +    struct pcan_usb_pro_msg um;
>> +    u8 tmp[32];
>> +    u32 ccbt;
>> +
>> +    ccbt = (dev->can.ctrlmode&  CAN_CTRLMODE_3_SAMPLES) ? 0x00800000 
>> : 0;
>> +    ccbt |= (bt->sjw - 1)<<  24;
>> +    ccbt |= (bt->phase_seg2 - 1)<<  20;
>> +    ccbt |= (bt->prop_seg + bt->phase_seg1 - 1)<<  16; /* = tseg1 */
>> +    ccbt |= bt->brp - 1;
> Does the PCAN-USB uses a NXP LPC21xx controller?
I suppose you ask for the "PCAN-USB Pro" adapter, right? Yes it does.
>> +
>> +    netdev_dbg(dev->netdev, "ccbt=0x%08x\n", ccbt);
>> +
>> +    pcan_usb_pro_msg_init_empty(&um, tmp, sizeof(tmp));
>> +    pcan_usb_pro_add_rec(&um, PCAN_USBPRO_SETBTR, dev->ctrl_idx, ccbt);
>> +
>> +    return pcan_usb_pro_send_cmd(dev,&um);
>> +}
>> +/*
>> + * callback for bulk IN urb
>> + */
>> +static int pcan_usb_pro_decode_buf(struct peak_usb_device *dev, 
>> struct urb *urb)
>> +{
>> +    struct pcan_usb_pro_interface *usb_if = pcan_usb_pro_dev_if(dev);
>> +    struct net_device *netdev = dev->netdev;
>> +    struct pcan_usb_pro_msg usb_msg;
>> +    u8 *rec_ptr, *msg_end;
>> +    u16 rec_cnt;
>> +    int err = 0;
>> +
>> +    rec_ptr = pcan_usb_pro_msg_init(&usb_msg, urb->transfer_buffer,
>> +        urb->actual_length);
>> +    if (!rec_ptr) {
>> +        netdev_err(netdev, "bad msg hdr len %d\n", urb->actual_length);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* loop reading all the records from the incoming message */
>> +    msg_end = urb->transfer_buffer + urb->actual_length;
>> +    rec_cnt = le16_to_cpu(*usb_msg.u.rec_cnt_rd);
>> +    for (; rec_cnt>  0; rec_cnt--) {
>> +        union pcan_usb_pro_rec *pr = (union pcan_usb_pro_rec *)rec_ptr;
>> +        int sizeof_rec = pcan_usb_pro_sizeof_rec(pr->data_type);
>> +
>> +        if (sizeof_rec<= 0) {
>> +            netdev_err(netdev,
>> +                "got unsupported rec in usb msg:\n");
>> +            err = -ENOTSUPP;
>> +            break;
>> +        }
>> +
>> +        /* check if the record goes out of current packet */
>> +        if (rec_ptr + sizeof_rec>  msg_end) {
>> +            netdev_err(netdev,
>> +                "got frag rec: should inc usb rx buf size\n");
>> +            err = -EBADMSG;
>> +            break;
>> +        }
>> +
>> +        switch (pr->data_type) {
>> +        case PCAN_USBPRO_RXMSG8:
>> +        case PCAN_USBPRO_RXMSG4:
>> +        case PCAN_USBPRO_RXMSG0:
>> +        case PCAN_USBPRO_RXRTR:
>> +            err = pcan_usb_pro_handle_canmsg(usb_if,&pr->rx_msg);
>> +            if (err<  0)
>> +                goto fail;
>> +            break;
>> +
>> +        case PCAN_USBPRO_RXSTATUS:
>> +            err = pcan_usb_pro_handle_error(usb_if,&pr->rx_status);
>> +            if (err<  0)
>> +                goto fail;
>> +            break;
>> +
>> +        case PCAN_USBPRO_RXTS:
>> +            err = pcan_usb_pro_handle_ts(usb_if,&pr->rx_ts);
> No handling in error case, is that right?
to be coherent, all the "pcan_usb_pro_handle_xxx()" functions return an 
int... But this one always returns 0... So, I changed it into "void 
pcan_usb_pro_handle_ts()" now.
>> +            break;
>> +
>> +        default:
>> +            netdev_err(netdev,
>> +                "unhandled rec type 0x%02x (%d): ignored\n",
>> +                pr->data_type, pr->data_type);
>> +            break;
>> +        }
>> +
>> +        rec_ptr += sizeof_rec;
>> +    }
>> +
>> +fail:
>> +    if (err)
>> +        dump_mem("received msg",
>> +            urb->transfer_buffer, urb->actual_length);
>> +
>> +    return err;
>> +}
>> +
>> +static int pcan_usb_pro_encode_msg(struct peak_usb_device *dev,
>> +    struct sk_buff *skb, u8 *obuf, size_t *size)
>> +{
>> +    struct can_frame *cf = (struct can_frame *)skb->data;
>> +    u8 data_type, len, flags;
>> +    struct pcan_usb_pro_msg usb_msg;
>> +    int err = 0;
>> +
>> +    pcan_usb_pro_msg_init_empty(&usb_msg, obuf, *size);
>> +
>> +    if ((cf->can_id&  CAN_RTR_FLAG) || (cf->can_dlc == 0))
>> +        data_type = PCAN_USBPRO_TXMSG0;
>> +
> Remove empty line.
Ok.
>> +    else if (cf->can_dlc<= 4)
>> +        data_type = PCAN_USBPRO_TXMSG4;
>> +    else
>> +        data_type = PCAN_USBPRO_TXMSG8;
>> +
>> +    len = (dev->ctrl_idx<<  4) | (cf->can_dlc&  0x0f);
>> +
>> +    flags = 0;
>> +    if (cf->can_id&  CAN_EFF_FLAG)
>> +        flags |= 0x02;
>> +    if (cf->can_id&  CAN_RTR_FLAG)
>> +        flags |= 0x01;
>> +
>> +    err = pcan_usb_pro_add_rec(&usb_msg, data_type, 0, flags, len,
>> +        cf->can_id, cf->data);
> \err\ is not checked.
Yes you're right, and it does not need to be. So I removed the "err" 
local variable from the function.
>> +
>> +    *size = usb_msg.rec_buffer_len;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * called when probing to initialize a device object.
>> + */
>> +static int pcan_usb_pro_init(struct peak_usb_device *dev)
>> +{
>> +    struct pcan_usb_pro_interface *usb_if;
>> +    struct pcan_usb_pro_device *pdev = (struct pcan_usb_pro_device 
>> *)dev;
>> +
>> +    /* do this for 1st channel only */
>> +    if (!dev->prev_siblings) {
>> +        struct pcan_usb_pro_fwinfo fi;
>> +        struct pcan_usb_pro_blinfo bi;
>> +
>> +        /* tell the device the can driver is running */
>> +        pcan_usb_pro_drv_loaded(dev, 1);
>> +
>> +        if (pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>> +            PCAN_USBPRO_INFO_FIRMWARE,&fi, sizeof(fi))>= 0)
>> +            netdev_info(dev->netdev,
>> +                "%s fw v%d.%d.%d (%02d/%02d/%02d) fw 0x%08x\n",
>> +                pcan_usb_pro.name,
>> +                fi.version[0], fi.version[1], fi.version[2],
>> +                fi.day, fi.month, fi.year, fi.fw_type);
>> +
>> +        if (pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>> +            PCAN_USBPRO_INFO_BOOTLOADER,&bi, sizeof(bi))>= 0) {
>> +            netdev_info(dev->netdev,
>> +                "bootloader v%d.%d.%d (%02d/%02d/%02d)\n",
>> +                bi.version[0], bi.version[1], bi.version[2],
>> +                bi.day, bi.month, bi.year);
>> +
>> +            netdev_info(dev->netdev,
>> +                "serial %08X.%08X hw 0x%08x rev 0x%08x\n",
>> +                bi.serial_num_hi, bi.serial_num_lo,
>> +                bi.hw_type, bi.hw_rev);
>> +
>> +            dev->device_rev = (u8)bi.hw_rev;
>> +        }
>> +
>> +        usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>> +            GFP_KERNEL);
>> +        if (!usb_if)
>> +            return -ENOMEM;
> Is it necessary to call pcan_usb_pro_drv_loaded(dev, 0) here?
I think you're right too... But I preferred to move the (dev, 1) call 
from the beginning of the block to its end (immediately after 
cm_ignore_count = 5).
>> +
>> +        /* number of ts msgs to ignore before taking one into 
>> account */
>> +        usb_if->cm_ignore_count = 5;
>> +
> Remove empty line.
Ok.

>> +    } else {
>> +        usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>> +    }
>> +
>> +    pdev->usb_if = usb_if;
>> +    usb_if->dev[dev->ctrl_idx] = dev;
>> +
>> +    /* set LED in default state (end of init phase) */
>> +    pcan_usb_pro_set_led(dev, 0, 1);
>> +
>> +    return 0;
>> +}
>> +
>> +static void pcan_usb_pro_exit(struct peak_usb_device *dev)
>> +{
>> +    struct pcan_usb_pro_device *pdev = (struct pcan_usb_pro_device 
>> *)dev;
>> +
>> +    /*
>> +     * when rmmod called before unplug and if down, should reset things
>> +     * before leaving
>> +     */
>> +    if (dev->can.state != CAN_STATE_STOPPED)
>> +
> Remove empty line. Brackets may increase readability here.
Ok and brackets added.
>> +        /* set bus off on the corresponding channel */
>> +        pcan_usb_pro_set_bus(dev, 0);
>> +
>> +    /* if channel #0 (only) */
>> +    if (dev->ctrl_idx == 0) {
>> +        /* turn off calibration message if any device were opened */
>> +        if (pdev->usb_if->dev_opened_count>  0)
>> +            pcan_usb_pro_set_ts(dev, 0);
>> +
>> +        /* tell the PCAN-USB Pro device driver is being unloaded */
>> +        pcan_usb_pro_drv_loaded(dev, 0);
>> +    }
>> +}
Thanks!