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 <charlesyeh...@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
