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

[Johan Hovold]

On Thu, Nov 22, 2018 at 04:00:17PM +0800, JackyChou wrote:
> 
> From: JackyChou <jackychou@xxxxxxxxxxx>

Thanks for the update. This patch appears to have been corrupted (tabs
replaced with spaces) so it cannot be applied. Try sending the patch to
yourself first and make sure you can apply it with git am (and/or run
checkpatch on the result).

> For now, pause to add PID 0x7843 in the driver.

This doesn't belong in the commit message. But there's no reason not to
include the patch adding the PID in the same series. Just send a
two-part series with this patch as the first patch, and the PID one as
the second patch (depending on the first).

Take a look at git-format-patch and git-send-email which can simply the
process.

Also make sure each commit message is self-contained, and that the patch
summary (Subject) reflects what each patch does.
 
> Simplify the processes of some functions.

Please be a bit more specific.

> Such modifications will not affect the old devices and will make
> the addition of new product (0x7843) more flexible in the future.

This can still stay here.

> Signed-off-by: JackyChou <jackychou@xxxxxxxxxxx>
> ---
>  drivers/usb/serial/mos7840.c | 48 ++++++++++++------------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index b42bad85097a..0ca945dce377 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -298,15 +298,10 @@ 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 == 2 && port->port_number != 0)
> +		val |= ((__u16)port->port_number + 2) << 8;
> +	else
>  		val |= ((__u16)port->port_number + 1) << 8;
> -	} else {
> -		if (port->port_number == 0) {
> -			val |= ((__u16)port->port_number + 1) << 8;
> -		} else {
> -			val |= ((__u16)port->port_number + 2) << 8;
> -		}
> -	}

Perhaps this would be more readable if you used a port offset here as
well, and only make sure to map the second port in the two-port case to
port offset three.

Thanks,
Johan