Hi Johan & Greg,
Thansk for you check the patch file..
PL2303 is a general name on the market. In fact, PL2303 has the
following various ICs.
A type: PL2303H,
B type: PL2303XA, PL2303HXA, PL2303HXB, PL2303HXC, PL2303HXD,
PL2303EA, PL2303SA, PL2303RA, PL2303TA, PL2303TB
C type:PL2303HXN: PL2303GC, PL2303GS, PL2303GB, PL2303GT, PL2303GL,
PL2303GE. <<--used different PID, different VENDOR_REQUEST
"Please split out in a separate patch and explain why it is needed.
Alsoreplace the "magical" constants with descriptive defines."
I will push 3 patch file.
1. Support PL2303TB(old chip, B type), only add new VID_PID
2. Support PL2303HXD(old chip, B type), /* only support External
Pull-Up Mode on PL2303HXD chip*. /
3. Support PL2303HXN(new chip, C type), add new VID_PID, new
VENDOR_WRITE_REQUEST/ VENDOR_READ_REQUEST, new H/W, S/W flow control
setting.
I will use "sudo ./scripts/checkpatch.pl --file
drivers/usb/serial/pl2303.c" & use 'git send-email' to send the
patch( avoid Tabs all got turned into spaces)
Johan Hovold <johan@xxxxxxxxxx> 於 2018年12月18日 週二 下午6:02寫道：
>
> On Mon, Dec 17, 2018 at 12:51:24PM +0800, Charles Yeh wrote:
> > From b9fd71c64d4d0d939a7a27e08a74d81f960ff5ea Mon Sep 17 00:00:00 2001
> > From: Charles Yeh <charlesyeh522@xxxxxxxxx>
> > Date: Sat, 15 Dec 2018 07:10:17 +0800
> > Subject: [PATCH] Add Proliic new chip: PL2303TB & PL2303N(G)
> >
> > Add new PID to support PL2303TB
>
> Is this also an HXN-type device?
>
> > Add mew PID to support PL2303(N)GC/GB/GS/GT/GL/GE
> > Add new request to support PL2303N(G)
> >
> > Signed-off-by: Charles Yeh <charlesyeh522@xxxxxxxxx>
> > ---
> > drivers/usb/serial/pl2303.c | 106 +++++++++++++++++++++++++++++-------
> > drivers/usb/serial/pl2303.h | 11 ++++
> > 2 files changed, 97 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index a4e0d13fc121..0001b527f07f 100644
> > --- a/drivers/usb/serial/pl2303.c
> > +++ b/drivers/usb/serial/pl2303.c
> > @@ -31,6 +31,8 @@
> > #define PL2303_QUIRK_UART_STATE_IDX0 BIT(0)
> > #define PL2303_QUIRK_LEGACY BIT(1)
> > #define PL2303_QUIRK_ENDPOINT_HACK BIT(2)
> > +#define PL2303_QUIRK_LEGACY_HX BIT(3) /* old IC type */
> > +#define PL2303_QUIRK_LEGACY_N BIT(4) /* new IC type */
>
> I don't there's any need to add new "quirk" flags for these types. Just
> use the type stored in the interface private data directly (struct
> pl2303_serial_private).
>
> > static const struct usb_device_id id_table[] = {
> > { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID),
> > @@ -46,6 +48,13 @@ static const struct usb_device_id id_table[] = {
> > { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_HCR331) },
> > { 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),
> > @@ -123,9 +132,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
> > @@ -144,6 +155,7 @@ 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
> > };
> >
> > @@ -172,6 +184,11 @@ static const struct pl2303_type_data
> > pl2303_type_data[TYPE_COUNT] = {
> > },
> > [TYPE_HX] = {
> > .max_baud_rate = 12000000,
> > + .quirks = PL2303_QUIRK_LEGACY_HX, /* old chip type */
> > + },
> > + [TYPE_HXN] = {
> > + .max_baud_rate = 12000000,
> > + .quirks = PL2303_QUIRK_LEGACY_N, /* New chip type */
> > },
> > };
> >
> > @@ -179,10 +196,16 @@ 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;
>
> This should be u8.
>
> > +
> > + /* old / new read request ?*/
>
> You can just drop the comment.
>
> > + if (spriv->quirks & PL2303_QUIRK_LEGACY_N) request= VENDOR_READ_NREQUEST;
> > + else request= VENDOR_READ_REQUEST;
>
> But please fix up the indentation of this (as Greg already pointed out).
>
> >
> > 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__,
> > @@ -201,12 +224,18 @@ 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;
>
> u8
>
> >
> > dev_dbg(dev, "%s - [%04x] = %02x\n", __func__, value, index);
> >
> > + /* old / new write request ? */
> > + if (spriv->quirks & PL2303_QUIRK_LEGACY_N) request= VENDOR_WRITE_NREQUEST;
> > + else request= VENDOR_WRITE_REQUEST;
>
> Same as above.
>
> > +
> > 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__,
> > @@ -286,6 +315,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)
> > @@ -307,26 +337,38 @@ static int pl2303_startup(struct usb_serial *serial)
> > type = TYPE_01; /* type 1 */
> > dev_dbg(&serial->interface->dev, "device type: %d\n", type);
> >
> > + /* new chip ? */
> > + if(serial->dev->descriptor.bcdUSB == 0x0200) {
> > + res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> > + VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > + 0x8484, 0, buf, 1, 100);
>
> What are you reading here? What is 0x8484?
>
> > + if (res != 1) {
> > + type = TYPE_HXN; /* type 2 */
> > + }
> > + }
> > +
> > 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);
> >
> > @@ -673,13 +715,33 @@ 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);
> > + else if(spriv->quirks & PL2303_QUIRK_LEGACY_N)
> > + pl2303_vendor_write(serial, 0x0A, 0xFA); /* New chip */
>
> What is 0x0a and 0xfa? Please use defines rather than "magical"
> constants.
>
> > 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);
> > + if(spriv->quirks & PL2303_QUIRK_LEGACY_N)
> > + pl2303_vendor_write(serial, 0x0A, 0xEE); /* New chip */
>
> Same here.
>
> > + else
> > + pl2303_vendor_write(serial, 0x0, 0xc0);
> > } else {
> > - pl2303_vendor_write(serial, 0x0, 0x0);
> > + if(spriv->quirks & PL2303_QUIRK_LEGACY_N)
> > + pl2303_vendor_write(serial, 0x0A, 0xFF); /* New chip */
> > + else
> > + pl2303_vendor_write(serial, 0x0, 0x0);
> > + }
> > +
> > + /* Old chip, External Pull-Up Mode */
> > + if(spriv->quirks & PL2303_QUIRK_LEGACY_HX){
> > + pl2303_vendor_read(serial, 0x8484, buf);
> > + pl2303_vendor_write(serial, 0x0404, 0x09);
> > + pl2303_vendor_read(serial, 0x8484, buf);
> > + pl2303_vendor_read(serial, 0x8383, buf);
> > + if((u16)*buf & 0x08){
>
> This looks odd as pl2303_vendor_read() only reads one byte.
>
> > + pl2303_vendor_write(serial, 0x0, 0x31);
> > + pl2303_vendor_write(serial, 0x1, 0x01);
> > + }
>
> And this looks like an unrelated change since it affects older devices
> and not HXN.
>
> Please split out in a separate patch and explain why it is needed. Also
> replace the "magical" constants with descriptive defines.
>
> > }
> >
> > kfree(buf);
> > @@ -720,8 +782,12 @@ 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->quirks & PL2303_QUIRK_LEGACY_N)
> > + pl2303_vendor_write(serial, 7, 0); /* reset new chip */
>
> Please add a define for 7 here too.
>
> > + else {
> > + pl2303_vendor_write(serial, 8, 0);
> > + pl2303_vendor_write(serial, 9, 0);
> > + }
> > }
>
> Thanks,
> Johan
