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
