USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)

[Johan Hovold <johan@kernel.org>]

Hi Charles,

Please fix your mail setup so that you can respond inline as is
customary on the lists. Your mails are currently unnecessarily hard to
read due to it not being clear what is quoted text and what isn't.

Specifically, quoted text should have an extra "> " at the start of the
line, and don't copy text and respond to it (just reply *inline*).

Currently there's a second copy of the mail your responding to at the
end of your mails. That bit should instead contain your replies (with
unnecessary context removed).

On Tue, Apr 09, 2019 at 05:52:45PM +0800, Charles Yeh wrote:
> Hi Johan,
>       1. The HXN register layout is entirely different from HX and
>            earlier devices.
> 
>         2. HXN use the same CDC class requests (line encoding, etc) as
>            earlier revisions.
> 
> Can you send me documentation for the HXN protocol? That would help a
> lot in finding the right abstraction level for this.
> 
> Ans：
> Yes,The HXN register layout is entirely different from HX and earlier devices,
> Yes, HXN use the same CDC class requests (line encoding, etc) as
> earlier revisions

Ok, thanks for confirming.

> I don't have a PL2303G (HXN) Word file, I only have Excel's table file,
> please refer to PL2303_Linux\Vendor_Requesut.png

Did you forget to attach the png? Excel files describing the protocol
would do just fine.

> Or you can install window driver use PL2303G demo board.

> and then USB Bus Hound to analysis the vendor request.

No, I'm certainly not going to reverse engineer your Windows driver in
order to help you.

Surely, you must have some documentation for your devices?

> The original Prolific company is hoping:
> TYPE_HX and TYPE_01 used old driver(pl2303.c & pl2303.h)
> The new PL2303G (TYPE_HXN) uses new driver(plser.c & plser.h),
> 
> You can go to the website of Prolific, in the website,
> 
> TYPE_HX and TYPE_01 are called
> PL2303 USB to Serial Bridge Controllers (All Chip Versions)
> 
> TYPE_HXN is called
> PL2303G USB to Serial Bridge Controllers (All Chip Versions).
> 
> We provide two different file paths on the website, the name.file..
> is to distinguish between TYPE_HX/01 and TYPE_HXN
> please refer to PL2303_Linux\Window_support_hx_hxn.jpg
> 
> TYPE_HX and TYPE_01:the windows file name is ser2pl.sys & ser2pl64.sys.
> 
> TYPE_HXN:the windows file name is plser.sys & plser64.sys.
> 
> please refer to PL2303_Linux\window_plser_ser2pl.bmp
> 
> But Greg disagreed with the Linux OS about new File name(plser.c & plser.h)
> So we did that we merged with the existing pl2303.c(h)
> into a new chip (PL2303G, TYPE_HXN)

Yes, the plser driver you submitted was just a copy of pl2303, with
minimal changes to support pl2303g. With that much code being shared you
need to add support for the new device type to the current driver.

But the lack of documentation of the protocols used is making it hard to
determine the appropriate abstraction level.

> > +#define TYPE_HXN_FLOWCONTROL_REG     0x0A
> 
> > +#define TYPE_HXN_HARDWAREFLOW_DATA   0xFA
> > +#define TYPE_HXN_SOFTWAREFLOW_DATA   0xEE
> > +#define TYPE_HXN_NOFLOW_DATA         0xFF
> 
> What exactly does bits 0x15 (bits 4, 2 and 0) do?
> Ans:please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> 
> Is register 0x0a really only used for flow control?
> Ans: Yes, please refer to PL2303_Linux\PL2303G_TYPE_HXN_UART_Flow.jpg,
> 0x0A(Bit 2, 3, 4)is UART flow control settting,
> it only supports TYPE_HXN, no supports TYPE_HX/01.

Ok, thanks (found the link at the end of the mail now).

> In an earlier version of your patch, you also checked bcdUSB here. Why
> did you remove it?
> 
> > +             res = usb_control_msg(serial->dev,
> > +                     usb_rcvctrlpipe(serial->dev, 0),
> > +                     VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
> > +                     TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);
> 
> Please name the registers after what they do, not how you use them. What
> is register 0 that you read here? Does it have a name?
> 
> Ans: Because the "kbuild test robot <lkp@intel.com>" reply me a error.
> Please refer to Email: "Tuesday, December 25, 2018, at 11:39 pm"
> or refer to PL2303_Linux\remoe_checked_bcdUSB.txt

Yes, you needed to use helper function to deal with endianess as was
pointed out to you in that thread. Why didn't you use the helper, and
instead just dropped the check?

> Why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does?
> Is that even correct?
> Ans: Yes, it is correct,
> The Output(TX/DTR/RTS) Tri-state H/W design is different.
> 
> TYPE_01: RS-232 Output Tri-state:
> 0: RS-232 output in output mode;
> 1: RS-232 output tri-state.
> 
> TYPE_HX_Serial Port (TXD, RTS, and DTR) Output Enable:
> 00 – Disable output signals (Tri-State) at all time;
> 01 – Disable output signals (Tri-State) during suspend;
> 10, 11 – Set output signals to HIGH during suspend;

Ok.

> >       if (C_CRTSCTS(tty)) {
> > -             if (spriv->quirks & PL2303_QUIRK_LEGACY)
> > -                     pl2303_vendor_write(serial, 0x0, 0x41);
> 
> Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support
> both auto-rts and auto-cts?
> 
> Ans: Yes, it is correct,
> The Hardware UART flow control settting design is different
> between TYPT_01 and TYPT_HX.
> 
> Although I have been in Prolific Company for almost 16 years..
> But TYPE_01 (PL2303H) has been discontinued(EOL) for 13 years...
> Now can't find this IC(TYPT_01: PL2303H) on the market..

Sure, but we still need to make sure we don't break any working setups
using these old devices, so I still need to understand how the older
protocols work.

> The line"if (spriv->quirks & PL2303_QUIRK_LEGACY)"
> you are asking now is not what I wrote...
> I don't know why to define it as PL2303_QUIRK_LEGACY.

I added that at some point in an attempt to clean up this
reverse-engineering driver. You can read it as a TYPE_01 test. 

> I am mainly responsible for the PL2303 driver
> Windows, WinCE, Mac & Android in Prolific.
> On the Linux driver side, only when the customer has a problem,
> or want to add new features (such as GPIO control, new Baud rate),
> I will rewrite the Linux code.
> Rewritten Linux code will only be given to the corresponding customer.
> 
> Just like this time, because I want to add PL2303G (TYPE_HXN) to Linux,
> I just rewritten the Linux code...
> For some definitions in the Linux code,
> such as why it is defined as PL2303_QUIRK_LEGACY,
> I really don't know very well
> 
> Before rewriting the Linux code,
> I can only guess "PL2303_QUIRK_LEGACY" that this refers to the
> old PL2303 (PL2303H, TYPE_01)..

Right.

> After the actual test (I have tested the old PL2303H chip on Linux),
> when I plug in the PL2303H (TYPE_01), the Linux code will execute the code:
> if (spriv->quirks & PL2303_QUIRK_LEGACY) itinerary code.
> So I confirm This "PL2303_QUIRK_LEGACY" of code refers to PL2303H(TYPE_01)

Correct, but my question was about bit 0x20 for TYPE_01 devices; why
isn't it being as set as for TYPE_HX?

> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_SOFTWAREFLOW_DATA);
> >               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);
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_SOFTWAREFLOW_DATA);
> >       } else {
> > -             pl2303_vendor_write(serial, 0x0, 0x0);
> > +             if (spriv->type == &pl2303_type_data[TYPE_HXN])
> > +                     pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
> > +                                             TYPE_HXN_NOFLOW_DATA);
> > +             else
> > +                     pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
> > +                                             TYPE_HX_01_NOFLOW_DATA);
> 
> As already mentioned, the above is hardly readable. When studying the
> current driver, I noticed a couple of bugs that I'm preparing fixes for.
> 
> Specifically, we shouldn't be overwriting the entire control register,
> which changes the tranceiver suspend mode. Are there similar problems
> with not doing bit updates of register 0x0a?
> 
> Either way, rebasing your patches on top of those should allow this to
> be cleaned up somewhat.
> 
> 
> Ans: the three chips TYPE_01 / TYPE_HX / TYPE_HXN have their own independent
>  flow control Register settings...
> What advice do you have for handling here?
> 
> Here I have actually tested the three chips TYPE_01 / TYPE_HX / TYPE_HXN
> TYPE_01: PL2303H
> TYPE_HX: PL2303HXA/ PL2303XA/ PL2303HXD/PL2303TA/PL2303TB/PL2303RA/ PL2303SA.
> TYPE_HXN: PL2303GC, PL2303GS, PL2303GL...
> 
> The test result is OK..

Well, without documentation it is hard to give advice. But the
information you provided above, indicates that you should not be
overwriting these registers completely for HXN either when updating the
flow-control settings. Only the bits 2..4 (and possibly 0..1) should be
written.

Take a look at the patches I merged for doing this on the older devices.
You should be able to use the same helper when updating these
registers.

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=f64c3ab230682e8395a7fbd01f3eb5140c837d4e

> > +                     pl2303_vendor_write(serial,
> > +                                     HXN_RESET_DOWN_UPSTREAM_REG,
> > +                                     HXN_RESET_DOWN_UPSTREAM_DATA);
> 
> Can you write anything to this register to reset the buffers, or does it
> have to be 0?
> Ans: It have to reset to 0, please refer to PL2303_Linux\PL2303G_ChipReset.jpg

But that jpg shows only bit 0 and 1 being used for data pipe reset. And
shouldn't you be writing 0x02 to reset those?

> > +             else {
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG1,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> > +                     pl2303_vendor_write(serial,
> > +                                     HX_RESET_DOWN_UPSTREAM_REG2,
> > +                                     HX_RESET_DOWN_UPSTREAM_DATA);
> 
> I assume the older versions allow for the and up and down buffers to be
> reset independently? Please name these registers accordingly.
> Ans: Yes, reset independently,
> please refer to PL2303_Linux\PL2303_TYPE_HX_Reset.jpg
> The original code is
>         /* reset upstream data pipes */
>         pl2303_vendor_write(serial, 8, 0);
>         pl2303_vendor_write(serial, 9, 0);
> 
> You previously suggested that I am not using "magical" constants directly,
> Please use defines rather than "magical" Constants.
> What advice do you have for handling here?

Name register and bit-definitions after what they do. We need code to be
clean and mostly self-documenting. In this case, based on the
information provided above, you could use

	#define PL2303_RESET_DOWNSTREAM_PIPE	8
	#define PL2303_RESET_UPSTREAM_PIPE	9

for example.

As the protocol appears to mandate writing 0 to these registers, it
may be ok to keep that as a numerical constant.

Johan