Skip to contentSkip to conversation

x86: Fixed MCS7820 device attach problem

PATCH In review Donald 4 participants 11 messages Mar 13, 2012 – Apr 11, 2012

Subsystems (2)
lore
Share & tools
lore

Donald

author
#0 lore raw

PATCH 1/1 x86: Fixed MCS7820 device attach problem

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.

---

 drivers/usb/serial/mos7840.c |   83
+++++++++++++++++++++++++++++------------- 
 1 files changed, 57 insertions(+), 26 deletions(-) 
drivers/usb/serial/mos7840.c +1 -0
index c72abd5..e4885b6 100644 
@@ -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; 

}

Signature · 1 line
1.7.7.6
maintainer
#1 lore raw
Quoted 11 lines · 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.

---

 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

author
#2 lore raw

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

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.

---
 drivers/usb/serial/mos7840.c |  203
++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 197 insertions(+), 6 deletions(-)
drivers/usb/serial/mos7840.c +2 -1
index e4885b6..4a90092 100644
@@ -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);

Signature · 1 line
1.7.7.6
#3 lore raw

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

Review

Quoted 5 lines
(quoted diff) +4 -0
+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.

Review

Quoted 6 lines
(quoted diff) +6 -0
+static void mos7810_led_off(unsigned long arg)
+{
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	if (!mcs)
+		return;

Ditto

Review

Quoted 7 lines
(quoted diff) +4 -0
+	/* 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);

(quoted diff) +2 -0
+		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

Review

Quoted 5 lines
(quoted diff) +5 -0
+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

Quoted 4 lines
(quoted diff) +1 -0
+		/* If this a MCS7810 device, both test patterns must match

*/

(quoted diff) +1 -0
+		if (((test_pattern >> i) & 0x0001) != ((~(Data >> 1)) &

0x0001))

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

again trivia...

Review

Quoted 2 lines
(quoted diff) +2 -0
+			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)

Review

Quoted 7 lines
(quoted diff) +7 -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);
+

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

author
#4 lore raw

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

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:[email protected]]
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"

Review

Quoted 3 lines
(quoted diff) +3 -0
+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.

Review

Quoted 5 lines
(quoted diff) +5 -0
+static void mos7810_led_off(unsigned long arg) {
+	struct moschip_port *mcs = (struct moschip_port *) arg;
+
+	if (!mcs)
+		return;

Ditto

Review

Quoted 7 lines
(quoted diff) +4 -0
+	/* 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);

(quoted diff) +2 -0
+		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

Review

Quoted 4 lines
(quoted diff) +4 -0
+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

Quoted 4 lines
(quoted diff) +1 -0
+		/* If this a MCS7810 device, both test patterns must match

*/

(quoted diff) +1 -0
+		if (((test_pattern >> i) & 0x0001) != ((~(Data >> 1)) &

0x0001))

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

again trivia...

Review

Quoted 2 lines
(quoted diff) +2 -0
+			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)

Review

Quoted 7 lines
(quoted diff) +7 -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);
+

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

maintainer
#5 lore raw

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

In reply to Donald
Quoted 9 lines · 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

Quoted 6 lines

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

author
#6 lore raw

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

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:[email protected]]
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"

Quoted 9 lines · 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

Quoted 6 lines

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

author
#7 lore raw

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

In reply to 'Greg KH'

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.

---
 drivers/usb/serial/mos7840.c |  215
++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 208 insertions(+), 7 deletions(-)
drivers/usb/serial/mos7840.c +8 -0
index c526550..1c85654 100644
@@ -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);

Signature · 1 line
1.7.7.6
#8 lore raw

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

Quoted 7 lines · 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.

Quoted 7 lines

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

author
#9 lore raw

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

Hi Johan,

Thank you for your kind and quick response.

Regards,
Donald

-----Original Message-----
From: Johan Hovold [mailto:[email protected]]
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"

Quoted 8 lines · 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.

Quoted 7 lines

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

maintainer
#10 lore raw

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

In reply to Donald
Quoted 6 lines · 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.

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