Re: [PATCH] net: usb: qmi_wwan: add common Huawei modem IDs

[Bjørn Mork]

jackyphuti <jackympoka22@xxxxxxxxx> writes:

> Add common Huawei product IDs to qmi_wwan fixed-interface mappings
> for broader generic Huawei modem coverage.

Why?  Does your patch fix something? How does it make the coverage
"broader"?

The choice of numeric constants instead of made up macro names was quite
deliberate in this driver. Did you not notice the style in the other
entries?

Most of the numbers, including your examples, have no meaningful
descriptions. Many of the vendors, and in particular Huawei, have reused
their device id numbers for a large number of products. No matter which
arbitrary product name you should choose, it will be confusing and wrong
for most of the products using that id.

More importantly:  Macros in source code are used to make the code
easier to read and understand for developers.  My personal experience
with device id names from other USB drivers, like option.c, was and is
that the effect is the opposite. I have to go look up a macro placed
somewhere else to actually understand what device some entry is pointing
to.

Most entries have a product name in a comment instead. It's probably not
clear to anyone, but this was intended as a hint pointing to the source
of that entry.  In any case - it already gives you everything your macro
would do without any of the drawbacks.

Did AI tell you this?  Why not?  I believe I've explained it before, but
maybe that was in an AT free dream?

> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -216,10 +216,10 @@ static int qmimux_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		if (netif_rx(skbn) != NET_RX_SUCCESS) {
>  			net->stats.rx_errors++;
>  			return 0;
> -		} else {
> -			dev_sw_netstats_rx_add(net, pkt_len);
>  		}
>  
> +		dev_sw_netstats_rx_add(net, pkt_len);
> +

This change is unrelated and not described.



>  /* map QMI/wwan function by a fixed interface number */
>  #define QMI_FIXED_INTF(vend, prod, num) \
> @@ -1024,23 +1031,23 @@ static const struct usb_device_id products[] = {
>  	},
>  	{	/* Novatel USB551L and MC551 */
>  		USB_DEVICE_AND_INTERFACE_INFO(0x1410, 0xb001,
> -		                              USB_CLASS_COMM,
> -		                              USB_CDC_SUBCLASS_ETHERNET,
> -		                              USB_CDC_PROTO_NONE),
> +					      USB_CLASS_COMM,
> +					      USB_CDC_SUBCLASS_ETHERNET,
> +					      USB_CDC_PROTO_NONE),
>  		.driver_info        = (unsigned long)&qmi_wwan_info,
>  	},


Why did you change this whitespace.

Sorry, but this is the sort of garbage which gives AI a bad name.
Please do not post any followups.

Thanks


Bjørn