Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On 10/28/2013 01:06 PM, Olivier Sobrie wrote:
> On Mon, Oct 28, 2013 at 12:29:31AM +0100, Marc Kleine-Budde wrote:
>> On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
>>> Some devices, like the Kvaser Memorator Professional, have several
>>> bulk in endpoints. Only the first one found should be used by the
>>> driver. The same holds for the bulk out endpoint.
>>
>> Can you give reference where this information comes from? e.g.
>> documentation or another driver?
> 
> Someone using a Kvaser Memorator contacted me and said that its device was
> not working. I found out that the usb endpoint used was not correct and I
> saw in the Kvaser official driver (leaf) that the detection was made
> similarly to what is done in the patch below.
> So the reference is the "leaf" driver of Kvaser.
> 
>>
>>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>>> ---
>>>  drivers/net/can/usb/kvaser_usb.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>>> index 3b95465..bdbf5bf 100644
>>> --- a/drivers/net/can/usb/kvaser_usb.c
>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>> @@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
>>>  
>>>  	iface_desc = &intf->altsetting[0];
>>>  
>>> -	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
>>> +	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
>>> +	     ++i) {
>>>  		endpoint = &iface_desc->endpoint[i].desc;
>>>  
>>> -		if (usb_endpoint_is_bulk_in(endpoint))
>>> +		if (!*in && usb_endpoint_is_bulk_in(endpoint))
>>>  			*in = endpoint;
>>>  
>>> -		if (usb_endpoint_is_bulk_out(endpoint))
>>> +		if (!*out && usb_endpoint_is_bulk_out(endpoint))
>>>  			*out = endpoint;
>>
>> I personally would add a break or return here, together with a short
>> comment:
>>
>> 	/* only use first endpoint for in and out */
>> 	if (*in && *out)
>> 		return;
>>
>>>  	}
>>>  }
>>>
> 
> Fine for me. We can even maybe add a return code to the function and check
> this return code intead of checking again (!dev->bulk_in || !dev->bulk_out).
> By the way, the current return code (-ENOMEM) is maybe not appropriate in
> case the endpoints are not found.

Good idea, have a look at v3.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]