Re: [PATCH v2] net: usb: qmi_wwan: add Telit Cinterion FE990D50 composition

Jakub Kicinski <[email protected]>
Newsgroups org.kernel.vger.stable,org.kernel.vger.linux-kernel,org.kernel.vger.linux-usb,org.kernel.vger.netdev
Message-ID <[email protected]>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: qmi_wwan: add Telit Cinterion FE990D50 composition

Adds a qmi_wwan products[] entry for USB ID 1bc7:0991 interface 0, using
the SET_DTR quirk, for the Telit Cinterion FE990D50 rmnet + tty + ADPL +
adb composition.

> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 94cdb61dca83d..147beaa61340d 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -1360,6 +1360,7 @@ static const struct usb_device_id products[] = {
>  	{QMI_FIXED_INTF(0x1bbb, 0x0203, 2)},	/* Alcatel L800MA */
>  	{QMI_FIXED_INTF(0x2357, 0x0201, 4)},	/* TP-LINK HSUPA Modem MA180 */
>  	{QMI_FIXED_INTF(0x2357, 0x9000, 4)},	/* TP-LINK MA260 */
> +	{QMI_QUIRK_SET_DTR(0x1bc7, 0x0991, 0)}, /* Telit LE910C1-EUX */
                                                   ^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is the device name in this comment the intended one?  The subject line and
the descriptor dump in the changelog both describe 1bc7:0991 as the
FE990D50:

    P:  Vendor=1bc7 ProdID=0991 Rev=06.06
    S:  Manufacturer=Telit Cinterion
    S:  Product=FE990

The comment looks like a copy of the next line, which is the entry for the
real LE910C1-EUX at a different product ID and interface number:

>  	{QMI_QUIRK_SET_DTR(0x1bc7, 0x1031, 3)}, /* Telit LE910C1-EUX */

drivers/usb/serial/option.c already labels the same product ID as the
FE990D50 in option_ids[]:

	{ USB_DEVICE_AND_INTERFACE_INFO(TELIT_VENDOR_ID, 0x0991, 0xff, 0xff, 0x30),	/* Telit FE990D50 (rmnet) */

Would /* Telit FE990D50 */ be a better fit here, matching the existing
naming for the rest of the family in this table (/* Telit FE990A */ for
0x1080 and /* Telit FE990B */ for 0x10b0)?  As it stands, two entries for
different hardware carry the same label, which makes it easy for a later
addition to touch the wrong line.

The match itself looks fine: interface 0 of 0x0991 has bInterfaceProtocol
0x50, which none of the option.c filters (0x30, 0x40, 0x60) claim, so only
the comment appears to need a change.

>  	{QMI_QUIRK_SET_DTR(0x1bc7, 0x1034, 2)}, /* Telit LE910C4-WWX */
>  	{QMI_QUIRK_SET_DTR(0x1bc7, 0x1037, 4)}, /* Telit LE910C4-WWX */
-- 
pw-bot: cr
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.