[U-Boot] [PATCH v2 4/4] updates for DFU and atmel usba udc

[Marcel Janssen]

Hi Remy,

> 2011/2/15 Marcel Janssen <korgull at home.nl>:
> > On Tuesday, February 15, 2011 07:43:34 pm Remy Bohmer wrote:
> >> Hi,
> >> 
> >> 2011/2/13 Marcel Janssen <korgull at home.nl>:
> >> > From: Marcel <korgull at home.nl>
> >> > 
> >> > Signed-off-by: Marcel <korgull at home.nl>
> >> > ---
> >> >  arch/arm/cpu/arm926ejs/at91/led.c   |  119
> >> > +++++++++++++++++++++++++++++++++-
> >> 
> >> Why is this part if this patch?
> >> It does not seem to be related to USB stuff. Please make it a seperate
> >> patch.
> > 
> > I optionally use the LED's in DFU mode so that there's interaction while
> > upgrading the board.
> 
> U-boot has a terminal/monitor. It is not up to the driver to control
> LEDs that might or might not be on a board.

OK, good to know. I'll check that out, but not today.
For now I can remove the LED code I think. Than in a couple of months I can 
can check how to use the monitor code.

> > You might believe the host could handle this, but I like
> > the device to indicate activity as well.
> > Somehow I couldn't get it working without changing led.c I think, but I
> > may have missed something.
> 
> The problem is here that you mix up sub-systems.
> Modifying LED code should be independent from USB driver code, and
> really not in the same patch.
>
> >> >  common/Makefile                     |    1 +
> >> >  common/update_dfu.c                 |    2 -
> >> >  drivers/usb/gadget/usbdfu.c         |   14 +++--
> 
> These files should be completely generic code, that even would work on
> X86, PowerPC and so on.

Agree on that. It would be optiomal. Not everything is, the first time :-)

> >> >  drivers/usb/gadget/atmel_usba_udc.c |    8 +-
> 
> Only this driver file should be Atmel USBA specific, but NOT SoC
> specific, and absolutely not board or board config related.

ok.

> >> Please, only make a patch series with only USB-DFU stuff in it, drop
> >> the add-board code from this series. The board code is not acceptable
> >> for mainstream since it does not follow the new
> >> u-boot-atmel->rework110202 tree style of adding at91 boards. It will
> >> take you a huge amount of effort to make it suitable.
> >> Further, both parts of the patch series are not related and you are
> >> now creating a link between the Atmel tree and the USB tree, which
> >> makes it even more difficult.
> > 
> > I'm actually busy fixing the board code for u-boot-atmel->rework110202
> > 
> > Most of it is working so I hope I can create the patches as you like
> > them.
> 
> Hmm, Let's make it even more black/white: I do not have to like the
> board code. ;-)
> Reinhard is the Atmel maintainer. He needs to pull in the Board code.
> I only care about generic USB code... ;-)))

hmmm.
The black and white this is that after today I don't have a single hour to 
spend on this code :-)

> Please make 2 unrelated patch series (1 series for USB DFU support, 1
> series for board support), or it will never hit mainline...
> AND: I would really recommend to build a decent USB/DFU patch series
> first. Forget the board for now. The board seems to depend on DFU
> support, not the other way around.
> If generic DFU code is really generic and acceptable, I will pull it
> in my tree. I am willing to fix small issues in the series to assist
> you, but I will not do major redesigns...

Well, the board  does not depend on DFU. Not even the USBD controller is 
activated in the board config. It is left up to a script to handle that though 
update_dfu.c which defines a command for that. I may have left some parts in 
that don't really belong there, I haven't check it yet.

Best regards,
Marcel