Re: [PATCH v1 1/4] usb: serial: mxuport: add support for more MXU50U UART devices

[Johan Hovold <johan@kernel.org>]

On Tue, Mar 24, 2026 at 11:50:38AM +0800, Crescent Hsieh wrote:
> Add support for additional Moxa devices using the MXU50U UART family.

Please mention which models since its not obvious from the PID defines
which mostly reflects the PID itself.

You should amend the driver header as well which lists the currently
supported models.

> Extend the device ID table and port-count handling for 3-, 5-, 6- and
> 7-port devices, and update firmware selection to use the matching
> MXU50U firmware images and version offsets.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  drivers/usb/serial/mxuport.c | 132 +++++++++++++++++++++++++++++++----
>  1 file changed, 118 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index ad5fdf55a02e..034b506322c2 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
> @@ -39,6 +39,25 @@
>  #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
> +
> +#define MX_MU250U_PID		0x0250
> +#define MX_MU450U_PID		0x0450
> +#define MX_MU850U_PID		0x0850
> +
> +#define MX_MU850U_6PORT_PID	0x7002
> +#define MX_MUX50U_3PORT_PID	0x7003
> +#define MX_MU850U_5PORT_PID	0x7004
> +#define MX_MU850U_7PORT_PID	0x7005
> +
>  /* Definitions for USB info */
>  #define HEADER_SIZE		4
>  #define EVENT_LENGTH		8
> @@ -48,6 +67,9 @@
>  #define VER_ADDR_1		0x20
>  #define VER_ADDR_2		0x24
>  #define VER_ADDR_3		0x28
> +#define NEW_ADDR_1		0x86
> +#define NEW_ADDR_2		0x88
> +#define NEW_ADDR_3		0x8A

These are still offsets for the version so please include that in the
name. And "new" tends to get old, better to include the family name, so
something like:

	MXU50U_VER_ADDR_n

>  /* Definitions for USB vendor request */
>  #define RQ_VENDOR_NONE			0x00
> @@ -147,9 +169,13 @@
>  #define MX_WAIT_FOR_SEND_NEXT		0x0080
>  
>  #define MX_UPORT_2_PORT			BIT(0)
> -#define MX_UPORT_4_PORT			BIT(1)
> -#define MX_UPORT_8_PORT			BIT(2)
> -#define MX_UPORT_16_PORT		BIT(3)
> +#define MX_UPORT_3_PORT			BIT(1)
> +#define MX_UPORT_4_PORT			BIT(2)
> +#define MX_UPORT_5_PORT			BIT(3)
> +#define MX_UPORT_6_PORT			BIT(4)
> +#define MX_UPORT_7_PORT			BIT(5)
> +#define MX_UPORT_8_PORT			BIT(6)
> +#define MX_UPORT_16_PORT		BIT(7)

We don't need 8 bits to encode up to 16 ports. Please rebase this on the
patch I just submitted here which cleans this up:

	https://lore.kernel.org/r/20260507141114.284470-1-johan@kernel.org

> @@ -1053,6 +1119,7 @@ static int mxuport_probe(struct usb_serial *serial,
>  	int local_ver;
>  	char buf[32];
>  	int err;
> +	bool is_mux50u = false;
>  
>  	/* Load our firmware */
>  	err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_QUERY_FW_CONFIG, 0, 0);
> @@ -1065,12 +1132,41 @@ static int mxuport_probe(struct usb_serial *serial,
>  	if (err < 0)
>  		return err;
>  
> -	dev_dbg(&serial->interface->dev, "Device firmware version v%x.%x.%x\n",
> +	dev_dbg(&serial->interface->dev, "Device firmware version v%d.%d.%d\n",

This looks like an unrelated change. Please do this in preparatory patch
explaining why you prefer decimal notation.

>  		(version & 0xff0000) >> 16,
>  		(version & 0xff00) >> 8,
>  		(version & 0xff));
>  
> -	snprintf(buf, sizeof(buf) - 1, "moxa/moxa-%04x.fw", productid);
> +	switch (productid) {
> +	case MX_UPORT1252_PID:
> +	case MX_UPORT1253_PID:
> +	case MX_UPORT1411_PID:
> +	case MX_UPORT1452_PID:
> +	case MX_UPORT1453_PID:
> +	case MX_UPORT1619_PID:
> +	case MX_UPORT1659_PID:
> +	case MX_UPORT165A_PID:
> +	case MX_UPORT165B_PID:
> +		is_mux50u = true;
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-up-mux50u.fw");
> +
> +		break;
> +	case MX_MU250U_PID:
> +	case MX_MU450U_PID:
> +	case MX_MU850U_PID:
> +	case MX_MU850U_6PORT_PID:
> +	case MX_MUX50U_3PORT_PID:
> +	case MX_MU850U_5PORT_PID:
> +	case MX_MU850U_7PORT_PID:
> +		is_mux50u = true;
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-pf-mux50u.fw");

We shouldn't have to maintain two tables with PIDs so please encode this
information in the id-table (using two new flags) if you really can't
query the device about this.

We already request the running firmware version. That's not enough to
determine if this is a mux50u device? Perhaps there is some other way?
Can the descriptors be used (e.g. bcdUSB)?

What does "up" vs "pf" stand for? Please mention this in commit message
as well (i.e. the two subfamilies).

> +
> +		break;
> +	default:
> +		snprintf(buf, sizeof(buf) - 1, "moxa/moxa-%04x.fw", productid);
> +
> +		break;
> +	}
>  
>  	err = request_firmware(&fw_p, buf, &serial->interface->dev);
>  	if (err) {
> @@ -1080,14 +1176,22 @@ static int mxuport_probe(struct usb_serial *serial,
>  		/* Use the firmware already in the device */
>  		err = 0;
>  	} else {
> -		local_ver = ((fw_p->data[VER_ADDR_1] << 16) |
> -			     (fw_p->data[VER_ADDR_2] << 8) |
> -			     fw_p->data[VER_ADDR_3]);
> +		if (is_mux50u) {
> +			local_ver = ((fw_p->data[NEW_ADDR_1] << 16) |
> +				     (fw_p->data[NEW_ADDR_2] << 8) |
> +				     (fw_p->data[NEW_ADDR_3]));
> +		} else {
> +			local_ver = ((fw_p->data[VER_ADDR_1] << 16) |
> +				     (fw_p->data[VER_ADDR_2] << 8) |
> +				     (fw_p->data[VER_ADDR_3]));
> +		}

This is probably better handled in a helper function which can make sure
that the right offsets are used depending on the family. For example:

	mxuport_parse_fw_version(const struct firmware *fw, bool mux50u)

>  		dev_dbg(&serial->interface->dev,
> -			"Available firmware version v%x.%x.%x\n",
> -			fw_p->data[VER_ADDR_1], fw_p->data[VER_ADDR_2],
> -			fw_p->data[VER_ADDR_3]);
> -		if (local_ver > version) {
> +			"Available firmware version v%d.%d.%d\n",
> +			(local_ver & 0xff0000) >> 16,
> +			(local_ver & 0xff00) >> 8,
> +			(local_ver & 0xff));
> +
> +		if (local_ver != version) {

This looks like a behaviour change that should be motivated. Why is it
OK to downgrade the device fw?

>  			err = mxuport_download_fw(serial, fw_p);
>  			if (err)
>  				goto out;
> @@ -1098,7 +1202,7 @@ static int mxuport_probe(struct usb_serial *serial,
>  	}
>  
>  	dev_info(&serial->interface->dev,
> -		 "Using device firmware version v%x.%x.%x\n",
> +		 "Using device firmware version v%d.%d.%d\n",
>  		 (version & 0xff0000) >> 16,
>  		 (version & 0xff00) >> 8,
>  		 (version & 0xff));

Johan