From: Johan Hovold <johan@kernel.org>
To: Charles Yeh <charlesyeh522@gmail.com>
Cc: gregkh@linuxfoundation.org, johan@kernel.org,
charles-yeh@prolific.com.tw, linux-usb@vger.kernel.org
Subject: Add Proliic new chip: PL2303TB & PL2303N(G)
Date: Tue, 18 Dec 2018 11:02:42 +0100 [thread overview]
Message-ID: <20181218100242.GK20658@localhost> (raw)
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@gmail.com>
> 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@gmail.com>
> ---
> 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
next reply other threads:[~2018-12-18 10:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-18 10:02 Johan Hovold [this message]
-- strict thread matches above, loose matches on Subject: below --
2019-01-09 1:12 Add Proliic new chip: PL2303TB & PL2303N(G) Yeh.Charles [葉榮鑫]
2019-01-08 17:01 Johan Hovold
2019-01-08 14:16 Johan Hovold
2018-12-26 12:28 Charles Yeh
2018-12-19 10:35 Greg Kroah-Hartman
2018-12-19 10:26 Charles Yeh
2018-12-17 13:03 Greg Kroah-Hartman
2018-12-17 4:51 Charles Yeh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181218100242.GK20658@localhost \
--to=johan@kernel.org \
--cc=charles-yeh@prolific.com.tw \
--cc=charlesyeh522@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.