Re: [PATCH] USB: serial: option: add Fibocom L7xx modules

[Victor Fragoso]

On Thu, 2023-10-26 at 20:13 +0700, Lars Melin wrote:
> On 10/26/2023 8:24, Victor Fragoso wrote:
> > Add support for Fibocom L7xx module series and variants.
> > 
> > L716-EU-60 (ECM):
> > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 17 Spd=480  MxCh= 0
> > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > P:  Vendor=19d2 ProdID=0579 Rev= 1.00
> > S:  Manufacturer=Fibocom,Incorporated
> > S:  Product=Fibocom Mobile Boardband
> > S:  SerialNumber=1234567890ABCDEF
> > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > 
> > L716-EU-60 (RNDIS):
> > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 21 Spd=480  MxCh= 0
> > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > P:  Vendor=2cb7 ProdID=0001 Rev= 1.00
> > S:  Manufacturer=Fibocom,Incorporated
> > S:  Product=Fibocom Mobile Boardband
> > S:  SerialNumber=1234567890ABCDEF
> > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > 
> > L716-EU-10 (ECM):
> > T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 21 Spd=480  MxCh= 0
> > D:  Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> > P:  Vendor=2cb7 ProdID=0001 Rev= 1.00
> > S:  Manufacturer=Fibocom,Incorporated
> > S:  Product=Fibocom Mobile Boardband
> > S:  SerialNumber=1234567890ABCDEF
> > C:* #Ifs= 7 Cfg#= 1 Atr=e0 MxPwr=500mA
> > A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> > I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> > E:  Ad=87(I) Atr=03(Int.) MxPS=  16 Ivl=32ms
> > I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> > E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
> > E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > I:* If#= 6 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=42 Prot=01 Driver=usbfs
> > E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> > 
> > Signed-off-by: Victor Fragoso <victorffs@xxxxxxxxxxx>
> > ---
> >   drivers/usb/serial/option.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> > index 45dcfaadaf98..4ba3dc352d65 100644
> > --- a/drivers/usb/serial/option.c
> > +++ b/drivers/usb/serial/option.c
> > @@ -2262,6 +2262,11 @@ static const struct usb_device_id option_ids[] =
> > {
> >   	{ USB_DEVICE_INTERFACE_CLASS(0x2cb7, 0x01a2, 0xff)
> > },			/* Fibocom FM101-GL (laptop MBIM) */
> >   	{ USB_DEVICE_INTERFACE_CLASS(0x2cb7, 0x01a4,
> > 0xff),			/* Fibocom FM101-GL (laptop MBIM) */
> >   	  .driver_info = RSVD(4) },
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0001, 0xff, 0xff,
> > 0xff) },	/* Fibocom L71x */
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0001, 0x0a, 0x00,
> > 0xff) },	/* Fibocom L71x */
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x2cb7, 0x0100, 0xff, 0xff,
> > 0xff) },	/* Fibocom L71x */
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x19d2, 0x0256, 0xff, 0xff,
> > 0xff) },	/* Fibocom L71x */
> > +	{ USB_DEVICE_AND_INTERFACE_INFO(0x19d2, 0x0579, 0xff, 0xff,
> > 0xff) },	/* Fibocom L71x */
> >   	{ USB_DEVICE_INTERFACE_CLASS(0x2df3, 0x9d03, 0xff)
> > },			/* LongSung M5710 */
> >   	{ USB_DEVICE_INTERFACE_CLASS(0x305a, 0x1404, 0xff)
> > },			/* GosunCn GM500 RNDIS */
> >   	{ USB_DEVICE_INTERFACE_CLASS(0x305a, 0x1405, 0xff)
> > },			/* GosunCn GM500 MBIM */
> 
> 
> Hi Victor, thanks for the patch, there is unfortunately the following 
> errors in it:
> The device list is sorted in ascending order based on vid:pid, you have 
> inserted all of your added Id's in the wrong place.
> 
> 19d2:0579 is a ZTE device Id and should be placed among the other 19d2 
> devices.
> 
> You have not included usb-devices output for 19d2:0256 and 2cb7:0100, 
> and I have strong reasons to believe that they should not be included in 
> the option driver.
> If you are of another opinion then please show the usb-devices output 
> for them, otherwise remove them from the patch.
> 
> You have added support for an interface with the attributes 0a/00/ff , 
> there is no such interface in your provided usb-devices listing, 
> interface Class 0a does not even belong to the option driver.
> 
> Thanks
> Lars

Hi Lars, sorry about the wrong order, I will correct it.

But about the ZTE device ID, I belive that we should insert among
Fibocom drivers because we are talking about a Fibocom module that is
using an ZTE Chipset.
So, this is exactly the same situation from Fibocom L610 IDs (0x1782,
0x4d10 / 0x1782, 0x4d11) that is using the Unisoc Chipset but were
inserted among Fibocom drivers.

And about the usb-devices output, let me explain better:
I am a Field Application Enginner at Fibocom Brazil and I am using the
IDs from our internal and official documentation.
On this documents its suggested to add all this IDs because it will
guarantee that can be used on all the variants devices from L71x series
(that can change according to different part number, region support or
network protocol).
Unfortunately, I don't have all the modules variations available with
me right now to test and share all the outputs.

Can we continue with all the IDs that I have inserted?
Or do you prefer to keep just the devices that I tested by myself until
now?

Victor Fragoso