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

From: Victor Fragoso
Date: Tue Nov 07 2023 - 17:07:14 EST

Next message: Nicolas Dufresne: "Re: [PATCH v4 06/11] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper"
Previous message: Nhat Pham: "Re: mm/filemap.c:4249:1: sparse: sparse: Using plain integer as NULL pointer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, 2023-10-28 at 02:59 +0700, Lars Melin wrote:
> On 10/28/2023 0:55, Victor Fragoso wrote:
> > 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
>
> Johan may have a different opinion from mine and he is the one to
> decide, my take is that there is no value in having a minor part of the
> list grouped by mfgr and the major part of the list sorted by USB Id.
> We have also seen in the past that when a mfgr1 buys a chipset from
> mfgr2 and then sells his product with mfgr2 Id instead of using one of
> his own Id's then there is also not uncommon that mfgr3 is also buying
> the same chipset without changing Id. Hence "Manufacturer name" is not
> unique but the vid is..
>
> The list should not be seen or used as a cross reference between
> mfgr:product name and vid:pid, anyone who needs to search the driver
> source to see if a device is supported ought to know its vid:pid so can
> search on those and that works much better if the list is sorted by
> vid:pid in ascending order. Personally I'd rather see a source without
> all the unnecessary defines, only vid:pid based and with the
> mfgr/product as an optional comment.
>
> When adding Id's to the driver it is you who should know the devices you
> are adding, don't add Id's if you have not personally confirmed their
> interface composition and usage.
> Those who told you add these Id's apparently also told you that there
> are two versions of 2cb7:0001, one with ECM interfaces and one with
> RNDIS interfaces but your usb-devices output show both to be identical..
> There is no RNDIS interface in your listings, both of them will load
> the cdc_ether driver.
>
> br
> Lars
>

Lars, understood. Your point makes sense and I could accept it with no
problems.

Johan, can you please share your opinion to decide?

1- Should I create a new commit inserting the PID grouped among to
Fibocom's VID? Or separately on Fibocom's VID section and another one
on ZTE's VID section?
2- Should I remove the generic Fibocom product comment and add just the
product/mode that I could test it? e.g. Fibocom L716-EU (ECM)

Victor Fragoso

Next message: Nicolas Dufresne: "Re: [PATCH v4 06/11] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper"
Previous message: Nhat Pham: "Re: mm/filemap.c:4249:1: sparse: sparse: Using plain integer as NULL pointer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]