Re: [PATCH v2] Input: bcm5974 - check endpoint type before starting traffic

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

Hi Dmitry,

On 14.10.23 05:03, Dmitry Torokhov wrote:
> Hi Javier,
> 
> On Thu, Oct 12, 2023 at 06:51:49PM +0200, Javier Carrasco wrote:
>>  
>> +static bool bcm5974_ep_is_int_in(struct usb_host_interface *iface, int addr)
>> +{
>> +	struct usb_endpoint_descriptor *endpoint;
>> +	int i;
>> +
>> +	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
>> +		endpoint = &iface->endpoint[i].desc;
>> +		if (endpoint->bEndpointAddress == addr) {
>> +			if (usb_endpoint_is_int_in(endpoint))
>> +				return true;
>> +		}
>> +	}
>> +	return false;
>> +}
> 
> This essentially reimplements usb_find_endpoint() in a sense, so can we
> instead do:
> 
> 	ep = usb_find_endpoint(iface, addr);
> 	if (!ep || !usb_endpoint_is_int_in(ep)) {
> 		dev_err(...);
> 		return ...;
> 	}
> 
Thanks for your feedback. usb_find_endpoint is a static function from
the usb core and in principle is not available here, but your code
snippet seems to reinterpret usb_check_int_endpoints() for a single
address. I would suggest using usb_check_int_endpoints and pass both
addresses at the same time (if both exist, of course).
> 
> Also it looks like the handling of button endpoint is interleaved with
> the trackpad endpoint, I wonder if it would not be better if we have a
> separate "if (cfg->tp_type == TYPE1)" where we would do the check,
> allocate URB, and did all the rest of set up for button transfers.
Using usb_check_int_endpoints would solve this immediately.
> 
> Thanks.
> 
Thanks and best regards,
Javier Carrasco