Re: [PATCH] USB: serial: mos7840: Add a product ID for the new product

[Johan Hovold]

On Mon, Nov 12, 2018 at 03:16:05PM +0800, Jackychou wrote:
> From: JackyChou <jackychou@xxxxxxxxxxx>
> 
> Add a new PID 0x7843 to the driver.
> Let the new products be able to set up 3 serial ports with the driver.
> 
> Signed-off-by: JackyChou <jackychou@xxxxxxxxxxx>
> ---
>  drivers/usb/serial/mos7840.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index b42bad85097a..7667b22fa2f7 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -94,6 +94,7 @@
>  /* The native mos7840/7820 component */
>  #define USB_VENDOR_ID_MOSCHIP           0x9710
>  #define MOSCHIP_DEVICE_ID_7840          0x7840
> +#define MOSCHIP_DEVICE_ID_7843          0x7843
>  #define MOSCHIP_DEVICE_ID_7820          0x7820
>  #define MOSCHIP_DEVICE_ID_7810          0x7810
>  /* The native component can have its vendor/device id's overridden
> @@ -176,6 +177,7 @@ enum mos7840_flag {
>  
>  static const struct usb_device_id id_table[] = {
>  	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
> +	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7843)},
>  	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
>  	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
>  	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
> @@ -298,7 +300,7 @@ static int mos7840_set_uart_reg(struct usb_serial_port *port, __u16 reg,
>  	val = val & 0x00ff;
>  	/* For the UART control registers, the application number need
>  	   to be Or'ed */
> -	if (port->serial->num_ports == 4) {
> +	if (port->serial->num_ports == 4 || port->serial->num_ports == 3) {

Please use num_ports > 2 here.

>  		val |= ((__u16)port->port_number + 1) << 8;
>  	} else {
>  		if (port->port_number == 0) {
> @@ -332,7 +334,7 @@ static int mos7840_get_uart_reg(struct usb_serial_port *port, __u16 reg,
>  		return -ENOMEM;
>  
>  	/* Wval  is same as application number */
> -	if (port->serial->num_ports == 4) {
> +	if (port->serial->num_ports == 4 || port->serial->num_ports == 3) {

Same here.

>  		Wval = ((__u16)port->port_number + 1) << 8;
>  	} else {
>  		if (port->port_number == 0) {
> @@ -2071,9 +2073,12 @@ static int mos7840_probe(struct usb_serial *serial,
>  			VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
>  
>  	/* For a MCS7840 device GPIO0 must be set to 1 */
> -	if (buf[0] & 0x01)
> -		device_type = MOSCHIP_DEVICE_ID_7840;
> -	else if (mos7810_check(serial))
> +	if (buf[0] & 0x01) {
> +		if (product == MOSCHIP_DEVICE_ID_7843)
> +			device_type = MOSCHIP_DEVICE_ID_7843;

What about 7843 devices that use a different PID? Is there a reliable
way to detect the type without relying on PID?

Otherwise, there's no point in verifying GPIO0 for these ones.

> +		else
> +			device_type = MOSCHIP_DEVICE_ID_7840;
> +	} else if (mos7810_check(serial))

Nit: if you add braces to one of the branches you need to add it to all
of them.

>  		device_type = MOSCHIP_DEVICE_ID_7810;
>  	else
>  		device_type = MOSCHIP_DEVICE_ID_7820;
> @@ -2091,7 +2096,10 @@ static int mos7840_calc_num_ports(struct usb_serial *serial,
>  	int device_type = (unsigned long)usb_get_serial_data(serial);
>  	int num_ports;
>  
> -	num_ports = (device_type >> 4) & 0x000F;
> +	if (device_type == MOSCHIP_DEVICE_ID_7843)
> +		num_ports = 3;
> +	else
> +		num_ports = (device_type >> 4) & 0x000F;
>  
>  	/*
>  	 * num_ports is currently never zero as device_type is one of
> @@ -2146,7 +2154,8 @@ static int mos7840_port_probe(struct usb_serial_port *port)
>  		mos7840_port->SpRegOffset = 0x0;
>  		mos7840_port->ControlRegOffset = 0x1;
>  		mos7840_port->DcrRegOffset = 0x4;
> -	} else if ((mos7840_port->port_num == 2) && (serial->num_ports == 4)) {
> +	} else if ((mos7840_port->port_num == 2) &&
> +		   ((serial->num_ports == 4) || (serial->num_ports == 3))) {
>  		mos7840_port->SpRegOffset = 0x8;
>  		mos7840_port->ControlRegOffset = 0x9;
>  		mos7840_port->DcrRegOffset = 0x16;
> @@ -2154,7 +2163,8 @@ static int mos7840_port_probe(struct usb_serial_port *port)
>  		mos7840_port->SpRegOffset = 0xa;
>  		mos7840_port->ControlRegOffset = 0xb;
>  		mos7840_port->DcrRegOffset = 0x19;
> -	} else if ((mos7840_port->port_num == 3) && (serial->num_ports == 4)) {
> +	} else if ((mos7840_port->port_num == 3) &&
> +		   ((serial->num_ports == 4) || (serial->num_ports == 3))) {
>  		mos7840_port->SpRegOffset = 0xa;
>  		mos7840_port->ControlRegOffset = 0xb;
>  		mos7840_port->DcrRegOffset = 0x19;

This is getting rather ugly. Can't you clean up the current code so that
it covers your device without the need of such changes first? 

>From the looks of it, the only reason for the above is to map the
offsets for port 2 in the 2-port case to the offsets of port 3. It would
be more straight-forward to handle that particular case separately.

This also relates to the set/get_uart_reg functions above, which also
should handle the 2-port case specifically instead.

Thanks,
Johan