Hi Dave,
> There's a lot of code motion in the first four patches
> (with no explanation) that seems to be greatly larger than
> the net effect of applying all four patches.
> I did so, just to see the end result, which was a lot more 'reviewable',
> ending up with this..
>
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index cf8add9..15097a4 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -310,6 +310,28 @@ static unsigned int pl2303_buf_get(struct pl2303_buf
> *pb, char *buf,
> return count;
> }
>
> +static int pl2303_vendor_read(__u16 value, __u16 index,
> + struct usb_serial *serial, unsigned char *buf)
> +{
> + int res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> + VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> + value, index, buf, 1, 100);
> + dbg("0x%x:0x%x:0x%x:0x%x %d - %x", VENDOR_READ_REQUEST_TYPE,
> + VENDOR_READ_REQUEST, value, index, res, buf[0]);
> + return res;
> +}
> +
> +static int pl2303_vendor_write(__u16 value, __u16 index,
> + struct usb_serial *serial)
> +{
> + int res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> + VENDOR_WRITE_REQUEST, VENDOR_WRITE_REQUEST_TYPE,
> + value, index, NULL, 0, 100);
> + dbg("0x%x:0x%x:0x%x:0x%x %d", VENDOR_WRITE_REQUEST_TYPE,
> + VENDOR_WRITE_REQUEST, value, index, res);
> + return res;
> +}
> +
> static int pl2303_startup(struct usb_serial *serial)
> {
> struct pl2303_private *priv;
> @@ -671,7 +693,7 @@ static int pl2303_open(struct usb_serial_port *port,
> struct file *filp)
> struct ktermios tmp_termios;
> struct usb_serial *serial = port->serial;
> struct pl2303_private *priv = usb_get_serial_port_data(port);
> - unsigned char *buf;
> + unsigned char buf;
> int result;
>
> dbg("%s - port %d", __FUNCTION__, port->number);
> @@ -681,43 +703,27 @@ static int pl2303_open(struct usb_serial_port *port,
> struct file *filp)
> usb_clear_halt(serial->dev, port->read_urb->pipe);
> }
>
> - buf = kmalloc(10, GFP_KERNEL);
> - if (buf==NULL)
> - return -ENOMEM;
> -
> -#define FISH(a,b,c,d)
> \
> - result=usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev,0),
> \
> - b, a, c, d, buf, 1, 100);
> \
> - dbg("0x%x:0x%x:0x%x:0x%x %d - %x",a,b,c,d,result,buf[0]);
> -
> -#define SOUP(a,b,c,d)
> \
> - result=usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev,0),
> \
> - b, a, c, d, NULL, 0, 100);
> \
> - dbg("0x%x:0x%x:0x%x:0x%x %d",a,b,c,d,result);
> -
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8484, 0);
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 0x0404, 0);
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8484, 0);
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8383, 0);
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8484, 0);
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 0x0404, 1);
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8484, 0);
> - FISH (VENDOR_READ_REQUEST_TYPE, VENDOR_READ_REQUEST, 0x8383, 0);
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 0, 1);
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 1, 0);
> + pl2303_vendor_read(0x8484, 0, serial, &buf);
> + pl2303_vendor_write(0x0404, 0, serial);
> + pl2303_vendor_read(0x8484, 0, serial, &buf);
> + pl2303_vendor_read(0x8383, 0, serial, &buf);
> + pl2303_vendor_read(0x8484, 0, serial, &buf);
> + pl2303_vendor_write(0x0404, 1, serial);
> + pl2303_vendor_read(0x8484, 0, serial, &buf);
> + pl2303_vendor_read(0x8383, 0, serial, &buf);
> + pl2303_vendor_write(0, 1, serial);
> + pl2303_vendor_write(1, 0, serial);
>
> if (priv->type == HX) {
> /* HX chip */
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 2, 0x44);
> + pl2303_vendor_write(2, 0x44, serial);
> /* reset upstream data pipes */
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 8, 0);
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 9, 0);
> + pl2303_vendor_write(8, 0, serial);
> + pl2303_vendor_write(9, 0, serial);
> } else {
> - SOUP (VENDOR_WRITE_REQUEST_TYPE, VENDOR_WRITE_REQUEST, 2, 0x24);
> + pl2303_vendor_write(2, 0x24, serial);
> }
>
> - kfree(buf);
> -
> /* Setup termios */
> if (port->tty) {
> pl2303_set_termios(port, &tmp_termios);
patch looks great and the code actually becomes readable now.
> After these changes, pl2303_vendor_read returns an int, and
> we assign it to a char. Is that intentional ? Does it matter?
Can't follow what you mean. The return value of it is never used. The
return value of the URB is assigned via a parameter pointer.
Regards
Marcel
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
