[PATCH 1/2] net: qmi_wwan: add Telit LE920 newer firmware support

[PATCH 1/2] net: qmi_wwan: add Telit LE920 newer firmware support

[Fabio Porcedda]

Newer firmware use a new pid and a different interface.

Signed-off-by: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/qmi_wwan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 6312332..5f6b6fa 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -714,6 +714,7 @@ static const struct usb_device_id products[] = {
 	{QMI_FIXED_INTF(0x2357, 0x0201, 4)},	/* TP-LINK HSUPA Modem MA180 */
 	{QMI_FIXED_INTF(0x2357, 0x9000, 4)},	/* TP-LINK MA260 */
 	{QMI_FIXED_INTF(0x1bc7, 0x1200, 5)},	/* Telit LE920 */
+	{QMI_FIXED_INTF(0x1bc7, 0x1201, 2)},	/* Telit LE920 */
 	{QMI_FIXED_INTF(0x1e2d, 0x12d1, 4)},	/* Cinterion PLxx */
 
 	/* 4. Gobi 1000 devices */
-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[Fabio Porcedda]

Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
---
 drivers/net/usb/qmi_wwan.c | 56 +++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 5f6b6fa..0e59f9e 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -143,16 +143,22 @@ static const struct net_device_ops qmi_wwan_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 };
 
-/* using a counter to merge subdriver requests with our own into a combined state */
+/* using a counter to merge subdriver requests with our own into a
+ * combined state
+ */
 static int qmi_wwan_manage_power(struct usbnet *dev, int on)
 {
 	struct qmi_wwan_state *info = (void *)&dev->data;
 	int rv = 0;
 
-	dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__, atomic_read(&info->pmcount), on);
+	dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__,
+		atomic_read(&info->pmcount), on);
 
-	if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
-		/* need autopm_get/put here to ensure the usbcore sees the new value */
+	if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
+	    (!on && atomic_dec_and_test(&info->pmcount))) {
+		/* need autopm_get/put here to ensure the usbcore sees
+		 * the new value
+		 */
 		rv = usb_autopm_get_interface(dev->intf);
 		if (rv < 0)
 			goto err;
@@ -199,7 +205,8 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev)
 	atomic_set(&info->pmcount, 0);
 
 	/* register subdriver */
-	subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, 4096, &qmi_wwan_cdc_wdm_manage_power);
+	subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc,
+					 4096, &qmi_wwan_cdc_wdm_manage_power);
 	if (IS_ERR(subdriver)) {
 		dev_err(&info->control->dev, "subdriver registration failed\n");
 		rv = PTR_ERR(subdriver);
@@ -228,7 +235,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 	struct usb_driver *driver = driver_of(intf);
 	struct qmi_wwan_state *info = (void *)&dev->data;
 
-	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < sizeof(struct qmi_wwan_state)));
+	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) <
+		      sizeof(struct qmi_wwan_state)));
 
 	/* set up initial state */
 	info->control = intf;
@@ -250,7 +258,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 				goto err;
 			}
 			if (h->bLength != sizeof(struct usb_cdc_header_desc)) {
-				dev_dbg(&intf->dev, "CDC header len %u\n", h->bLength);
+				dev_dbg(&intf->dev, "CDC header len %u\n",
+					h->bLength);
 				goto err;
 			}
 			break;
@@ -260,7 +269,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 				goto err;
 			}
 			if (h->bLength != sizeof(struct usb_cdc_union_desc)) {
-				dev_dbg(&intf->dev, "CDC union len %u\n", h->bLength);
+				dev_dbg(&intf->dev, "CDC union len %u\n",
+					h->bLength);
 				goto err;
 			}
 			cdc_union = (struct usb_cdc_union_desc *)buf;
@@ -271,15 +281,15 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 				goto err;
 			}
 			if (h->bLength != sizeof(struct usb_cdc_ether_desc)) {
-				dev_dbg(&intf->dev, "CDC ether len %u\n",  h->bLength);
+				dev_dbg(&intf->dev, "CDC ether len %u\n",
+					h->bLength);
 				goto err;
 			}
 			cdc_ether = (struct usb_cdc_ether_desc *)buf;
 			break;
 		}
 
-		/*
-		 * Remember which CDC functional descriptors we've seen.  Works
+		/* Remember which CDC functional descriptors we've seen.  Works
 		 * for all types we care about, of which USB_CDC_ETHERNET_TYPE
 		 * (0x0f) is the highest numbered
 		 */
@@ -293,10 +303,14 @@ next_desc:
 
 	/* Use separate control and data interfaces if we found a CDC Union */
 	if (cdc_union) {
-		info->data = usb_ifnum_to_if(dev->udev, cdc_union->bSlaveInterface0);
-		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 || !info->data) {
-			dev_err(&intf->dev, "bogus CDC Union: master=%u, slave=%u\n",
-				cdc_union->bMasterInterface0, cdc_union->bSlaveInterface0);
+		info->data = usb_ifnum_to_if(dev->udev,
+					     cdc_union->bSlaveInterface0);
+		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 ||
+		    !info->data) {
+			dev_err(&intf->dev,
+				"bogus CDC Union: master=%u, slave=%u\n",
+				cdc_union->bMasterInterface0,
+				cdc_union->bSlaveInterface0);
 			goto err;
 		}
 	}
@@ -374,8 +388,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
 	struct qmi_wwan_state *info = (void *)&dev->data;
 	int ret;
 
-	/*
-	 * Both usbnet_suspend() and subdriver->suspend() MUST return 0
+	/* Both usbnet_suspend() and subdriver->suspend() MUST return 0
 	 * in system sleep context, otherwise, the resume callback has
 	 * to recover device from previous suspend failure.
 	 */
@@ -383,7 +396,8 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
 	if (ret < 0)
 		goto err;
 
-	if (intf == info->control && info->subdriver && info->subdriver->suspend)
+	if (intf == info->control && info->subdriver &&
+	    info->subdriver->suspend)
 		ret = info->subdriver->suspend(intf, message);
 	if (ret < 0)
 		usbnet_resume(intf);
@@ -396,7 +410,8 @@ static int qmi_wwan_resume(struct usb_interface *intf)
 	struct usbnet *dev = usb_get_intfdata(intf);
 	struct qmi_wwan_state *info = (void *)&dev->data;
 	int ret = 0;
-	bool callsub = (intf == info->control && info->subdriver && info->subdriver->resume);
+	bool callsub = (intf == info->control && info->subdriver &&
+			info->subdriver->resume);
 
 	if (callsub)
 		ret = info->subdriver->resume(intf);
@@ -777,7 +792,8 @@ static const struct usb_device_id products[] = {
 };
 MODULE_DEVICE_TABLE(usb, products);
 
-static int qmi_wwan_probe(struct usb_interface *intf, const struct usb_device_id *prod)
+static int qmi_wwan_probe(struct usb_interface *intf,
+			  const struct usb_device_id *prod)
 {
 	struct usb_device_id *id = (struct usb_device_id *)prod;
 
-- 
1.8.4

Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[Bjørn Mork]

Sorry, I really don't see the point of this.  Yes, the lines are longer
than 80 columns, but breaking them don't improve the readability at
all.  On the contrary, IMHO.

So NAK from me for this part.


Bjørn

Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Signed-off-by: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/net/usb/qmi_wwan.c | 56 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 5f6b6fa..0e59f9e 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -143,16 +143,22 @@ static const struct net_device_ops qmi_wwan_netdev_ops = {
>  	.ndo_validate_addr	= eth_validate_addr,
>  };
>  
> -/* using a counter to merge subdriver requests with our own into a combined state */
> +/* using a counter to merge subdriver requests with our own into a
> + * combined state
> + */
>  static int qmi_wwan_manage_power(struct usbnet *dev, int on)
>  {
>  	struct qmi_wwan_state *info = (void *)&dev->data;
>  	int rv = 0;
>  
> -	dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__, atomic_read(&info->pmcount), on);
> +	dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__,
> +		atomic_read(&info->pmcount), on);
>  
> -	if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
> -		/* need autopm_get/put here to ensure the usbcore sees the new value */
> +	if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
> +	    (!on && atomic_dec_and_test(&info->pmcount))) {
> +		/* need autopm_get/put here to ensure the usbcore sees
> +		 * the new value
> +		 */
>  		rv = usb_autopm_get_interface(dev->intf);
>  		if (rv < 0)
>  			goto err;
> @@ -199,7 +205,8 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev)
>  	atomic_set(&info->pmcount, 0);
>  
>  	/* register subdriver */
> -	subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, 4096, &qmi_wwan_cdc_wdm_manage_power);
> +	subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc,
> +					 4096, &qmi_wwan_cdc_wdm_manage_power);
>  	if (IS_ERR(subdriver)) {
>  		dev_err(&info->control->dev, "subdriver registration failed\n");
>  		rv = PTR_ERR(subdriver);
> @@ -228,7 +235,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>  	struct usb_driver *driver = driver_of(intf);
>  	struct qmi_wwan_state *info = (void *)&dev->data;
>  
> -	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < sizeof(struct qmi_wwan_state)));
> +	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) <
> +		      sizeof(struct qmi_wwan_state)));
>  
>  	/* set up initial state */
>  	info->control = intf;
> @@ -250,7 +258,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>  				goto err;
>  			}
>  			if (h->bLength != sizeof(struct usb_cdc_header_desc)) {
> -				dev_dbg(&intf->dev, "CDC header len %u\n", h->bLength);
> +				dev_dbg(&intf->dev, "CDC header len %u\n",
> +					h->bLength);
>  				goto err;
>  			}
>  			break;
> @@ -260,7 +269,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>  				goto err;
>  			}
>  			if (h->bLength != sizeof(struct usb_cdc_union_desc)) {
> -				dev_dbg(&intf->dev, "CDC union len %u\n", h->bLength);
> +				dev_dbg(&intf->dev, "CDC union len %u\n",
> +					h->bLength);
>  				goto err;
>  			}
>  			cdc_union = (struct usb_cdc_union_desc *)buf;
> @@ -271,15 +281,15 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>  				goto err;
>  			}
>  			if (h->bLength != sizeof(struct usb_cdc_ether_desc)) {
> -				dev_dbg(&intf->dev, "CDC ether len %u\n",  h->bLength);
> +				dev_dbg(&intf->dev, "CDC ether len %u\n",
> +					h->bLength);
>  				goto err;
>  			}
>  			cdc_ether = (struct usb_cdc_ether_desc *)buf;
>  			break;
>  		}
>  
> -		/*
> -		 * Remember which CDC functional descriptors we've seen.  Works
> +		/* Remember which CDC functional descriptors we've seen.  Works
>  		 * for all types we care about, of which USB_CDC_ETHERNET_TYPE
>  		 * (0x0f) is the highest numbered
>  		 */
> @@ -293,10 +303,14 @@ next_desc:
>  
>  	/* Use separate control and data interfaces if we found a CDC Union */
>  	if (cdc_union) {
> -		info->data = usb_ifnum_to_if(dev->udev, cdc_union->bSlaveInterface0);
> -		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 || !info->data) {
> -			dev_err(&intf->dev, "bogus CDC Union: master=%u, slave=%u\n",
> -				cdc_union->bMasterInterface0, cdc_union->bSlaveInterface0);
> +		info->data = usb_ifnum_to_if(dev->udev,
> +					     cdc_union->bSlaveInterface0);
> +		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 ||
> +		    !info->data) {
> +			dev_err(&intf->dev,
> +				"bogus CDC Union: master=%u, slave=%u\n",
> +				cdc_union->bMasterInterface0,
> +				cdc_union->bSlaveInterface0);
>  			goto err;
>  		}
>  	}
> @@ -374,8 +388,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>  	struct qmi_wwan_state *info = (void *)&dev->data;
>  	int ret;
>  
> -	/*
> -	 * Both usbnet_suspend() and subdriver->suspend() MUST return 0
> +	/* Both usbnet_suspend() and subdriver->suspend() MUST return 0
>  	 * in system sleep context, otherwise, the resume callback has
>  	 * to recover device from previous suspend failure.
>  	 */
> @@ -383,7 +396,8 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>  	if (ret < 0)
>  		goto err;
>  
> -	if (intf == info->control && info->subdriver && info->subdriver->suspend)
> +	if (intf == info->control && info->subdriver &&
> +	    info->subdriver->suspend)
>  		ret = info->subdriver->suspend(intf, message);
>  	if (ret < 0)
>  		usbnet_resume(intf);
> @@ -396,7 +410,8 @@ static int qmi_wwan_resume(struct usb_interface *intf)
>  	struct usbnet *dev = usb_get_intfdata(intf);
>  	struct qmi_wwan_state *info = (void *)&dev->data;
>  	int ret = 0;
> -	bool callsub = (intf == info->control && info->subdriver && info->subdriver->resume);
> +	bool callsub = (intf == info->control && info->subdriver &&
> +			info->subdriver->resume);
>  
>  	if (callsub)
>  		ret = info->subdriver->resume(intf);
> @@ -777,7 +792,8 @@ static const struct usb_device_id products[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, products);
>  
> -static int qmi_wwan_probe(struct usb_interface *intf, const struct usb_device_id *prod)
> +static int qmi_wwan_probe(struct usb_interface *intf,
> +			  const struct usb_device_id *prod)
>  {
>  	struct usb_device_id *id = (struct usb_device_id *)prod;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] net: qmi_wwan: add Telit LE920 newer firmware support

[Bjørn Mork]

Fabio Porcedda <fabio.porcedda@gmail.com> writes:

> Newer firmware use a new pid and a different interface.
>
> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
> ---
>  drivers/net/usb/qmi_wwan.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 6312332..5f6b6fa 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -714,6 +714,7 @@ static const struct usb_device_id products[] = {
>  	{QMI_FIXED_INTF(0x2357, 0x0201, 4)},	/* TP-LINK HSUPA Modem MA180 */
>  	{QMI_FIXED_INTF(0x2357, 0x9000, 4)},	/* TP-LINK MA260 */
>  	{QMI_FIXED_INTF(0x1bc7, 0x1200, 5)},	/* Telit LE920 */
> +	{QMI_FIXED_INTF(0x1bc7, 0x1201, 2)},	/* Telit LE920 */
>  	{QMI_FIXED_INTF(0x1e2d, 0x12d1, 4)},	/* Cinterion PLxx */
>  
>  	/* 4. Gobi 1000 devices */

You can add my

Acked-by: Bjørn Mork <bjorn@mork.no>

to this patch if you like.  But I guess you may have to resend it if the
netdev maintainer agree with me regarding the line length patch.


Bjørn

Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[Fabio Porcedda]

Hi Bjørn,
thanks for reviewing.

On Wed, Sep 25, 2013 at 3:31 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Sorry, I really don't see the point of this.  Yes, the lines are longer
> than 80 columns, but breaking them don't improve the readability at
> all.  On the contrary, IMHO.
>
> So NAK from me for this part.

Which changes do you think do not improve the readability?
I'm sure that at least some of them actually improve the readability.

Best regards
Fabio Porcedda

> Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>
>> Signed-off-by: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/net/usb/qmi_wwan.c | 56 +++++++++++++++++++++++++++++-----------------
>>  1 file changed, 36 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
>> index 5f6b6fa..0e59f9e 100644
>> --- a/drivers/net/usb/qmi_wwan.c
>> +++ b/drivers/net/usb/qmi_wwan.c
>> @@ -143,16 +143,22 @@ static const struct net_device_ops qmi_wwan_netdev_ops = {
>>       .ndo_validate_addr      = eth_validate_addr,
>>  };
>>
>> -/* using a counter to merge subdriver requests with our own into a combined state */
>> +/* using a counter to merge subdriver requests with our own into a
>> + * combined state
>> + */
>>  static int qmi_wwan_manage_power(struct usbnet *dev, int on)
>>  {
>>       struct qmi_wwan_state *info = (void *)&dev->data;
>>       int rv = 0;
>>
>> -     dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__, atomic_read(&info->pmcount), on);
>> +     dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__,
>> +             atomic_read(&info->pmcount), on);
>>
>> -     if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
>> -             /* need autopm_get/put here to ensure the usbcore sees the new value */
>> +     if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
>> +         (!on && atomic_dec_and_test(&info->pmcount))) {
>> +             /* need autopm_get/put here to ensure the usbcore sees
>> +              * the new value
>> +              */
>>               rv = usb_autopm_get_interface(dev->intf);
>>               if (rv < 0)
>>                       goto err;
>> @@ -199,7 +205,8 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev)
>>       atomic_set(&info->pmcount, 0);
>>
>>       /* register subdriver */
>> -     subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, 4096, &qmi_wwan_cdc_wdm_manage_power);
>> +     subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc,
>> +                                      4096, &qmi_wwan_cdc_wdm_manage_power);
>>       if (IS_ERR(subdriver)) {
>>               dev_err(&info->control->dev, "subdriver registration failed\n");
>>               rv = PTR_ERR(subdriver);
>> @@ -228,7 +235,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>>       struct usb_driver *driver = driver_of(intf);
>>       struct qmi_wwan_state *info = (void *)&dev->data;
>>
>> -     BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < sizeof(struct qmi_wwan_state)));
>> +     BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) <
>> +                   sizeof(struct qmi_wwan_state)));
>>
>>       /* set up initial state */
>>       info->control = intf;
>> @@ -250,7 +258,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>>                               goto err;
>>                       }
>>                       if (h->bLength != sizeof(struct usb_cdc_header_desc)) {
>> -                             dev_dbg(&intf->dev, "CDC header len %u\n", h->bLength);
>> +                             dev_dbg(&intf->dev, "CDC header len %u\n",
>> +                                     h->bLength);
>>                               goto err;
>>                       }
>>                       break;
>> @@ -260,7 +269,8 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>>                               goto err;
>>                       }
>>                       if (h->bLength != sizeof(struct usb_cdc_union_desc)) {
>> -                             dev_dbg(&intf->dev, "CDC union len %u\n", h->bLength);
>> +                             dev_dbg(&intf->dev, "CDC union len %u\n",
>> +                                     h->bLength);
>>                               goto err;
>>                       }
>>                       cdc_union = (struct usb_cdc_union_desc *)buf;
>> @@ -271,15 +281,15 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
>>                               goto err;
>>                       }
>>                       if (h->bLength != sizeof(struct usb_cdc_ether_desc)) {
>> -                             dev_dbg(&intf->dev, "CDC ether len %u\n",  h->bLength);
>> +                             dev_dbg(&intf->dev, "CDC ether len %u\n",
>> +                                     h->bLength);
>>                               goto err;
>>                       }
>>                       cdc_ether = (struct usb_cdc_ether_desc *)buf;
>>                       break;
>>               }
>>
>> -             /*
>> -              * Remember which CDC functional descriptors we've seen.  Works
>> +             /* Remember which CDC functional descriptors we've seen.  Works
>>                * for all types we care about, of which USB_CDC_ETHERNET_TYPE
>>                * (0x0f) is the highest numbered
>>                */
>> @@ -293,10 +303,14 @@ next_desc:
>>
>>       /* Use separate control and data interfaces if we found a CDC Union */
>>       if (cdc_union) {
>> -             info->data = usb_ifnum_to_if(dev->udev, cdc_union->bSlaveInterface0);
>> -             if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 || !info->data) {
>> -                     dev_err(&intf->dev, "bogus CDC Union: master=%u, slave=%u\n",
>> -                             cdc_union->bMasterInterface0, cdc_union->bSlaveInterface0);
>> +             info->data = usb_ifnum_to_if(dev->udev,
>> +                                          cdc_union->bSlaveInterface0);
>> +             if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 ||
>> +                 !info->data) {
>> +                     dev_err(&intf->dev,
>> +                             "bogus CDC Union: master=%u, slave=%u\n",
>> +                             cdc_union->bMasterInterface0,
>> +                             cdc_union->bSlaveInterface0);
>>                       goto err;
>>               }
>>       }
>> @@ -374,8 +388,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>>       struct qmi_wwan_state *info = (void *)&dev->data;
>>       int ret;
>>
>> -     /*
>> -      * Both usbnet_suspend() and subdriver->suspend() MUST return 0
>> +     /* Both usbnet_suspend() and subdriver->suspend() MUST return 0
>>        * in system sleep context, otherwise, the resume callback has
>>        * to recover device from previous suspend failure.
>>        */
>> @@ -383,7 +396,8 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>>       if (ret < 0)
>>               goto err;
>>
>> -     if (intf == info->control && info->subdriver && info->subdriver->suspend)
>> +     if (intf == info->control && info->subdriver &&
>> +         info->subdriver->suspend)
>>               ret = info->subdriver->suspend(intf, message);
>>       if (ret < 0)
>>               usbnet_resume(intf);
>> @@ -396,7 +410,8 @@ static int qmi_wwan_resume(struct usb_interface *intf)
>>       struct usbnet *dev = usb_get_intfdata(intf);
>>       struct qmi_wwan_state *info = (void *)&dev->data;
>>       int ret = 0;
>> -     bool callsub = (intf == info->control && info->subdriver && info->subdriver->resume);
>> +     bool callsub = (intf == info->control && info->subdriver &&
>> +                     info->subdriver->resume);
>>
>>       if (callsub)
>>               ret = info->subdriver->resume(intf);
>> @@ -777,7 +792,8 @@ static const struct usb_device_id products[] = {
>>  };
>>  MODULE_DEVICE_TABLE(usb, products);
>>
>> -static int qmi_wwan_probe(struct usb_interface *intf, const struct usb_device_id *prod)
>> +static int qmi_wwan_probe(struct usb_interface *intf,
>> +                       const struct usb_device_id *prod)
>>  {
>>       struct usb_device_id *id = (struct usb_device_id *)prod;



-- 
Fabio Porcedda
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[Bjørn Mork]

Fabio Porcedda <fabio.porcedda@gmail.com> writes:

> Hi Bjørn,
> thanks for reviewing.
>
> On Wed, Sep 25, 2013 at 3:31 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> Sorry, I really don't see the point of this.  Yes, the lines are longer
>> than 80 columns, but breaking them don't improve the readability at
>> all.  On the contrary, IMHO.
>>
>> So NAK from me for this part.
>
> Which changes do you think do not improve the readability?

Anything that breaks a previously unbroken argument list will reduce the
readability in my opinion.  The lines can of course not be unlimited,
but there is no need to set the limit as low as 80 columns.  Feedback
I've got from developers using e.g. 80 column braille devices is that
longer lines isn't really a problem for them either.

The point is that the properly broken lines aren't that much more
readable than a line broken by your terminal, even if you set it to 80
columns.  You do of course not get the proper indendation of the broken
lines, but you get a terminal hint telling you that it is a continuation
line.  Which is often better that having to figure it out based on the
code.

This isn't to say that I don't think writing shorter lines is a goal.
I'd really like the code to be nice and compact, avoiding this
discussion completely by just keeping every line shorter than 80.  But
I'm just not that good a programmer :-)

I'd surely appreciate any patch moving the code in that direction.

> I'm sure that at least some of them actually improve the readability.

Yes, reformatting the comments improves the readability.  I just don't
think that makes any sense on it's own, if the surrounding lines are
kept longer.

If the code can be rewritten to actually *be* shorter than 80 columns
instead of just breaking too long code lines, then that is a definite
improvement.  You didn't have many examples of this, I believe.  But
reducing the indent level, removing unnecessary function parameters, or
splitting declaration and initialization are changes which often reduce
the line length while improving the readability at the same time.
Breaking lines rarely do.

This is the only one of your code changes which I can be convinced to
agreeing may improve readability:

-     if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
+     if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
+         (!on && atomic_dec_and_test(&info->pmcount))) {


But it mostly points out a piece of code which is too complex in the
first place, begging to be rewritten in a more elegant form.

Just for the record:  All this is of course only my personal opinions,
and I am fine with being overridden even if I originally wrote the ugly
code :-)

David decides.


Bjørn

RE: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[David Laight]

> Anything that breaks a previously unbroken argument list will reduce the
> readability in my opinion.  The lines can of course not be unlimited,
> but there is no need to set the limit as low as 80 columns.  Feedback
> I've got from developers using e.g. 80 column braille devices is that
> longer lines isn't really a problem for them either.

The main reason for limiting the line length is so that things look
'sensible' when you have a lot of screen windows displaying different
files. You don't want wrapped code, and you definitely don't want
the RHS of long lines hidden.
With a 1600x1200 monitor I'll display six 80x40 windows (and probably
have some more partially visible ones).

Personally I indent continuation lines by 4 chars if using 8 char
'normal' indentation and 8 chars if using 4. This gives a lot more
room on the continuation lines than the Linux 'line up with the
previous line'.

> This is the only one of your code changes which I can be convinced to
> agreeing may improve readability:
> 
> -     if ((on && || (!on && atomic_dec_and_test(&info->pmcount))) {
> +     if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
> +         (!on && atomic_dec_and_test(&info->pmcount))) {

That can be written succinctly as:
	if (on ? atomic_add_return(1, &info->pmcount) == 1)
	       : atomic_dec_and_test(&info->pmcount)) {
although that construct is somewhat frowned upon!

	David

Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Wed, 25 Sep 2013 15:31:39 +0200

> Sorry, I really don't see the point of this.  Yes, the lines are longer
> than 80 columns, but breaking them don't improve the readability at
> all.  On the contrary, IMHO.
> 
> So NAK from me for this part.

Sorry, breaking things up like this is what we do everywhere.

It's not strictly 80 columns but "about 80 columns".

And the only place we don't break things up is for messages that end
up in the kernel log.  Those strings can take the line as long as is
reasonably necessary.

But at argument delimiters and operators for long complex expressions,
we break it up so that the lines do not significantly exceed 80
columns.

Anything else is your opinion, and not what we've been trying to do
consistently across the kernel and in particular in the networking.

Therefore this patch is perfectly fine and I intent to apply it.

Re: [PATCH 1/2] net: qmi_wwan: add Telit LE920 newer firmware support

[David Miller]

From: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Wed, 25 Sep 2013 11:21:25 +0200

> Newer firmware use a new pid and a different interface.
> 
> Signed-off-by: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applied to net-next
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

[David Miller]

From: Fabio Porcedda <fabio.porcedda@gmail.com>
Date: Wed, 25 Sep 2013 11:21:26 +0200

> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>

Applied to net-next, thanks.