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

[PATCH] [PATCH v7] 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.

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:
v7:
1. Add PL2303_HXN_RESET_CONTROL_MASK define.
2. In pl2303_open,use PL2303_HXN_RESET_CONTROL_MASK & PL2303_HXN_RESET_CONTROL
   to reset the upstream and downstream pipe data
3. Ignore "WARNING: line over 80 characters" at #776,#782,#790

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.
5. In pl2303_open,modify reset the upstream and downstream pipe data: 0x00 to
   0x03

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 | 117 +++++++++++++++++++++++++++++-------
 drivers/usb/serial/pl2303.h |   7 ++-
 2 files changed, 101 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index d7abde14b3cf..b6dfddb7a154 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,23 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_CTS			0x80
 
 #define PL2303_FLOWCTRL_MASK		0xf0
+#define PL2303_HXN_FLOWCTRL_MASK	0x1C
+#define PL2303_HXN_FLOWCTRL		0x0A
+#define PL2303_READ_TYPE_HX_STATUS	0x8080
+
+#define PL2303_HXN_RESET_CONTROL_MASK	0x03
+#define PL2303_HXN_RESET_CONTROL	0x07
+#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 +203,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 +241,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 +268,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 +361,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 +383,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 +772,31 @@ static void pl2303_set_termios(struct tty_struct *tty,
 	}
 
 	if (C_CRTSCTS(tty)) {
-		if (spriv->quirks & PL2303_QUIRK_LEGACY)
+		if (spriv->quirks & PL2303_QUIRK_LEGACY) {
 			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
-		else
+		} 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);
@@ -765,8 +835,11 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (spriv->quirks & PL2303_QUIRK_LEGACY) {
 		usb_clear_halt(serial->dev, port->write_urb->pipe);
 		usb_clear_halt(serial->dev, port->read_urb->pipe);
-	} else {
+	} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
 		/* reset upstream data pipes */
+		pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
+				PL2303_HXN_RESET_CONTROL_MASK, 0x03);
+	} else {
 		pl2303_vendor_write(serial, 8, 0);
 		pl2303_vendor_write(serial, 9, 0);
 	}
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 v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Charles Yeh]

Is there any need to modify it?
If there is no need to modify, how long does it take to complete REVIEW?

Charles Yeh

Charles Yeh <charlesyeh522@gmail.com> 於 2019年7月2日 週二 下午8:30寫道：
>
> 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.
>
> 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:
> v7:
> 1. Add PL2303_HXN_RESET_CONTROL_MASK define.
> 2. In pl2303_open,use PL2303_HXN_RESET_CONTROL_MASK & PL2303_HXN_RESET_CONTROL
>    to reset the upstream and downstream pipe data
> 3. Ignore "WARNING: line over 80 characters" at #776,#782,#790
>
> 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.
> 5. In pl2303_open,modify reset the upstream and downstream pipe data: 0x00 to
>    0x03
>
> 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 | 117 +++++++++++++++++++++++++++++-------
>  drivers/usb/serial/pl2303.h |   7 ++-
>  2 files changed, 101 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index d7abde14b3cf..b6dfddb7a154 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,23 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_CTS                       0x80
>
>  #define PL2303_FLOWCTRL_MASK           0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK       0x1C
> +#define PL2303_HXN_FLOWCTRL            0x0A
> +#define PL2303_READ_TYPE_HX_STATUS     0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL_MASK  0x03
> +#define PL2303_HXN_RESET_CONTROL       0x07
> +#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 +203,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 +241,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 +268,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 +361,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 +383,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 +772,31 @@ static void pl2303_set_termios(struct tty_struct *tty,
>         }
>
>         if (C_CRTSCTS(tty)) {
> -               if (spriv->quirks & PL2303_QUIRK_LEGACY)
> +               if (spriv->quirks & PL2303_QUIRK_LEGACY) {
>                         pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
> -               else
> +               } 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);
> @@ -765,8 +835,11 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
>         if (spriv->quirks & PL2303_QUIRK_LEGACY) {
>                 usb_clear_halt(serial->dev, port->write_urb->pipe);
>                 usb_clear_halt(serial->dev, port->read_urb->pipe);
> -       } else {
> +       } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>                 /* reset upstream data pipes */
> +               pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> +                               PL2303_HXN_RESET_CONTROL_MASK, 0x03);
> +       } else {
>                 pl2303_vendor_write(serial, 8, 0);
>                 pl2303_vendor_write(serial, 9, 0);
>         }
> 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 v7] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Fri, Jul 05, 2019 at 10:57:11AM +0800, Charles Yeh wrote:
> Is there any need to modify it?
> If there is no need to modify, how long does it take to complete REVIEW?

Please be patient, only ask after a week or two, all of us have lots and
lots of other patches to also review.

thanks,

greg k-h

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

[Johan Hovold]

On Tue, Jul 02, 2019 at 08:30:06PM +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.

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
> changelog:
> v7:
> 1. Add PL2303_HXN_RESET_CONTROL_MASK define.
> 2. In pl2303_open,use PL2303_HXN_RESET_CONTROL_MASK & PL2303_HXN_RESET_CONTROL
>    to reset the upstream and downstream pipe data
> 3. Ignore "WARNING: line over 80 characters" at #776,#782,#790
 
>  #define PL2303_FLOWCTRL_MASK		0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK	0x1C
> +#define PL2303_HXN_FLOWCTRL		0x0A

I asked you to keep related defines together (and to move the mask where
the register define was, not the other way round). Please move these to
the other HXN defines below, and keep the register address defines
before the corresponding bit defines.

> +#define PL2303_READ_TYPE_HX_STATUS	0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL_MASK	0x03

This makes no sense. The whole register is used for reset. If you want a
define that can be used for resetting both pipes then add two separate
defines for up and down respectively, and add a third define for
resetting both buffer as a bitwise OR of the two.

Remember that the code should be self-documenting as far as possible so
picking descriptive names is important.

Also move this one after the corresponding register address define
below.

> +#define PL2303_HXN_RESET_CONTROL	0x07
> +#define PL2303_HXN_CTRL_XON_XOFF	0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS		0x18
> +#define PL2303_HXN_CTRL_NONE		0x1C

> @@ -765,8 +835,11 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	if (spriv->quirks & PL2303_QUIRK_LEGACY) {
>  		usb_clear_halt(serial->dev, port->write_urb->pipe);
>  		usb_clear_halt(serial->dev, port->read_urb->pipe);
> -	} else {
> +	} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>  		/* reset upstream data pipes */

This comment belongs in the last else block. Your new code shouldn't
need one.

> +		pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> +				PL2303_HXN_RESET_CONTROL_MASK, 0x03);

So two things; first, do you really need to read back the current value?
I would assume that it always reads back as 0 and that writing 0x03 in
this case would be sufficient to reset both buffers.

Second, please use a define for 0x03; no magic constants, as we have
discussed before. You don't need a separate mask define if you're always
resetting both buffers together (just use the same value define twice).

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

Johan

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

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年7月16日 週二 下午4:49寫道：
> >  #define PL2303_FLOWCTRL_MASK         0xf0
> > +#define PL2303_HXN_FLOWCTRL_MASK     0x1C
> > +#define PL2303_HXN_FLOWCTRL          0x0A
>
> I asked you to keep related defines together (and to move the mask where
> the register define was, not the other way round). Please move these to
> the other HXN defines below, and keep the register address defines
> before the corresponding bit defines.

Charles Ans:
I am not 100% sure what you mean, please see if it is defined below

#define PL2303_FLOWCTRL_MASK        0xf0

#define PL2303_READ_TYPE_HX_STATUS    0x8080

#define PL2303_HXN_CTRL_XON_XOFF    0x0C
#define PL2303_HXN_CTRL_RTS_CTS        0x18
#define PL2303_HXN_CTRL_NONE        0x1C
#define PL2303_HXN_FLOWCTRL_MASK    0x1C
#define PL2303_HXN_FLOWCTRL        0x0A

#define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
#define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
#define PL2303_HXN_RESET_CONTROL    0x07

> > +
> > +#define PL2303_HXN_RESET_CONTROL_MASK        0x03
> This makes no sense. The whole register is used for reset. If you want a
> define that can be used for resetting both pipes then add two separate
> defines for up and down respectively, and add a third define for
> resetting both buffer as a bitwise OR of the two.

Charles Ans:
Yes,The whole register is used for reset.
Bit 0 and bit 1 are used for up & downstream data pipe,
Bit 2 for interface reset
Bit 4 for chip reset.

But I only reset bit 0 & bit 1.


> Also move this one after the corresponding register address define
> below.
>
> > +#define PL2303_HXN_RESET_CONTROL     0x07
> > +#define PL2303_HXN_CTRL_XON_XOFF     0x0C
> > +#define PL2303_HXN_CTRL_RTS_CTS              0x18
> > +#define PL2303_HXN_CTRL_NONE         0x1C

Charles Ans:
I am not 100% sure what you mean, please see if it is defined below

#define PL2303_FLOWCTRL_MASK        0xf0

#define PL2303_READ_TYPE_HX_STATUS    0x8080

#define PL2303_HXN_CTRL_XON_XOFF    0x0C
#define PL2303_HXN_CTRL_RTS_CTS        0x18
#define PL2303_HXN_CTRL_NONE        0x1C
#define PL2303_HXN_FLOWCTRL_MASK    0x1C
#define PL2303_HXN_FLOWCTRL        0x0A

#define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
#define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
#define PL2303_HXN_RESET_CONTROL    0x07

> > +     } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> >               /* reset upstream data pipes */
>
> This comment belongs in the last else block. Your new code shouldn't
> need one.

Charles Ans:
OK, I will remove this comment.


>
> > +             pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> > +                             PL2303_HXN_RESET_CONTROL_MASK, 0x03);
>
> So two things; first, do you really need to read back the current value?
> I would assume that it always reads back as 0 and that writing 0x03 in
> this case would be sufficient to reset both buffers.
>

Charles Ans:
 Yes, I want to read back the current value.
because the whole register is used for reset.
Bit 0 and bit 1 are used for up & downstream data pipe,
Bit 2 for interface reset
Bit 4 for chip reset.

But I only reset bit 0 & bit 1.

> Second, please use a define for 0x03; no magic constants, as we have
> discussed before. You don't need a separate mask define if you're always
> resetting both buffers together (just use the same value define twice).

Charles Ans:
OK, I will define for 0x03.

#define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03


Charles Yeh.

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

[Charles Yeh]

Hello ,
       Any update on this?

Charles.

Charles Yeh <charlesyeh522@gmail.com> 於 2019年8月27日 週二 下午4:40寫道：
>
> Johan Hovold <johan@kernel.org> 於 2019年7月16日 週二 下午4:49寫道：
> > >  #define PL2303_FLOWCTRL_MASK         0xf0
> > > +#define PL2303_HXN_FLOWCTRL_MASK     0x1C
> > > +#define PL2303_HXN_FLOWCTRL          0x0A
> >
> > I asked you to keep related defines together (and to move the mask where
> > the register define was, not the other way round). Please move these to
> > the other HXN defines below, and keep the register address defines
> > before the corresponding bit defines.
>
> Charles Ans:
> I am not 100% sure what you mean, please see if it is defined below
>
> #define PL2303_FLOWCTRL_MASK        0xf0
>
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
>
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A
>
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07
>
> > > +
> > > +#define PL2303_HXN_RESET_CONTROL_MASK        0x03
> > This makes no sense. The whole register is used for reset. If you want a
> > define that can be used for resetting both pipes then add two separate
> > defines for up and down respectively, and add a third define for
> > resetting both buffer as a bitwise OR of the two.
>
> Charles Ans:
> Yes,The whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
>
> But I only reset bit 0 & bit 1.
>
>
> > Also move this one after the corresponding register address define
> > below.
> >
> > > +#define PL2303_HXN_RESET_CONTROL     0x07
> > > +#define PL2303_HXN_CTRL_XON_XOFF     0x0C
> > > +#define PL2303_HXN_CTRL_RTS_CTS              0x18
> > > +#define PL2303_HXN_CTRL_NONE         0x1C
>
> Charles Ans:
> I am not 100% sure what you mean, please see if it is defined below
>
> #define PL2303_FLOWCTRL_MASK        0xf0
>
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
>
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A
>
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07
>
> > > +     } else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> > >               /* reset upstream data pipes */
> >
> > This comment belongs in the last else block. Your new code shouldn't
> > need one.
>
> Charles Ans:
> OK, I will remove this comment.
>
>
> >
> > > +             pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> > > +                             PL2303_HXN_RESET_CONTROL_MASK, 0x03);
> >
> > So two things; first, do you really need to read back the current value?
> > I would assume that it always reads back as 0 and that writing 0x03 in
> > this case would be sufficient to reset both buffers.
> >
>
> Charles Ans:
>  Yes, I want to read back the current value.
> because the whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
>
> But I only reset bit 0 & bit 1.
>
> > Second, please use a define for 0x03; no magic constants, as we have
> > discussed before. You don't need a separate mask define if you're always
> > resetting both buffers together (just use the same value define twice).
>
> Charles Ans:
> OK, I will define for 0x03.
>
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
>
>
> Charles Yeh.

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

[Johan Hovold]

On Tue, Aug 27, 2019 at 04:40:39PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年7月16日 週二 下午4:49寫道：
> > >  #define PL2303_FLOWCTRL_MASK         0xf0
> > > +#define PL2303_HXN_FLOWCTRL_MASK     0x1C
> > > +#define PL2303_HXN_FLOWCTRL          0x0A
> >
> > I asked you to keep related defines together (and to move the mask where
> > the register define was, not the other way round). Please move these to
> > the other HXN defines below, and keep the register address defines
> > before the corresponding bit defines.
> 
> Charles Ans:

[ You don't need to prefix your replies like this, I can tell from the
  number of > signs. ]

> I am not 100% sure what you mean, please see if it is defined below
> 
> #define PL2303_FLOWCTRL_MASK        0xf0
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A

Yes, that's better, but you're mixing register addresses, bit values and
masks above. Perhaps things would be more clear if you but a _REG suffix
on the register defines and order things as follows:

	#define PL2303_HXN_<name1>_REG			0xX1
	#define PL2303_HXN_<name1>_<field>_MASK		0xY1
	#define PL2303_HXN_<name1>_<field>_<value>	0xZ1

	#define PL2303_HXN_<name2>_REG			0xX2
	#define PL2303_HXN_<name2>_<field>_MASK		0xY2
	#define PL2303_HXN_<name2>_<field>_<value>	0xZ2

The idea is simply to keep related defines together and not mix
register address, masks and value defines.

Keep registers sorted by address, and bit masks and values by bit order
(e.g. MSB first).

> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07
> 
> > > +
> > > +#define PL2303_HXN_RESET_CONTROL_MASK        0x03
> > This makes no sense. The whole register is used for reset. If you want a
> > define that can be used for resetting both pipes then add two separate
> > defines for up and down respectively, and add a third define for
> > resetting both buffer as a bitwise OR of the two.
> 
> Charles Ans:
> Yes,The whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
> 
> But I only reset bit 0 & bit 1.

Yes, but you need to reflect that in how you name your defines. Add two
separate defines for up and downstream data pipe reset. If you want you
add a third as the bitwise-OR of the two as well (perhaps with a _BOTH
suffix in the name).

> > Also move this one after the corresponding register address define
> > below.
> >
> > > +#define PL2303_HXN_RESET_CONTROL     0x07
> > > +#define PL2303_HXN_CTRL_XON_XOFF     0x0C
> > > +#define PL2303_HXN_CTRL_RTS_CTS              0x18
> > > +#define PL2303_HXN_CTRL_NONE         0x1C
> 
> Charles Ans:
> I am not 100% sure what you mean, please see if it is defined below
> 
> #define PL2303_FLOWCTRL_MASK        0xf0
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_CTRL_XON_XOFF    0x0C
> #define PL2303_HXN_CTRL_RTS_CTS        0x18
> #define PL2303_HXN_CTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL        0x0A
> 
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE_MASK    0x03
> #define PL2303_HXN_RESET_CONTROL    0x07

I meant that you should move the bit values (masks) after the register
address that they apply to (as also mentioned above). For example,

	#define PL2303_HXN_RESET_REG	 			0x07
	#define PL2303_HXN_RESET_UPSTREAM_PIPE			0x02
	#define PL2303_HXN_RESET_DOWNSTREAM_PIPE		0x01

> > > +             pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> > > +                             PL2303_HXN_RESET_CONTROL_MASK, 0x03);
> >
> > So two things; first, do you really need to read back the current value?
> > I would assume that it always reads back as 0 and that writing 0x03 in
> > this case would be sufficient to reset both buffers.
> >
> 
> Charles Ans:
>  Yes, I want to read back the current value.
> because the whole register is used for reset.
> Bit 0 and bit 1 are used for up & downstream data pipe,
> Bit 2 for interface reset
> Bit 4 for chip reset.
> 
> But I only reset bit 0 & bit 1.

Yes, but that doesn't imply that you need to read back the old value.

I'm assuming it would either always read back as 0, or you would read
back the previous value written, which means you could end up resetting
something you did not intend.

Either way, you should not read back the current value when resetting
the data pipes.

> > Second, please use a define for 0x03; no magic constants, as we have
> > discussed before. You don't need a separate mask define if you're always
> > resetting both buffers together (just use the same value define twice).
> 
> Charles Ans:
> OK, I will define for 0x03.
> 
> #define PL2303_HXN_RESET_UP_DOWNSTREAM_DATA_PIPE    0x03

As mentioned above, add separate defines for each pipe. You can also add
a third as their bitwise OR.

Johan

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

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年9月20日 週五 下午3:56寫道：
> Yes, that's better, but you're mixing register addresses, bit values and
> masks above. Perhaps things would be more clear if you but a _REG suffix
> on the register defines and order things as follows:
>
>         #define PL2303_HXN_<name1>_REG                  0xX1
>         #define PL2303_HXN_<name1>_<field>_MASK         0xY1
>         #define PL2303_HXN_<name1>_<field>_<value>      0xZ1
>
>         #define PL2303_HXN_<name2>_REG                  0xX2
>         #define PL2303_HXN_<name2>_<field>_MASK         0xY2
>         #define PL2303_HXN_<name2>_<field>_<value>      0xZ2
>
> The idea is simply to keep related defines together and not mix
> register address, masks and value defines.
>
> Keep registers sorted by address, and bit masks and values by bit order
> (e.g. MSB first).

Thank you for your reply

Charles Ans:
The new define is follows

#define PL2303_READ_TYPE_HX_STATUS    0x8080

#define PL2303_HXN_FLOWCTRL_REG        0x0A
#define PL2303_HXN_FLOWCTRL_MASK    0x1C
#define PL2303_HXN_FLOWCTRL_NONE        0x1C
#define PL2303_HXN_FLOWCTRL_RTS_CTS        0x18
#define PL2303_HXN_FLOWCTRL_XON_XOFF    0x0C

#define PL2303_HXN_RESET_REG    0x07
#define PL2303_HXN_RESET_UPSTREAM_PIPE    0x02
#define PL2303_HXN_RESET_DOWNSTREAM_PIPE    0x01


> Yes, but that doesn't imply that you need to read back the old value.
>
> I'm assuming it would either always read back as 0, or you would read
> back the previous value written, which means you could end up resetting
> something you did not intend.
>
> Either way, you should not read back the current value when resetting
> the data pipes.
>

Thank you for your reply

Charles Ans:
The new code is follows

    pl2303_vendor_write(serial,
                PL2303_HXN_RESET_REG,
                PL2303_HXN_RESET_UPSTREAM_PIPE |
PL2303_HXN_RESET_DOWNSTREAM_PIPE);


Please confirm the above new define & code..
If there is no problem.. I will write a new Patch file.

Charles.

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

[Johan Hovold]

On Mon, Sep 23, 2019 at 05:53:34PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年9月20日 週五 下午3:56寫道：
> > Yes, that's better, but you're mixing register addresses, bit values and
> > masks above. Perhaps things would be more clear if you but a _REG suffix
> > on the register defines and order things as follows:
> >
> >         #define PL2303_HXN_<name1>_REG                  0xX1
> >         #define PL2303_HXN_<name1>_<field>_MASK         0xY1
> >         #define PL2303_HXN_<name1>_<field>_<value>      0xZ1
> >
> >         #define PL2303_HXN_<name2>_REG                  0xX2
> >         #define PL2303_HXN_<name2>_<field>_MASK         0xY2
> >         #define PL2303_HXN_<name2>_<field>_<value>      0xZ2
> >
> > The idea is simply to keep related defines together and not mix
> > register address, masks and value defines.
> >
> > Keep registers sorted by address, and bit masks and values by bit order
> > (e.g. MSB first).
> 
> Thank you for your reply
> 
> Charles Ans:
> The new define is follows
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_FLOWCTRL_REG        0x0A
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_RTS_CTS        0x18
> #define PL2303_HXN_FLOWCTRL_XON_XOFF    0x0C
> 
> #define PL2303_HXN_RESET_REG    0x07
> #define PL2303_HXN_RESET_UPSTREAM_PIPE    0x02
> #define PL2303_HXN_RESET_DOWNSTREAM_PIPE    0x01

That looks much better. But please move the reset defines above the
flow control ones to keep the registers sorted by address (0x7 < 0xa).

> > Yes, but that doesn't imply that you need to read back the old value.
> >
> > I'm assuming it would either always read back as 0, or you would read
> > back the previous value written, which means you could end up resetting
> > something you did not intend.
> >
> > Either way, you should not read back the current value when resetting
> > the data pipes.
> >
> 
> Thank you for your reply
> 
> Charles Ans:
> The new code is follows
> 
>     pl2303_vendor_write(serial,
>                 PL2303_HXN_RESET_REG,
>                 PL2303_HXN_RESET_UPSTREAM_PIPE |
> PL2303_HXN_RESET_DOWNSTREAM_PIPE);
> 
> 
> Please confirm the above new define & code..
> If there is no problem.. I will write a new Patch file.

Also looks good, thanks. Just move the reset define block as mentioned
above.

Johan

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

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年9月23日 週一 下午6:24寫道：
> That looks much better. But please move the reset defines above the
> flow control ones to keep the registers sorted by address (0x7 < 0xa).

Thank you for your reply

Charles Ans:
The new define is follows

#define PL2303_READ_TYPE_HX_STATUS    0x8080

#define PL2303_HXN_RESET_REG    0x07
#define PL2303_HXN_RESET_UPSTREAM_PIPE    0x02
#define PL2303_HXN_RESET_DOWNSTREAM_PIPE    0x01

#define PL2303_HXN_FLOWCTRL_REG        0x0A
#define PL2303_HXN_FLOWCTRL_MASK    0x1C
#define PL2303_HXN_FLOWCTRL_NONE        0x1C
#define PL2303_HXN_FLOWCTRL_RTS_CTS        0x18
#define PL2303_HXN_FLOWCTRL_XON_XOFF    0x0C


> Also looks good, thanks. Just move the reset define block as mentioned
> above.

Thank you for your reply


Please confirm the above new define
If there is no problem.. I will write a new Patch file.

Charles.

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

[Johan Hovold]

On Mon, Sep 23, 2019 at 06:35:19PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年9月23日 週一 下午6:24寫道：
> > That looks much better. But please move the reset defines above the
> > flow control ones to keep the registers sorted by address (0x7 < 0xa).
> 
> Thank you for your reply
> 
> Charles Ans:
> The new define is follows
> 
> #define PL2303_READ_TYPE_HX_STATUS    0x8080
> 
> #define PL2303_HXN_RESET_REG    0x07
> #define PL2303_HXN_RESET_UPSTREAM_PIPE    0x02
> #define PL2303_HXN_RESET_DOWNSTREAM_PIPE    0x01
> 
> #define PL2303_HXN_FLOWCTRL_REG        0x0A
> #define PL2303_HXN_FLOWCTRL_MASK    0x1C
> #define PL2303_HXN_FLOWCTRL_NONE        0x1C
> #define PL2303_HXN_FLOWCTRL_RTS_CTS        0x18
> #define PL2303_HXN_FLOWCTRL_XON_XOFF    0x0C
> 
> 
> > Also looks good, thanks. Just move the reset define block as mentioned
> > above.
> 
> Thank you for your reply
> 
> 
> Please confirm the above new define
> If there is no problem.. I will write a new Patch file.

Yes, the above looks good.

Johan

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

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年9月23日 週一 下午9:08寫道：
>
> Yes, the above looks good.
>

Thank you for your reply

I have already written a new patch[v8] file,
if you have free time. Please check as soon as possible...thanks!

Charles.

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

[Johan Hovold]

On Wed, Sep 25, 2019 at 09:20:07AM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年9月23日 週一 下午9:08寫道：
> >
> > Yes, the above looks good.
> >
> 
> Thank you for your reply
> 
> I have already written a new patch[v8] file,
> if you have free time. Please check as soon as possible...thanks!

I'll start processing patches for 5.4 next week when the merge window
closes.

Meanwhile you can double check that you've considered all
review-feedback you've gotten so far.

I don't think you ever replied to my last comment about the reset
register and why I thought using plain write (not read, mask, write) was
the right thing to do.

Does the register always read back as 0, or does it read back as the
last value written?

Johan

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

[Charles Yeh]

Johan Hovold <johan@kernel.org> 於 2019年9月25日 週三 下午4:06寫道：
> Meanwhile you can double check that you've considered all
> review-feedback you've gotten so far.
>
> I don't think you ever replied to my last comment about the reset
> register and why I thought using plain write (not read, mask, write) was
> the right thing to do.
>
> Does the register always read back as 0, or does it read back as the
> last value written?

Charles Ans:
I just asked my colleague, who is RD of design PL2303 hardware,
His answer is to read 0 forever.

Does the register always read back as 0, or does it read back as the
last value written?
Ans: Yes, the register"#define PL2303_HXN_RESET_REG 0x07" always
read back as 0.

I hope the above content has an answer to your question:
If there are other questions, please try to raise it.. thanks

Charles.

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

[Johan Hovold]

On Wed, Sep 25, 2019 at 05:36:23PM +0800, Charles Yeh wrote:
> Johan Hovold <johan@kernel.org> 於 2019年9月25日 週三 下午4:06寫道：
> > Meanwhile you can double check that you've considered all
> > review-feedback you've gotten so far.
> >
> > I don't think you ever replied to my last comment about the reset
> > register and why I thought using plain write (not read, mask, write) was
> > the right thing to do.
> >
> > Does the register always read back as 0, or does it read back as the
> > last value written?
> 
> Charles Ans:
> I just asked my colleague, who is RD of design PL2303 hardware,
> His answer is to read 0 forever.
> 
> Does the register always read back as 0, or does it read back as the
> last value written?
> Ans: Yes, the register"#define PL2303_HXN_RESET_REG 0x07" always
> read back as 0.
> 
> I hope the above content has an answer to your question:
> If there are other questions, please try to raise it.. thanks

It does, thanks for confirming.

Johan