Hi all,
I renewed and resend this patch.
This is the blackfin architecture for 2.6.18, again. As we promised,
we fixed some issues in our old patches as following.
- use serial core in that driver
- Fix up that ioctl so it a) doesn't sleep in spinlock and b) compiles
- Use generic IRQ framework
- Review all the volatiles, consolidate them in some helper-in-header-file.
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.
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
Signed-off-by: Luke Yang <[email protected]>
--
Best regards,
Luke Yang
[email protected]
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]
On Fri, 22 Sep 2006 17:48:03 +0800, Luke Yang wrote:
>> 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.
>
No this is bad design. Each time you want to access those memory map
register you should use read{bwl}/in{bwl} variants.
After all memory mapped register are not so different of mmio on standard
PC.
For example on au1x00 mips platform [1], some helper are defined to access
those memory map registers.
using volatile is awful : instead of a clean API to access those
registers, you often forgot that volatile is need which lead to very hard
to debug bugs (depend of compiler optimization, state of memory, ...).
For example on a embedded platform (ADI fusiv) where volatile are used
everywhere, some developers forgot some, which lead to random lockup on
boot.
Matthieu
[1]
include/asm-mips/mach-au1x00/au1000.h
static inline u32 au_readl(unsigned long reg)
{
return (*(volatile u32 *)reg);
}