Re: [PATCH] usb: serial: ti_usb_3410_5052: add MOXA UPORT 11x0 support

From: Johan Hovold
Date: Sun Apr 24 2016 - 11:59:24 EST

Next message: Takashi Iwai: "[PATCH 2/2] ALSA: timer: Introduce stop_sync op"
Previous message: Pavel Machek: "Re: [PATCH 1/2] extcon: arizona: Add device bindings for the micd configurations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Mar 02, 2016 at 10:46:13AM +0100, Mathieu OTHACEHE wrote:
> Add support for :
>
> - UPort 1110 : 1 port RS-232 USB to Serial Hub.
> - UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
> - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
> - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
> - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
>
> These devices are based on TI 3410 chip.
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@xxxxxxxxx>
> ---

Thanks for the patch and sorry for the late review.

> This patch add support for MOXA UPORT 11x0 devices to ti_usb_3410_5052.
> The informations are extracted from the now reverted mxu11x0 driver.
>
> There is a delicate point in the attach callback.
> The driver is comparing bNumConfigurations to 1 to see if the firmware
> has to be downloaded. I guess bNumConfigurations is 2 after firmware download
> and device reseting.
>
> On MOXA 1110, bNumConfigurations is always 1, before and after firmware download.
> So, I also check that the interface has only 1 endpoint to determine if we have
> to download firmware or not.

That sounds reasonable.

> drivers/usb/serial/ti_usb_3410_5052.c | 44 ++++++++++++++++++++++++++++++++---
> drivers/usb/serial/ti_usb_3410_5052.h | 8 +++++++
> 2 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 2694df2..b9119de 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -80,6 +80,7 @@ struct ti_device {
> int td_open_port_count;
> struct usb_serial *td_serial;
> int td_is_3410;
> + int td_model;
> int td_urb_error;
> };
>
> @@ -160,6 +161,11 @@ static const struct usb_device_id ti_id_table_3410[] = {
> { USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
> { USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
> { USB_DEVICE(HONEYWELL_VENDOR_ID, HONEYWELL_HGI80_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },

Keep these sorted internally at least.

> { } /* terminator */
> };
>
> @@ -193,6 +199,11 @@ static const struct usb_device_id ti_id_table_combined[] = {
> { USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
> { USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
> { USB_DEVICE(HONEYWELL_VENDOR_ID, HONEYWELL_HGI80_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },

Ditto.

> { } /* terminator */
> };
>
> @@ -277,6 +288,11 @@ MODULE_FIRMWARE("mts_gsm.fw");
> MODULE_FIRMWARE("mts_edge.fw");
> MODULE_FIRMWARE("mts_mt9234mu.fw");
> MODULE_FIRMWARE("mts_mt9234zba.fw");
> +MODULE_FIRMWARE("moxa/moxa-1110.fw");
> +MODULE_FIRMWARE("moxa/moxa-1130.fw");
> +MODULE_FIRMWARE("moxa/moxa-1131.fw");
> +MODULE_FIRMWARE("moxa/moxa-1150.fw");
> +MODULE_FIRMWARE("moxa/moxa-1151.fw");
>
> module_param(closing_wait, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(closing_wait,
> @@ -292,6 +308,7 @@ static int ti_startup(struct usb_serial *serial)
> {
> struct ti_device *tdev;
> struct usb_device *dev = serial->dev;
> + struct usb_host_interface *cur_altsetting;
> int status;
>
> dev_dbg(&dev->dev,
> @@ -315,8 +332,13 @@ static int ti_startup(struct usb_serial *serial)
> dev_dbg(&dev->dev, "%s - device type is %s\n", __func__,
> tdev->td_is_3410 ? "3410" : "5052");
>
> - /* if we have only 1 configuration, download firmware */
> - if (dev->descriptor.bNumConfigurations == 1) {
> + tdev->td_model = le16_to_cpu(dev->descriptor.idProduct);

Instead of storing the model (which really is the vid-dependent pid),
just set an rs485-only flag when you detect a Moxa 1130/1131 here.

> + cur_altsetting = serial->interface->cur_altsetting;
> +
> + /* if we have only 1 configuration and 1 endpoint, download firmware */
> + if (dev->descriptor.bNumConfigurations == 1 &&
> + cur_altsetting->desc.bNumEndpoints == 1) {

Indent continuation lines two tabs further, or rewrite so that you don't
need a line break.

> status = ti_download_firmware(tdev);
>
> if (status != 0)
> @@ -371,7 +393,15 @@ static int ti_port_probe(struct usb_serial_port *port)
> port->port.closing_wait = msecs_to_jiffies(10 * closing_wait);
> tport->tp_port = port;
> tport->tp_tdev = usb_get_serial_data(port->serial);
> - tport->tp_uart_mode = 0; /* default is RS232 */
> +
> + switch (tport->tp_tdev->td_model) {
> + case MXU1_1130_PRODUCT_ID:
> + case MXU1_1131_PRODUCT_ID:
> + tport->tp_uart_mode = TI_UART_485_RECEIVER_DISABLED;
> + break;
> + default:
> + tport->tp_uart_mode = TI_UART_232; /* default is RS232 */
> + }

Use the flag set mentioned above instead.

>
> usb_set_serial_port_data(port, tport);
>
> @@ -1479,6 +1509,14 @@ static int ti_download_firmware(struct ti_device *tdev)
> strcpy(buf, "mts_mt9234zba.fw");
> break; }
> }
> +
> + if (le16_to_cpu(dev->descriptor.idVendor) == MXU1_VENDOR_ID) {
> + snprintf(buf,
> + sizeof(buf),
> + "moxa/moxa-%04x.fw",
> + le16_to_cpu(dev->descriptor.idProduct));
> + }
> +

Looks like this code could use a few vid/pid temporaries.

I'm not sure it makes sense to try to load a "ti_usb-v110a-p1150.fw"
firmware before requesting the moxa firmware. Avoids a confusing:

usb 1-2.2: Direct firmware load for ti_usb-v110a-p1150.fw failed with error -2

message too.

> if (buf[0] == '\0') {
> if (tdev->td_is_3410)
> strcpy(buf, "ti_3410.fw");
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.h b/drivers/usb/serial/ti_usb_3410_5052.h
> index 98f35c6..93ac6ad 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.h
> +++ b/drivers/usb/serial/ti_usb_3410_5052.h
> @@ -60,6 +60,14 @@
> #define HONEYWELL_VENDOR_ID 0x10ac
> #define HONEYWELL_HGI80_PRODUCT_ID 0x0102 /* Honeywell HGI80 */
>
> +/* Moxa UPORT 11x0 vendor and product IDs */
> +#define MXU1_VENDOR_ID 0x110a
> +#define MXU1_1110_PRODUCT_ID 0x1110
> +#define MXU1_1130_PRODUCT_ID 0x1130
> +#define MXU1_1150_PRODUCT_ID 0x1150
> +#define MXU1_1151_PRODUCT_ID 0x1151
> +#define MXU1_1131_PRODUCT_ID 0x1131

Keep these sorted too.

> +
> /* Commands */
> #define TI_GET_VERSION 0x01
> #define TI_GET_PORT_STATUS 0x02

I did a quick test of the patch using a Moxa 1150-device. Works at
115200, but communication appeared broken at 9600. Looks like the baud
rate calculations are similar but not identical to what the Moxa driver
does. Is this something you have looked into?

Thanks,
Johan

Next message: Takashi Iwai: "[PATCH 2/2] ALSA: timer: Introduce stop_sync op"
Previous message: Pavel Machek: "Re: [PATCH 1/2] extcon: arizona: Add device bindings for the micd configurations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]