Re: ADI Blackfin porting for kernel-2.6.13

[Jesper Juhl <jesper.juhl@gmail.com>]

On Friday 23 September 2005 07:37, Luke Yang wrote:
> Hi all,
> 
>    This is the ADI Blackfin patch for kernel 2.6.13.  I know this
> patch doesn' meet the kernel patch submission format, but this patch
> is only for reviewing, shows the changes we made.  And I'll send new
> patch for kernel 2.6.14 for you to merge into kernel.
> 
>    This patch mainly includes the arch/bfinnommu architecture files
> and some blackfin specific drivers.  The only change to the common
> files is that we change binfmt.c and related header file, added one
> flat binary format for blackfin. We are considering removing this
> change in next release.
> 
>   The patch is too big to put here , url is
> http://blackfin.uclinux.org/frs/download.php/570/bfinnonnu-linux-2.6.13.patch
> 

Ok, I went through the files in arch/bfinnommu/kernel/ and I have some
comments (I don't have the time to look at the rest of it).
I think you need to do some work before merging this.

I've attached a patch (compressed due to its size) that contain all the 
changes I refer to below. It's just one big patch with all the changes I 
made while I went through the files. It's not split out into logically 
seperate changes - sorry. I just made different types of changes along the 
way, and the attached patch is the result.


You need to clean this stuff up with respect to CodingStyle. Currently the 
files are quite inconsistent in the style(s) they use, and they don't follow
the kernels CodingStyle very closely either.

You have a lot of very deeply nested code (especially in dma.c) that is quite 
hard to follow and also violates the 80column rule. Try to reduce the nesting
depth.
I've made most of the code fit in 80 columns, but I've probably missed a few 
spots.

the *_interrupt functions in dma.c are just begging for a few helper functions
to share common code and reduce complexity. I've reordered them a bit to make 
them more readable, but there's lots more that could be done.

There's *lots* of trailing whitespace all over the place. Please get rid of 
that prior to merging.

There's *lots* of cases of mixing of spaces and tabs for indentation. Indent 
with tabs only please, and certainly don't have lines like 
"<space><tab><space><space><space><space><tab>code" 
like you have at the moment.
I've cleaned up all the trailing whitespace, and some of the other space+tab
braindamage, but there's probably still something left to do.

There are quite a lot of casts in the code, and some of them are completely 
pointless. There's no need to cast the return value of kmalloc(), there's no
need to cast assignments to/from void pointers, there's no need to cast 
assignments between types where normal C promotion rules obtain the same
result.
Such unnesesary casting is harmful. It defeats what limited typechecking C 
has, so the compiler can't help and warn when you do something bad, it also 
lessens the usefullnes of tools such as sparse.
I've cleaned up some of this, but not everything.

You seem to be quite fond of MixingCapsAndLowercaseChars in variable names.
Please don't unless you really have to. Stick to purely lowercase for variable
names as much as possible.
I've renamed a few vars, just as examples.

I stumbled across some unused variables as well that I removed, but there may 
be more.

I've cleaned up a lot of other CodingStyle issues as well. Check the attached 
patch and you'll see.

In dma.c :
  DMA_RESULT set_dma_transfer_size(unsigned int channel, char size)
  DMA_RESULT get_dma_transfer_size(unsigned int channel, unsigned short *size)
Seems inconsistent. Why "char size" for set_dma_transfer_size(), but
"unsigned short *size" for get_dma_transfer_size() ??
An if char is what you want to use for size in set_dma_transfer_size(), then 
shouldn't that be "unsigned char" ?


I think that's all, read the patch (or apply it and look at the results) to 
get all the details. Now I'll go get some sleep...


Ohh, before I leave, one more note:  I have no way at all to test this code, 
so it's quite possible I've made mistakes in the patch I'm attaching, so do 
review it carefully before applying bits from it.


-- 
Jesper Juhl <jesper.juhl@gmail.com>

Attachment: blackfin-arch_bfinnommu_kernel.patch.bz2
Description: BZip2 compressed data