| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 16 Nov 2018 10:55:53 +0100
From: Johan Hovold <johan@...nel.org>
To: JackyChou <jackychou@...x.com.tw>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
louis@...x.com.tw
Subject: Re: [PATCH] USB: serial: mos7840: Add a product ID for the new
product
On Fri, Nov 16, 2018 at 02:36:56PM +0800, JackyChou wrote:
> From: JackyChou <jackychou@...x.com.tw>
>
> Add a new PID 0x7843 to the driver.
> Let the new products be able to set up 3 serial ports with the driver.
>
> Because the development of new product is based on 4 serial ports,
> but some users only need 3 serial ports. There is no way to set it from
> the hardware, so let the driver set 3 serial ports with PID 0x7843.
>
> Signed-off-by: JackyChou <jackychou@...x.com.tw>
> ---
Thanks for the update.
Always include a short changelog here so we know what has changed since
earlier versions.
Also include the patch revision in the Subject (i.e. "[PATCH v2] USB: ...").
> drivers/usb/serial/mos7840.c | 45 ++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index b42bad85097a..57ef25a2f7bb 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,15 +300,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;
> - }
> - }
This looks good, but please to this in a separate preparatory patch as
this is an independent change from adding support for you new device.
> dev_dbg(&port->dev, "%s application number is %x\n", __func__, val);
> return usb_control_msg(dev, usb_sndctrlpipe(dev, 0), MCS_WRREQ,
> MCS_WR_RTYPE, val, reg, NULL, 0,
> @@ -332,15 +329,10 @@ 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 == 2 && port->port_number != 0)
> + Wval = ((__u16)port->port_number + 2) << 8;
> + else
> Wval = ((__u16)port->port_number + 1) << 8;
> - } else {
> - if (port->port_number == 0) {
> - Wval = ((__u16)port->port_number + 1) << 8;
> - } else {
> - Wval = ((__u16)port->port_number + 2) << 8;
> - }
> - }
Same here.
> dev_dbg(&port->dev, "%s application number is %x\n", __func__,
> Wval);
> ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), MCS_RDREQ,
> MCS_RD_RTYPE, Wval, reg, buf,
> VENDOR_READ_LENGTH,
> @@ -2071,12 +2063,16 @@ 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;
And as I mentioned in my previous comments, if you're going to match in
PID, there's no need to check GPIO0. Just add code to handle 7843 to the
start of the function.
> + else
> + device_type = MOSCHIP_DEVICE_ID_7840;
> + } else if (mos7810_check(serial)) {
> device_type = MOSCHIP_DEVICE_ID_7810;
> - else
> + } else {
> device_type = MOSCHIP_DEVICE_ID_7820;
> + }
>
> kfree(buf);
> out:
> @@ -2091,7 +2087,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 +2145,7 @@ 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 > 2))
> {
> mos7840_port->SpRegOffset = 0x8;
> mos7840_port->ControlRegOffset = 0x9;
> mos7840_port->DcrRegOffset = 0x16;
> @@ -2154,7 +2153,7 @@ 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 > 2))
> {
> mos7840_port->SpRegOffset = 0xa;
> mos7840_port->ControlRegOffset = 0xb;
> mos7840_port->DcrRegOffset = 0x19;
Can't this be handled similarly to set_uart_reg() above? Looks like you
can calculate the offsets (and remove the if-else clause) and only make
sure to map port 2 in the two-port case to physical port 3.
This would also go in the preparatory first patch of a two-part series.
Thanks,
Johan
Powered by blists - more mailing lists