linux-mips
[Top] [All Lists]

Re: [PATCH] I/O helpers rework

To: macro@mips.com
Subject: Re: [PATCH] I/O helpers rework
From: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Date: Fri, 07 Jan 2005 00:45:21 +0900 (JST)
Cc: ralf@linux-mips.org, linux-mips@linux-mips.org, macro@linux-mips.org
In-reply-to: <Pine.LNX.4.61.0412151936460.14855@perivale.mips.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <Pine.LNX.4.61.0412151936460.14855@perivale.mips.com>
Sender: linux-mips-bounce@linux-mips.org
>>>>> On Wed, 15 Dec 2004 21:13:37 +0000 (GMT), "Maciej W. Rozycki" 
>>>>> <macro@mips.com> said:

macro>  As the whole file seemed a bit messy to me I decided to
macro> rewrite these functions/macros completely.  To ease long-term
macro> maintenance I created common templates for all classes of
macro> accesses which expand to appropriate code for different
macro> transfer unit width.  I made all operations to be expressed as
macro> inline functions to catch dangerous/incorrect uses.  The result
macro> are the following function classes:

Thanks for your good job.  I have a few comments/requests.

1. How about adding 'volatile' and '__iomem' to *read*()/*write*() ?
   While some archs use 'volatile void __iomem *' and some are not, I
   think *read*()/*write*()/ioremap()/iounmap() should use same type.
   This will remove some compiler warnings.

2. How about using 'const void *' for outs*()?  This will remove some
   compiler warnings too.

3. In *in*()/*out*(), it would be better to call __swizzle_addr*()
   AFTER adding mips_io_port_base.  This unifies the meaning of the
   argument of __swizzle_addr*() (always virtual address).  Then,
   mach-specific __swizzle_addr*() can to every evil thing based on
   the argument.

4. How about enclosing all *ioswab*() by '#ifndef' ?  Also how about
   passing virtual address to *ioswab*() ?  I mean something like:

# ifndef ioswabw
#  define ioswabw(a,x)          le16_to_cpu(x)
# endif
# ifndef __raw_ioswabw
#  define __raw_ioswabw(a,x)    (x)
# endif
...
        __val = pfx##ioswab##bwlq(__mem, val);                          \

  Then we can provide mach-specific *ioswab*() in mach-*/mangle-port.h
  and can do every evil thing based on its argument.  It is usefull on
  machines which have regions with defirrent endian conversion scheme.
  Also, this can clean up CONFIG_SGI_IP22 from io.h
  (mach-ip22/mangle-port.h can provide its own *ioswabw*()).


---
Atsushi Nemoto

<Prev in Thread] Current Thread [Next in Thread>