Re: [PATCH v2 2/4] USB: serial: mxuport: add MUX50U-based device support

[Crescent Hsieh <crescentcy.hsieh@moxa.com>]

On Tue, Jul 21, 2026 at 04:59:05PM +0200, Johan Hovold wrote:
> On Tue, Jun 23, 2026 at 04:01:37PM +0800, Crescent Hsieh wrote:
> > @@ -7,8 +7,11 @@
> >   *
> >   *	Supports the following Moxa USB to serial converters:
> >   *	 2 ports : UPort 1250, UPort 1250I
> > + *		   UPort 1250 G2, UPort 1250I G2
> >   *	 4 ports : UPort 1410, UPort 1450, UPort 1450I
> > + *		   UPort 1410 G2, UPort 1450 G2, UPort 1450I G2
> >   *	 8 ports : UPort 1610-8, UPort 1650-8
> > + *		   UPort 1610-8 G2, UPort 1650-8 G2
> >   *	16 ports : UPort 1610-16, UPort 1650-16
> 
> Should you add the MUx50U ones here as well for completeness? At least
> some of them seem to be missing.
> 
> But perhaps it's just me not getting how these PIDs map to products.
>
> >  */
> > 
> > @@ -37,6 +40,24 @@
> >  #define MX_UPORT1613_PID	0x1613
> >  #define MX_UPORT1653_PID	0x1653
> >  
> > +#define MX_UPORT1252_PID	0x1252
> > +#define MX_UPORT1253_PID	0x1253
> > +#define MX_UPORT1411_PID	0x1411
> > +#define MX_UPORT1452_PID	0x1452
> > +#define MX_UPORT1453_PID	0x1453
> > +#define MX_UPORT1619_PID	0x1619
> > +#define MX_UPORT1659_PID	0x1659
> > +#define MX_UPORT165A_PID	0x165a
> > +#define MX_UPORT165B_PID	0x165b
> 
> I realise this is how the existing PID defines are named, but would it
> not be possible to name the defines after the products they are used
> for?

Okay. I will complete the supported-device list and rename the PID
definitions after their corresponding product names in v3.

> >  /* Table of devices that work with this driver */
> >  static const struct usb_device_id mxuport_idtable[] = {
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1250_PID),
> > -	  .driver_info = MX_PORTS(2) },
> > +	  .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1251_PID),
> > -	  .driver_info = MX_PORTS(2) },
> > +	  .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID),
> > -	  .driver_info = MX_PORTS(4) },
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1450_PID),
> > -	  .driver_info = MX_PORTS(4) },
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1451_PID),
> > -	  .driver_info = MX_PORTS(4) },
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1618_PID),
> > -	  .driver_info = MX_PORTS(8) },
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1658_PID),
> > -	  .driver_info = MX_PORTS(8) },
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1613_PID),
> > -	  .driver_info = MX_PORTS(16) },
> > +	  .driver_info = MX_DEVICE_INFO(16, MX_FW_UPORT_G1) },
> >  	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1653_PID),
> > -	  .driver_info = MX_PORTS(16) },
> 
> My initial reaction was that it would be better to just leave the gen1
> entries alone (with family implicitly set to MX_FW_UPORT_G1), but I
> guess this is fine too.
> 
> > +	  .driver_info = MX_DEVICE_INFO(16, MX_FW_UPORT_G1) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1252_PID),
> > +	  .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1253_PID),
> > +	  .driver_info = MX_DEVICE_INFO(2, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1411_PID),
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1452_PID),
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1453_PID),
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1619_PID),
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1659_PID),
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT165A_PID),
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT165B_PID),
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_UPORT_G2) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU250U_PID),
> > +	  .driver_info = MX_DEVICE_INFO(2, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU450U_PID),
> > +	  .driver_info = MX_DEVICE_INFO(4, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_PID),
> > +	  .driver_info = MX_DEVICE_INFO(8, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_6PORT_PID),
> > +	  .driver_info = MX_DEVICE_INFO(6, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MUX50U_3PORT_PID),
> > +	  .driver_info = MX_DEVICE_INFO(3, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_5PORT_PID),
> > +	  .driver_info = MX_DEVICE_INFO(5, MX_FW_PLATFORM_UART) },
> > +	{ USB_DEVICE(MX_USBSERIAL_VID, MX_MU850U_7PORT_PID),
> > +	  .driver_info = MX_DEVICE_INFO(7, MX_FW_PLATFORM_UART) },
> >  	{}			/* Terminating entry */
> >  };
> 
> Perhaps the family defines can be shortened to make this a bit more
> readable, for example:
> 
> 	MX_FW_UP_G1
> 	MX_FW_UP_G2
> 	MX_FW_PF
> 
> or even
> 
> 	MX_UP_G1
> 	MX_UP_G2
> 	MX_PF
> 
> I didn't try and see what the result looks like. Perhaps the more
> verbose names are preferred.

I used the verbose names to keep the firmware-family naming explicit,
but I agree that readability is important as well.

Listing each device-table entry on a single line would make the table
easier to read, but doing so directly would result in overly long lines.
Another option would be to introduce a small helper macro:

#define MX_DEVICE(pid, ports, fw)			\
	{ USB_DEVICE(MX_USBSERIAL_VID, pid),		\
	  .driver_info = MX_DEVICE_INFO(ports, fw) }

The ID table could then be written as:

MX_DEVICE(MX_UPORT1250_PID,  2, MX_FW_UPORT_G1),
MX_DEVICE(MX_UPORT1250I_PID, 2, MX_FW_UPORT_G1),
...
MX_DEVICE(MX_UPORT1250_G2_PID,   2, MX_FW_UPORT_G2),
MX_DEVICE(MX_UPORT12150I_G2_PID, 2, MX_FW_UPORT_G2),
...
MX_DEVICE(MX_MU850U,           8, MX_FW_PLATFORM_UART),
MX_DEVICE(MX_MUX50U_6PORT_PID, 6, MX_FW_PLATFORM_UART),

This would allow the firmware-family names to remain explicit while
keeping each device entry on a single line.

---
Sincerely,
Crescent Hsieh