Re: [patch 7/8] File Storage Gadget Support

On Mon, 19 Mar 2007, Felipe Balbi wrote:

> 	This patch adds some ifdefs to make the file-storage gadget work
> 	as a usb_function module.

This is really ugly.  You have added _lots_ of changes like this:

> @@ -1127,3 +1150,8 @@ static void fsg_disconnect(struct usb_ga
>  {
> +#ifdef USB_COMPOSITE_DEVICE
> +	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> +	struct fsg_dev *fsg = get_composite_data(cdev);
> +#else
>  	struct fsg_dev		*fsg = get_gadget_data(gadget);
> +#endif

Why not instead set things up in a header file so that when 
USB_COMPOSITE_DEVICE isn't defined, we have

#define get_composite_data(cdev)	cdev

Then just do

	struct fsg_dev *fsg = get_composite_data(get_gadget_data(gadget));

You also have lots of changes like this:

> @@ -1303,2 +1339,6 @@ static int class_setup_req(struct fsg_de
>  
> +#ifdef USB_COMPOSITE_DEVICE
> +	w_index -= cdev->current_func->interface_shift;
> +#endif /* USB_COMPOSITE_DEVICE */

If you would simply add an "interface_shift" field to struct fsg_dev
(normally 0), then this could simply be:

	w_index -= fsg->interface_shift;

Better yet, define a macro like this:

#ifdef USB_COMPOSITE_DEVICE
#define ADJUST_INTF(i)		do {i -= fsg->interface_shift;} while (0)
#else
#define ADJUST_INTF(i)		do {} while (0)
#endif

Together these two changes would remove over half of your giant patch.

Alan Stern


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel