From: Johan Hovold <johan@kernel.org>
To: Mathieu OTHACEHE <m.othacehe@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: serial: ti_usb_3410_5052: add MOXA UPORT 11x0 support
Date: Sun, 24 Apr 2016 17:59:06 +0200 [thread overview]
Message-ID: <20160424155906.GB18866@localhost> (raw)
In-Reply-To: <1456911973-25406-1-git-send-email-m.othacehe@gmail.com>
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@gmail.com>
> ---
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 prev parent reply other threads:[~2016-04-24 15:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-02 9:46 [PATCH] usb: serial: ti_usb_3410_5052: add MOXA UPORT 11x0 support Mathieu OTHACEHE
2016-04-24 15:59 ` Johan Hovold [this message]
[not found] <20160502180407.GA4330@gmail.com>
2016-05-02 18:37 ` Mathieu OTHACEHE
2016-05-03 8:14 ` Johan Hovold
2016-05-03 11:46 ` Mathieu OTHACEHE
2016-05-04 7:40 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160424155906.GB18866@localhost \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.othacehe@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox