lkml.org 
[lkml]   [2012]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: Patch "USB: serial: mos7840: Supported MCS7810 device"
    Date
    Hi Alan,

    Thank you for your kind comments for this patch. I will refine the patch
    according to your comments and re-submit it then.

    Regards,
    Donald

    -----Original Message-----
    From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
    Sent: Thursday, March 29, 2012 11:04 PM
    To: Donald
    Cc: 'Greg KH'; 'open list:USB SUBSYSTEM'; 'open list'
    Subject: Re: Patch "USB: serial: mos7840: Supported MCS7810 device"


    > +static void mos7810_control_callback(struct urb *urb) {
    > + if (!urb)
    > + return;

    How can this ever occur. If its not theoretically possible then this check
    is just going to hide bugs in future.

    > +static void mos7810_led_off(unsigned long arg) {
    > + struct moschip_port *mcs = (struct moschip_port *) arg;
    > +
    > + if (!mcs)
    > + return;

    Ditto

    > + /* Turn on MCS7810 LED */
    > + if (serial->num_ports == 1 && mos7840_port->mos7810_led_flag == 0) {
    > + mos7840_port->mos7810_led_flag = 1;
    > + mos7810_set_reg(mos7840_port, 0x0301,
    > MODEM_CONTROL_REGISTER);
    > + mod_timer(&mos7840_port->mos7810_led_timer1, jiffies + 500);
    > + }

    The assumption that serial->num_ports = 1 means "has LED" is going to cause
    problems later if another deivce appears with 1 port and no LED. I think it
    would be much clearer to have something like

    if (mos7840_port->has_led)

    for these tests of the driver

    > +static int mos7810_check(struct usb_serial *serial) {
    > + int i, pass_count = 0;
    > + __u16 Data = 0, MCR_Data = 0;
    > + __u16 test_pattern = 0x55AA, write_pattern;

    generally linux style is all lower case for egister names. Not a big deal

    > + /* If this a MCS7810 device, both test patterns must match
    > */
    > + if (((test_pattern >> i) & 0x0001) != ((~(Data >> 1)) &
    > 0x0001))

    if (((test_pattern >> i) ^ (~Data >> 1)) & 0x0001) ?

    again trivia...


    > + mos7840_port->mos7810_led_timer1.expires =
    > + jiffies + 500;

    This 500 value depends upon the HZ value of the kernel which won't always be
    1000 on all platforms. You probably want

    jiffies + msecs_to_jiffies(500);

    (or indeed HZ/2)

    > + /* Turn off MCS7810 LED */
    > + usb_control_msg(serial->dev,
    > + usb_sndctrlpipe(serial->dev, 0),
    > + MCS_WRREQ, MCS_WR_RTYPE, 0x0300,
    > + MODEM_CONTROL_REGISTER, NULL, 0,
    > + MOS_WDR_TIMEOUT);
    > +

    Might be clearer if these occurences were a separate small function - gcc
    will inline them if it makes sense, and it will be clearer.


    Alan




    \
     
     \ /
      Last update: 2012-03-30 04:23    [W:0.023 / U:120.580 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site