Thanks a lot. I fixed most of the issues. And I'll resend the patch
after you and other people finishes the first time review.
On 9/22/06, Randy.Dunlap <[email protected]> wrote:
On Fri, 22 Sep 2006 13:04:43 +0800 Luke Yang wrote:
> And we also fixed a lot of other issues and ported it to 2.6.18 now.
> As usual, this architecture patch is too big so I just give a link
> here. Please review it and give you comments, we really appreciate.
It's 2 MB. I doubt that many people will review it. :(
Yes it is big. But as an architecture patch, it is this big :(
Thank you very much for reviewing it.
> This is a big patch so I only put a link here:
> http://blackfin.uclinux.org/frs/download.php/1010/blackfin_arch_2.6.18.patch
I have some comments on it. The summary of most of them is
that the patch doesn't quite look like a Linux patch.
You may have heard that comment previously... I don't know.
Here are some detailed comments.
1. For Kconfig help text, help text should be indented 2 spaces.
Several of these need to be cleaned up.
Done.
2. Kconfig help text: don't make text lines go past column 80
for 2 reasons: (1) we want editors in a xterm to be able to read
source files easily (without lots of wrapping) and (2) users should
be able to read an item's help text [in menuconfig] without needing
to scroll left/right (with arrow keys).
Done.
3. Kconfig help text: end sentences with a period (".").
Done
4. Kconfig and header files: excessive and gratuitous use of
spaces around parentheses.
example 1:
+ depends on ( BFIN533_STAMP || BFIN533_BLUETECHNIX_CM )
should be:
+ depends on (BFIN533_STAMP || BFIN533_BLUETECHNIX_CM)
(these parens actually aren't needed at all, but if used,
some spaces there should be lost)
example 2:
+#if defined( CONFIG_BLKFIN_DCACHE )
should be:
+#if defined(CONFIG_BLKFIN_DCACHE)
Done.
5. Several of the head.S files have really sloppy indentation or
formatting in them.
Fixed.
6. Don't introduce new typedefs. If the kernel header files
already have a typedef and you need to use that (e.g., in
include/asm-blackfin/*.h), that's OK, but it's not OK to add
new typedefs [with a few rare, extreme exceptions].
And structs are almost NEVER typedeffed. Just use struct xyzzy
{
...
};
I removed some "new typedef" we added. But for the common ones in
some of the header files, they have to be there for now...
7. In function prototypes and externs (as in header files),
include parameter names, not just data types.
Fixed.
8. No space between "*" and parameter name. Example:
+void spi_enable(spi_device_t * spi_dev);
Fixed.
should be:
+void spi_enable(spi_device_t *spi_dev);
9. In struct sport_register, what are all of those volatiles
for? Use of volatile usually
indicates bad locking or bad memory barriers, etc., somewhere.
This is not a bug. In embedded, a memory mapped register should be
defined as volatile, becasue the hardware may change it without
notifying the software.
10. In struct sport_dev, use a mutex instead of a semaphore
for the mutex.
Fixed.
11. Don't use C99 //-style comments in Linux kernel.
Just use /* ... */ on one line for short comments.
Long comment style is:
/*
* This is the Linux kernel long format style.
* Use it for multi-line comments.
*/
Fixed.
12. Are the ioctl interfaces in include/asm-blackfin/dpmc.h
documented somewhere?
See Documentation/ABI/README and Documentation/SubmitChecklist.
Removed for now. And we will submit it again later with the dpmc
driver and a document.
13. printk() calls usually should begin with a KERN_* level.
I looked through and fixed some necessary ones. Especially in the
header folder.
14. Don't comment obvious things, like this one:
+#define OFFSET_( x ) ((x) & 0x0000FFFF) /* define macro for offset */
Removed. At least some of them.. And we will review code later to
improve the comment quality.
15. Is this really needed?
+#define ZERO 0x0
Maybe not. I removed it.
16. Most of the source files have history and changelog comments in
them. Those should go into the SCM changelogs, not in the source
files.
17. #defines that are of the form
#define VALUE expression
should almost always have parentheses around (expression)
to prevent accidental misuse later on.
Added parentheses for some of the places in the code.
18. Looks like this diff:
diff -urN linux-2.6.18/init/Kconfig.orig linux-2.6.18.patch1/init/Kconfig.orig
should not have been included in the patch file at all.
Right. Removed.
That's it for my Kconfig/Makefile/header files comments.
I'll have C source code comments by this weekend.
---
There are also a bunch of corrected spelling typos and more detailed
comments in this file:
http://www.xenotime.net/linux/doc/blackfin-arch-meta.patch
(it's only 84 KB :)
---
~Randy
--
Best regards,
Luke Yang
[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
