Re: [PATCH 1/3] [BFIN] Blackfin architecture patch for 2.6.18

[Luke Yang]

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@xxxxxxxxxxxx> 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@xxxxxxxxx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/