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

On Mon, Jul 27, 2026 at 06:27:58PM +0800, Crescent Hsieh wrote:
> 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:

> > >  /* 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.

That sounds like a good solution.

Johan