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

[Johan Hovold <johan@kernel.org>]

On Tue, Jul 02, 2019 at 08:30:06PM +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.

> Signed-off-by: Charles Yeh <charlesyeh522@gmail.com>
> ---
> changelog:
> v7:
> 1. Add PL2303_HXN_RESET_CONTROL_MASK define.
> 2. In pl2303_open,use PL2303_HXN_RESET_CONTROL_MASK & PL2303_HXN_RESET_CONTROL
>    to reset the upstream and downstream pipe data
> 3. Ignore "WARNING: line over 80 characters" at #776,#782,#790
 
>  #define PL2303_FLOWCTRL_MASK		0xf0
> +#define PL2303_HXN_FLOWCTRL_MASK	0x1C
> +#define PL2303_HXN_FLOWCTRL		0x0A

I asked you to keep related defines together (and to move the mask where
the register define was, not the other way round). Please move these to
the other HXN defines below, and keep the register address defines
before the corresponding bit defines.

> +#define PL2303_READ_TYPE_HX_STATUS	0x8080
> +
> +#define PL2303_HXN_RESET_CONTROL_MASK	0x03

This makes no sense. The whole register is used for reset. If you want a
define that can be used for resetting both pipes then add two separate
defines for up and down respectively, and add a third define for
resetting both buffer as a bitwise OR of the two.

Remember that the code should be self-documenting as far as possible so
picking descriptive names is important.

Also move this one after the corresponding register address define
below.

> +#define PL2303_HXN_RESET_CONTROL	0x07
> +#define PL2303_HXN_CTRL_XON_XOFF	0x0C
> +#define PL2303_HXN_CTRL_RTS_CTS		0x18
> +#define PL2303_HXN_CTRL_NONE		0x1C

> @@ -765,8 +835,11 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	if (spriv->quirks & PL2303_QUIRK_LEGACY) {
>  		usb_clear_halt(serial->dev, port->write_urb->pipe);
>  		usb_clear_halt(serial->dev, port->read_urb->pipe);
> -	} else {
> +	} else if (spriv->type == &pl2303_type_data[TYPE_HXN]) {
>  		/* reset upstream data pipes */

This comment belongs in the last else block. Your new code shouldn't
need one.

> +		pl2303_update_reg(serial, PL2303_HXN_RESET_CONTROL,
> +				PL2303_HXN_RESET_CONTROL_MASK, 0x03);

So two things; first, do you really need to read back the current value?
I would assume that it always reads back as 0 and that writing 0x03 in
this case would be sufficient to reset both buffers.

Second, please use a define for 0x03; no magic constants, as we have
discussed before. You don't need a separate mask define if you're always
resetting both buffers together (just use the same value define twice).

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

Johan