Re: [Uclinux-dist-devel] [PATCH v3] add the driver for Analog Devices Blackfin on-chip CAN controllers

Hennerich, Michael wrote:

quoted

Wolfgang Grandegger wrote:

quoted

Mike Frysinger wrote:

quoted

On Thu, Dec 10, 2009 at 04:11, Wolfgang Grandegger wrote:

quoted

Barry Song wrote:

quoted

+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/types.h>

I think you don't need "types.h" as the code no longer uses

"uint*_t".

quoted

quoted

linux/types.h declares all types, like u* which this driver still

uses

quoted

I just remember that "linux/types.h" needs to be added for the

uint*_t

quoted

quoted

types. At a first glance I do not see __u8/u8 being defined in that
header file but I might have missed something.

quoted

quoted

Well, I'm still not a friend of the following inline functions,
especially the *one-liners* which are called just *once*. With the

usage

quoted

quoted

quoted

of structs they seem even more useless.

seems like it would make more sense to not even use the read/write
functions either.  just declare the regs as volatile and assign/read
the struct directly.

Two times no. Don't use volatile and proper accessor functions. See:

http://lxr.linux.no/#linux+v2.6.32/Documentation/volatile-considered-

harmful.txt

I was just wondering if bfin_read/write16 would not be the proper
accessor functions. readw/writew seems to be implemented differently:

http://lxr.linux.no/#linux+v2.6.32/arch/blackfin/include/asm/io.h#L44

Puh, they do an cli,nop,nop,sync..sti for the access. This also nicely
shows why accessor functions should be used to access device registers.

Well, just curious. I don't really know the blackfin arch.

Well - on Blackfin its absolutely ok to access System Memory Mapped
Registers using structs.
At any rate volatile is then required to prevent the compiler to
optimize accesses away.
IMHO this is a pretty legal use of volatile, and used in hundreds of
places all over the kernel. 
 
When accessing external controllers accessor functions from io.h must be
used.
There are two things to consider here:
1) weak ordering of reads and writes 
2) killed and reissued reads (especially harmful when reading from
FIFOs)

OK, anyway, I believe that it's good practice to hide all such details
by using proper accessor functions.

Wolfgang.