[PATCH 1/1] x86: Fixed MCS7820 device attach problem

[PATCH 1/1] x86: Fixed MCS7820 device attach problem

[Donald]

A MCS7820 device supports two serial ports and a MCS7840 device supports
four serial ports. Both devices use the same driver, but the attach function
in driver was unable to correctly handle the port numbers for MCS7820
device. This problem has been fixed in this patch and this fix has been
verified on x86 Linux kernel 3.2.9 with both MCS7820 and MCS7840 devices.

Signed-off-by: Donald Lee <donald@asix.com.tw> 

--- 
 drivers/usb/serial/mos7840.c |   83
+++++++++++++++++++++++++++++------------- 
 1 files changed, 57 insertions(+), 26 deletions(-) 
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c 
index c72abd5..e4885b6 100644 
--- a/drivers/usb/serial/mos7840.c 
+++ b/drivers/usb/serial/mos7840.c 
@@ -174,6 +174,7 @@ 
  
 #define CLK_MULTI_REGISTER         ((__u16)(0x02)) 
 #define CLK_START_VALUE_REGISTER   ((__u16)(0x03)) 
+#define GPIO_REGISTER              ((__u16)(0x07)) 
  
 #define SERIAL_LCR_DLAB            ((__u16)(0x0080)) 
  
@@ -1103,14 +1104,25 @@ static int mos7840_open(struct tty_struct *tty,
struct usb_serial_port *port) 
        mos7840_port->read_urb = port->read_urb; 
  
        /* set up our bulk in urb */ 
- 
-       usb_fill_bulk_urb(mos7840_port->read_urb, 
-                         serial->dev, 
-                         usb_rcvbulkpipe(serial->dev, 
-                                         port->bulk_in_endpointAddress), 
-                         port->bulk_in_buffer, 
-                         mos7840_port->read_urb->transfer_buffer_length, 
-                         mos7840_bulk_in_callback, mos7840_port); 
+       if ((serial->num_ports == 2) 
+               && ((((__u16)port->number - 
+                       (__u16)(port->serial->minor)) % 2) != 0)) { 
+               usb_fill_bulk_urb(mos7840_port->read_urb, 
+                       serial->dev, 
+                       usb_rcvbulkpipe(serial->dev, 
+                               (port->bulk_in_endpointAddress) + 2), 
+                       port->bulk_in_buffer, 
+                       mos7840_port->read_urb->transfer_buffer_length, 
+                       mos7840_bulk_in_callback, mos7840_port); 
+       } else { 
+               usb_fill_bulk_urb(mos7840_port->read_urb, 
+                       serial->dev, 
+                       usb_rcvbulkpipe(serial->dev, 
+                               port->bulk_in_endpointAddress), 
+                       port->bulk_in_buffer, 
+                       mos7840_port->read_urb->transfer_buffer_length, 
+                       mos7840_bulk_in_callback, mos7840_port); 
+       } 
  
        dbg("mos7840_open: bulkin endpoint is %d", 
            port->bulk_in_endpointAddress); 
@@ -1521,13 +1533,25 @@ static int mos7840_write(struct tty_struct *tty,
struct usb_serial_port *port, 
        memcpy(urb->transfer_buffer, current_position, transfer_size); 
  
        /* fill urb with data and submit  */ 
-       usb_fill_bulk_urb(urb, 
-                         serial->dev, 
-                         usb_sndbulkpipe(serial->dev, 
-                                         port->bulk_out_endpointAddress), 
-                         urb->transfer_buffer, 
-                         transfer_size, 
-                         mos7840_bulk_out_data_callback, mos7840_port); 
+       if ((serial->num_ports == 2) 
+               && ((((__u16)port->number - 
+                       (__u16)(port->serial->minor)) % 2) != 0)) { 
+               usb_fill_bulk_urb(urb, 
+                       serial->dev, 
+                       usb_sndbulkpipe(serial->dev, 
+                               (port->bulk_out_endpointAddress) + 2), 
+                       urb->transfer_buffer, 
+                       transfer_size, 
+                       mos7840_bulk_out_data_callback, mos7840_port); 
+       } else { 
+               usb_fill_bulk_urb(urb, 
+                       serial->dev, 
+                       usb_sndbulkpipe(serial->dev, 
+                               port->bulk_out_endpointAddress), 
+                       urb->transfer_buffer, 
+                       transfer_size, 
+                       mos7840_bulk_out_data_callback, mos7840_port); 
+       } 
  
        data1 = urb->transfer_buffer; 
        dbg("bulkout endpoint is %d", port->bulk_out_endpointAddress); 
@@ -1840,7 +1864,7 @@ static int mos7840_send_cmd_write_baud_rate(struct
moschip_port *mos7840_port, 
  
        } else { 
 #ifdef HW_flow_control 
-               / *setting h/w flow control bit to 0 */ 
+               /* setting h/w flow control bit to 0 */ 
                Data = 0xb; 
                mos7840_port->shadowMCR = Data; 
                status = mos7840_set_uart_reg(port, MODEM_CONTROL_REGISTER, 
@@ -2309,19 +2333,26 @@ static int mos7840_ioctl(struct tty_struct *tty, 
  
 static int mos7840_calc_num_ports(struct usb_serial *serial) 
 { 
-       int mos7840_num_ports = 0; 
- 
-       dbg("numberofendpoints: cur %d, alt %d", 
-           (int)serial->interface->cur_altsetting->desc.bNumEndpoints, 
-           (int)serial->interface->altsetting->desc.bNumEndpoints); 
-       if (serial->interface->cur_altsetting->desc.bNumEndpoints == 5) { 
-               mos7840_num_ports = serial->num_ports = 2; 
-       } else if (serial->interface->cur_altsetting->desc.bNumEndpoints ==
9) { 
+       __u16 Data = 0x00; 
+       int ret = 0; 
+       int mos7840_num_ports; 
+ 
+       ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), 
+               MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &Data, 
+               VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT); 
+ 
+       if ((Data&0x01) == 0) { 
+               mos7840_num_ports = 2; 
+               serial->num_bulk_in = 2; 
+               serial->num_bulk_out = 2; 
+               serial->num_ports = 2; 
+       } else { 
+               mos7840_num_ports = 4; 
                serial->num_bulk_in = 4; 
                serial->num_bulk_out = 4; 
-               mos7840_num_ports = serial->num_ports = 4; 
+               serial->num_ports = 4; 
        } 
-       dbg ("mos7840_num_ports = %d", mos7840_num_ports); 
+ 
        return mos7840_num_ports; 
 } 
  
-- 
1.7.7.6

Re: [PATCH 1/1] x86: Fixed MCS7820 device attach problem

['Greg KH']

On Tue, Mar 13, 2012 at 03:54:46PM +0800, Donald wrote:
> A MCS7820 device supports two serial ports and a MCS7840 device supports
> four serial ports. Both devices use the same driver, but the attach function
> in driver was unable to correctly handle the port numbers for MCS7820
> device. This problem has been fixed in this patch and this fix has been
> verified on x86 Linux kernel 3.2.9 with both MCS7820 and MCS7840 devices.
> 
> Signed-off-by: Donald Lee <donald@asix.com.tw> 
> 
> --- 
>  drivers/usb/serial/mos7840.c |   83
> +++++++++++++++++++++++++++++------------- 

This is great, but your email client turned the tabs into spaces and
line-wrapped the patch, and added trailing spaces on every line, making
this impossible to apply :(

Care to read Documentation/email_clients.txt and resend it after fixing
things up on your end?

And this isn't limited to x86, so you don't need that in the subject:
line, it's a USB issue for all platforms.

thanks,

greg k-h

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

[Donald]

Hi Greg,

I am submitting this patch that supports MCS7810 device for the mos7840
driver. If you see any problem regarding this patch, please let me know at
any time. Thank you for your help.

Regards,
Donald

Patch Description:
This patch added the support of MCS7810 device for the mos7840 driver. The
MCS7810 device supports single USB2.0-to-Serial port with a LED indicator
for reflecting transmission or reception activity. Please be noted that part
of codes in his patch are relating to the codes in last submitted patch that
fixed MCS7820 device attach problem.

Signed-off-by: Donald Lee <donald@asix.com.tw>
---
 drivers/usb/serial/mos7840.c |  203
++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 197 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index e4885b6..4a90092 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -114,6 +114,7 @@
 #define USB_VENDOR_ID_MOSCHIP           0x9710
 #define MOSCHIP_DEVICE_ID_7840          0x7840
 #define MOSCHIP_DEVICE_ID_7820          0x7820
+#define MOSCHIP_DEVICE_ID_7810          0x7810
 /* The native component can have its vendor/device id's overridden
  * in vendor-specific implementations.  Such devices can be handled
  * by making a change here, in moschip_port_id_table, and in
@@ -184,10 +185,10 @@
 #define NUM_URBS                        16	/* URB Count */
 #define URB_TRANSFER_BUFFER_SIZE        32	/* URB Size  */
 
-
 static const struct usb_device_id moschip_port_id_table[] = {
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
+	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4)},
@@ -209,6 +210,7 @@ static const struct usb_device_id
moschip_port_id_table[] = {
 static const struct usb_device_id moschip_id_table_combined[]
__devinitconst = {
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
+	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4)},
@@ -261,6 +263,12 @@ struct moschip_port {
 	struct urb *write_urb_pool[NUM_URBS];
 	char busy[NUM_URBS];
 	bool read_urb_busy;
+
+	/* For MCS7810 LED */
+	int mos7810_led_flag;
+	struct timer_list mos7810_led_timer1;	/* For LED on 500ms */
+	struct timer_list mos7810_led_timer2;	/* For LED off 500ms */
+
 };
 
 
@@ -572,6 +580,68 @@ static int mos7840_get_reg(struct moschip_port *mcs,
__u16 Wval, __u16 reg,
 	return ret;
 }
 
+static void mos7810_control_callback(struct urb *urb)
+{
+	if (!urb)
+		return;
+
+	switch (urb->status) {
+	case 0:
+		/* Success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* This urb is terminated, clean up */
+		dbg("%s - urb shutting down with status: %d", __func__,
+			urb->status);
+		break;
+	default:
+		dbg("%s - nonzero urb status received: %d", __func__,
+			urb->status);
+	}
+}
+
+static int mos7810_set_reg(struct moschip_port *mcs, __u16 Wval, __u16 reg)
+{
+	struct usb_device *dev = mcs->port->serial->dev;
+	struct usb_ctrlrequest *dr = mcs->dr;
+
+	dr->bRequestType = MCS_WR_RTYPE;
+	dr->bRequest = MCS_WRREQ;
+	dr->wValue = cpu_to_le16(Wval);
+	dr->wIndex = cpu_to_le16(reg);
+	dr->wLength = cpu_to_le16(0);
+
+	usb_fill_control_urb(mcs->control_urb, dev, usb_sndctrlpipe(dev, 0),
+		(unsigned char *)dr, NULL, 0, mos7810_control_callback,
NULL);
+
+	return usb_submit_urb(mcs->control_urb, GFP_ATOMIC);
+}
+
+static void mos7810_led_off(unsigned long arg)
+{
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	if (!mcs)
+		return;
+
+	/* Turn off MCS7810 LED */
+	mos7810_set_reg(mcs, 0x0300, MODEM_CONTROL_REGISTER);
+	mod_timer(&mcs->mos7810_led_timer2, jiffies + 500);
+}
+
+static void mos7810_led_flag_off(unsigned long arg)
+{
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	if (!mcs)
+		return;
+
+	mcs->mos7810_led_flag = 0;
+
+}
+
 
/***************************************************************************
**
  * mos7840_interrupt_callback
  *	this is the callback function for when we have received data on the
@@ -792,6 +862,12 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 		return;
 	}
 
+	/* 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);
+	}
 
 	mos7840_port->read_urb->dev = serial->dev;
 
@@ -1556,6 +1632,18 @@ static int mos7840_write(struct tty_struct *tty,
struct usb_serial_port *port,
 	data1 = urb->transfer_buffer;
 	dbg("bulkout endpoint is %d", port->bulk_out_endpointAddress);
 
+	/* Turn on MCS7810 LED */
+	if (serial->num_ports == 1 && mos7840_port->mos7810_led_flag == 0) {
+
+		mos7840_port->mos7810_led_flag = 1;
+
+		usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev,
0),
+			MCS_WRREQ, MCS_WR_RTYPE, 0x0301,
+			MODEM_CONTROL_REGISTER, NULL, 0, MOS_WDR_TIMEOUT);
+
+		mod_timer(&mos7840_port->mos7810_led_timer1, jiffies + 500);
+	}
+
 	/* send it down the pipe */
 	status = usb_submit_urb(urb, GFP_ATOMIC);
 
@@ -2331,6 +2419,48 @@ static int mos7840_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+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;
+
+	/* Store MCR setting */
+	usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+		MCS_RDREQ, MCS_RD_RTYPE, 0x0300, MODEM_CONTROL_REGISTER,
+		&MCR_Data, VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
+
+	for (i = 0; i < 16; i++) {
+		write_pattern = 0x0300 | (((test_pattern >> i) & 0x0001) <<
1);
+
+		/* Send the 1-bit test pattern out to RTS pin */
+		usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev,
0),
+			MCS_WRREQ, MCS_WR_RTYPE, write_pattern,
+			MODEM_CONTROL_REGISTER, NULL, 0, MOS_WDR_TIMEOUT);
+
+		/* Read the test pattern back from GPIO1 pin */
+		usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev,
0),
+			MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &Data,
+			VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
+
+		/* If this a MCS7810 device, both test patterns must match
*/
+		if (((test_pattern >> i) & 0x0001) != ((~(Data >> 1)) &
0x0001))
+			break;
+
+		pass_count++;
+	}
+
+	/* Restore MCR setting */
+	usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
MCS_WRREQ,
+		MCS_WR_RTYPE, 0x0300 | MCR_Data, MODEM_CONTROL_REGISTER,
NULL,
+		0, MOS_WDR_TIMEOUT);
+
+	if (pass_count == 16)
+		return 1;
+
+	return 0;
+}
+
 static int mos7840_calc_num_ports(struct usb_serial *serial)
 {
 	__u16 Data = 0x00;
@@ -2341,16 +2471,35 @@ static int mos7840_calc_num_ports(struct usb_serial
*serial)
 		MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &Data,
 		VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
 
-	if ((Data&0x01) == 0) {
+	if (serial->dev->descriptor.idProduct == MOSCHIP_DEVICE_ID_7810) {
+		mos7840_num_ports = 1;
+		serial->num_bulk_in = 1;
+		serial->num_bulk_out = 1;
+		serial->num_ports = 1;
+	} else if (serial->dev->descriptor.idProduct ==
+						MOSCHIP_DEVICE_ID_7820) {
 		mos7840_num_ports = 2;
 		serial->num_bulk_in = 2;
 		serial->num_bulk_out = 2;
 		serial->num_ports = 2;
 	} else {
-		mos7840_num_ports = 4;
-		serial->num_bulk_in = 4;
-		serial->num_bulk_out = 4;
-		serial->num_ports = 4;
+		/* For a MCS7840 device GPIO0 must be set to 1*/
+		if ((Data & 0x01) == 1) {
+			mos7840_num_ports = 4;
+			serial->num_bulk_in = 4;
+			serial->num_bulk_out = 4;
+			serial->num_ports = 4;
+		} else if (mos7810_check(serial)) {
+			mos7840_num_ports = 1;
+			serial->num_bulk_in = 1;
+			serial->num_bulk_out = 1;
+			serial->num_ports = 1;
+		} else {
+			mos7840_num_ports = 2;
+			serial->num_bulk_in = 2;
+			serial->num_bulk_out = 2;
+			serial->num_ports = 2;
+		}
 	}
 
 	return mos7840_num_ports;
@@ -2567,6 +2716,33 @@ static int mos7840_startup(struct usb_serial *serial)
 			status = -ENOMEM;
 			goto error;
 		}
+
+		/* Initialize MCS7810 LED timers */
+		if (serial->num_ports == 1) {
+			init_timer(&mos7840_port->mos7810_led_timer1);
+			mos7840_port->mos7810_led_timer1.function =
+							mos7810_led_off;
+			mos7840_port->mos7810_led_timer1.expires =
+							jiffies + 500;
+			mos7840_port->mos7810_led_timer1.data =
+						(unsigned long)mos7840_port;
+
+			init_timer(&mos7840_port->mos7810_led_timer2);
+			mos7840_port->mos7810_led_timer2.function =
+
mos7810_led_flag_off;
+			mos7840_port->mos7810_led_timer2.expires =
+							jiffies + 500;
+			mos7840_port->mos7810_led_timer2.data =
+						(unsigned long)mos7840_port;
+
+			mos7840_port->mos7810_led_flag = 0;
+
+			/* 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);
+		}
 	}
 	dbg ("mos7840_startup: all ports configured...........");
 
@@ -2658,6 +2834,21 @@ static void mos7840_release(struct usb_serial
*serial)
 		mos7840_port = mos7840_get_port_private(serial->port[i]);
 		dbg("mos7840_port %d = %p", i, mos7840_port);
 		if (mos7840_port) {
+
+			if (serial->num_ports == 1) {
+				/* 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);
+
+				del_timer_sync(&mos7840_port->
+							mos7810_led_timer1);
+				del_timer_sync(&mos7840_port->
+							mos7810_led_timer2);
+			}
+
 			kfree(mos7840_port->ctrl_buf);
 			kfree(mos7840_port->dr);
 			kfree(mos7840_port);
-- 
1.7.7.6

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

[Alan Cox]

> +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

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

[Donald]

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

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

['Greg KH']

On Thu, Mar 29, 2012 at 09:33:36PM +0800, Donald wrote:
> Hi Greg,
> 
> I am submitting this patch that supports MCS7810 device for the mos7840
> driver. If you see any problem regarding this patch, please let me know at
> any time. Thank you for your help.
> 
> Regards,
> Donald
> 
> Patch Description:
> This patch added the support of MCS7810 device for the mos7840 driver. The
> MCS7810 device supports single USB2.0-to-Serial port with a LED indicator
> for reflecting transmission or reception activity. Please be noted that part
> of codes in his patch are relating to the codes in last submitted patch that
> fixed MCS7820 device attach problem.

Along with Alan's comments, I need this to be able to be applied, and if
you include portions of the patch that I have already accepted, I can't
apply it, right?

Can you redo this patch against 3.4-rc1 when it comes out in a few days?

thanks,

greg k-h

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

[Donald]

Hi Greg,

Thank you for your quick reply. I will refine the patch according to Alan's
comments and redo this patch against 3.4-rc1 and re-submit it ASAP.

Regards,
Donald

-----Original Message-----
From: 'Greg KH' [mailto:gregkh@linuxfoundation.org] 
Sent: Thursday, March 29, 2012 11:14 PM
To: Donald
Cc: 'open list:USB SUBSYSTEM'; 'open list'
Subject: Re: Patch "USB: serial: mos7840: Supported MCS7810 device"

On Thu, Mar 29, 2012 at 09:33:36PM +0800, Donald wrote:
> Hi Greg,
> 
> I am submitting this patch that supports MCS7810 device for the 
> mos7840 driver. If you see any problem regarding this patch, please 
> let me know at any time. Thank you for your help.
> 
> Regards,
> Donald
> 
> Patch Description:
> This patch added the support of MCS7810 device for the mos7840 driver. 
> The MCS7810 device supports single USB2.0-to-Serial port with a LED 
> indicator for reflecting transmission or reception activity. Please be 
> noted that part of codes in his patch are relating to the codes in 
> last submitted patch that fixed MCS7820 device attach problem.

Along with Alan's comments, I need this to be able to be applied, and if you
include portions of the patch that I have already accepted, I can't apply
it, right?

Can you redo this patch against 3.4-rc1 when it comes out in a few days?

thanks,

greg k-h

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

[Donald]

Hi Greg,

I am re-submitting this patch that supports MCS7810 device for the mos7840
driver. This patch was created against 3.4-rc1 and has been verified on
3.4-rc1 also. If you see any problem regarding this patch, please let me
know at any time. Thank you for your help.

Besides, I found two things regarding 3.4-rc, firstly, it added a macro to
replace module_init() and module_exit(); secondly, the system will crash if
I remove module before unplug the dongle; while this issue doesn't happen on
Linux kernel 3.2.9. I had been tested mcs7840 and prolific-pl2303, both
drivers have the same scenario. I am not sure if this issue is relating to
the macro added in 3.4-rc and would like to know if you have any comment on
this issue.  

Regards,
Donald

Patch Description:
This patch added the support of MCS7810 device for the mos7840 driver. The
MCS7810 device supports single USB2.0-to-Serial port with a LED indicator
for reflecting transmission or reception activity.

Signed-off-by: Donald Lee <donald@asix.com.tw>
---
 drivers/usb/serial/mos7840.c |  215
++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 208 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index c526550..1c85654 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -114,6 +114,7 @@
 #define USB_VENDOR_ID_MOSCHIP           0x9710
 #define MOSCHIP_DEVICE_ID_7840          0x7840
 #define MOSCHIP_DEVICE_ID_7820          0x7820
+#define MOSCHIP_DEVICE_ID_7810          0x7810
 /* The native component can have its vendor/device id's overridden
  * in vendor-specific implementations.  Such devices can be handled
  * by making a change here, in moschip_port_id_table, and in
@@ -184,10 +185,17 @@
 #define NUM_URBS                        16	/* URB Count */
 #define URB_TRANSFER_BUFFER_SIZE        32	/* URB Size  */
 
+/* MCS7810 LED support */
+#define MCS7810_HAS_LED
+
+/* MCS7810 LED on/off milliseconds*/
+#define MCS7810_LED_ON_MS	500
+#define MCS7810_LED_OFF_MS	500
 
 static const struct usb_device_id moschip_port_id_table[] = {
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
+	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4)},
@@ -209,6 +217,7 @@ static const struct usb_device_id
moschip_port_id_table[] = {
 static const struct usb_device_id moschip_id_table_combined[]
__devinitconst = {
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
 	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
+	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P)},
 	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4)},
@@ -261,6 +270,13 @@ struct moschip_port {
 	struct urb *write_urb_pool[NUM_URBS];
 	char busy[NUM_URBS];
 	bool read_urb_busy;
+
+#ifdef MCS7810_HAS_LED
+	/* For MCS7810 LED */
+	int mos7810_led_flag;
+	struct timer_list mos7810_led_timer1;	/* Timer for LED on */
+	struct timer_list mos7810_led_timer2;	/* Timer for LED off */
+#endif
 };
 
 
@@ -572,6 +588,71 @@ static int mos7840_get_reg(struct moschip_port *mcs,
__u16 Wval, __u16 reg,
 	return ret;
 }
 
+#ifdef MCS7810_HAS_LED
+static void mos7810_control_callback(struct urb *urb)
+{
+	switch (urb->status) {
+	case 0:
+		/* Success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* This urb is terminated, clean up */
+		dbg("%s - urb shutting down with status: %d", __func__,
+			urb->status);
+		break;
+	default:
+		dbg("%s - nonzero urb status received: %d", __func__,
+			urb->status);
+	}
+}
+
+static void mos7810_set_led_async(struct moschip_port *mcs, __u16 wval,
+				__u16 reg)
+{
+	struct usb_device *dev = mcs->port->serial->dev;
+	struct usb_ctrlrequest *dr = mcs->dr;
+
+	dr->bRequestType = MCS_WR_RTYPE;
+	dr->bRequest = MCS_WRREQ;
+	dr->wValue = cpu_to_le16(wval);
+	dr->wIndex = cpu_to_le16(reg);
+	dr->wLength = cpu_to_le16(0);
+
+	usb_fill_control_urb(mcs->control_urb, dev, usb_sndctrlpipe(dev, 0),
+		(unsigned char *)dr, NULL, 0, mos7810_control_callback,
NULL);
+
+	usb_submit_urb(mcs->control_urb, GFP_ATOMIC);
+}
+
+static void mos7810_set_led_sync(struct usb_serial_port *port, __u16 reg,
+				__u16 val)
+{
+	struct usb_device *dev = port->serial->dev;
+
+	usb_control_msg(dev, usb_sndctrlpipe(dev, 0), MCS_WRREQ,
MCS_WR_RTYPE,
+			val, reg, NULL, 0, MOS_WDR_TIMEOUT);
+}
+
+static void mos7810_led_off(unsigned long arg)
+{
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	/* Turn off MCS7810 LED */
+	mos7810_set_led_async(mcs, 0x0300, MODEM_CONTROL_REGISTER);
+	mod_timer(&mcs->mos7810_led_timer2,
+				jiffies +
msecs_to_jiffies(MCS7810_LED_OFF_MS));
+}
+
+static void mos7810_led_flag_off(unsigned long arg)
+{
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	mcs->mos7810_led_flag = 0;
+}
+#endif
+
 
/***************************************************************************
**
  * mos7840_interrupt_callback
  *	this is the callback function for when we have received data on the
@@ -792,6 +873,16 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 		return;
 	}
 
+#ifdef MCS7810_HAS_LED
+	/* Turn on MCS7810 LED */
+	if (serial->num_ports == 1 && mos7840_port->mos7810_led_flag == 0) {
+		mos7840_port->mos7810_led_flag = 1;
+		mos7810_set_led_async(mos7840_port, 0x0301,
+					MODEM_CONTROL_REGISTER);
+		mod_timer(&mos7840_port->mos7810_led_timer1,
+				jiffies +
msecs_to_jiffies(MCS7810_LED_ON_MS));
+	}
+#endif
 
 	mos7840_port->read_urb_busy = true;
 	retval = usb_submit_urb(mos7840_port->read_urb, GFP_ATOMIC);
@@ -1554,6 +1645,16 @@ static int mos7840_write(struct tty_struct *tty,
struct usb_serial_port *port,
 	data1 = urb->transfer_buffer;
 	dbg("bulkout endpoint is %d", port->bulk_out_endpointAddress);
 
+#ifdef MCS7810_HAS_LED
+	/* Turn on MCS7810 LED */
+	if (serial->num_ports == 1 && mos7840_port->mos7810_led_flag == 0) {
+		mos7840_port->mos7810_led_flag = 1;
+		mos7810_set_led_sync(port, MODEM_CONTROL_REGISTER, 0x0301);
+		mod_timer(&mos7840_port->mos7810_led_timer1,
+				jiffies +
msecs_to_jiffies(MCS7810_LED_ON_MS));
+	}
+#endif
+
 	/* send it down the pipe */
 	status = usb_submit_urb(urb, GFP_ATOMIC);
 
@@ -2327,26 +2428,86 @@ static int mos7840_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static int mos7810_check(struct usb_serial *serial)
+{
+	int i, pass_count = 0;
+	__u16 data = 0, mcr_data = 0;
+	__u16 test_pattern = 0x55AA;
+
+	/* Store MCR setting */
+	usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+		MCS_RDREQ, MCS_RD_RTYPE, 0x0300, MODEM_CONTROL_REGISTER,
+		&mcr_data, VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
+
+	for (i = 0; i < 16; i++) {
+		/* Send the 1-bit test pattern out to MCS7810 test pin */
+		usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev,
0),
+			MCS_WRREQ, MCS_WR_RTYPE,
+			(0x0300 | (((test_pattern >> i) & 0x0001) << 1)),
+			MODEM_CONTROL_REGISTER, NULL, 0, MOS_WDR_TIMEOUT);
+
+		/* Read the test pattern back */
+		usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev,
0),
+			MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &data,
+			VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
+
+		/* If this is a MCS7810 device, both test patterns must
match */
+		if (((test_pattern >> i) ^ (~data >> 1)) & 0x0001)
+			break;
+
+		pass_count++;
+	}
+
+	/* Restore MCR setting */
+	usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
MCS_WRREQ,
+		MCS_WR_RTYPE, 0x0300 | mcr_data, MODEM_CONTROL_REGISTER,
NULL,
+		0, MOS_WDR_TIMEOUT);
+
+	if (pass_count == 16)
+		return 1;
+
+	return 0;
+}
+
 static int mos7840_calc_num_ports(struct usb_serial *serial)
 {
-	__u16 Data = 0x00;
+	__u16 data = 0x00;
 	int ret = 0;
 	int mos7840_num_ports;
 
 	ret = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-		MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &Data,
+		MCS_RDREQ, MCS_RD_RTYPE, 0, GPIO_REGISTER, &data,
 		VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
 
-	if ((Data & 0x01) == 0) {
+	if (serial->dev->descriptor.idProduct == MOSCHIP_DEVICE_ID_7810) {
+		mos7840_num_ports = 1;
+		serial->num_bulk_in = 1;
+		serial->num_bulk_out = 1;
+		serial->num_ports = 1;
+	} else if (serial->dev->descriptor.idProduct ==
+						MOSCHIP_DEVICE_ID_7820) {
 		mos7840_num_ports = 2;
 		serial->num_bulk_in = 2;
 		serial->num_bulk_out = 2;
 		serial->num_ports = 2;
 	} else {
-		mos7840_num_ports = 4;
-		serial->num_bulk_in = 4;
-		serial->num_bulk_out = 4;
-		serial->num_ports = 4;
+		/* For a MCS7840 device GPIO0 must be set to 1*/
+		if ((data & 0x01) == 1) {
+			mos7840_num_ports = 4;
+			serial->num_bulk_in = 4;
+			serial->num_bulk_out = 4;
+			serial->num_ports = 4;
+		} else if (mos7810_check(serial)) {
+			mos7840_num_ports = 1;
+			serial->num_bulk_in = 1;
+			serial->num_bulk_out = 1;
+			serial->num_ports = 1;
+		} else {
+			mos7840_num_ports = 2;
+			serial->num_bulk_in = 2;
+			serial->num_bulk_out = 2;
+			serial->num_ports = 2;
+		}
 	}
 
 	return mos7840_num_ports;
@@ -2563,6 +2724,33 @@ static int mos7840_startup(struct usb_serial *serial)
 			status = -ENOMEM;
 			goto error;
 		}
+
+#ifdef MCS7810_HAS_LED
+		/* Initialize MCS7810 LED timers */
+		if (serial->num_ports == 1) {
+			init_timer(&mos7840_port->mos7810_led_timer1);
+			mos7840_port->mos7810_led_timer1.function =
+							mos7810_led_off;
+			mos7840_port->mos7810_led_timer1.expires =
+				jiffies +
msecs_to_jiffies(MCS7810_LED_ON_MS);
+			mos7840_port->mos7810_led_timer1.data =
+						(unsigned long)mos7840_port;
+
+			init_timer(&mos7840_port->mos7810_led_timer2);
+			mos7840_port->mos7810_led_timer2.function =
+
mos7810_led_flag_off;
+			mos7840_port->mos7810_led_timer2.expires =
+				jiffies +
msecs_to_jiffies(MCS7810_LED_OFF_MS);
+			mos7840_port->mos7810_led_timer2.data =
+						(unsigned long)mos7840_port;
+
+			mos7840_port->mos7810_led_flag = 0;
+
+			/* Turn off MCS7810 LED */
+			mos7810_set_led_sync(serial->port[i],
+						MODEM_CONTROL_REGISTER,
0x0300);
+		}
+#endif
 	}
 	dbg ("mos7840_startup: all ports configured...........");
 
@@ -2638,6 +2826,7 @@ static void mos7840_release(struct usb_serial *serial)
 {
 	int i;
 	struct moschip_port *mos7840_port;
+
 	dbg("%s", " release :entering..........");
 
 	if (!serial) {
@@ -2654,6 +2843,18 @@ static void mos7840_release(struct usb_serial
*serial)
 		mos7840_port = mos7840_get_port_private(serial->port[i]);
 		dbg("mos7840_port %d = %p", i, mos7840_port);
 		if (mos7840_port) {
+#ifdef MCS7810_HAS_LED
+			if (serial->num_ports == 1) {
+				/* Turn off MCS7810 LED */
+				mos7810_set_led_sync(mos7840_port->port,
+						MODEM_CONTROL_REGISTER,
0x0300);
+
+				del_timer_sync(&mos7840_port->
+							mos7810_led_timer1);
+				del_timer_sync(&mos7840_port->
+							mos7810_led_timer2);
+			}
+#endif
 			kfree(mos7840_port->ctrl_buf);
 			kfree(mos7840_port->dr);
 			kfree(mos7840_port);
-- 
1.7.7.6

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

[Johan Hovold]

On Wed, Apr 11, 2012 at 05:32:53PM +0800, Donald wrote:
> Hi Greg,
> 
> I am re-submitting this patch that supports MCS7810 device for the mos7840
> driver. This patch was created against 3.4-rc1 and has been verified on
> 3.4-rc1 also. If you see any problem regarding this patch, please let me
> know at any time. Thank you for your help.
> 
> Besides, I found two things regarding 3.4-rc, firstly, it added a macro to
> replace module_init() and module_exit(); secondly, the system will crash if
> I remove module before unplug the dongle; while this issue doesn't happen on
> Linux kernel 3.2.9. I had been tested mcs7840 and prolific-pl2303, both
> drivers have the same scenario. I am not sure if this issue is relating to
> the macro added in 3.4-rc and would like to know if you have any comment on
> this issue.

This is a known issue which has been fixed by the following patch:

	http://marc.info/?l=linux-usb&m=133296552518354&w=2

It should show up in 3.4-rc shortly.

Thanks,
Johan

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

[Donald]

Hi Johan,

Thank you for your kind and quick response.

Regards,
Donald

-----Original Message-----
From: Johan Hovold [mailto:jhovold@gmail.com] 
Sent: Wednesday, April 11, 2012 7:08 PM
To: Donald
Cc: 'Greg KH'; 'open list:USB SUBSYSTEM'; 'open list'
Subject: Re: Patch "USB: serial: mos7840: Supported MCS7810 device"

On Wed, Apr 11, 2012 at 05:32:53PM +0800, Donald wrote:
> Hi Greg,
> 
> I am re-submitting this patch that supports MCS7810 device for the 
> mos7840 driver. This patch was created against 3.4-rc1 and has been 
> verified on
> 3.4-rc1 also. If you see any problem regarding this patch, please let 
> me know at any time. Thank you for your help.
> 
> Besides, I found two things regarding 3.4-rc, firstly, it added a 
> macro to replace module_init() and module_exit(); secondly, the system 
> will crash if I remove module before unplug the dongle; while this 
> issue doesn't happen on Linux kernel 3.2.9. I had been tested mcs7840 
> and prolific-pl2303, both drivers have the same scenario. I am not 
> sure if this issue is relating to the macro added in 3.4-rc and would 
> like to know if you have any comment on this issue.

This is a known issue which has been fixed by the following patch:

	http://marc.info/?l=linux-usb&m=133296552518354&w=2

It should show up in 3.4-rc shortly.

Thanks,
Johan

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

['Greg KH']

On Wed, Apr 11, 2012 at 05:32:53PM +0800, Donald wrote:
> Hi Greg,
> 
> I am re-submitting this patch that supports MCS7810 device for the mos7840
> driver. This patch was created against 3.4-rc1 and has been verified on
> 3.4-rc1 also. If you see any problem regarding this patch, please let me
> know at any time. Thank you for your help.

The patch is line-wrapped and can not be applied :(

Care to fix your email client (read the file
Documentation/email_clients.txt for some hints) and try it again?

thanks,

greg k-h