Re: [PATCH v2] USB: serial: pl2303: add new PID to support PL256X (TYPE_MP)

[Johan Hovold <johan@kernel.org>]

On Mon, Jun 08, 2026 at 06:18:19PM +0800, Charles Yeh wrote:

> > > --- a/drivers/usb/serial/pl2303.c
> > > +++ b/drivers/usb/serial/pl2303.c
> > > @@ -54,6 +54,7 @@ static const struct usb_device_id id_table[] = {
> > >       { 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(PL2303_VENDOR_ID, PL256X_PRODUCT_ID_4P) },
> >
> > Which devices is this? PL2533 (0x2533)?
> >
> > What about the other variants you mention above (PL2543, PL2565)? Do
> > they use separate PIDs?
> 
> PL256X_PRODUCT_ID_4P is for the PL2533 device, which uses PID 0x2533.

Shouldn't you name the define accordingly? That is something like

	PL2303_PRODUCT_ID_PL2533

with a common (driver) prefix and a product name. "4P" sounds too
generic to me.

[ The older ones where all variants of PL2303 so there was no need to
repeat "PL2303" as part of the product name. ]

> > Can you please post the output of usb-devices (or lsusb -v) for this
> > device?
> 
> Bus 002 Device 007: ID 067b:2533 Prolific Technology, Inc. PL2543
> USB2.0 HS Serial Port
> Couldn't open device, some information will be missing
> Negotiated speed: High Speed (480Mbps)
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0 [unknown]
>   bDeviceSubClass         0 [unknown]
>   bDeviceProtocol         0
>   bMaxPacketSize0        64
>   idVendor           0x067b Prolific Technology, Inc.
>   idProduct          0x2533 PL2543 USB2.0 HS Serial Port

Did you mean to say "PL2543" above (i.e. not "PL2533")?

Or is the string product string not correct here?

>   bcdDevice           33.04
>   iManufacturer           1 Prolific Technology Inc.
>   iProduct                2 PL2543 USB2.0 HS Serial Port
>   iSerial                 3 C00000000C0001
>   bNumConfigurations      1
>   Configuration Descriptor:
>     bLength                 9
>     bDescriptorType         2
>     wTotalLength       0x0081
>     bNumInterfaces          4
>     bConfigurationValue     1
>     iConfiguration          0
>     bmAttributes         0xa0
>       (Bus Powered)
>       Remote Wakeup
>     MaxPower              100mA
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber        0
>       bAlternateSetting       0
>       bNumEndpoints           3
>       bInterfaceClass       255 Vendor Specific Class
>       bInterfaceSubClass      0 [unknown]
>       bInterfaceProtocol      0

So they are vendor class. Thanks for the details.

> > > +#define PL256X_FLOWCTRL_MASK         0x43
> >
> > > +#define PL256X_FLOWCTRL_XON_XOFF     0x40
> > > +#define PL256X_FLOWCTRL_RTS_CTS              0x03
> >
> > Does the device support having hardware and software flow control
> > enabled at the same time?
> 
> Yes, the device supports enabling both hardware flow control (RTS/CTS)
> and software flow control (XON/XOFF) simultaneously.

Thanks, perhaps we can add support for that at some point.

> > > @@ -461,6 +510,14 @@ static int pl2303_detect_type(struct usb_serial *serial)
> > >               case 0x905:     /* GT-2AB */
> > >               case 0x1005:    /* GC-Q20 */
> > >                       return TYPE_HXN;
> > > +             case 0x3302:
> > > +             case 0x3304:
> > > +             case 0x4302:
> > > +             case 0x4304:
> > > +             case 0x6502:
> > > +             case 0x6504:
> > > +             case 0x6506:
> >
> > Can you add a comment here after each number so we know which device
> > type they map to?
> 
> Got it. I will add comments after each bcdDevice case to explicitly map
> them to their corresponding device types
> (e.g., PL2533-2P (SSOP),  PL2533-2P (QFN), etc.) in the v3 patch.

Thanks.

> > > -     if (type != TYPE_HXN) {
> > > +     if (type == TYPE_MP)
> > > +             pl2303_vendor_write(serial, spriv->flowctrl_reg, PL256X_FLOWCTRL_NONE);
> >
> > Why you need to disable flow control here? The setting should be updated
> > per termios when the device is later opened.
> 
> This is used to program the hardware default state during startup.

But my point is that it should not be needed as this is done when
opening the port (the call to set_termios() in pl2303_open()).

You may need to change the logic a little (e.g. type != HXN && type !=
MP), but that should be all that is needed here.

Johan