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
