On Friday 18 July 2014 22:59:53 Sam Ravnborg wrote:

On Wed, Jul 16, 2014 at 01:01:22PM +0200, Thierry Reding wrote:

quoted

From: Thierry Reding <redacted>

Currently driver writers need to use io{read,write}{8,16,32}_rep() when
accessing FIFO registers portably. This is bad for two reasons: it is
inconsistent with how other registers are accessed using the standard
{read,write}{b,w,l}() functions, which can lead to confusion. On some
architectures the io{read,write}*() functions also need to perform some
extra checks to determine whether an address is memory-mapped or refers
to I/O space. Drivers which can be expected to never use I/O can safely
use the {read,write}s{b,w,l,q}(), just like they use their non-string
variants and there's no need for these extra checks.

This patch implements generic versions of readsb(), readsw(), readsl(),
readsq(), writesb(), writesw(), writesl() and writesq(). Variants of
these string functions for I/O accesses (ins*() and outs*() as well as
ioread*_rep() and iowrite*_rep()) are now implemented in terms of the
new functions.

Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently
by drivers for devices that will only ever be memory-mapped and hence
don't need to access I/O space, whereas io{read,write}{8,16,32}_rep()
should be used by drivers for devices that can be either memory-mapped
or I/O-mapped.

While at it, also make sure that any of the functions provided as
fallback for architectures that don't override them can't be overridden
subsequently.

This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile
tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag,
OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't
find or build a cross-compiler that would run on my system. But by code
inspection they shouldn't break with this patch.

Signed-off-by: Thierry Reding <redacted>

Hi Thierry.

While browsing the resulting asm-generic/io.h it is irritating
that the functions are messed up like they are.

From the start of the file the IO accessors are defined in following order:
__raw_readb
__raw_readw
__raw_readl

readb
readw
readl

__raw_writeb
__raw_writew
__raw_writel

writeb
writew
writel

__raw_readq

readq

__raw_writeq

writeq


See how strange it looks?

It saves one #ifdef CONFIG_64BIT to do it like this, but I guess
you are right that reordering them slightly would be nice here.

A semi related question.
Why do we play all these macro tricks in io.h?

Example:

    #define writeb __raw_writeb

The consensus these days is to use static inline to have the
correct prototype but this file is contains a lot of these
macro conversions.

All these things are not introduced by your patch but now that
you show some love and care for this file maybe we could take
the next step and bring more order to the current semi chaos?

The macros are mainly there so we can test their presence with
#ifdef. The interface is complex enough that there is probably
an architecture for any combination of overrides: most should
override the __raw_*() functions with inline assembly, but a lot
don't do that and it works because of implementation details of
the compiler. Some may need to override either readl() or
readl_relaxed() but not the other one.

For this reason, we want architecture-level files that include
the asm-generic version to use macros (or macro + inline function)
rather than a plain inline.

I was arguing earlier that we don't need the extra macros in the
asm-generic version, but it also doesn't hurt and it can make
it slightly easier for new architectures to copy the bits from
asm-generic they want to override.

	Arnd