diff mbox series

USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)

Message ID 20190213123000.4656-1-charlesyeh522@gmail.com (mailing list archive)
State New, archived
Headers show
Series USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN) | expand

Commit Message

Charles Yeh Feb. 13, 2019, 12:30 p.m. UTC
Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
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 can 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 TYPE_HX_READ_STATUS_REG 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 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.

Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
---
 drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
 drivers/usb/serial/pl2303.h |   7 ++
 2 files changed, 113 insertions(+), 25 deletions(-)

Comments

Charles Yeh Feb. 19, 2019, 6:47 a.m. UTC | #1
Hi Johan & Greg,


Do you have received a new patch"[PATCH] USB:serial:pl2303:Add new PID
to support PL2303HXN (TYPE_HXN)"?

If you have received a new patch, has the content been confirmed?

Or tell me where needs to be modified.

Thanks!

Charles.


Charles Yeh <charlesyeh522@gmail.com> 於 2019年2月13日 週三 下午8:30寫道:


>
> Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
> 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 can 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 TYPE_HX_READ_STATUS_REG 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 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.
>
> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
>  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
>  drivers/usb/serial/pl2303.h |   7 ++
>  2 files changed, 113 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> @@ -129,9 +135,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
> @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR             0x40
>  #define UART_CTS                       0x80
>
> +#define TYPE_HX_READ_STATUS_REG                0x8080
> +#define TYPE_HXN_FLOWCONTROL_REG       0x0A
> +#define TYPE_HXN_HARDWAREFLOW_DATA     0xFA
> +#define TYPE_HXN_SOFTWAREFLOW_DATA     0xEE
> +#define TYPE_HXN_NOFLOW_DATA           0xFF
> +#define TYPE_HX_01_FLOWCONTROL_REG     0x00
> +#define TYPE_01_HARDWAREFLOW_DATA      0x41
> +#define TYPE_HX_HARDWAREFLOW_DATA      0x61
> +#define TYPE_HX_01_SOFTWAREFLOW_DATA   0xC0
> +#define TYPE_HX_01_NOFLOW_DATA         0x00
> +#define UART_XON_CHAR                  0x11
> +#define UART_XOFF_CHAR                 0x13
> +#define HX_RESET_DOWN_UPSTREAM_REG1    0x08
> +#define HX_RESET_DOWN_UPSTREAM_REG2    0x09
> +#define HX_RESET_DOWN_UPSTREAM_DATA    0x00
> +#define HXN_RESET_DOWN_UPSTREAM_REG    0x07
> +#define HXN_RESET_DOWN_UPSTREAM_DATA   0x00
> +
>  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
>  };
>
> @@ -179,16 +206,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__,
> @@ -207,12 +244,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__,
> @@ -292,6 +336,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)
> @@ -313,26 +358,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,
> +                       TYPE_HX_READ_STATUS_REG, 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);
>
> @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
>         }
>
>         if (C_CRTSCTS(tty)) {
> -               if (spriv->quirks & PL2303_QUIRK_LEGACY)
> -                       pl2303_vendor_write(serial, 0x0, 0x41);
> +               if (spriv->type == &pl2303_type_data[TYPE_01])
> +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                               TYPE_01_HARDWAREFLOW_DATA);
> +               else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +                                               TYPE_HXN_HARDWAREFLOW_DATA);
> +               else
> +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                               TYPE_HX_HARDWAREFLOW_DATA);
> +       } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> +                       UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
> +               if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +                                               TYPE_HXN_SOFTWAREFLOW_DATA);
>                 else
> -                       pl2303_vendor_write(serial, 0x0, 0x61);
> -       } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> -                       STOP_CHAR(tty) == 0x13) {
> -               pl2303_vendor_write(serial, 0x0, 0xc0);
> +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                               TYPE_HX_01_SOFTWAREFLOW_DATA);
>         } else {
> -               pl2303_vendor_write(serial, 0x0, 0x0);
> +               if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +                                               TYPE_HXN_NOFLOW_DATA);
> +               else
> +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                               TYPE_HX_01_NOFLOW_DATA);
>         }
>
>         kfree(buf);
> @@ -726,8 +797,18 @@ 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,
> +                                       HXN_RESET_DOWN_UPSTREAM_REG,
> +                                       HXN_RESET_DOWN_UPSTREAM_DATA);
> +               else {
> +                       pl2303_vendor_write(serial,
> +                                       HX_RESET_DOWN_UPSTREAM_REG1,
> +                                       HX_RESET_DOWN_UPSTREAM_DATA);
> +                       pl2303_vendor_write(serial,
> +                                       HX_RESET_DOWN_UPSTREAM_REG2,
> +                                       HX_RESET_DOWN_UPSTREAM_DATA);
> +               }
>         }
>
>         /* Setup termios */
> diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> index 559941ca884d..898ddc1a7302 100644
> --- a/drivers/usb/serial/pl2303.h
> +++ b/drivers/usb/serial/pl2303.h
> @@ -21,6 +21,13 @@
>  #define PL2303_PRODUCT_ID_MOTOROLA     0x0307
>  #define PL2303_PRODUCT_ID_ZTEK         0xe1f1
>
> +/* PL2303HXN , TYPE_HXN */
> +#define PL2303G_PRODUCT_ID_GC  0x23A3
> +#define PL2303G_PRODUCT_ID_GB  0x23B3
> +#define PL2303G_PRODUCT_ID_GT  0x23C3
> +#define PL2303G_PRODUCT_ID_GL  0x23D3
> +#define PL2303G_PRODUCT_ID_GE  0x23E3
> +#define PL2303G_PRODUCT_ID_GS  0x23F3
>
>  #define ATEN_VENDOR_ID         0x0557
>  #define ATEN_VENDOR_ID2                0x0547
> --
> 2.19.1
>
Charles Yeh March 4, 2019, 1:24 a.m. UTC | #2
Hi Johan,

Is it free to check out the patch file provided before?
If you can, please see first
1. [PATCH] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
2. [PATCH] [v2]USB:serial:pl2303:add new Pull-Up mode to support
PL2303HXD (TYPE_HX)

1 & 2 are independent..

Charles Yeh <charlesyeh522@gmail.com> 於 2019年2月19日 週二 下午2:47寫道:
>
> Hi Johan & Greg,
>
>
> Do you have received a new patch"[PATCH] USB:serial:pl2303:Add new PID
> to support PL2303HXN (TYPE_HXN)"?
>
> If you have received a new patch, has the content been confirmed?
>
> Or tell me where needs to be modified.
>
> Thanks!
>
> Charles.
>
>
> Charles Yeh <charlesyeh522@gmail.com> 於 2019年2月13日 週三 下午8:30寫道:
>
>
> >
> > Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
> > 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 can 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 TYPE_HX_READ_STATUS_REG 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 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.
> >
> > Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> > ---
> >  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
> >  drivers/usb/serial/pl2303.h |   7 ++
> >  2 files changed, 113 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> > +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> > +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> > +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> > +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> > +       { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> > @@ -129,9 +135,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
> > @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
> >  #define UART_OVERRUN_ERROR             0x40
> >  #define UART_CTS                       0x80
> >
> > +#define TYPE_HX_READ_STATUS_REG                0x8080
> > +#define TYPE_HXN_FLOWCONTROL_REG       0x0A
> > +#define TYPE_HXN_HARDWAREFLOW_DATA     0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA     0xEE
> > +#define TYPE_HXN_NOFLOW_DATA           0xFF
> > +#define TYPE_HX_01_FLOWCONTROL_REG     0x00
> > +#define TYPE_01_HARDWAREFLOW_DATA      0x41
> > +#define TYPE_HX_HARDWAREFLOW_DATA      0x61
> > +#define TYPE_HX_01_SOFTWAREFLOW_DATA   0xC0
> > +#define TYPE_HX_01_NOFLOW_DATA         0x00
> > +#define UART_XON_CHAR                  0x11
> > +#define UART_XOFF_CHAR                 0x13
> > +#define HX_RESET_DOWN_UPSTREAM_REG1    0x08
> > +#define HX_RESET_DOWN_UPSTREAM_REG2    0x09
> > +#define HX_RESET_DOWN_UPSTREAM_DATA    0x00
> > +#define HXN_RESET_DOWN_UPSTREAM_REG    0x07
> > +#define HXN_RESET_DOWN_UPSTREAM_DATA   0x00
> > +
> >  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
> >  };
> >
> > @@ -179,16 +206,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__,
> > @@ -207,12 +244,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__,
> > @@ -292,6 +336,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)
> > @@ -313,26 +358,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,
> > +                       TYPE_HX_READ_STATUS_REG, 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);
> >
> > @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
> >         }
> >
> >         if (C_CRTSCTS(tty)) {
> > -               if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                       pl2303_vendor_write(serial, 0x0, 0x41);
> > +               if (spriv->type == &pl2303_type_data[TYPE_01])
> > +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                               TYPE_01_HARDWAREFLOW_DATA);
> > +               else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                               TYPE_HXN_HARDWAREFLOW_DATA);
> > +               else
> > +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                               TYPE_HX_HARDWAREFLOW_DATA);
> > +       } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> > +                       UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
> > +               if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                               TYPE_HXN_SOFTWAREFLOW_DATA);
> >                 else
> > -                       pl2303_vendor_write(serial, 0x0, 0x61);
> > -       } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > -                       STOP_CHAR(tty) == 0x13) {
> > -               pl2303_vendor_write(serial, 0x0, 0xc0);
> > +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                               TYPE_HX_01_SOFTWAREFLOW_DATA);
> >         } else {
> > -               pl2303_vendor_write(serial, 0x0, 0x0);
> > +               if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                       pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                               TYPE_HXN_NOFLOW_DATA);
> > +               else
> > +                       pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                               TYPE_HX_01_NOFLOW_DATA);
> >         }
> >
> >         kfree(buf);
> > @@ -726,8 +797,18 @@ 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,
> > +                                       HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                       HXN_RESET_DOWN_UPSTREAM_DATA);
> > +               else {
> > +                       pl2303_vendor_write(serial,
> > +                                       HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                       HX_RESET_DOWN_UPSTREAM_DATA);
> > +                       pl2303_vendor_write(serial,
> > +                                       HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                       HX_RESET_DOWN_UPSTREAM_DATA);
> > +               }
> >         }
> >
> >         /* Setup termios */
> > diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> > index 559941ca884d..898ddc1a7302 100644
> > --- a/drivers/usb/serial/pl2303.h
> > +++ b/drivers/usb/serial/pl2303.h
> > @@ -21,6 +21,13 @@
> >  #define PL2303_PRODUCT_ID_MOTOROLA     0x0307
> >  #define PL2303_PRODUCT_ID_ZTEK         0xe1f1
> >
> > +/* PL2303HXN , TYPE_HXN */
> > +#define PL2303G_PRODUCT_ID_GC  0x23A3
> > +#define PL2303G_PRODUCT_ID_GB  0x23B3
> > +#define PL2303G_PRODUCT_ID_GT  0x23C3
> > +#define PL2303G_PRODUCT_ID_GL  0x23D3
> > +#define PL2303G_PRODUCT_ID_GE  0x23E3
> > +#define PL2303G_PRODUCT_ID_GS  0x23F3
> >
> >  #define ATEN_VENDOR_ID         0x0557
> >  #define ATEN_VENDOR_ID2                0x0547
> > --
> > 2.19.1
> >
Johan Hovold April 2, 2019, 7:22 a.m. UTC | #3
On Wed, Feb 13, 2019 at 08:30:00PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)

Ok, let's get back to this one.

First a general comment; please make sure to address all review
comments. I've already pointed out some issues that still hasn't been
fixed and I've asked questions that have gone unanswered.

If you disagree on something then just say so, but ignoring feedback is
just going to make this take longer than necessary.

For a start, please fix up the Subject line as we already discussed, and
make sure to wrap your commit messages at 72 columns or so (I've reflown
the rest of the message below).

Always include a changelog (below the cut-off line) when resending so we
know what changed when you update your patches.

> 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 can 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
> TYPE_HX_READ_STATUS_REG 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 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.

That's a good summary of the differences, but on a more general level,
can you confirm the following:

	1. The HXN register layout is entirely different from HX and
	   earlier devices.

	2. HXN use the same CDC class requests (line encoding, etc) as
	   earlier revisions.

Can you send me documentation for the HXN protocol? That would help a
lot in finding the right abstraction level for this.

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
>  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
>  drivers/usb/serial/pl2303.h |   7 ++
>  2 files changed, 113 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> @@ -129,9 +135,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
> @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_OVERRUN_ERROR		0x40
>  #define UART_CTS			0x80
>  
> +#define TYPE_HX_READ_STATUS_REG		0x8080
> +#define TYPE_HXN_FLOWCONTROL_REG	0x0A

> +#define TYPE_HXN_HARDWAREFLOW_DATA	0xFA
> +#define TYPE_HXN_SOFTWAREFLOW_DATA	0xEE
> +#define TYPE_HXN_NOFLOW_DATA		0xFF

What exactly does bits 0x15 (bits 4, 2 and 0) do?

Is register 0x0a really only used for flow control?

> +#define TYPE_HX_01_FLOWCONTROL_REG	0x00
> +#define TYPE_01_HARDWAREFLOW_DATA	0x41
> +#define TYPE_HX_HARDWAREFLOW_DATA	0x61
> +#define TYPE_HX_01_SOFTWAREFLOW_DATA	0xC0
> +#define TYPE_HX_01_NOFLOW_DATA		0x00
> +#define UART_XON_CHAR			0x11
> +#define UART_XOFF_CHAR			0x13
> +#define HX_RESET_DOWN_UPSTREAM_REG1	0x08
> +#define HX_RESET_DOWN_UPSTREAM_REG2	0x09
> +#define HX_RESET_DOWN_UPSTREAM_DATA	0x00
> +#define HXN_RESET_DOWN_UPSTREAM_REG	0x07
> +#define HXN_RESET_DOWN_UPSTREAM_DATA	0x00
> +
>  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
>  };
>  
> @@ -179,16 +206,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__,
> @@ -207,12 +244,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__,
> @@ -292,6 +336,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)
> @@ -313,26 +358,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) {

In an earlier version of your patch, you also checked bcdUSB here. Why
did you remove it?

> +		res = usb_control_msg(serial->dev,
> +			usb_rcvctrlpipe(serial->dev, 0),
> +			VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> +			TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);

Please name the registers after what they do, not how you use them. What
is register 0 that you read here? Does it have a name?

> +		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);
> +	}

Fair enough, since the HXN doesn't use the same registers, this needs to
be done only for HXN or earlier, but we should probably add an
initialisation callback instead of spreading conditionals throughout the
driver.

I think I know roughly what the code above does now, but since you are
the only ones with access to the documentation, perhaps you can explain
why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
Is that even correct?

>  	kfree(buf);
>  
> @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
>  	}
>  
>  	if (C_CRTSCTS(tty)) {
> -		if (spriv->quirks & PL2303_QUIRK_LEGACY)
> -			pl2303_vendor_write(serial, 0x0, 0x41);

Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
both auto-rts and auto-cts?

> +		if (spriv->type == &pl2303_type_data[TYPE_01])
> +			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +						TYPE_01_HARDWAREFLOW_DATA);
> +		else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +						TYPE_HXN_HARDWAREFLOW_DATA);
> +		else
> +			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +						TYPE_HX_HARDWAREFLOW_DATA);
> +	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> +			UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {

No need to add defines for the start and stop char here.

> +		if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +						TYPE_HXN_SOFTWAREFLOW_DATA);
>  		else
> -			pl2303_vendor_write(serial, 0x0, 0x61);
> -	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> -			STOP_CHAR(tty) == 0x13) {
> -		pl2303_vendor_write(serial, 0x0, 0xc0);
> +			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +						TYPE_HX_01_SOFTWAREFLOW_DATA);
>  	} else {
> -		pl2303_vendor_write(serial, 0x0, 0x0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +						TYPE_HXN_NOFLOW_DATA);
> +		else
> +			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +						TYPE_HX_01_NOFLOW_DATA);

As already mentioned, the above is hardly readable. When studying the
current driver, I noticed a couple of bugs that I'm preparing fixes for.

Specifically, we shouldn't be overwriting the entire control register,
which changes the tranceiver suspend mode. Are there similar problems
with not doing bit updates of register 0x0a?

Either way, rebasing your patches on top of those should allow this to
be cleaned up somewhat.

>  	}
>  
>  	kfree(buf);
> @@ -726,8 +797,18 @@ 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])

You need to added braces ({}) to both branches here.

> +			pl2303_vendor_write(serial,
> +					HXN_RESET_DOWN_UPSTREAM_REG,
> +					HXN_RESET_DOWN_UPSTREAM_DATA);

Can you write anything to this register to reset the buffers, or does it
have to be 0?

> +		else {
> +			pl2303_vendor_write(serial,
> +					HX_RESET_DOWN_UPSTREAM_REG1,
> +					HX_RESET_DOWN_UPSTREAM_DATA);
> +			pl2303_vendor_write(serial,
> +					HX_RESET_DOWN_UPSTREAM_REG2,
> +					HX_RESET_DOWN_UPSTREAM_DATA);

I assume the older versions allow for the and up and down buffers to be
reset independently? Please name these registers accordingly.

> +		}
>  	}
>  
>  	/* Setup termios */
> diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> index 559941ca884d..898ddc1a7302 100644
> --- a/drivers/usb/serial/pl2303.h
> +++ b/drivers/usb/serial/pl2303.h
> @@ -21,6 +21,13 @@
>  #define PL2303_PRODUCT_ID_MOTOROLA	0x0307
>  #define PL2303_PRODUCT_ID_ZTEK		0xe1f1
>  
> +/* PL2303HXN , TYPE_HXN */
> +#define PL2303G_PRODUCT_ID_GC	0x23A3
> +#define PL2303G_PRODUCT_ID_GB	0x23B3
> +#define PL2303G_PRODUCT_ID_GT	0x23C3
> +#define PL2303G_PRODUCT_ID_GL	0x23D3
> +#define PL2303G_PRODUCT_ID_GE	0x23E3
> +#define PL2303G_PRODUCT_ID_GS	0x23F3

Just use PL2303_ as prefix.

>  
>  #define ATEN_VENDOR_ID		0x0557
>  #define ATEN_VENDOR_ID2		0x0547

Thanks,
Johan
Charles Yeh April 3, 2019, 4:51 a.m. UTC | #4
Hi Johan,
     Thanks for you check the patch..
     I will reply to you on the next Monday.
   Because I am currently on a business trip in China (3/28~4/6)

Johan Hovold <johan@kernel.org> 於 2019年4月2日 週二 下午3:22寫道:
>
> On Wed, Feb 13, 2019 at 08:30:00PM +0800, Charles Yeh wrote:
> > Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
>
> Ok, let's get back to this one.
>
> First a general comment; please make sure to address all review
> comments. I've already pointed out some issues that still hasn't been
> fixed and I've asked questions that have gone unanswered.
>
> If you disagree on something then just say so, but ignoring feedback is
> just going to make this take longer than necessary.
>
> For a start, please fix up the Subject line as we already discussed, and
> make sure to wrap your commit messages at 72 columns or so (I've reflown
> the rest of the message below).
>
> Always include a changelog (below the cut-off line) when resending so we
> know what changed when you update your patches.
>
> > 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 can 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
> > TYPE_HX_READ_STATUS_REG 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 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.
>
> That's a good summary of the differences, but on a more general level,
> can you confirm the following:
>
>         1. The HXN register layout is entirely different from HX and
>            earlier devices.
>
>         2. HXN use the same CDC class requests (line encoding, etc) as
>            earlier revisions.
>
> Can you send me documentation for the HXN protocol? That would help a
> lot in finding the right abstraction level for this.
>
> > Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> > ---
> >  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
> >  drivers/usb/serial/pl2303.h |   7 ++
> >  2 files changed, 113 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> > @@ -129,9 +135,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
> > @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
> >  #define UART_OVERRUN_ERROR           0x40
> >  #define UART_CTS                     0x80
> >
> > +#define TYPE_HX_READ_STATUS_REG              0x8080
> > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
>
> > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > +#define TYPE_HXN_NOFLOW_DATA         0xFF
>
> What exactly does bits 0x15 (bits 4, 2 and 0) do?
>
> Is register 0x0a really only used for flow control?
>
> > +#define TYPE_HX_01_FLOWCONTROL_REG   0x00
> > +#define TYPE_01_HARDWAREFLOW_DATA    0x41
> > +#define TYPE_HX_HARDWAREFLOW_DATA    0x61
> > +#define TYPE_HX_01_SOFTWAREFLOW_DATA 0xC0
> > +#define TYPE_HX_01_NOFLOW_DATA               0x00
> > +#define UART_XON_CHAR                        0x11
> > +#define UART_XOFF_CHAR                       0x13
> > +#define HX_RESET_DOWN_UPSTREAM_REG1  0x08
> > +#define HX_RESET_DOWN_UPSTREAM_REG2  0x09
> > +#define HX_RESET_DOWN_UPSTREAM_DATA  0x00
> > +#define HXN_RESET_DOWN_UPSTREAM_REG  0x07
> > +#define HXN_RESET_DOWN_UPSTREAM_DATA 0x00
> > +
> >  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
> >  };
> >
> > @@ -179,16 +206,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__,
> > @@ -207,12 +244,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__,
> > @@ -292,6 +336,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)
> > @@ -313,26 +358,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) {
>
> In an earlier version of your patch, you also checked bcdUSB here. Why
> did you remove it?
>
> > +             res = usb_control_msg(serial->dev,
> > +                     usb_rcvctrlpipe(serial->dev, 0),
> > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
>
> Please name the registers after what they do, not how you use them. What
> is register 0 that you read here? Does it have a name?
>
> > +             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);
> > +     }
>
> Fair enough, since the HXN doesn't use the same registers, this needs to
> be done only for HXN or earlier, but we should probably add an
> initialisation callback instead of spreading conditionals throughout the
> driver.
>
> I think I know roughly what the code above does now, but since you are
> the only ones with access to the documentation, perhaps you can explain
> why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> Is that even correct?
>
> >       kfree(buf);
> >
> > @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
> >       }
> >
> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_vendor_write(serial, 0x0, 0x41);
>
> Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> both auto-rts and auto-cts?
>
> > +             if (spriv->type == &pl2303_type_data[TYPE_01])
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_01_HARDWAREFLOW_DATA);
> > +             else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_HARDWAREFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_HARDWAREFLOW_DATA);
> > +     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> > +                     UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
>
> No need to add defines for the start and stop char here.
>
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> >               else
> > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > -                     STOP_CHAR(tty) == 0x13) {
> > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> >       } else {
> > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_NOFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_NOFLOW_DATA);
>
> As already mentioned, the above is hardly readable. When studying the
> current driver, I noticed a couple of bugs that I'm preparing fixes for.
>
> Specifically, we shouldn't be overwriting the entire control register,
> which changes the tranceiver suspend mode. Are there similar problems
> with not doing bit updates of register 0x0a?
>
> Either way, rebasing your patches on top of those should allow this to
> be cleaned up somewhat.
>
> >       }
> >
> >       kfree(buf);
> > @@ -726,8 +797,18 @@ 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])
>
> You need to added braces ({}) to both branches here.
>
> > +                     pl2303_vendor_write(serial,
> > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
>
> Can you write anything to this register to reset the buffers, or does it
> have to be 0?
>
> > +             else {
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
>
> I assume the older versions allow for the and up and down buffers to be
> reset independently? Please name these registers accordingly.
>
> > +             }
> >       }
> >
> >       /* Setup termios */
> > diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> > index 559941ca884d..898ddc1a7302 100644
> > --- a/drivers/usb/serial/pl2303.h
> > +++ b/drivers/usb/serial/pl2303.h
> > @@ -21,6 +21,13 @@
> >  #define PL2303_PRODUCT_ID_MOTOROLA   0x0307
> >  #define PL2303_PRODUCT_ID_ZTEK               0xe1f1
> >
> > +/* PL2303HXN , TYPE_HXN */
> > +#define PL2303G_PRODUCT_ID_GC        0x23A3
> > +#define PL2303G_PRODUCT_ID_GB        0x23B3
> > +#define PL2303G_PRODUCT_ID_GT        0x23C3
> > +#define PL2303G_PRODUCT_ID_GL        0x23D3
> > +#define PL2303G_PRODUCT_ID_GE        0x23E3
> > +#define PL2303G_PRODUCT_ID_GS        0x23F3
>
> Just use PL2303_ as prefix.
>
> >
> >  #define ATEN_VENDOR_ID               0x0557
> >  #define ATEN_VENDOR_ID2              0x0547
>
> Thanks,
> Johan
Charles Yeh April 9, 2019, 9:52 a.m. UTC | #5
Hi Johan,
      1. The HXN register layout is entirely different from HX and
           earlier devices.

        2. HXN use the same CDC class requests (line encoding, etc) as
           earlier revisions.

Can you send me documentation for the HXN protocol? That would help a
lot in finding the right abstraction level for this.

Ans:
Yes,The HXN register layout is entirely different from HX and earlier devices,
Yes, HXN use the same CDC class requests (line encoding, etc) as
earlier revisions

I don't have a PL2303G (HXN) Word file, I only have Excel's table file,
please refer to PL2303_Linux\Vendor_Requesut.png
Or you can install window driver use PL2303G demo board.
(The Fedex tracking no. is 8117 8594 0521)
and then USB Bus Hound to analysis the vendor request.

The original Prolific company is hoping:
TYPE_HX and TYPE_01 used old driver(pl2303.c & pl2303.h)
The new PL2303G (TYPE_HXN) uses new driver(plser.c & plser.h),

You can go to the website of Prolific, in the website,

TYPE_HX and TYPE_01 are called
PL2303 USB to Serial Bridge Controllers (All Chip Versions)

TYPE_HXN is called
PL2303G USB to Serial Bridge Controllers (All Chip Versions).

We provide two different file paths on the website, the name.file..
is to distinguish between TYPE_HX/01 and TYPE_HXN
please refer to PL2303_Linux\Window_support_hx_hxn.jpg

TYPE_HX and TYPE_01:the windows file name is ser2pl.sys & ser2pl64.sys.

TYPE_HXN:the windows file name is plser.sys & plser64.sys.

please refer to PL2303_Linux\window_plser_ser2pl.bmp

But Greg disagreed with the Linux OS about new File name(plser.c & plser.h)
So we did that we merged with the existing pl2303.c(h)
into a new chip (PL2303G, TYPE_HXN)

> +#define TYPE_HXN_FLOWCONTROL_REG     0x0A

> +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> +#define TYPE_HXN_NOFLOW_DATA         0xFF

What exactly does bits 0x15 (bits 4, 2 and 0) do?
Ans:please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,

Is register 0x0a really only used for flow control?
Ans: Yes, please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
0x0A(Bit 2, 3, 4)is UART flow control settting,
it only supports TYPE_HXN, no supports TYPE_HX/01.


In an earlier version of your patch, you also checked bcdUSB here. Why
did you remove it?

> +             res = usb_control_msg(serial->dev,
> +                     usb_rcvctrlpipe(serial->dev, 0),
> +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);

Please name the registers after what they do, not how you use them. What
is register 0 that you read here? Does it have a name?

Ans: Because the "kbuild test robot <lkp@intel.com>" reply me a error.
Please refer to Email: "Tuesday, December 25, 2018, at 11:39 pm"
or refer to PL2303_Linux\remoe_checked_bcdUSB.txt


Why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
Is that even correct?
Ans: Yes, it is correct,
The Output(TX/DTR/RTS) Tri-state H/W design is different.

TYPE_01: RS-232 Output Tri-state:
0: RS-232 output in output mode;
1: RS-232 output tri-state.

TYPE_HX_Serial Port (TXD, RTS, and DTR) Output Enable:
00 – Disable output signals (Tri-State) at all time;
01 – Disable output signals (Tri-State) during suspend;
10, 11 – Set output signals to HIGH during suspend;



>       if (C_CRTSCTS(tty)) {
> -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> -                     pl2303_vendor_write(serial, 0x0, 0x41);

Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
both auto-rts and auto-cts?

Ans: Yes, it is correct,
The Hardware UART flow control settting design is different
between TYPT_01 and TYPT_HX.

Although I have been in Prolific Company for almost 16 years..
But TYPE_01 (PL2303H) has been discontinued(EOL) for 13 years...
Now can't find this IC(TYPT_01: PL2303H) on the market..

The line"if (spriv->quirks & PL2303_QUIRK_LEGACY)"
you are asking now is not what I wrote...
I don't know why to define it as PL2303_QUIRK_LEGACY.

I am mainly responsible for the PL2303 driver
Windows, WinCE, Mac & Android in Prolific.
On the Linux driver side, only when the customer has a problem,
or want to add new features (such as GPIO control, new Baud rate),
I will rewrite the Linux code.
Rewritten Linux code will only be given to the corresponding customer.

Just like this time, because I want to add PL2303G (TYPE_HXN) to Linux,
I just rewritten the Linux code...
For some definitions in the Linux code,
such as why it is defined as PL2303_QUIRK_LEGACY,
I really don't know very well

Before rewriting the Linux code,
I can only guess "PL2303_QUIRK_LEGACY" that this refers to the
old PL2303 (PL2303H, TYPE_01)..
After the actual test (I have tested the old PL2303H chip on Linux),
when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)



> +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
>               else
> -                     pl2303_vendor_write(serial, 0x0, 0x61);
> -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> -                     STOP_CHAR(tty) == 0x13) {
> -             pl2303_vendor_write(serial, 0x0, 0xc0);
> +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
>       } else {
> -             pl2303_vendor_write(serial, 0x0, 0x0);
> +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> +                                             TYPE_HXN_NOFLOW_DATA);
> +             else
> +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> +                                             TYPE_HX_01_NOFLOW_DATA);

As already mentioned, the above is hardly readable. When studying the
current driver, I noticed a couple of bugs that I'm preparing fixes for.

Specifically, we shouldn't be overwriting the entire control register,
which changes the tranceiver suspend mode. Are there similar problems
with not doing bit updates of register 0x0a?

Either way, rebasing your patches on top of those should allow this to
be cleaned up somewhat.


Ans: the three chips TYPE_01 / TYPE_HX / TYPE_HXN have their own independent
 flow control Register settings...
What advice do you have for handling here?

Here I have actually tested the three chips TYPE_01 / TYPE_HX / TYPE_HXN
TYPE_01: PL2303H
TYPE_HX: PL2303HXA/ PL2303XA/ PL2303HXD/PL2303TA/PL2303TB/PL2303RA/ PL2303SA.
TYPE_HXN: PL2303GC, PL2303GS, PL2303GL...

The test result is OK..



> +                     pl2303_vendor_write(serial,
> +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> +                                     HXN_RESET_DOWN_UPSTREAM_DATA);

Can you write anything to this register to reset the buffers, or does it
have to be 0?
Ans: It have to reset to 0, please refer to PL2303_Linux\PL2303G_ChipReset.jpg


> +             else {
> +                     pl2303_vendor_write(serial,
> +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> +                     pl2303_vendor_write(serial,
> +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> +                                     HX_RESET_DOWN_UPSTREAM_DATA);

I assume the older versions allow for the and up and down buffers to be
reset independently? Please name these registers accordingly.
Ans: Yes, reset independently,
please refer to PL2303_Linux\PL2303_TYPE_HX_Reset.jpg
The original code is
        /* reset upstream data pipes */
        pl2303_vendor_write(serial, 8, 0);
        pl2303_vendor_write(serial, 9, 0);

You previously suggested that I am not using "magical" constants directly,
Please use defines rather than "magical" Constants.
What advice do you have for handling here?


> +/* PL2303HXN , TYPE_HXN */
> +#define PL2303G_PRODUCT_ID_GC        0x23A3
> +#define PL2303G_PRODUCT_ID_GB        0x23B3
> +#define PL2303G_PRODUCT_ID_GT        0x23C3
> +#define PL2303G_PRODUCT_ID_GL        0x23D3
> +#define PL2303G_PRODUCT_ID_GE        0x23E3
> +#define PL2303G_PRODUCT_ID_GS        0x23F3

Just use PL2303_ as prefix.
Ans: OK, I will use PL2303_ as prefix.,

PL2303_Linux \ File:please download the file from app box.
https://app.box.com/s/914zdvugkx6ok2gfr22uykj247l8046m


Charles.

Johan Hovold <johan@kernel.org> 於 2019年4月2日 週二 下午3:22寫道:
>
> On Wed, Feb 13, 2019 at 08:30:00PM +0800, Charles Yeh wrote:
> > Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
>
> Ok, let's get back to this one.
>
> First a general comment; please make sure to address all review
> comments. I've already pointed out some issues that still hasn't been
> fixed and I've asked questions that have gone unanswered.
>
> If you disagree on something then just say so, but ignoring feedback is
> just going to make this take longer than necessary.
>
> For a start, please fix up the Subject line as we already discussed, and
> make sure to wrap your commit messages at 72 columns or so (I've reflown
> the rest of the message below).
>
> Always include a changelog (below the cut-off line) when resending so we
> know what changed when you update your patches.
>
> > 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 can 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
> > TYPE_HX_READ_STATUS_REG 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 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.
>
> That's a good summary of the differences, but on a more general level,
> can you confirm the following:
>
>         1. The HXN register layout is entirely different from HX and
>            earlier devices.
>
>         2. HXN use the same CDC class requests (line encoding, etc) as
>            earlier revisions.
>
> Can you send me documentation for the HXN protocol? That would help a
> lot in finding the right abstraction level for this.
>
> > Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> > ---
> >  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
> >  drivers/usb/serial/pl2303.h |   7 ++
> >  2 files changed, 113 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> > @@ -129,9 +135,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
> > @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
> >  #define UART_OVERRUN_ERROR           0x40
> >  #define UART_CTS                     0x80
> >
> > +#define TYPE_HX_READ_STATUS_REG              0x8080
> > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
>
> > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > +#define TYPE_HXN_NOFLOW_DATA         0xFF
>
> What exactly does bits 0x15 (bits 4, 2 and 0) do?
>
> Is register 0x0a really only used for flow control?
>
> > +#define TYPE_HX_01_FLOWCONTROL_REG   0x00
> > +#define TYPE_01_HARDWAREFLOW_DATA    0x41
> > +#define TYPE_HX_HARDWAREFLOW_DATA    0x61
> > +#define TYPE_HX_01_SOFTWAREFLOW_DATA 0xC0
> > +#define TYPE_HX_01_NOFLOW_DATA               0x00
> > +#define UART_XON_CHAR                        0x11
> > +#define UART_XOFF_CHAR                       0x13
> > +#define HX_RESET_DOWN_UPSTREAM_REG1  0x08
> > +#define HX_RESET_DOWN_UPSTREAM_REG2  0x09
> > +#define HX_RESET_DOWN_UPSTREAM_DATA  0x00
> > +#define HXN_RESET_DOWN_UPSTREAM_REG  0x07
> > +#define HXN_RESET_DOWN_UPSTREAM_DATA 0x00
> > +
> >  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
> >  };
> >
> > @@ -179,16 +206,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__,
> > @@ -207,12 +244,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__,
> > @@ -292,6 +336,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)
> > @@ -313,26 +358,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) {
>
> In an earlier version of your patch, you also checked bcdUSB here. Why
> did you remove it?
>
> > +             res = usb_control_msg(serial->dev,
> > +                     usb_rcvctrlpipe(serial->dev, 0),
> > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
>
> Please name the registers after what they do, not how you use them. What
> is register 0 that you read here? Does it have a name?
>
> > +             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);
> > +     }
>
> Fair enough, since the HXN doesn't use the same registers, this needs to
> be done only for HXN or earlier, but we should probably add an
> initialisation callback instead of spreading conditionals throughout the
> driver.
>
> I think I know roughly what the code above does now, but since you are
> the only ones with access to the documentation, perhaps you can explain
> why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> Is that even correct?
>
> >       kfree(buf);
> >
> > @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
> >       }
> >
> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_vendor_write(serial, 0x0, 0x41);
>
> Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> both auto-rts and auto-cts?
>
> > +             if (spriv->type == &pl2303_type_data[TYPE_01])
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_01_HARDWAREFLOW_DATA);
> > +             else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_HARDWAREFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_HARDWAREFLOW_DATA);
> > +     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> > +                     UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
>
> No need to add defines for the start and stop char here.
>
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> >               else
> > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > -                     STOP_CHAR(tty) == 0x13) {
> > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> >       } else {
> > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_NOFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_NOFLOW_DATA);
>
> As already mentioned, the above is hardly readable. When studying the
> current driver, I noticed a couple of bugs that I'm preparing fixes for.
>
> Specifically, we shouldn't be overwriting the entire control register,
> which changes the tranceiver suspend mode. Are there similar problems
> with not doing bit updates of register 0x0a?
>
> Either way, rebasing your patches on top of those should allow this to
> be cleaned up somewhat.
>
> >       }
> >
> >       kfree(buf);
> > @@ -726,8 +797,18 @@ 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])
>
> You need to added braces ({}) to both branches here.
>
> > +                     pl2303_vendor_write(serial,
> > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
>
> Can you write anything to this register to reset the buffers, or does it
> have to be 0?
>
> > +             else {
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
>
> I assume the older versions allow for the and up and down buffers to be
> reset independently? Please name these registers accordingly.
>
> > +             }
> >       }
> >
> >       /* Setup termios */
> > diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> > index 559941ca884d..898ddc1a7302 100644
> > --- a/drivers/usb/serial/pl2303.h
> > +++ b/drivers/usb/serial/pl2303.h
> > @@ -21,6 +21,13 @@
> >  #define PL2303_PRODUCT_ID_MOTOROLA   0x0307
> >  #define PL2303_PRODUCT_ID_ZTEK               0xe1f1
> >
> > +/* PL2303HXN , TYPE_HXN */
> > +#define PL2303G_PRODUCT_ID_GC        0x23A3
> > +#define PL2303G_PRODUCT_ID_GB        0x23B3
> > +#define PL2303G_PRODUCT_ID_GT        0x23C3
> > +#define PL2303G_PRODUCT_ID_GL        0x23D3
> > +#define PL2303G_PRODUCT_ID_GE        0x23E3
> > +#define PL2303G_PRODUCT_ID_GS        0x23F3
>
> Just use PL2303_ as prefix.
>
> >
> >  #define ATEN_VENDOR_ID               0x0557
> >  #define ATEN_VENDOR_ID2              0x0547
>
> Thanks,
> Johan
Charles Yeh April 12, 2019, 2:33 a.m. UTC | #6
Hi Johan,

Do you have time to check the contents of my reply?
If you have any questions about the content of my reply,
please feel free to ask. Thank you!

My boss hopes to pass this patch as soon as possible.

There are some companies (HTC, HUAWEI, ASUS),
their software engineers have modified the patch with me 3 to 4 months ago,
and can use the new PL2303G (Type_HXN) or
the old PL2303 (TYPE_HX). ) on the Linux OS.
But these companies want to be able to update the patch from the Linux kernel
 instead of using the patch from me.

Charles Yeh <charlesyeh522@gmail.com> 於 2019年4月9日 週二 下午5:52寫道:
>
> Hi Johan,
>       1. The HXN register layout is entirely different from HX and
>            earlier devices.
>
>         2. HXN use the same CDC class requests (line encoding, etc) as
>            earlier revisions.
>
> Can you send me documentation for the HXN protocol? That would help a
> lot in finding the right abstraction level for this.
>
> Ans:
> Yes,The HXN register layout is entirely different from HX and earlier devices,
> Yes, HXN use the same CDC class requests (line encoding, etc) as
> earlier revisions
>
> I don't have a PL2303G (HXN) Word file, I only have Excel's table file,
> please refer to PL2303_Linux\Vendor_Requesut.png
> Or you can install window driver use PL2303G demo board.
> (The Fedex tracking no. is 8117 8594 0521)
> and then USB Bus Hound to analysis the vendor request.
>
> The original Prolific company is hoping:
> TYPE_HX and TYPE_01 used old driver(pl2303.c & pl2303.h)
> The new PL2303G (TYPE_HXN) uses new driver(plser.c & plser.h),
>
> You can go to the website of Prolific, in the website,
>
> TYPE_HX and TYPE_01 are called
> PL2303 USB to Serial Bridge Controllers (All Chip Versions)
>
> TYPE_HXN is called
> PL2303G USB to Serial Bridge Controllers (All Chip Versions).
>
> We provide two different file paths on the website, the name.file..
> is to distinguish between TYPE_HX/01 and TYPE_HXN
> please refer to PL2303_Linux\Window_support_hx_hxn.jpg
>
> TYPE_HX and TYPE_01:the windows file name is ser2pl.sys & ser2pl64.sys.
>
> TYPE_HXN:the windows file name is plser.sys & plser64.sys.
>
> please refer to PL2303_Linux\window_plser_ser2pl.bmp
>
> But Greg disagreed with the Linux OS about new File name(plser.c & plser.h)
> So we did that we merged with the existing pl2303.c(h)
> into a new chip (PL2303G, TYPE_HXN)
>
> > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
>
> > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > +#define TYPE_HXN_NOFLOW_DATA         0xFF
>
> What exactly does bits 0x15 (bits 4, 2 and 0) do?
> Ans:please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
>
> Is register 0x0a really only used for flow control?
> Ans: Yes, please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> 0x0A(Bit 2, 3, 4)is UART flow control settting,
> it only supports TYPE_HXN, no supports TYPE_HX/01.
>
>
> In an earlier version of your patch, you also checked bcdUSB here. Why
> did you remove it?
>
> > +             res = usb_control_msg(serial->dev,
> > +                     usb_rcvctrlpipe(serial->dev, 0),
> > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
>
> Please name the registers after what they do, not how you use them. What
> is register 0 that you read here? Does it have a name?
>
> Ans: Because the "kbuild test robot <lkp@intel.com>" reply me a error.
> Please refer to Email: "Tuesday, December 25, 2018, at 11:39 pm"
> or refer to PL2303_Linux\remoe_checked_bcdUSB.txt
>
>
> Why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> Is that even correct?
> Ans: Yes, it is correct,
> The Output(TX/DTR/RTS) Tri-state H/W design is different.
>
> TYPE_01: RS-232 Output Tri-state:
> 0: RS-232 output in output mode;
> 1: RS-232 output tri-state.
>
> TYPE_HX_Serial Port (TXD, RTS, and DTR) Output Enable:
> 00 – Disable output signals (Tri-State) at all time;
> 01 – Disable output signals (Tri-State) during suspend;
> 10, 11 – Set output signals to HIGH during suspend;
>
>
>
> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_vendor_write(serial, 0x0, 0x41);
>
> Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> both auto-rts and auto-cts?
>
> Ans: Yes, it is correct,
> The Hardware UART flow control settting design is different
> between TYPT_01 and TYPT_HX.
>
> Although I have been in Prolific Company for almost 16 years..
> But TYPE_01 (PL2303H) has been discontinued(EOL) for 13 years...
> Now can't find this IC(TYPT_01: PL2303H) on the market..
>
> The line"if (spriv->quirks & PL2303_QUIRK_LEGACY)"
> you are asking now is not what I wrote...
> I don't know why to define it as PL2303_QUIRK_LEGACY.
>
> I am mainly responsible for the PL2303 driver
> Windows, WinCE, Mac & Android in Prolific.
> On the Linux driver side, only when the customer has a problem,
> or want to add new features (such as GPIO control, new Baud rate),
> I will rewrite the Linux code.
> Rewritten Linux code will only be given to the corresponding customer.
>
> Just like this time, because I want to add PL2303G (TYPE_HXN) to Linux,
> I just rewritten the Linux code...
> For some definitions in the Linux code,
> such as why it is defined as PL2303_QUIRK_LEGACY,
> I really don't know very well
>
> Before rewriting the Linux code,
> I can only guess "PL2303_QUIRK_LEGACY" that this refers to the
> old PL2303 (PL2303H, TYPE_01)..
> After the actual test (I have tested the old PL2303H chip on Linux),
> when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)
>
>
>
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> >               else
> > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > -                     STOP_CHAR(tty) == 0x13) {
> > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> >       } else {
> > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_NOFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_NOFLOW_DATA);
>
> As already mentioned, the above is hardly readable. When studying the
> current driver, I noticed a couple of bugs that I'm preparing fixes for.
>
> Specifically, we shouldn't be overwriting the entire control register,
> which changes the tranceiver suspend mode. Are there similar problems
> with not doing bit updates of register 0x0a?
>
> Either way, rebasing your patches on top of those should allow this to
> be cleaned up somewhat.
>
>
> Ans: the three chips TYPE_01 / TYPE_HX / TYPE_HXN have their own independent
>  flow control Register settings...
> What advice do you have for handling here?
>
> Here I have actually tested the three chips TYPE_01 / TYPE_HX / TYPE_HXN
> TYPE_01: PL2303H
> TYPE_HX: PL2303HXA/ PL2303XA/ PL2303HXD/PL2303TA/PL2303TB/PL2303RA/ PL2303SA.
> TYPE_HXN: PL2303GC, PL2303GS, PL2303GL...
>
> The test result is OK..
>
>
>
> > +                     pl2303_vendor_write(serial,
> > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
>
> Can you write anything to this register to reset the buffers, or does it
> have to be 0?
> Ans: It have to reset to 0, please refer to PL2303_Linux\PL2303G_ChipReset.jpg
>
>
> > +             else {
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
>
> I assume the older versions allow for the and up and down buffers to be
> reset independently? Please name these registers accordingly.
> Ans: Yes, reset independently,
> please refer to PL2303_Linux\PL2303_TYPE_HX_Reset.jpg
> The original code is
>         /* reset upstream data pipes */
>         pl2303_vendor_write(serial, 8, 0);
>         pl2303_vendor_write(serial, 9, 0);
>
> You previously suggested that I am not using "magical" constants directly,
> Please use defines rather than "magical" Constants.
> What advice do you have for handling here?
>
>
> > +/* PL2303HXN , TYPE_HXN */
> > +#define PL2303G_PRODUCT_ID_GC        0x23A3
> > +#define PL2303G_PRODUCT_ID_GB        0x23B3
> > +#define PL2303G_PRODUCT_ID_GT        0x23C3
> > +#define PL2303G_PRODUCT_ID_GL        0x23D3
> > +#define PL2303G_PRODUCT_ID_GE        0x23E3
> > +#define PL2303G_PRODUCT_ID_GS        0x23F3
>
> Just use PL2303_ as prefix.
> Ans: OK, I will use PL2303_ as prefix.,
>
> PL2303_Linux \ File:please download the file from app box.
> https://app.box.com/s/914zdvugkx6ok2gfr22uykj247l8046m
>
>
> Charles.
>
> Johan Hovold <johan@kernel.org> 於 2019年4月2日 週二 下午3:22寫道:
> >
> > On Wed, Feb 13, 2019 at 08:30:00PM +0800, Charles Yeh wrote:
> > > Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)
> >
> > Ok, let's get back to this one.
> >
> > First a general comment; please make sure to address all review
> > comments. I've already pointed out some issues that still hasn't been
> > fixed and I've asked questions that have gone unanswered.
> >
> > If you disagree on something then just say so, but ignoring feedback is
> > just going to make this take longer than necessary.
> >
> > For a start, please fix up the Subject line as we already discussed, and
> > make sure to wrap your commit messages at 72 columns or so (I've reflown
> > the rest of the message below).
> >
> > Always include a changelog (below the cut-off line) when resending so we
> > know what changed when you update your patches.
> >
> > > 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 can 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
> > > TYPE_HX_READ_STATUS_REG 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 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.
> >
> > That's a good summary of the differences, but on a more general level,
> > can you confirm the following:
> >
> >         1. The HXN register layout is entirely different from HX and
> >            earlier devices.
> >
> >         2. HXN use the same CDC class requests (line encoding, etc) as
> >            earlier revisions.
> >
> > Can you send me documentation for the HXN protocol? That would help a
> > lot in finding the right abstraction level for this.
> >
> > > Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> > > ---
> > >  drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
> > >  drivers/usb/serial/pl2303.h |   7 ++
> > >  2 files changed, 113 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > > index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
> > > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
> > > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
> > > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
> > > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
> > > +     { USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
> > > @@ -129,9 +135,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
> > > @@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
> > >  #define UART_OVERRUN_ERROR           0x40
> > >  #define UART_CTS                     0x80
> > >
> > > +#define TYPE_HX_READ_STATUS_REG              0x8080
> > > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
> >
> > > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > > +#define TYPE_HXN_NOFLOW_DATA         0xFF
> >
> > What exactly does bits 0x15 (bits 4, 2 and 0) do?
> >
> > Is register 0x0a really only used for flow control?
> >
> > > +#define TYPE_HX_01_FLOWCONTROL_REG   0x00
> > > +#define TYPE_01_HARDWAREFLOW_DATA    0x41
> > > +#define TYPE_HX_HARDWAREFLOW_DATA    0x61
> > > +#define TYPE_HX_01_SOFTWAREFLOW_DATA 0xC0
> > > +#define TYPE_HX_01_NOFLOW_DATA               0x00
> > > +#define UART_XON_CHAR                        0x11
> > > +#define UART_XOFF_CHAR                       0x13
> > > +#define HX_RESET_DOWN_UPSTREAM_REG1  0x08
> > > +#define HX_RESET_DOWN_UPSTREAM_REG2  0x09
> > > +#define HX_RESET_DOWN_UPSTREAM_DATA  0x00
> > > +#define HXN_RESET_DOWN_UPSTREAM_REG  0x07
> > > +#define HXN_RESET_DOWN_UPSTREAM_DATA 0x00
> > > +
> > >  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
> > >  };
> > >
> > > @@ -179,16 +206,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__,
> > > @@ -207,12 +244,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__,
> > > @@ -292,6 +336,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)
> > > @@ -313,26 +358,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) {
> >
> > In an earlier version of your patch, you also checked bcdUSB here. Why
> > did you remove it?
> >
> > > +             res = usb_control_msg(serial->dev,
> > > +                     usb_rcvctrlpipe(serial->dev, 0),
> > > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
> >
> > Please name the registers after what they do, not how you use them. What
> > is register 0 that you read here? Does it have a name?
> >
> > > +             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);
> > > +     }
> >
> > Fair enough, since the HXN doesn't use the same registers, this needs to
> > be done only for HXN or earlier, but we should probably add an
> > initialisation callback instead of spreading conditionals throughout the
> > driver.
> >
> > I think I know roughly what the code above does now, but since you are
> > the only ones with access to the documentation, perhaps you can explain
> > why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> > Is that even correct?
> >
> > >       kfree(buf);
> > >
> > > @@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
> > >       }
> > >
> > >       if (C_CRTSCTS(tty)) {
> > > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > > -                     pl2303_vendor_write(serial, 0x0, 0x41);
> >
> > Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> > both auto-rts and auto-cts?
> >
> > > +             if (spriv->type == &pl2303_type_data[TYPE_01])
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_01_HARDWAREFLOW_DATA);
> > > +             else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > > +                                             TYPE_HXN_HARDWAREFLOW_DATA);
> > > +             else
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_HX_HARDWAREFLOW_DATA);
> > > +     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
> > > +                     UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
> >
> > No need to add defines for the start and stop char here.
> >
> > > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> > >               else
> > > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > > -                     STOP_CHAR(tty) == 0x13) {
> > > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> > >       } else {
> > > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > > +                                             TYPE_HXN_NOFLOW_DATA);
> > > +             else
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_HX_01_NOFLOW_DATA);
> >
> > As already mentioned, the above is hardly readable. When studying the
> > current driver, I noticed a couple of bugs that I'm preparing fixes for.
> >
> > Specifically, we shouldn't be overwriting the entire control register,
> > which changes the tranceiver suspend mode. Are there similar problems
> > with not doing bit updates of register 0x0a?
> >
> > Either way, rebasing your patches on top of those should allow this to
> > be cleaned up somewhat.
> >
> > >       }
> > >
> > >       kfree(buf);
> > > @@ -726,8 +797,18 @@ 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])
> >
> > You need to added braces ({}) to both branches here.
> >
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
> >
> > Can you write anything to this register to reset the buffers, or does it
> > have to be 0?
> >
> > > +             else {
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> >
> > I assume the older versions allow for the and up and down buffers to be
> > reset independently? Please name these registers accordingly.
> >
> > > +             }
> > >       }
> > >
> > >       /* Setup termios */
> > > diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> > > index 559941ca884d..898ddc1a7302 100644
> > > --- a/drivers/usb/serial/pl2303.h
> > > +++ b/drivers/usb/serial/pl2303.h
> > > @@ -21,6 +21,13 @@
> > >  #define PL2303_PRODUCT_ID_MOTOROLA   0x0307
> > >  #define PL2303_PRODUCT_ID_ZTEK               0xe1f1
> > >
> > > +/* PL2303HXN , TYPE_HXN */
> > > +#define PL2303G_PRODUCT_ID_GC        0x23A3
> > > +#define PL2303G_PRODUCT_ID_GB        0x23B3
> > > +#define PL2303G_PRODUCT_ID_GT        0x23C3
> > > +#define PL2303G_PRODUCT_ID_GL        0x23D3
> > > +#define PL2303G_PRODUCT_ID_GE        0x23E3
> > > +#define PL2303G_PRODUCT_ID_GS        0x23F3
> >
> > Just use PL2303_ as prefix.
> >
> > >
> > >  #define ATEN_VENDOR_ID               0x0557
> > >  #define ATEN_VENDOR_ID2              0x0547
> >
> > Thanks,
> > Johan
Johan Hovold April 15, 2019, 8:56 a.m. UTC | #7
Hi Charles,

Please fix your mail setup so that you can respond inline as is
customary on the lists. Your mails are currently unnecessarily hard to
read due to it not being clear what is quoted text and what isn't.

Specifically, quoted text should have an extra "> " at the start of the
line, and don't copy text and respond to it (just reply *inline*).

Currently there's a second copy of the mail your responding to at the
end of your mails. That bit should instead contain your replies (with
unnecessary context removed).

On Tue, Apr 09, 2019 at 05:52:45PM +0800, Charles Yeh wrote:
> Hi Johan,
>       1. The HXN register layout is entirely different from HX and
>            earlier devices.
> 
>         2. HXN use the same CDC class requests (line encoding, etc) as
>            earlier revisions.
> 
> Can you send me documentation for the HXN protocol? That would help a
> lot in finding the right abstraction level for this.
> 
> Ans:
> Yes,The HXN register layout is entirely different from HX and earlier devices,
> Yes, HXN use the same CDC class requests (line encoding, etc) as
> earlier revisions

Ok, thanks for confirming.

> I don't have a PL2303G (HXN) Word file, I only have Excel's table file,
> please refer to PL2303_Linux\Vendor_Requesut.png

Did you forget to attach the png? Excel files describing the protocol
would do just fine.

> Or you can install window driver use PL2303G demo board.

> and then USB Bus Hound to analysis the vendor request.

No, I'm certainly not going to reverse engineer your Windows driver in
order to help you.

Surely, you must have some documentation for your devices?

> The original Prolific company is hoping:
> TYPE_HX and TYPE_01 used old driver(pl2303.c & pl2303.h)
> The new PL2303G (TYPE_HXN) uses new driver(plser.c & plser.h),
> 
> You can go to the website of Prolific, in the website,
> 
> TYPE_HX and TYPE_01 are called
> PL2303 USB to Serial Bridge Controllers (All Chip Versions)
> 
> TYPE_HXN is called
> PL2303G USB to Serial Bridge Controllers (All Chip Versions).
> 
> We provide two different file paths on the website, the name.file..
> is to distinguish between TYPE_HX/01 and TYPE_HXN
> please refer to PL2303_Linux\Window_support_hx_hxn.jpg
> 
> TYPE_HX and TYPE_01:the windows file name is ser2pl.sys & ser2pl64.sys.
> 
> TYPE_HXN:the windows file name is plser.sys & plser64.sys.
> 
> please refer to PL2303_Linux\window_plser_ser2pl.bmp
> 
> But Greg disagreed with the Linux OS about new File name(plser.c & plser.h)
> So we did that we merged with the existing pl2303.c(h)
> into a new chip (PL2303G, TYPE_HXN)

Yes, the plser driver you submitted was just a copy of pl2303, with
minimal changes to support pl2303g. With that much code being shared you
need to add support for the new device type to the current driver.

But the lack of documentation of the protocols used is making it hard to
determine the appropriate abstraction level.

> > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
> 
> > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > +#define TYPE_HXN_NOFLOW_DATA         0xFF
> 
> What exactly does bits 0x15 (bits 4, 2 and 0) do?
> Ans:please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> 
> Is register 0x0a really only used for flow control?
> Ans: Yes, please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> 0x0A(Bit 2, 3, 4)is UART flow control settting,
> it only supports TYPE_HXN, no supports TYPE_HX/01.

Ok, thanks (found the link at the end of the mail now).

> In an earlier version of your patch, you also checked bcdUSB here. Why
> did you remove it?
> 
> > +             res = usb_control_msg(serial->dev,
> > +                     usb_rcvctrlpipe(serial->dev, 0),
> > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
> 
> Please name the registers after what they do, not how you use them. What
> is register 0 that you read here? Does it have a name?
> 
> Ans: Because the "kbuild test robot <lkp@intel.com>" reply me a error.
> Please refer to Email: "Tuesday, December 25, 2018, at 11:39 pm"
> or refer to PL2303_Linux\remoe_checked_bcdUSB.txt

Yes, you needed to use helper function to deal with endianess as was
pointed out to you in that thread. Why didn't you use the helper, and
instead just dropped the check?

> Why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> Is that even correct?
> Ans: Yes, it is correct,
> The Output(TX/DTR/RTS) Tri-state H/W design is different.
> 
> TYPE_01: RS-232 Output Tri-state:
> 0: RS-232 output in output mode;
> 1: RS-232 output tri-state.
> 
> TYPE_HX_Serial Port (TXD, RTS, and DTR) Output Enable:
> 00 – Disable output signals (Tri-State) at all time;
> 01 – Disable output signals (Tri-State) during suspend;
> 10, 11 – Set output signals to HIGH during suspend;

Ok.

> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_vendor_write(serial, 0x0, 0x41);
> 
> Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> both auto-rts and auto-cts?
> 
> Ans: Yes, it is correct,
> The Hardware UART flow control settting design is different
> between TYPT_01 and TYPT_HX.
> 
> Although I have been in Prolific Company for almost 16 years..
> But TYPE_01 (PL2303H) has been discontinued(EOL) for 13 years...
> Now can't find this IC(TYPT_01: PL2303H) on the market..

Sure, but we still need to make sure we don't break any working setups
using these old devices, so I still need to understand how the older
protocols work.

> The line"if (spriv->quirks & PL2303_QUIRK_LEGACY)"
> you are asking now is not what I wrote...
> I don't know why to define it as PL2303_QUIRK_LEGACY.

I added that at some point in an attempt to clean up this
reverse-engineering driver. You can read it as a TYPE_01 test. 

> I am mainly responsible for the PL2303 driver
> Windows, WinCE, Mac & Android in Prolific.
> On the Linux driver side, only when the customer has a problem,
> or want to add new features (such as GPIO control, new Baud rate),
> I will rewrite the Linux code.
> Rewritten Linux code will only be given to the corresponding customer.
> 
> Just like this time, because I want to add PL2303G (TYPE_HXN) to Linux,
> I just rewritten the Linux code...
> For some definitions in the Linux code,
> such as why it is defined as PL2303_QUIRK_LEGACY,
> I really don't know very well
> 
> Before rewriting the Linux code,
> I can only guess "PL2303_QUIRK_LEGACY" that this refers to the
> old PL2303 (PL2303H, TYPE_01)..

Right.

> After the actual test (I have tested the old PL2303H chip on Linux),
> when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)

Correct, but my question was about bit 0x20 for TYPE_01 devices; why
isn't it being as set as for TYPE_HX?

> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> >               else
> > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > -                     STOP_CHAR(tty) == 0x13) {
> > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> >       } else {
> > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_NOFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_NOFLOW_DATA);
> 
> As already mentioned, the above is hardly readable. When studying the
> current driver, I noticed a couple of bugs that I'm preparing fixes for.
> 
> Specifically, we shouldn't be overwriting the entire control register,
> which changes the tranceiver suspend mode. Are there similar problems
> with not doing bit updates of register 0x0a?
> 
> Either way, rebasing your patches on top of those should allow this to
> be cleaned up somewhat.
> 
> 
> Ans: the three chips TYPE_01 / TYPE_HX / TYPE_HXN have their own independent
>  flow control Register settings...
> What advice do you have for handling here?
> 
> Here I have actually tested the three chips TYPE_01 / TYPE_HX / TYPE_HXN
> TYPE_01: PL2303H
> TYPE_HX: PL2303HXA/ PL2303XA/ PL2303HXD/PL2303TA/PL2303TB/PL2303RA/ PL2303SA.
> TYPE_HXN: PL2303GC, PL2303GS, PL2303GL...
> 
> The test result is OK..

Well, without documentation it is hard to give advice. But the
information you provided above, indicates that you should not be
overwriting these registers completely for HXN either when updating the
flow-control settings. Only the bits 2..4 (and possibly 0..1) should be
written.

Take a look at the patches I merged for doing this on the older devices.
You should be able to use the same helper when updating these
registers.

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=f64c3ab230682e8395a7fbd01f3eb5140c837d4e

> > +                     pl2303_vendor_write(serial,
> > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
> 
> Can you write anything to this register to reset the buffers, or does it
> have to be 0?
> Ans: It have to reset to 0, please refer to PL2303_Linux\PL2303G_ChipReset.jpg

But that jpg shows only bit 0 and 1 being used for data pipe reset. And
shouldn't you be writing 0x02 to reset those?

> > +             else {
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> 
> I assume the older versions allow for the and up and down buffers to be
> reset independently? Please name these registers accordingly.
> Ans: Yes, reset independently,
> please refer to PL2303_Linux\PL2303_TYPE_HX_Reset.jpg
> The original code is
>         /* reset upstream data pipes */
>         pl2303_vendor_write(serial, 8, 0);
>         pl2303_vendor_write(serial, 9, 0);
> 
> You previously suggested that I am not using "magical" constants directly,
> Please use defines rather than "magical" Constants.
> What advice do you have for handling here?

Name register and bit-definitions after what they do. We need code to be
clean and mostly self-documenting. In this case, based on the
information provided above, you could use

	#define PL2303_RESET_DOWNSTREAM_PIPE	8
	#define PL2303_RESET_UPSTREAM_PIPE	9

for example.

As the protocol appears to mandate writing 0 to these registers, it
may be ok to keep that as a numerical constant.

Johan
Charles Yeh April 17, 2019, 10:50 a.m. UTC | #8
Hello Sir,

Please download the PL2303_Linux_0419.zip from
https://app.box.com/s/uh9kldrdldjnmjffku8gkdvvaq5496tk

> > After the actual test (I have tested the old PL2303H chip on Linux),
> > when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> > if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> > So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)
>
> Correct, but my question was about bit 0x20 for TYPE_01 devices; why
> isn't it being as set as for TYPE_HX?
>

TYPE_01 / TYPE_HX / TYPE_HXN are different hardware design.
Please refer to PL2303_Linux_0419\PL2303_TYPE_01_UART_Flow.jpg &
PL2303_TYPE_HX_UART_Flow.jpg


>
> Well, without documentation it is hard to give advice. But the
> information you provided above, indicates that you should not be
> overwriting these registers completely for HXN either when updating the
> flow-control settings. Only the bits 2..4 (and possibly 0..1) should be
> written.
>
> Take a look at the patches I merged for doing this on the older devices.
> You should be able to use the same helper when updating these
> registers.

Please refer to PL2303_Linux_0419\
PL2303_TYPE_01_UART_Flow.jpg &
PL2303_TYPE_HX_UART_Flow.jpg &
PL2303G_TYPE_HXN_UART_Flow.jpg



If you have other questions (whether it's my newly written patch code or
the code that originally existed), please try to raise it... Thanks!

The code that originally existed; not what I wrote..
But I have rewritten many times to a lot of customers...
probably can also guess the usage of the original code.


I just used
"git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git"
to get the latest Linux kernel.

The code I see in the pl2303_set_termios function
is still pl2303_vendor_write(serial,0x0,0x41),
Not pl2303_update_reg(serial,0,PL2303_FLOWCTRL_MASK,0x40);


Can I write a new patch?
"git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git"
But I don't want to rewrite the original code.
For example "pl2303_vendor_write(serial,0x0,0x41)"
Because that's the old code.. Not that I added the patch code this time.


Charles.

Johan Hovold <johan@kernel.org> 於 2019年4月15日 週一 下午4:56寫道:
>
> Hi Charles,
>
> Please fix your mail setup so that you can respond inline as is
> customary on the lists. Your mails are currently unnecessarily hard to
> read due to it not being clear what is quoted text and what isn't.
>
> Specifically, quoted text should have an extra "> " at the start of the
> line, and don't copy text and respond to it (just reply *inline*).
>
> Currently there's a second copy of the mail your responding to at the
> end of your mails. That bit should instead contain your replies (with
> unnecessary context removed).
>
> On Tue, Apr 09, 2019 at 05:52:45PM +0800, Charles Yeh wrote:
> > Hi Johan,
> >       1. The HXN register layout is entirely different from HX and
> >            earlier devices.
> >
> >         2. HXN use the same CDC class requests (line encoding, etc) as
> >            earlier revisions.
> >
> > Can you send me documentation for the HXN protocol? That would help a
> > lot in finding the right abstraction level for this.
> >
> > Ans:
> > Yes,The HXN register layout is entirely different from HX and earlier devices,
> > Yes, HXN use the same CDC class requests (line encoding, etc) as
> > earlier revisions
>
> Ok, thanks for confirming.
>
> > I don't have a PL2303G (HXN) Word file, I only have Excel's table file,
> > please refer to PL2303_Linux\Vendor_Requesut.png
>
> Did you forget to attach the png? Excel files describing the protocol
> would do just fine.
>
> > Or you can install window driver use PL2303G demo board.
>
> > and then USB Bus Hound to analysis the vendor request.
>
> No, I'm certainly not going to reverse engineer your Windows driver in
> order to help you.
>
> Surely, you must have some documentation for your devices?
>
> > The original Prolific company is hoping:
> > TYPE_HX and TYPE_01 used old driver(pl2303.c & pl2303.h)
> > The new PL2303G (TYPE_HXN) uses new driver(plser.c & plser.h),
> >
> > You can go to the website of Prolific, in the website,
> >
> > TYPE_HX and TYPE_01 are called
> > PL2303 USB to Serial Bridge Controllers (All Chip Versions)
> >
> > TYPE_HXN is called
> > PL2303G USB to Serial Bridge Controllers (All Chip Versions).
> >
> > We provide two different file paths on the website, the name.file..
> > is to distinguish between TYPE_HX/01 and TYPE_HXN
> > please refer to PL2303_Linux\Window_support_hx_hxn.jpg
> >
> > TYPE_HX and TYPE_01:the windows file name is ser2pl.sys & ser2pl64.sys.
> >
> > TYPE_HXN:the windows file name is plser.sys & plser64.sys.
> >
> > please refer to PL2303_Linux\window_plser_ser2pl.bmp
> >
> > But Greg disagreed with the Linux OS about new File name(plser.c & plser.h)
> > So we did that we merged with the existing pl2303.c(h)
> > into a new chip (PL2303G, TYPE_HXN)
>
> Yes, the plser driver you submitted was just a copy of pl2303, with
> minimal changes to support pl2303g. With that much code being shared you
> need to add support for the new device type to the current driver.
>
> But the lack of documentation of the protocols used is making it hard to
> determine the appropriate abstraction level.
>
> > > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
> >
> > > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > > +#define TYPE_HXN_NOFLOW_DATA         0xFF
> >
> > What exactly does bits 0x15 (bits 4, 2 and 0) do?
> > Ans:please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> >
> > Is register 0x0a really only used for flow control?
> > Ans: Yes, please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> > 0x0A(Bit 2, 3, 4)is UART flow control settting,
> > it only supports TYPE_HXN, no supports TYPE_HX/01.
>
> Ok, thanks (found the link at the end of the mail now).
>
> > In an earlier version of your patch, you also checked bcdUSB here. Why
> > did you remove it?
> >
> > > +             res = usb_control_msg(serial->dev,
> > > +                     usb_rcvctrlpipe(serial->dev, 0),
> > > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
> >
> > Please name the registers after what they do, not how you use them. What
> > is register 0 that you read here? Does it have a name?
> >
> > Ans: Because the "kbuild test robot <lkp@intel.com>" reply me a error.
> > Please refer to Email: "Tuesday, December 25, 2018, at 11:39 pm"
> > or refer to PL2303_Linux\remoe_checked_bcdUSB.txt
>
> Yes, you needed to use helper function to deal with endianess as was
> pointed out to you in that thread. Why didn't you use the helper, and
> instead just dropped the check?
>
> > Why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> > Is that even correct?
> > Ans: Yes, it is correct,
> > The Output(TX/DTR/RTS) Tri-state H/W design is different.
> >
> > TYPE_01: RS-232 Output Tri-state:
> > 0: RS-232 output in output mode;
> > 1: RS-232 output tri-state.
> >
> > TYPE_HX_Serial Port (TXD, RTS, and DTR) Output Enable:
> > 00 – Disable output signals (Tri-State) at all time;
> > 01 – Disable output signals (Tri-State) during suspend;
> > 10, 11 – Set output signals to HIGH during suspend;
>
> Ok.
>
> > >       if (C_CRTSCTS(tty)) {
> > > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > > -                     pl2303_vendor_write(serial, 0x0, 0x41);
> >
> > Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> > both auto-rts and auto-cts?
> >
> > Ans: Yes, it is correct,
> > The Hardware UART flow control settting design is different
> > between TYPT_01 and TYPT_HX.
> >
> > Although I have been in Prolific Company for almost 16 years..
> > But TYPE_01 (PL2303H) has been discontinued(EOL) for 13 years...
> > Now can't find this IC(TYPT_01: PL2303H) on the market..
>
> Sure, but we still need to make sure we don't break any working setups
> using these old devices, so I still need to understand how the older
> protocols work.
>
> > The line"if (spriv->quirks & PL2303_QUIRK_LEGACY)"
> > you are asking now is not what I wrote...
> > I don't know why to define it as PL2303_QUIRK_LEGACY.
>
> I added that at some point in an attempt to clean up this
> reverse-engineering driver. You can read it as a TYPE_01 test.
>
> > I am mainly responsible for the PL2303 driver
> > Windows, WinCE, Mac & Android in Prolific.
> > On the Linux driver side, only when the customer has a problem,
> > or want to add new features (such as GPIO control, new Baud rate),
> > I will rewrite the Linux code.
> > Rewritten Linux code will only be given to the corresponding customer.
> >
> > Just like this time, because I want to add PL2303G (TYPE_HXN) to Linux,
> > I just rewritten the Linux code...
> > For some definitions in the Linux code,
> > such as why it is defined as PL2303_QUIRK_LEGACY,
> > I really don't know very well
> >
> > Before rewriting the Linux code,
> > I can only guess "PL2303_QUIRK_LEGACY" that this refers to the
> > old PL2303 (PL2303H, TYPE_01)..
>
> Right.
>
> > After the actual test (I have tested the old PL2303H chip on Linux),
> > when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> > if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> > So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)
>
> Correct, but my question was about bit 0x20 for TYPE_01 devices; why
> isn't it being as set as for TYPE_HX?
>
> > > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> > >               else
> > > -                     pl2303_vendor_write(serial, 0x0, 0x61);
> > > -     } else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
> > > -                     STOP_CHAR(tty) == 0x13) {
> > > -             pl2303_vendor_write(serial, 0x0, 0xc0);
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> > >       } else {
> > > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > > +                                             TYPE_HXN_NOFLOW_DATA);
> > > +             else
> > > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > > +                                             TYPE_HX_01_NOFLOW_DATA);
> >
> > As already mentioned, the above is hardly readable. When studying the
> > current driver, I noticed a couple of bugs that I'm preparing fixes for.
> >
> > Specifically, we shouldn't be overwriting the entire control register,
> > which changes the tranceiver suspend mode. Are there similar problems
> > with not doing bit updates of register 0x0a?
> >
> > Either way, rebasing your patches on top of those should allow this to
> > be cleaned up somewhat.
> >
> >
> > Ans: the three chips TYPE_01 / TYPE_HX / TYPE_HXN have their own independent
> >  flow control Register settings...
> > What advice do you have for handling here?
> >
> > Here I have actually tested the three chips TYPE_01 / TYPE_HX / TYPE_HXN
> > TYPE_01: PL2303H
> > TYPE_HX: PL2303HXA/ PL2303XA/ PL2303HXD/PL2303TA/PL2303TB/PL2303RA/ PL2303SA.
> > TYPE_HXN: PL2303GC, PL2303GS, PL2303GL...
> >
> > The test result is OK..
>
> Well, without documentation it is hard to give advice. But the
> information you provided above, indicates that you should not be
> overwriting these registers completely for HXN either when updating the
> flow-control settings. Only the bits 2..4 (and possibly 0..1) should be
> written.
>
> Take a look at the patches I merged for doing this on the older devices.
> You should be able to use the same helper when updating these
> registers.
>
>         https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=f64c3ab230682e8395a7fbd01f3eb5140c837d4e
>
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
> >
> > Can you write anything to this register to reset the buffers, or does it
> > have to be 0?
> > Ans: It have to reset to 0, please refer to PL2303_Linux\PL2303G_ChipReset.jpg
>
> But that jpg shows only bit 0 and 1 being used for data pipe reset. And
> shouldn't you be writing 0x02 to reset those?
>
> > > +             else {
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > > +                     pl2303_vendor_write(serial,
> > > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> >
> > I assume the older versions allow for the and up and down buffers to be
> > reset independently? Please name these registers accordingly.
> > Ans: Yes, reset independently,
> > please refer to PL2303_Linux\PL2303_TYPE_HX_Reset.jpg
> > The original code is
> >         /* reset upstream data pipes */
> >         pl2303_vendor_write(serial, 8, 0);
> >         pl2303_vendor_write(serial, 9, 0);
> >
> > You previously suggested that I am not using "magical" constants directly,
> > Please use defines rather than "magical" Constants.
> > What advice do you have for handling here?
>
> Name register and bit-definitions after what they do. We need code to be
> clean and mostly self-documenting. In this case, based on the
> information provided above, you could use
>
>         #define PL2303_RESET_DOWNSTREAM_PIPE    8
>         #define PL2303_RESET_UPSTREAM_PIPE      9
>
> for example.
>
> As the protocol appears to mandate writing 0 to these registers, it
> may be ok to keep that as a numerical constant.
>
> Johan
Johan Hovold April 17, 2019, 11:13 a.m. UTC | #9
On Wed, Apr 17, 2019 at 06:50:55PM +0800, Charles Yeh wrote:
> Hello Sir,
> 
> Please download the PL2303_Linux_0419.zip from
> https://app.box.com/s/uh9kldrdldjnmjffku8gkdvvaq5496tk
> 
> > > After the actual test (I have tested the old PL2303H chip on Linux),
> > > when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> > > if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> > > So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)
> >
> > Correct, but my question was about bit 0x20 for TYPE_01 devices; why
> > isn't it being as set as for TYPE_HX?
> >
> 
> TYPE_01 / TYPE_HX / TYPE_HXN are different hardware design.
> Please refer to PL2303_Linux_0419\PL2303_TYPE_01_UART_Flow.jpg &
> PL2303_TYPE_HX_UART_Flow.jpg

Thanks, that will be very helpful.

> > Well, without documentation it is hard to give advice. But the
> > information you provided above, indicates that you should not be
> > overwriting these registers completely for HXN either when updating the
> > flow-control settings. Only the bits 2..4 (and possibly 0..1) should be
> > written.
> >
> > Take a look at the patches I merged for doing this on the older devices.
> > You should be able to use the same helper when updating these
> > registers.
> 
> Please refer to PL2303_Linux_0419\
> PL2303_TYPE_01_UART_Flow.jpg &
> PL2303_TYPE_HX_UART_Flow.jpg &
> PL2303G_TYPE_HXN_UART_Flow.jpg
> 
> 
> 
> If you have other questions (whether it's my newly written patch code or
> the code that originally existed), please try to raise it... Thanks!

I will, thanks. I'm looking at how best to abstract this now.

> The code that originally existed; not what I wrote..
> But I have rewritten many times to a lot of customers...
> probably can also guess the usage of the original code.
> 
> 
> I just used
> "git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git"
> to get the latest Linux kernel.
> 
> The code I see in the pl2303_set_termios function
> is still pl2303_vendor_write(serial,0x0,0x41),
> Not pl2303_update_reg(serial,0,PL2303_FLOWCTRL_MASK,0x40);

Yes, you need to base it on my usb-next branch (or linux-next):

	git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

> Can I write a new patch?
> "git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git"
> But I don't want to rewrite the original code.
> For example "pl2303_vendor_write(serial,0x0,0x41)"
> Because that's the old code.. Not that I added the patch code this time.

Got it, but please take a look my usb-next branch.

I'll try to clean up the current driver some more based on your
feedback. I'll get back to you as soon as have something concrete
and we'll get the HXN support in place.

Meanwhile here are few questions:

 1. Can bcdUSB be used to detect the HXN devices?

 2. Does the old (type 01 and hx) devices require the register address
    to be ORed with 0x80 when reading?

 3. And the HXN does not need that ORing with 0x80 it seems?

 4. How do you use the chip-reset register (0x7) on HXN; do you write
    0x03 if you only want to reset both data pipes?

And please try to respond inline when replying, no need to keep second
copy at the end of your mails.

Thanks,
Johan
Charles Yeh April 17, 2019, 1:48 p.m. UTC | #10
Johan Hovold <johan@kernel.org> 於 2019年4月17日 週三 下午7:13寫道:

> > The code I see in the pl2303_set_termios function
> > is still pl2303_vendor_write(serial,0x0,0x41),
> > Not pl2303_update_reg(serial,0,PL2303_FLOWCTRL_MASK,0x40);
>
> Yes, you need to base it on my usb-next branch (or linux-next):
>
>         git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

OK, I will use
"git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git "


> I'll try to clean up the current driver some more based on your
> feedback. I'll get back to you as soon as have something concrete
> and we'll get the HXN support in place.

Thank you very much!

>
> Meanwhile here are few questions:
>
>  1. Can bcdUSB be used to detect the HXN devices?

No , you can't use bcdUSB be used to detect the HXN devices.
>
>  2. Does the old (type 01 and hx) devices require the register address
>     to be ORed with 0x80 when reading?

Yes, the old (type_01 and HX ) needs require the register address to be
ORed with 0x80 when reading.


>  3. And the HXN does not need that ORing with 0x80 it seems?
Yes, HXN's hardware design is different old (type_01 and hx).

>  4. How do you use the chip-reset register (0x7) on HXN; do you write
>     0x03 if you only want to reset both data pipes?
Yes, I only want to reset both data pipes.

Charles.
diff mbox series

Patch

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bb3f9aa4a909..d7d557e01390 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, PL2303G_PRODUCT_ID_GC) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_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),
@@ -129,9 +135,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
@@ -145,11 +153,30 @@  MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define TYPE_HX_READ_STATUS_REG		0x8080
+#define TYPE_HXN_FLOWCONTROL_REG	0x0A
+#define TYPE_HXN_HARDWAREFLOW_DATA	0xFA
+#define TYPE_HXN_SOFTWAREFLOW_DATA	0xEE
+#define TYPE_HXN_NOFLOW_DATA		0xFF
+#define TYPE_HX_01_FLOWCONTROL_REG	0x00
+#define TYPE_01_HARDWAREFLOW_DATA	0x41
+#define TYPE_HX_HARDWAREFLOW_DATA	0x61
+#define TYPE_HX_01_SOFTWAREFLOW_DATA	0xC0
+#define TYPE_HX_01_NOFLOW_DATA		0x00
+#define UART_XON_CHAR			0x11
+#define UART_XOFF_CHAR			0x13
+#define HX_RESET_DOWN_UPSTREAM_REG1	0x08
+#define HX_RESET_DOWN_UPSTREAM_REG2	0x09
+#define HX_RESET_DOWN_UPSTREAM_DATA	0x00
+#define HXN_RESET_DOWN_UPSTREAM_REG	0x07
+#define HXN_RESET_DOWN_UPSTREAM_DATA	0x00
+
 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
 };
 
@@ -179,16 +206,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__,
@@ -207,12 +244,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__,
@@ -292,6 +336,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)
@@ -313,26 +358,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,
+			TYPE_HX_READ_STATUS_REG, 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);
 
@@ -677,15 +733,30 @@  static void pl2303_set_termios(struct tty_struct *tty,
 	}
 
 	if (C_CRTSCTS(tty)) {
-		if (spriv->quirks & PL2303_QUIRK_LEGACY)
-			pl2303_vendor_write(serial, 0x0, 0x41);
+		if (spriv->type == &pl2303_type_data[TYPE_01])
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_01_HARDWAREFLOW_DATA);
+		else if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_HARDWAREFLOW_DATA);
+		else
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_HARDWAREFLOW_DATA);
+	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
+			UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_SOFTWAREFLOW_DATA);
 		else
-			pl2303_vendor_write(serial, 0x0, 0x61);
-	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
-			STOP_CHAR(tty) == 0x13) {
-		pl2303_vendor_write(serial, 0x0, 0xc0);
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_01_SOFTWAREFLOW_DATA);
 	} else {
-		pl2303_vendor_write(serial, 0x0, 0x0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_NOFLOW_DATA);
+		else
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_01_NOFLOW_DATA);
 	}
 
 	kfree(buf);
@@ -726,8 +797,18 @@  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,
+					HXN_RESET_DOWN_UPSTREAM_REG,
+					HXN_RESET_DOWN_UPSTREAM_DATA);
+		else {
+			pl2303_vendor_write(serial,
+					HX_RESET_DOWN_UPSTREAM_REG1,
+					HX_RESET_DOWN_UPSTREAM_DATA);
+			pl2303_vendor_write(serial,
+					HX_RESET_DOWN_UPSTREAM_REG2,
+					HX_RESET_DOWN_UPSTREAM_DATA);
+		}
 	}
 
 	/* Setup termios */
diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
index 559941ca884d..898ddc1a7302 100644
--- a/drivers/usb/serial/pl2303.h
+++ b/drivers/usb/serial/pl2303.h
@@ -21,6 +21,13 @@ 
 #define PL2303_PRODUCT_ID_MOTOROLA	0x0307
 #define PL2303_PRODUCT_ID_ZTEK		0xe1f1
 
+/* PL2303HXN , TYPE_HXN */
+#define PL2303G_PRODUCT_ID_GC	0x23A3
+#define PL2303G_PRODUCT_ID_GB	0x23B3
+#define PL2303G_PRODUCT_ID_GT	0x23C3
+#define PL2303G_PRODUCT_ID_GL	0x23D3
+#define PL2303G_PRODUCT_ID_GE	0x23E3
+#define PL2303G_PRODUCT_ID_GS	0x23F3
 
 #define ATEN_VENDOR_ID		0x0557
 #define ATEN_VENDOR_ID2		0x0547