Petr Štetiar <ynezz@xxxxxxx> writes:
> Bjørn Mork <bjorn@xxxxxxx> [2015-11-04 13:15:10]:
>
>> Based on that, I wonder if it wouldn't be more appropriate to simply do
>> this as a device specific quirk in the qmi_wwan probe?
>
> So rather something like this?
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 2a7c1be..2dafc69 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -822,6 +822,7 @@ static const struct usb_device_id products[] = {
> {QMI_GOBI_DEVICE(0x05c6, 0x9245)}, /* Samsung Gobi 2000 Modem device (VL176) */
> {QMI_GOBI_DEVICE(0x03f0, 0x251d)}, /* HP Gobi 2000 Modem device (VP412) */
> {QMI_GOBI_DEVICE(0x05c6, 0x9215)}, /* Acer Gobi 2000 Modem device (VP413) */
> + {QMI_FIXED_INTF(0x05c6, 0x9215, 4)}, /* Quectel EC20 Mini PCIe Module */
> {QMI_GOBI_DEVICE(0x05c6, 0x9265)}, /* Asus Gobi 2000 Modem device (VR305) */
> {QMI_GOBI_DEVICE(0x05c6, 0x9235)}, /* Top Global Gobi 2000 Modem device (VR306) */
> {QMI_GOBI_DEVICE(0x05c6, 0x9275)}, /* iRex Technologies Gobi 2000 Modem device (VR307) */
> @@ -853,6 +854,23 @@ static const struct usb_device_id products[] = {
> };
> MODULE_DEVICE_TABLE(usb, products);
>
> or rather something like this?
>
> +#define QUECTEL_EC20_VENDORID 0x05c6
> +#define QUECTEL_EC20_PRODUCTID 0x9215
> +#define QUECTEL_EC20_NINTERFACES 5
> +#define QUECTEL_EC20_QMI_IFACE_FIX 4
Not directly related to the issue at hand, but I sort of hate macros
like that. They provide little or no value, and make it much more
difficult to see what is going on. In particular, if you went this
route then you would probably want to define
#define ACER_GOBI2K_VENDORID 0x05c6
#define ACER_GOBI2K_PRODUCTID 0x9215
#define ACER_GOBI2K_NINTERFACES 4
#define ACER_GOBI2K_QMI_IFACE_FIX 0
to make this complete. And noone would notice that
ACER_GOBI2K_PRODUCTID and QUECTEL_EC20_PRODUCTID are the same value.
In fact, the QUECTEL_EC20_VENDORID is really a Qualcomm device ID.
That's confusing already. Please use literals instead.
> +static int quectel_ec20_detected(struct usb_interface *intf)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> +
> + if (dev->actconfig &&
> + le16_to_cpu(dev->descriptor.idVendor) == QUECTEL_EC20_VENDORID &&
> + le16_to_cpu(dev->descriptor.idProduct) == QUECTEL_EC20_PRODUCTID &&
Not that performance matters, but you can make these compile time
operations on BE by doing
dev->descriptor.idVendor == cpu_to_le16(QUECTEL_EC20_VENDORID) &&
dev->descriptor.idProduct == cpu_to_le16(QUECTEL_EC20_PRODUCTID) &&
instead.
> + dev->actconfig->desc.bNumInterfaces == QUECTEL_EC20_NINTERFACES)
> + return 1;
> +
> + return 0;
> +}
> +
> static int qmi_wwan_probe(struct usb_interface *intf,
> const struct usb_device_id *prod)
> {
> @@ -868,6 +886,12 @@ static int qmi_wwan_probe(struct usb_interface *intf,
> id->driver_info = (unsigned long)&qmi_wwan_info;
> }
>
> + /* Quectel EC20 quirk where we're fixing QMI iterface index from 0 to 4 */
> + if (quectel_ec20_detected(intf)) {
> + dev_dbg(&intf->dev, "enabling Quectel EC20 interface quirk\n");
> + id->bInterfaceNumber = QUECTEL_EC20_QMI_IFACE_FIX;
> + }
> +
> return usbnet_probe(intf, id);
> }
I didn't think if that possibiity. Does it work?
But modifying the match table like this is a little hacky in any case, I
guess. I know we do it for the dynamic part of the table, but that's
because there is no other way making dynamic device IDs work in
combination with the usbnet driver_info.
Better to go for the first alternative with a double entry in the match
table. But it will need code to reject interface #0 on the Quectel
EC20, so qcserial can handle it.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
