[sane-devel] patch for sanei_usb.c

[Henning Meier-Geinitz]

Hi,

On Tue, Sep 07, 2004 at 08:21:19PM +0000, gerard klaver wrote:
> 1. Detects endpoints for bAlternateSetting > 0 (for example webcams)
> 2. Some %d debug values for endpoints are changed to 0x%x
> 3. More debug info for control and isochronous mode

Please use "indent -gnu" before committing.

> +        /* looking for the max. bAlternateSetting value */
> +      alt_setting_nr = 0;
> +	 while  ( alt_setting_nr == (dev->config[0].interface[0].altsetting[alt_setting_nr].bAlternateSetting )) 
> +	 {
> +		 alt_setting_nr++;
> +	    }
> +
> +	 num_altsetting = alt_setting_nr;
> +	for (alt_setting_nr = 0;
> +		   alt_setting_nr < num_altsetting;
> +		   alt_setting_nr++)
> +	{

I think this is what your code actually does: The first part just checks
if the altsetting number of the interface descriptor it looks at is
the same as it's position in the array of alternative settings. I
think this is alway the case. So we always read beyond the limits of
the interface descriptor. I guess it's just luck that there isn't a
segfault.

bAlternateSetting is the number of the alternate setting, not the
total number of settings and not the currently used one. By default,
always altsetting 0 is used. 

usb_interface.num_altsetting contains the total number of alternate
settings. Use that one as the upper limit (- 1).

I'm not sure if that works the way you did it. E.g. don't we need to
set the alternate setting before using the endpoints
(usb_set_altinterface ())? I don't have devices with more that one
altsetting, so I can't test. And can't we get in trouble if the is
e.g. one bulk-in endpoint in altsetting 0; one bulk-in and one
bulk-out in altsetting 1? I guess sanei_usb_read would use the
endpoint from altsetting 0 and sanei_usb_write the one from altsetting
1.

> +	DBG (5, "sanei_usb_open: alt_setting_nr: %d\n", alt_setting_nr);
> +        interface = &dev->config[0].interface->altsetting[alt_setting_nr];
> +       


> +         DBG (5, "sanei_usb_open:num:: %d endpoint:: 0x%x\n", num, endpoint);

That gives:
sanei_usb.c:668: warning: unsigned int format, pointer arg (arg 4)

(endpoint is the pointer to the endpoint struct).

And please try to use a debug format similar to the one that's already
in the code. The "::" looks like c++ :-)

> +	  transfer_type = endpoint->bmAttributes & USB_ENDPOINT_TYPE_MASK;
>  	  address = endpoint->bEndpointAddress & USB_ENDPOINT_ADDRESS_MASK;
>  	  direction = endpoint->bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> +
> +         DBG (5, "sanei_usb_open: direction :: %d)\n", direction);
>  	  transfer_type = endpoint->bmAttributes & USB_ENDPOINT_TYPE_MASK;
>  
> +         DBG (5, "sanei_usb_open:  address: %d transfertype: %d\n",
> +	         address, transfer_type);

While you are at it, you could add some code to print what all that
means (e.g. the direction) :-)

> +	  else if (transfer_type == USB_ENDPOINT_TYPE_CONTROL)

Do devices with a control endpoint (other than endpoint 0) exist? The
USB spec says that you are correct but I have never seen one.

Otherwise the patch looks ok.

Bye,
  Henning