[U-Boot] [PATCH 2/3] Connect the AT91 UDC to USB CDC-ethernet support

[Wolfgang Denk]

Dear Remy Bohmer,

In message <AANLkTi=z0SX9YrXgH-_ogcPdUXhyrHADNErHVVBoNW7L at mail.gmail.com> you wrote:
> 
> > Use struct access for SoC registers. Offset adressing is depreciated. See
> > the
> > struct in my at91_dbgu.h. Use readl/writel to access the hardware.
> 
> Most of the code is copied from Linux. Do you really think we should
> rewrite it all?

Indeed. Using I/O accessors is mandatory in U-Boot.

> > Don't define this here, UART handling is in drivers/serial/atmel_usart.?
> > The first 9 registers in the DBGU are identical to the normal USARTS.
> 
> Indeed, but again, it is copied from Linux...

Maybe we can do it better?

> > Don't include other header files at this point. If a c source
> > requires both DBGU and CPU headers, it shall include both.
> 
> This is how the file looks like in Linux...

You got me here. Of course allowing for easy updates from Linux is
important, but then, we will have bigger differences anyway, for
example for the I/O accessors.

> See Linux...

This is not a general purpose, answers-all-questions/kills-them-all
argument. Don't over-use it. Linux is great, but there is bad code in
Linux like everywhere else. Blindly copying for no better reason that
that Linux is doing that ways is NOT a good idea.

> This file is copied from Linux. That was the goal of this port; to be
> able to integrate new devices and fixes easier in the future. It was
> not our goal to make it nicer compared to Linux. If the comments are
> wrong, I would suggest to fix it in Linux first after which we
> integrate it in U-boot.

Come on.  Here a bit statistics:

-> diff -u linux/drivers/usb/gadget/at91_udc.c u-boot/drivers/usb/gadget/at91_udc.c | wc -l
1057
-> wc -l linux/drivers/usb/gadget/at91_udc.c u_boot/drivers/usb/gadget/at91_udc.c
 1908 linux/drivers/usb/gadget/at91_udc.c
 1553 u-boot/drivers/usb/gadget/at91_udc.c

There are so many lines changed already in the code that claoming
"This file is copied from Linux." does not make much sense any more.

And many of the code changes are to the worse actually.

For example:

	-       ep->is_in = usb_endpoint_dir_in(desc);
	+       ep->is_in = (desc->bEndpointAddress & USB_DIR_IN) != 0;

It is NOT the Linux code that is using CamelCase identifiers, for
example.


> >> +               hang();
> >
> > Does this mean the system is dead and would need a watchdog or reset
> > button to come alive again?
> 
> Yep, if that happens things are really broken...

So broken that you cannot get back to the command line interpreter?
Really?

> >> +       // terminer chaque requete dans la queue
> >
> > C++ comment in unknown language ;)?
> 
> Linux code...

And that justifies it all.


> > There are more coding style problems in the source, but
> > for source pulled from the kernel that might be ok (Wolfgang?)
> 
> I understand your remarks, and for new written code they are 100%
> valid to me (I would even not have posted it at all like this),
> however, most of this code comes from Linux. It is a port to U-boot.
> True, things can be done differently, but that would make future
> syncing and integration of new drivers much more difficult.

Face it: the code as is can already be synced only manually. It is way
too different already.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I am not now, nor have I ever been, a member of the demigodic party.
                                                   -- Dennis Ritchie