RE: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device matching

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx]
> Sent: 2011年11月14日 17:32
> To: Yao, Costa
> Cc: padovan@xxxxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] Bluetooth: btusb: Use USB_INTERFACE_INFO to do device
> matching
> 
> Hi Costa,
> 
> > 1 Use USB_INTERFACE_INFO to do device matching, other than
> USB_DEVICE_INFO.
> > 2 And we try to find the first interface containing the interrupt endpoint.
> >
> > Signed-off-by: Costa Yao <cqyao@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/bluetooth/btusb.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 9db2476..80d7e2d 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -58,7 +58,7 @@ static struct usb_driver btusb_driver;
> >
> >  static struct usb_device_id btusb_table[] = {
> >  	/* Generic Bluetooth USB device */
> > -	{ USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
> > +	{ USB_INTERFACE_INFO(0xe0, 0x01, 0x01) },
> >
> >  	/* Broadcom SoftSailing reporting vendor specific */
> >  	{ USB_DEVICE(0x05ac, 0x21e1) },
> 
> this actually means that some of the USB_DEVICE entries can be removed.
> Please go through our commit logs for btusb.c and figure out which ones can be
> removed and send a separate patch for that.
Ok. I will send a separate patch for removing some USB_DEVICE entries. 
It makes sense to do remove later. So I will continue to modify this patch for your review firstly. 

> 
> > @@ -915,9 +915,14 @@ static int btusb_probe(struct usb_interface
> > *intf,
> >
> >  	BT_DBG("intf %p id %p", intf, id);
> >
> > -	/* interface numbers are hardcoded in the spec */
> > -	if (intf->cur_altsetting->desc.bInterfaceNumber != 0)
> > +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> > +		ep_desc = &intf->cur_altsetting->endpoint[i].desc;
> > +
> > +		if (usb_endpoint_is_int_in(ep_desc))
> > +			break;
> > +
> >  		return -ENODEV;
> > +	}
> 
> There is no point in walking the endpoint list twice in the probe function. Store
> possible candidates for int_in, bulk_out and bulk_in descriptors so that later on
> we can just assign them to our data.
> 
> Then we can just check if this interface has int and proper bulk endpoints
> available. And in error case return ENODEV.
> 
> Actually thinking about it, we already do that, so just removing the hardcoded
> check for desc.bInterfaceNumber != 0 should do the trick.
> 
> >  	if (!id->driver_info) {
> >  		const struct usb_device_id *match;
> > @@ -1014,8 +1019,9 @@ static int btusb_probe(struct usb_interface
> > *intf,
> >
> >  	hdev->owner = THIS_MODULE;
> >
> > -	/* Interface numbers are hardcoded in the specification */
> > -	data->isoc = usb_ifnum_to_if(data->udev, 1);
> > +
> > +	data->isoc = usb_ifnum_to_if(data->udev,
> > +		1+intf->cur_altsetting->desc.bInterfaceNumber);
> 
> And here you need to to check all interfaces. Just assuming it is +1 might not be
> good enough. Or at least put a proper comment in here that we expect the
> ISOC interface to be +1. And then don't do 1+, I prefer to have
> bInterfaceNumber + 1.
> 
> Regards
> 
> Marcel
> 

��.n��������+%������w��{.n�����{����^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�

[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux