RE: Patch "USB: serial: mos7840: Supported MCS7810 device"

From: Donald
Date: Thu Mar 29 2012 - 22:20:59 EST


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@xxxxxxxxxxxxxxxxxxx]
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/