> > + 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.
>
OK. I will ignore "WARNING: line over 80 characters".
> > + } 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).
>
I have done it the way you do today...but after checking
./scripts/checkpatch.pl.. I got another warning message...
So I am a little confused now...
Previously before submitting.. must first pass ./scripts/checkpatch.pl
check. No ERROR, or WARRING message...
I will return to the office tomorrow ... I will post another warning
message (according to the way you mentioned)
> } 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.
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).
Charles.
Johan Hovold <jo...@kernel.org> 於 2019年7月1日 週一 下午9:32寫道：
>
> 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 <charlesyeh...@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
