On Wed, Dec 26, 2018 at 08:28:10PM +0800, Charles Yeh wrote:
> Add new PID to support PL2303TB (TYPE_HX)
> Add new PID to support PL2303(N)GC/GB/GS/GT/GL/GE (TYPE_HXN)
> Add new Flow Control to support PL2303(N)GC/GB/GS/GT/GL/GE (TYPE_HXN)
> Add new Pull-Up Mode to support PL2303HXD (TYPE_HX)
So the above should go in three separate patches as we already discussed
(PL2303TB PID, TYPE_HXN support + PIDs, "pull-up mode").
> Fixed warning:restricted__le16 degrades to integer
When you update and resend a patch, it's good to include changelog
information like this, but please put it below the cut-off line (---)
below so that it doesn't end up in the commit log.
Also include a patch revision in the Subject of the mail. Let's call
this one v2, so next time use the following Subject prefix:
[PATCH v3]: USB: serial: pl2303: add ...
(also after breaking the current patch into a series of three or more
patches).
> Signed-off-by: Charles Yeh <charlesyeh...@gmail.com>
> ---
> drivers/usb/serial/pl2303.c | 116 +++++++++++++++++++++++++++++-------
> drivers/usb/serial/pl2303.h | 11 ++++
> 2 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index a4e0d13fc121..8c71612e1811 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -46,6 +46,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 +130,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
> @@ -139,11 +148,21 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define UART_OVERRUN_ERROR 0x40
> #define UART_CTS 0x80
>
> +#define TYPE_HX_READ_OTP_STATUS_REGISTER 0x8484
> +#define TYPE_HX_EXTERNAL_PULLUP_MODE 0x08
> +#define TYPE_HX_PULLUP_MODE_REG 0x09
> +#define TYPE_HXN_UART_FLOWCONTROL 0x0A
> +#define TYPE_HXN_HARDWAREFLOW 0xFA
> +#define TYPE_HXN_SOFTWAREFLOW 0xEE
> +#define TYPE_HXN_NOFLOW 0xFF
> +#define TYPE_HXN_RESET_DOWN_UPSTREAM 0x07
> +
> 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
> };
>
> @@ -173,16 +192,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__,
> @@ -201,12 +230,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__,
> @@ -286,6 +322,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 +344,36 @@ 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_OTP_STATUS_REGISTER, 0, buf, 1, 100);
> + if (res != 1)
> + type = TYPE_HXN;
> + }
So HXN looks like an HX, but responds with an error when trying to read
any (?) register using VENDOR_READ_REQUEST? Isn't there any more direct
way of determining the device type?
My old HXD device report bcdUSB as 0x0110, and you tested you also
tested against 0x0200 in a previous version of this patch. How does
bcdUSB relate to the various types?
Note that you should use le16_to_cpu() to access bcdUSB safely also on
big-endian systems (which the kbuild test robot somewhat cryptically
reported).
> +
> 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);
> + }
Is this even needed for pre-HXN devices, or could perhaps some of it
just be removed? Can you explain what it does?
> kfree(buf);
>
> @@ -671,15 +718,37 @@ static void pl2303_set_termios(struct tty_struct *tty,
> }
>
> if (C_CRTSCTS(tty)) {
> - if (spriv->quirks & PL2303_QUIRK_LEGACY)
> + if (spriv->type == &pl2303_type_data[TYPE_01])
> pl2303_vendor_write(serial, 0x0, 0x41);
> + else if (spriv->type == &pl2303_type_data[TYPE_HXN])
> + pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
> + TYPE_HXN_HARDWAREFLOW);
> 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->type == &pl2303_type_data[TYPE_HXN])
> + pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
> + TYPE_HXN_SOFTWAREFLOW);
> + else
> + pl2303_vendor_write(serial, 0x0, 0xc0);
> } else {
> - pl2303_vendor_write(serial, 0x0, 0x0);
> + if (spriv->type == &pl2303_type_data[TYPE_HXN])
> + pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
> + TYPE_HXN_NOFLOW);
> + else
> + pl2303_vendor_write(serial, 0x0, 0x0);
> + }
So this is becoming a bit hard to read. The idea with the type struct
was to abstract the differences. We could add something like
u8 uart_flowctrl_reg;
u8 uart_flowctrl_hw;
u8 uart_flowctrl_sw;
u8 uart_flowctrl_none;
to struct pl2303_type_data, and replace the above with just three calls
to pl2303_vendor_write().
If you do this, then do this as a preparatory patch before adding HXN
support.
We should probably do something similar with the read and write requests
instead of adding conditionals in those paths.
> +
> + if (spriv->type == &pl2303_type_data[TYPE_HX]) {
> + pl2303_vendor_read(serial, 0x8484, buf);
> + pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
> + pl2303_vendor_read(serial, 0x8484, buf);
> + pl2303_vendor_read(serial, 0x8383, buf);
> + if ((u16)*buf & TYPE_HX_EXTERNAL_PULLUP_MODE) {
> + pl2303_vendor_write(serial, 0x0, 0x31);
> + pl2303_vendor_write(serial, 0x1, 0x01);
> + }
So this bit needs to go in it's own patch with a commit message
explaining why it is needed. Don't forget to replace the "magic
constants" with descriptive defines.
*buf is u8 and no need to cast to u16, as I also already pointed out in
my comments to an earlier version of the patch.
> }
>
> kfree(buf);
> @@ -720,8 +789,13 @@ 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,
> + TYPE_HXN_RESET_DOWN_UPSTREAM, 0);
> + else {
> + pl2303_vendor_write(serial, 8, 0);
> + pl2303_vendor_write(serial, 9, 0);
> + }
So this is a bit harder to abstract, but could be done using function
pointers although it's probably not worth it just yet.
Just make sure to do add bracket ({}) also to the branch you add here.
Thanks,
Johan
