[U-Boot] [PATCH v3] usb: new board-specific USB init interface

[Marek Vasut]

Dear Mateusz Zalega,

> On 09/05/13 19:51, Marek Vasut wrote:
> >>> Why not wrap board_usb_init() and board_usb_init_fail() into single
> >>> call. You now pass some flags to board_usb_init() already, so just add
> >>> another for the fail case. How does it sound to you?
> >> 
> >> Like overengineering. It would lead to "board_usb_init(USB_INIT_ALL,
> >> USB_INIT_DEVICE, USB_CLEANUP)" calls, which are not very readable.
> > 
> > This is not what I mean, see this:
> > 
> > int board_usb_init(int index, enum board_usb_init_type init)
> > 
> > Add a new "init" type (or maybe change the init field to be flags) that
> > will say "OK, do a fail init" ?
> 
> As for providing a way to do a selective cleanup if anything gone wrong,
> it's a good idea, but adding such functionality to board_usb_init()
> would be confusing, especially for newcomers. Why not do this in
> board_usb_init_fail(int index, enum board_usb_init_type)?
> 
> ...and maybe rename board_usb_init_fail to board_usb_cleanup.

Cleanup is nice name, yeah.

> >>> Moreover, the 'int index' should likely be unsigned int and the special
> >>> value to init all controllers at once should probably then be
> >>> 0xffffffff
> >> 
> >> Despite our greatest ambitions, I don't think we're likely to use more
> >> than 2^31-1 USB controllers at a time. Besides, negative values look
> >> better both in code and debugger session.
> > 
> > Thinking of it further, instead of using negative value here, like I
> > mentioned above, why not make the "board_usb_init_type" into a field of
> > flags , then add flag to init all controllers at once ?
> 
> That's unnecessary. It wouldn't lead to any practical advantage over
> existing interface.

The advantage would be you won't be mixing two things (value AND value with 
special meaning) into the "index" parameter.

Best regards,
Marek Vasut