Skip to contentSkip to conversation

USB: serial: option: add Fibocom NL668 series

DISCUSSION Johan Hovold 1 participant 1 message Dec 12, 2018 USB

lore
Share & tools
lore
#0 lore raw
Quoted 67 lines · Jörgen Storvist

Den Tue, 11 Dec 2018 09:32:36 +0100
skrev Re: [PATCH] USB: serial: option: add Fibocom NL668 series:

Quoted 49 lines · Jörgen Storvist
Quoted 13 lines

Added USB serial option driver support for Fibocom NL668 series cellular modules. Reserved USB endpoints 4, 5 and 6 for network + ADB interface.

---

Thanks for feedback!
Changes:
Removed name declarations for VID/PID
Added reserved endpoint for ADB interface

usb-devices

Quoted 14 lines
T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  7 Spd=480 MxCh= 0
D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=1508 ProdID=1001 Rev=03.18
S:  Manufacturer=Nodecom NL668 Modem
S:  Product=Nodecom NL668-CN Modem
S:  SerialNumber=5ced6a52
C:  #Ifs= 7 Cfg#= 1 Atr=a0 MxPwr=500mA
I:  If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I:  If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
I:  If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
I:  If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
I:  If#= 4 Alt= 0 #EPs= 1 Cls=02(commc) Sub=06 Prot=00 Driver=cdc_ether
I:  If#= 5 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
I:  If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=(none)  

Thanks for the update and usb-devices info (you can even put this in the changelog).

drivers/usb/serial/option.c static const struct usb_device_id option_ids[] = { · line 1945

Quoted 13 lines
 drivers/usb/serial/option.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/usb/serial/option.c +2 -0
index e24ff16..ca3e398 100644
@@ -1941,6 +1941,8 @@ static const struct usb_device_id option_ids[] = {
 	{ USB_DEVICE_AND_INTERFACE_INFO(WETELECOM_VENDOR_ID, WETELECOM_PRODUCT_6802, 0xff, 0xff, 0xff) },
 	{ USB_DEVICE_AND_INTERFACE_INFO(WETELECOM_VENDOR_ID, WETELECOM_PRODUCT_WMD300, 0xff, 0xff, 0xff) },
 	{ USB_DEVICE_AND_INTERFACE_INFO(0x03f0, 0x421d, 0xff, 0xff, 0xff) }, /* HP lt2523 (Novatel E371) */
+	{ USB_DEVICE(0x1508, 0x1001), /* Fibocom NL668 series */
+	  .driver_info = RSVD(4) | RSVD(5) | RSVD(6) },  

Looks like you can use USB_DEVICE_INTERFACE_CLASS() to match on the vendor class instead of blacklisting interface 4 and 5.

Quoted 5 lines

A more specific match is generally preferred over blacklisting (which adds some overhead).

Care to address that in a v3? Remember to include the patch revision in the subject line as well (e.g. "[PATCH v3] USB: ...").

It seems it would become problematic then if we change USB mode on the module to QMI instead of ECM network interface as they share same VID/PID value.

Or is there other way to still get endpoints 0-3 bound to option driver on class / interface info? Subclass is different on the first diagnostical serial interface so USB_DEVICE_AND_INTERFACE_INFO() wouldn't be successful then either?

Quoted 13 lines
T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  9 Spd=480 MxCh= 0
D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=1508 ProdID=1001 Rev=03.18
S:  Manufacturer=Nodecom NL668 Modem
S:  Product=Nodecom NL668-CN Modem
S:  SerialNumber=5ced6a52
C:  #Ifs= 6 Cfg#= 1 Atr=a0 MxPwr=500mA
I:  If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option	(Serial)
I:  If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option	(Serial)
I:  If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option	(Serial)
I:  If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=option	(Serial)
I:  If#= 4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)	(QMI/RMNET)
I:  If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=(none)	(ADB)

And you also have ADB as interface five here.

I guess your original patch matching on all interfaces and blacklisting interfaces 4, 5 and 6 is the best option. An alternative could be two entries for ff/ff/ff and ff/00/00, but then you'd still need to blacklist QMI so not that much better.

But please resubmit and include usb-devices for both messages in the commit message.

Please also base this on my usb-linus branch which already holds some new ids that prevents this one from applying.

https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/

Thanks,
Johan