Re: [PATCH] [PATCH v6] USB: serial: pl2303: Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold <johan@kernel.org>]

On Mon, Jul 01, 2019 at 08:21:14PM +0800, Charles Yeh wrote:
> Prolific has developed a new USB to UART chip: PL2303HXN
> PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
> 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 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
>    PL2303_READ_TYPE_HX_STATUS 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 reset
>    down/up stream used by TYPE_HX.
>    Therefore, we will also execute different instructions here.
>    The default of chip Reset Control is 0xFF(TYPE_HXN), therefore we will
>    write 0x00 to reset down/up stream(TYPE_HXN).
> 
> 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.
> 
> 6. In pl2303_update_reg: TYPE_HXN used different register for flow control.
>    Therefore, we will also execute different instructions here.
> 
> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
> changelog:
> v6:
> 1. Modify pl2303_update_reg:TYPE_HXN used different register for flow control.
>    Therefore, we will also execute different instructions here.
> 2. Modify define name: PL2303_HXN_RESET_DOWN_UPSTREAM to
>    PL2303_HXN_RESET_CONTROL
> 3. Re-Sorting flow-control register definition by address.
> 4. Indent continuation lines at least tw tabs.
> 
> v5:
> 1. Modify pl2303_update_reg
> 2. add a patch version on subject
> 3. add a space after each colon at subject line
> ---
>  drivers/usb/serial/pl2303.c | 127 +++++++++++++++++++++++++++++-------
>  drivers/usb/serial/pl2303.h |   7 +-
>  2 files changed, 108 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index d7abde14b3cf..d36d53b234c4 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, PL2303_PRODUCT_ID_GC) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
> +	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_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),
> @@ -130,9 +136,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
> @@ -147,12 +155,22 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define UART_CTS			0x80
>  
>  #define PL2303_FLOWCTRL_MASK		0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK	0x1C

Move after PL2303_HXN_RESET_CONTROL as I suggested.

> +#define PL2303_READ_TYPE_HX_STATUS	0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL	0x07
> +#define PL2303_HXN_FLOWCTRL		0x0A
> +#define PL2303_HXN_CTRL_XON_XOFF	0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS		0x18
> +#define PL2303_HXN_CTRL_NONE		0x1C

> @@ -719,14 +771,34 @@ static void pl2303_set_termios(struct tty_struct *tty,
>  	}
>  
>  	if (C_CRTSCTS(tty)) {
> -		if (spriv->quirks & PL2303_QUIRK_LEGACY)
> -			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x40);
> -		else
> -			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0x60);
> +		if (spriv->quirks & PL2303_QUIRK_LEGACY) {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0x40);

No need to break this line even if you end up with slightly more than 80
chars.

> +		} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_RTS_CTS);

Again, continuation lines should be indented at least two tabs further
(you only use one tab now).

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0x60);

No need to break line.

> +		}
>  	} else if (pl2303_enable_xonxoff(tty, spriv->type)) {
> -		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0xc0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_XON_XOFF);

Indentation.

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK,
> +				0xc0);

No need to break line.

> +		}
>  	} else {
> -		pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> +		if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
> +			pl2303_update_reg(serial, PL2303_HXN_FLOWCTRL,
> +				PL2303_HXN_FLOWCTRL_MASK,
> +				PL2303_HXN_CTRL_NONE);

Indentation.

> +		} else {
> +			pl2303_update_reg(serial, 0, PL2303_FLOWCTRL_MASK, 0);
> +		}
>  	}
>  
>  	kfree(buf);
> @@ -767,8 +839,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]) {

Merge this with else above to reduce indentation, that is

	} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {

> +			pl2303_vendor_write(serial, PL2303_HXN_RESET_CONTROL,
> +				0);

You again completely ignored my question about why you're wring 0
instead of 3 here.

I'll ignore your patch until you explain.

> +		} else {
> +			pl2303_vendor_write(serial, 8, 0);
> +			pl2303_vendor_write(serial, 9, 0);
> +		}
>  	}

Johan