[PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

Prolific has developed a new USB to UART chip: PL2303HXN
PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
the existing PL2303 series (TYPE_HX & TYPE_01).
Therefore, different Vendor requests are used to issue related commands.

1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
   new Vendor request,new flow control and other related instructions
   if TYPE_HXN is recognized.

2. Because the new PL2303HXN only accept the new Vendor request,
   the old Vendor request cannot be accepted (the error message
   will be returned)
   So first determine the TYPE_HX or TYPE_HXN through
   PL2303_READ_TYPE_HX_STATUS in pl2303_startup.

  2.1 If the return message is "1", then the PL2303 is the existing
      TYPE_HX/ TYPE_01 series.
      The other settings in pl2303_startup are to continue execution.
  2.2 If the return message is "not 1", then the PL2303 is the new
      TYPE_HXN series.
      The other settings in pl2303_startup are ignored.
      (PL2303HXN will directly use the default value in the hardware,
       no need to add additional settings through the software)

3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
   down/up stream used by TYPE_HX.
   Therefore, we will also execute different instructions here.
   The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
   write 0x00 to reset down/up stream(TYPE_HXN).

4. In pl2303_set_termios: The UART flow control instructions used by
   TYPE_HXN/TYPE_HX/TYPE_01 are different.
   Therefore, we will also execute different instructions here.

5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
   from the vendor request instruction used by TYPE_HX/TYPE_01,
   it will also execute different instructions here.

6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
   Therefore, we will also execute different instructions here.

Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
---
changelog:
v6:
1. Modify pl2303_update_reg:TYPE_HXN used different register for flow control.
   Therefore, we will also execute different instructions here.
2. Modify define name: PL2303_HXN_RESET_DOWN_UPSTREAM to
   PL2303_HXN_RESET_CONTROL
3. Re-Sorting flow-control register definition by address.
4. Indent continuation lines at least tw tabs.

v5:
1. Modify pl2303_update_reg
2. add a patch version on subject
3. add a space after each colon at subject line
---
 drivers/usb/serial/pl2303.c | 127 +++++++++++++++++++++++++++++-------
 drivers/usb/serial/pl2303.h |   7 +-
 2 files changed, 108 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index d7abde14b3cf..d36d53b234c4 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
 	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
 	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
 	{ USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
@@ -130,9 +136,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 #define VENDOR_WRITE_REQUEST_TYPE	0x40
 #define VENDOR_WRITE_REQUEST		0x01
+#define VENDOR_WRITE_NREQUEST		0x80
 
 #define VENDOR_READ_REQUEST_TYPE	0xc0
 #define VENDOR_READ_REQUEST		0x01
+#define VENDOR_READ_NREQUEST		0x81
 
 #define UART_STATE_INDEX		8
 #define UART_STATE_MSR_MASK		0x8b
@@ -147,12 +155,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_CTS			0x80
 
 #define PL2303_FLOWCTRL_MASK		0xf0
+#define PL2303_HXN_FLOWCTRL_MASK	0x1C
+#define PL2303_READ_TYPE_HX_STATUS	0x8080
+
+#define PL2303_HXN_RESET_CONTROL	0x07
+#define PL2303_HXN_FLOWCTRL		0x0A
+#define PL2303_HXN_CTRL_XON_XOFF	0x0C
+#define PL2303_HXN_CTRL_RTS_CTS		0x18
+#define PL2303_HXN_CTRL_NONE		0x1C
+
 
 static void pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */
 	TYPE_HX,	/* HX version of the pl2303 chip */
+	TYPE_HXN,	/* HXN version of the pl2303 chip */
 	TYPE_COUNT
 };
 
@@ -184,16 +202,26 @@ static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
 	[TYPE_HX] = {
 		.max_baud_rate		= 12000000,
 	},
+	[TYPE_HXN] = {
+		.max_baud_rate		= 12000000,
+	},
 };
 
 static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
 							unsigned char buf[1])
 {
 	struct device *dev = &serial->interface->dev;
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 	int res;
+	u8 request;
+
+	if (spriv->type == &pl2303_type_data[TYPE_HXN])
+		request = VENDOR_READ_NREQUEST;
+	else
+		request = VENDOR_READ_REQUEST;
 
 	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
+			request, VENDOR_READ_REQUEST_TYPE,
 			value, 0, buf, 1, 100);
 	if (res != 1) {
 		dev_err(dev, "%s - failed to read [%04x]: %d\n", __func__,
@@ -212,12 +240,19 @@ static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
 static int pl2303_vendor_write(struct usb_serial *serial, u16 value, u16 index)
 {
 	struct device *dev = &serial->interface->dev;
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 	int res;
+	u8 request;
 
 	dev_dbg(dev, "%s - [%04x] = %02x\n", __func__, value, index);
 
+	if (spriv->type == &pl2303_type_data[TYPE_HXN])
+		request = VENDOR_WRITE_NREQUEST;
+	else
+		request = VENDOR_WRITE_REQUEST;
+
 	res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			VENDOR_WRITE_REQUEST, VENDOR_WRITE_REQUEST_TYPE,
+			request, VENDOR_WRITE_REQUEST_TYPE,
 			value, index, NULL, 0, 100);
 	if (res) {
 		dev_err(dev, "%s - failed to write [%04x]: %d\n", __func__,
@@ -232,12 +267,17 @@ static int pl2303_update_reg(struct usb_serial *serial, u8 reg, u8 mask, u8 val)
 {
 	int ret = 0;
 	u8 *buf;
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 
 	buf = kmalloc(1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = pl2303_vendor_read(serial, reg | 0x80, buf);
+	if (spriv->type == &pl2303_type_data[TYPE_HXN])
+		ret = pl2303_vendor_read(serial, reg, buf);
+	else
+		ret = pl2303_vendor_read(serial, reg | 0x80, buf);
+
 	if (ret)
 		goto out_free;
 
@@ -320,6 +360,7 @@ static int pl2303_startup(struct usb_serial *serial)
 	struct pl2303_serial_private *spriv;
 	enum pl2303_type type = TYPE_01;
 	unsigned char *buf;
+	int res;
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)
@@ -341,26 +382,37 @@ static int pl2303_startup(struct usb_serial *serial)
 		type = TYPE_01;		/* type 1 */
 	dev_dbg(&serial->interface->dev, "device type: %d\n", type);
 
+	if (type == TYPE_HX) {
+		res = usb_control_msg(serial->dev,
+				usb_rcvctrlpipe(serial->dev, 0),
+				VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
+				PL2303_READ_TYPE_HX_STATUS, 0, buf, 1, 100);
+		if (res != 1)
+			type = TYPE_HXN;
+	}
+
 	spriv->type = &pl2303_type_data[type];
 	spriv->quirks = (unsigned long)usb_get_serial_data(serial);
 	spriv->quirks |= spriv->type->quirks;
 
 	usb_set_serial_data(serial, spriv);
 
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_write(serial, 0x0404, 0);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_read(serial, 0x8383, buf);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_write(serial, 0x0404, 1);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_read(serial, 0x8383, buf);
-	pl2303_vendor_write(serial, 0, 1);
-	pl2303_vendor_write(serial, 1, 0);
-	if (spriv->quirks & PL2303_QUIRK_LEGACY)
-		pl2303_vendor_write(serial, 2, 0x24);
-	else
-		pl2303_vendor_write(serial, 2, 0x44);
+	if (type != TYPE_HXN) {
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_write(serial, 0x0404, 0);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_read(serial, 0x8383, buf);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_write(serial, 0x0404, 1);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_read(serial, 0x8383, buf);
+		pl2303_vendor_write(serial, 0, 1);
+		pl2303_vendor_write(serial, 1, 0);
+		if (spriv->quirks & PL2303_QUIRK_LEGACY)
+			pl2303_vendor_write(serial, 2, 0x24);
+		else
+			pl2303_vendor_write(serial, 2, 0x44);
+	}
 
 	kfree(buf);
 
@@ -719,14 +771,34 @@ static void pl2303_set_termios(struct tty_struct *tty,
 	}
 
 	if (C_CRTSCTS(tty)) {
-		if (spriv->quirks & PL2303_QUIRK_LEGACY)
-			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
-		else
-			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x60);
+		if (spriv->quirks & PL2303_QUIRK_LEGACY) {
+			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
+				0x40);
+		} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
+			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
+				PL2303_HXN_FLOWCTRL_MASK,
+				PL2303_HXN_CTRL_RTS_CTS);
+		} else {
+			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
+				0x60);
+		}
 	} else if (pl2303_enable_xonxoff(tty, spriv->type)) {
-		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0xc0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
+			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
+				PL2303_HXN_FLOWCTRL_MASK,
+				PL2303_HXN_CTRL_XON_XOFF);
+		} else {
+			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
+				0xc0);
+		}
 	} else {
-		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
+			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
+				PL2303_HXN_FLOWCTRL_MASK,
+				PL2303_HXN_CTRL_NONE);
+		} else {
+			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
+		}
 	}
 
 	kfree(buf);
@@ -767,8 +839,13 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
 		usb_clear_halt(serial->dev, port->read_urb->pipe);
 	} else {
 		/* reset upstream data pipes */
-		pl2303_vendor_write(serial, 8, 0);
-		pl2303_vendor_write(serial, 9, 0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
+			pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
+				0);
+		} else {
+			pl2303_vendor_write(serial, 8, 0);
+			pl2303_vendor_write(serial, 9, 0);
+		}
 	}
 
 	/* Setup termios */
diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
index b0175f17d1a2..4a4a50586d5c 100644
--- a/drivers/usb/serial/pl2303.h
+++ b/drivers/usb/serial/pl2303.h
@@ -20,7 +20,12 @@
 #define PL2303_PRODUCT_ID_HCR331	0x331a
 #define PL2303_PRODUCT_ID_MOTOROLA	0x0307
 #define PL2303_PRODUCT_ID_ZTEK		0xe1f1
-
+#define PL2303_PRODUCT_ID_GC		0x23A3
+#define PL2303_PRODUCT_ID_GB		0x23B3
+#define PL2303_PRODUCT_ID_GT		0x23C3
+#define PL2303_PRODUCT_ID_GL		0x23D3
+#define PL2303_PRODUCT_ID_GE		0x23E3
+#define PL2303_PRODUCT_ID_GS		0x23F3
 
 #define ATEN_VENDOR_ID		0x0557
 #define ATEN_VENDOR_ID2		0x0547
-- 
2.20.1

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold]

On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN
> PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
> the existing PL2303 series (TYPE_HX & TYPE_01).
> Therefore, different Vendor requests are used to issue related commands.
> 
> 1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
>    new Vendor request,new flow control and other related instructions
>    if TYPE_HXN is recognized.
> 
> 2. Because the new PL2303HXN only accept the new Vendor request,
>    the old Vendor request cannot be accepted (the error message
>    will be returned)
>    So first determine the TYPE_HX or TYPE_HXN through
>    PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
> 
>   2.1 If the return message is "1", then the PL2303 is the existing
>       TYPE_HX/ TYPE_01 series.
>       The other settings in pl2303_startup are to continue execution.
>   2.2 If the return message is "not 1", then the PL2303 is the new
>       TYPE_HXN series.
>       The other settings in pl2303_startup are ignored.
>       (PL2303HXN will directly use the default value in the hardware,
>        no need to add additional settings through the software)
> 
> 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
>    down/up stream used by TYPE_HX.
>    Therefore, we will also execute different instructions here.
>    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
>    write 0x00 to reset down/up stream(TYPE_HXN).
> 
> 4. In pl2303_set_termios: The UART flow control instructions used by
>    TYPE_HXN/TYPE_HX/TYPE_01 are different.
>    Therefore, we will also execute different instructions here.
> 
> 5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
>    from the vendor request instruction used by TYPE_HX/TYPE_01,
>    it will also execute different instructions here.
> 
> 6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
>    Therefore, we will also execute different instructions here.
> 
> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
> changelog:
> v6:
> 1. Modify pl2303_update_reg:TYPE_HXN used different register for flow control.
>    Therefore, we will also execute different instructions here.
> 2. Modify define name: PL2303_HXN_RESET_DOWN_UPSTREAM to
>    PL2303_HXN_RESET_CONTROL
> 3. Re-Sorting flow-control register definition by address.
> 4. Indent continuation lines at least tw tabs.
> 
> v5:
> 1. Modify pl2303_update_reg
> 2. add a patch version on subject
> 3. add a space after each colon at subject line
> ---
>  drivers/usb/serial/pl2303.c | 127 +++++++++++++++++++++++++++++-------
>  drivers/usb/serial/pl2303.h |   7 +-
>  2 files changed, 108 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index d7abde14b3cf..d36d53b234c4 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
>  	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
>  	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
>  	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
>  	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
>  	{ USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
> @@ -130,9 +136,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  
>  #define VENDOR_WRITE_REQUEST_TYPE	0x40
>  #define VENDOR_WRITE_REQUEST		0x01
> +#define VENDOR_WRITE_NREQUEST		0x80
>  
>  #define VENDOR_READ_REQUEST_TYPE	0xc0
>  #define VENDOR_READ_REQUEST		0x01
> +#define VENDOR_READ_NREQUEST		0x81
>  
>  #define UART_STATE_INDEX		8
>  #define UART_STATE_MSR_MASK		0x8b
> @@ -147,12 +155,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_CTS			0x80
>  
>  #define PL2303_FLOWCTRL_MASK		0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK	0x1C

Move after PL2303_HXN_RESET_CONTROL as I suggested.

> +#define PL2303_READ_TYPE_HX_STATUS	0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL	0x07
> +#define PL2303_HXN_FLOWCTRL		0x0A
> +#define PL2303_HXN_CTRL_XON_XOFF	0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS		0x18
> +#define PL2303_HXN_CTRL_NONE		0x1C

> @@ -719,14 +771,34 @@ static void pl2303_set_termios(struct tty_struct *tty,
>  	}
>  
>  	if (C_CRTSCTS(tty)) {
> -		if (spriv->quirks & PL2303_QUIRK_LEGACY)
> -			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
> -		else
> -			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x60);
> +		if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0x40);

No need to break this line even if you end up with slightly more than 80
chars.

> +		} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_RTS_CTS);

Again, continuation lines should be indented at least two tabs further
(you only use one tab now).

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0x60);

No need to break line.

> +		}
>  	} else if (pl2303_enable_xonxoff(tty, spriv->type)) {
> -		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0xc0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_XON_XOFF);

Indentation.

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0xc0);

No need to break line.

> +		}
>  	} else {
> -		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_NONE);

Indentation.

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> +		}
>  	}
>  
>  	kfree(buf);
> @@ -767,8 +839,13 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
>  		usb_clear_halt(serial->dev, port->read_urb->pipe);
>  	} else {
>  		/* reset upstream data pipes */
> -		pl2303_vendor_write(serial, 8, 0);
> -		pl2303_vendor_write(serial, 9, 0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {

Merge this with else above to reduce indentation, that is

	} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {

> +			pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> +				0);

You again completely ignored my question about why you're wring 0
instead of 3 here.

I'll ignore your patch until you explain.

> +		} else {
> +			pl2303_vendor_write(serial, 8, 0);
> +			pl2303_vendor_write(serial, 9, 0);
> +		}
>  	}

Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold]

On Mon, Jul 01, 2019 at 03:32:33PM +0200, Johan Hovold wrote:
> On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote:
 
> >  #define PL2303_FLOWCTRL_MASK		0xf0
> > +#define PL2303_HXN_FLOWCTRL_MASK	0x1C
> 
> Move after PL2303_HXN_RESET_CONTROL as I suggested.

Sorry, I meant PL2303_HXN_FLOWCTRL here.

> > +#define PL2303_READ_TYPE_HX_STATUS	0x8080
> > +
> > +#define PL2303_HXN_RESET_CONTROL	0x07
> > +#define PL2303_HXN_FLOWCTRL		0x0A
> > +#define PL2303_HXN_CTRL_XON_XOFF	0x0C
> > +#define PL2303_HXN_CTRL_RTS_CTS		0x18
> > +#define PL2303_HXN_CTRL_NONE		0x1C

Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

> > +             if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > +                             0x40);
>
> No need to break this line even if you end up with slightly more than 80
> chars.
>
OK. I will ignore "WARNING: line over 80 characters".


> > +             } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > +                             PL2303_HXN_FLOWCTRL_MASK,
> > +                             PL2303_HXN_CTRL_RTS_CTS);
>
> Again, continuation lines should be indented at least two tabs further
> (you only use one tab now).
>

I have done it the way you do today...but after checking
./scripts/checkpatch.pl.. I got another warning message...
So I am a little confused now...
Previously before submitting.. must first pass ./scripts/checkpatch.pl
check. No ERROR, or WARRING message...

I will return to the office tomorrow ... I will post another warning
message (according to the way you mentioned)


>         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>
> > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > +                             0);
>
> You again completely ignored my question about why you're wring 0
> instead of 3 here.
>
> I'll ignore your patch until you explain.

3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
   down/up stream used by TYPE_HX.
   Therefore, we will also execute different instructions here.
   The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
   write 0x00 to reset down/up stream(TYPE_HXN).

Charles.

Johan Hovold <johan@kernel.org> 於 2019年7月1日 週一 下午9:32寫道：
>
> On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote:
> > Prolific has developed a new USB to UART chip: PL2303HXN
> > PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> > The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
> > the existing PL2303 series (TYPE_HX & TYPE_01).
> > Therefore, different Vendor requests are used to issue related commands.
> >
> > 1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
> >    new Vendor request,new flow control and other related instructions
> >    if TYPE_HXN is recognized.
> >
> > 2. Because the new PL2303HXN only accept the new Vendor request,
> >    the old Vendor request cannot be accepted (the error message
> >    will be returned)
> >    So first determine the TYPE_HX or TYPE_HXN through
> >    PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
> >
> >   2.1 If the return message is "1", then the PL2303 is the existing
> >       TYPE_HX/ TYPE_01 series.
> >       The other settings in pl2303_startup are to continue execution.
> >   2.2 If the return message is "not 1", then the PL2303 is the new
> >       TYPE_HXN series.
> >       The other settings in pl2303_startup are ignored.
> >       (PL2303HXN will directly use the default value in the hardware,
> >        no need to add additional settings through the software)
> >
> > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
> >    down/up stream used by TYPE_HX.
> >    Therefore, we will also execute different instructions here.
> >    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
> >    write 0x00 to reset down/up stream(TYPE_HXN).
> >
> > 4. In pl2303_set_termios: The UART flow control instructions used by
> >    TYPE_HXN/TYPE_HX/TYPE_01 are different.
> >    Therefore, we will also execute different instructions here.
> >
> > 5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
> >    from the vendor request instruction used by TYPE_HX/TYPE_01,
> >    it will also execute different instructions here.
> >
> > 6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
> >    Therefore, we will also execute different instructions here.
> >
> > Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> > ---
> > changelog:
> > v6:
> > 1. Modify pl2303_update_reg:TYPE_HXN used different register for flow control.
> >    Therefore, we will also execute different instructions here.
> > 2. Modify define name: PL2303_HXN_RESET_DOWN_UPSTREAM to
> >    PL2303_HXN_RESET_CONTROL
> > 3. Re-Sorting flow-control register definition by address.
> > 4. Indent continuation lines at least tw tabs.
> >
> > v5:
> > 1. Modify pl2303_update_reg
> > 2. add a patch version on subject
> > 3. add a space after each colon at subject line
> > ---
> >  drivers/usb/serial/pl2303.c | 127 +++++++++++++++++++++++++++++-------
> >  drivers/usb/serial/pl2303.h |   7 +-
> >  2 files changed, 108 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index d7abde14b3cf..d36d53b234c4 100644
> > --- a/drivers/usb/serial/pl2303.c
> > +++ b/drivers/usb/serial/pl2303.c
> > @@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
> >       { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
> >       { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
> >       { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
> >       { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
> >       { USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
> >       { USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
> > @@ -130,9 +136,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
> >
> >  #define VENDOR_WRITE_REQUEST_TYPE    0x40
> >  #define VENDOR_WRITE_REQUEST         0x01
> > +#define VENDOR_WRITE_NREQUEST                0x80
> >
> >  #define VENDOR_READ_REQUEST_TYPE     0xc0
> >  #define VENDOR_READ_REQUEST          0x01
> > +#define VENDOR_READ_NREQUEST         0x81
> >
> >  #define UART_STATE_INDEX             8
> >  #define UART_STATE_MSR_MASK          0x8b
> > @@ -147,12 +155,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
> >  #define UART_CTS                     0x80
> >
> >  #define PL2303_FLOWCTRL_MASK         0xf0
> > +#define PL2303_HXN_FLOWCTRL_MASK     0x1C
>
> Move after PL2303_HXN_RESET_CONTROL as I suggested.
>
> > +#define PL2303_READ_TYPE_HX_STATUS   0x8080
> > +
> > +#define PL2303_HXN_RESET_CONTROL     0x07
> > +#define PL2303_HXN_FLOWCTRL          0x0A
> > +#define PL2303_HXN_CTRL_XON_XOFF     0x0C
> > +#define PL2303_HXN_CTRL_RTS_CTS              0x18
> > +#define PL2303_HXN_CTRL_NONE         0x1C
>
> > @@ -719,14 +771,34 @@ static void pl2303_set_termios(struct tty_struct *tty,
> >       }
> >
> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
> > -             else
> > -                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x60);
> > +             if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > +                             0x40);
>
> No need to break this line even if you end up with slightly more than 80
> chars.
>
> > +             } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > +                             PL2303_HXN_FLOWCTRL_MASK,
> > +                             PL2303_HXN_CTRL_RTS_CTS);
>
> Again, continuation lines should be indented at least two tabs further
> (you only use one tab now).
>
> > +             } else {
> > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > +                             0x60);
>
> No need to break line.
>
> > +             }
> >       } else if (pl2303_enable_xonxoff(tty, spriv->type)) {
> > -             pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0xc0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > +                             PL2303_HXN_FLOWCTRL_MASK,
> > +                             PL2303_HXN_CTRL_XON_XOFF);
>
> Indentation.
>
> > +             } else {
> > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > +                             0xc0);
>
> No need to break line.
>
> > +             }
> >       } else {
> > -             pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > +                             PL2303_HXN_FLOWCTRL_MASK,
> > +                             PL2303_HXN_CTRL_NONE);
>
> Indentation.
>
> > +             } else {
> > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> > +             }
> >       }
> >
> >       kfree(buf);
> > @@ -767,8 +839,13 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
> >               usb_clear_halt(serial->dev, port->read_urb->pipe);
> >       } else {
> >               /* reset upstream data pipes */
> > -             pl2303_vendor_write(serial, 8, 0);
> > -             pl2303_vendor_write(serial, 9, 0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>
> Merge this with else above to reduce indentation, that is
>
>         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>
> > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > +                             0);
>
> You again completely ignored my question about why you're wring 0
> instead of 3 here.
>
> I'll ignore your patch until you explain.
>
> > +             } else {
> > +                     pl2303_vendor_write(serial, 8, 0);
> > +                     pl2303_vendor_write(serial, 9, 0);
> > +             }
> >       }
>
> Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold]

On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:
> > > +             if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> > > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > > +                             0x40);
> >
> > No need to break this line even if you end up with slightly more than 80
> > chars.
> >
> OK. I will ignore "WARNING: line over 80 characters".

Yeah, it's ok to go slightly above 80 chars when it improves readability.

> > > +             } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > > +                             PL2303_HXN_FLOWCTRL_MASK,
> > > +                             PL2303_HXN_CTRL_RTS_CTS);
> >
> > Again, continuation lines should be indented at least two tabs further
> > (you only use one tab now).
> >
> 
> I have done it the way you do today...but after checking
> ./scripts/checkpatch.pl.. I got another warning message...
> So I am a little confused now...
> Previously before submitting.. must first pass ./scripts/checkpatch.pl
> check. No ERROR, or WARRING message...
> 
> I will return to the office tomorrow ... I will post another warning
> message (according to the way you mentioned)

checkpatch isn't always right. Just remember to indent continuation
lines at least two tabs further, such as

	pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
			PL2303_HXN_FLOWCTRL_MASK,
			PL2303_HXN_CTRL_RTS_CTS);

> >         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> >
> > > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > > +                             0);
> >
> > You again completely ignored my question about why you're wring 0
> > instead of 3 here.
> >
> > I'll ignore your patch until you explain.
> 
> 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
>    down/up stream used by TYPE_HX.
>    Therefore, we will also execute different instructions here.
>    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
>    write 0x00 to reset down/up stream(TYPE_HXN).

I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
documentation said bit 0 and 1 are used to reset the up and downstream
pipes.

To be more specific; what happens if I

	1. set bit 0
	2. clear bit 0?

and leave the other bits alone (write back the same value, e.g. 0xfe).

Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

>
>         pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
>                         PL2303_HXN_FLOWCTRL_MASK,
>                         PL2303_HXN_CTRL_RTS_CTS);
>

Indent continuation lines at least two tabs<<--Done.

> I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
> documentation said bit 0 and 1 are used to reset the up and downstream
> pipes.
>
> To be more specific; what happens if I
>
>         1. set bit 0
>         2. clear bit 0?
>
> and leave the other bits alone (write back the same value, e.g. 0xfe).
>

You are right..
set "1" is reset.
set "0" is nothing.

I have used pl2303_update_reg instead pl2303_vendor_write which to reset
the upstream and downstream pipe data

Charles.


Johan Hovold <johan@kernel.org> 於 2019年7月1日 週一 下午11:29寫道：
>
> On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:
> > > > +             if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> > > > +                     pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> > > > +                             0x40);
> > >
> > > No need to break this line even if you end up with slightly more than 80
> > > chars.
> > >
> > OK. I will ignore "WARNING: line over 80 characters".
>
> Yeah, it's ok to go slightly above 80 chars when it improves readability.
>
> > > > +             } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > > +                     pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> > > > +                             PL2303_HXN_FLOWCTRL_MASK,
> > > > +                             PL2303_HXN_CTRL_RTS_CTS);
> > >
> > > Again, continuation lines should be indented at least two tabs further
> > > (you only use one tab now).
> > >
> >
> > I have done it the way you do today...but after checking
> > ./scripts/checkpatch.pl.. I got another warning message...
> > So I am a little confused now...
> > Previously before submitting.. must first pass ./scripts/checkpatch.pl
> > check. No ERROR, or WARRING message...
> >
> > I will return to the office tomorrow ... I will post another warning
> > message (according to the way you mentioned)
>
> checkpatch isn't always right. Just remember to indent continuation
> lines at least two tabs further, such as
>
>         pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
>                         PL2303_HXN_FLOWCTRL_MASK,
>                         PL2303_HXN_CTRL_RTS_CTS);
>
> > >         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > >
> > > > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > > > +                             0);
> > >
> > > You again completely ignored my question about why you're wring 0
> > > instead of 3 here.
> > >
> > > I'll ignore your patch until you explain.
> >
> > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
> >    down/up stream used by TYPE_HX.
> >    Therefore, we will also execute different instructions here.
> >    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
> >    write 0x00 to reset down/up stream(TYPE_HXN).
>
> I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
> documentation said bit 0 and 1 are used to reset the up and downstream
> pipes.
>
> To be more specific; what happens if I
>
>         1. set bit 0
>         2. clear bit 0?
>
> and leave the other bits alone (write back the same value, e.g. 0xfe).
>
> Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold]

[ Please do not top post, reply inline and trim irrelevant context.
  Specifically do not copy context to the top of the mail.

  Reordering your reply below. ]

On Wed, Jul 03, 2019 at 12:22:45AM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年7月1日 週一 下午11:29寫道：
> > On Mon, Jul 01, 2019 at 11:11:02PM +0800, Charles Yeh wrote:

> > > >         } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > > > > +                     pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> > > > > +                             0);
> > > >
> > > > You again completely ignored my question about why you're wring 0
> > > > instead of 3 here.
> > > >
> > > > I'll ignore your patch until you explain.
> > >
> > > 3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
> > >    down/up stream used by TYPE_HX.
> > >    Therefore, we will also execute different instructions here.
> > >    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
> > >    write 0x00 to reset down/up stream(TYPE_HXN).
> >
> > I'm asking why you write the value 0 instead of 3 (or say, 0xfc)? Your
> > documentation said bit 0 and 1 are used to reset the up and downstream
> > pipes.
> >
> > To be more specific; what happens if I
> >
> >         1. set bit 0
> >         2. clear bit 0?
> >
> > and leave the other bits alone (write back the same value, e.g. 0xfe).

> You are right..
> set "1" is reset.
> set "0" is nothing.
> 
> I have used pl2303_update_reg instead pl2303_vendor_write which to reset
> the upstream and downstream pipe data

Ok, thanks for confirming. Note that I asked you about this back in
April (15 April).

In that very same mail thread I also pointed out that you must not
ignore review feedback. Even if for some reason disagree with it you
should at least explain why.

Johan

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

I will first learn about inline gmail setting.. and then reply to you.

Charles.

Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年7月16日 週二 下午4:33寫道：
>
> Ok, thanks for confirming. Note that I asked you about this back in
> April (15 April).
>
> In that very same mail thread I also pointed out that you must not
> ignore review feedback. Even if for some reason disagree with it you
> should at least explain why.
>

I want to explain or answer your question..
but I am not familiar with using inline gmail
I will try to reply to your question using inline gmail later..

Charles.