Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles

[Johan Hovold <johan@kernel.org>]

On Wed, Feb 27, 2019 at 02:32:58PM +0000, Måns Rullgård wrote:
> Johan Hovold <johan@kernel.org> writes:
> 
> > Adding Bjørn.
> >
> > On Wed, Feb 27, 2019 at 11:57:16AM +0000, Måns Rullgård wrote:
> >> Johan Hovold <johan@kernel.org> writes:
> >> 
> >> > On Tue, Feb 26, 2019 at 05:07:10PM +0000, Mans Rullgard wrote:
> >> >> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
> >> >> of which are serial ports.  The fifth is a network interface supported
> >> >> by the qmi-wwan driver.  Furthermore, the serial ports do not support
> >> >> modem control signals.  Add driver_info flags to reflect this.
> >> >> 
> >> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> >> ---
> >> >>  drivers/usb/serial/option.c | 3 ++-
> >> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> >> >> index fb544340888b..af4cbfecc3ff 100644
> >> >> --- a/drivers/usb/serial/option.c
> >> >> +++ b/drivers/usb/serial/option.c
> >> >> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
> >> >>  	  .driver_info = RSVD(3) },
> >> >>  	{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
> >> >>  	{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
> >> >> -	{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
> >> >> +	{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
> >> >> +	  .driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
> >> >>  	/* Quectel products using Qualcomm vendor ID */
> >> >>  	{ USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
> >> >>  	{ USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
> >> >
> >> > Could you please provide the output of usb-devices (or lsusb -v) for
> >> > this device?
> >> 
> >> lsusb -v:
> >> [...]
> 
> > So the patch looks fine to me. The fifth interface is QMI, but hasn't
> > been available for use until now then, and this seems to have been the
> > vendors idea from the start:
> >
> > 	http://www.microchip.ua/simcom/WCDMA/APPNOTES/SIMCom_3G_Linux_driver_Application%20Note_V1.00.pdf
> 
> That document predates the qmi-wwan driver in the kernel.  Note that
> this driver has an ID table entry for interface 4 of this device.  Right
> now, whichever driver is probed first claims that interface.  I haven't
> actually tried using the QMI interface, though.

I didn't say it was correct, just that the vendor proposed binding to it
anyway.

> > And you're seeing errors when opening ports 0-3 due to the DTR calls
> > which I guess no one noticed or cared about before?
> 
> Right, some userspace tools complain about this.

Hmm. You shouldn't see any errors on open (they're not even logged), but
I guess your user space tools complains on receiving -EPROTO instead of
-EINVAL when trying to manage these signals directly?

> > Before you sent me the lsusb I searched for it and came across the below
> > thread where Bjørn's having a go at SIMCom. In it there's output from a
> > second device using the same id but with entirely different descriptors.
> >
> > 	https://forum.openwrt.org/t/lte-wireless-module-support-by-openwrt-led-on-tplink/13586?page=3
> >
> > If this is a common theme with this vendor we may need to be extra
> > careful when making changes.
> 
> Isn't this a common theme with most USB vendors, especially wireless things?
> 
> Regardless, setting the NCTRL flag should be harmless.

Well, there are devices that depend on getting these requests, at least
for the QMI interface. But we can always revert if anyone complains.

Now applied, thanks.

Johan