* Re: [PATCH 1/3] [BFIN] Blackfin architecture patch for 2.6.18
[not found] ` <20060921232921.88d2aa5d.rdunlap@xenotime.net>
@ 2006-09-22 9:48 ` Luke Yang
2006-09-22 17:34 ` Matthieu CASTET
0 siblings, 1 reply; 3+ messages in thread
From: Luke Yang @ 2006-09-22 9:48 UTC (permalink / raw)
To: Randy. Dunlap; +Cc: linux-kernel, Andrew Morton, Greg KH
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 <rdunlap@xenotime.net> 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
luke.adi@gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread