On Tue, Dec 27, 2005 at 03:41:55PM -0800, Bryan O'Sullivan wrote:
> +/*
> + * MMIO copy routines. These are guaranteed to operate in units denoted
> + * by their names. This style of operation is required by some devices.
> + */
Using kdoc style for new code is nice.
> +extern void fastcall __memcpy_toio32(volatile void __iomem *to, const void *from, size_t count);
> +
Minor rant: extern is always redundant for function prototypes in C.
I'd prefer that we adopt a standard of not using extern for functions,
as it would make use of extern for variables (almost always
inappropriate, especially in C files) stick out more.
While people claim this has some documentation value ("I _meant_ for
this to be exported"), I think it actually has a net negative effect,
as quite a number of people actually think the "extern" keyword does
some unspecified magic here and ignore the namespace pollution of their
theoretically "un-externed" but not explicitly static functions.
There isn't any sort of consensus on this point as far as I know, so
this is just me venting.
> /* Create a virtual mapping cookie for an IO port range */
> extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
> extern void ioport_unmap(void __iomem *);
> diff -r 789a24638663 -r 7b7b442a4d63 lib/iomap.c
> --- a/lib/iomap.c Tue Dec 27 09:27:10 2005 +0800
> +++ b/lib/iomap.c Tue Dec 27 15:41:48 2005 -0800
> @@ -187,6 +187,22 @@
> EXPORT_SYMBOL(iowrite16_rep);
> EXPORT_SYMBOL(iowrite32_rep);
>
> +/*
> + * Copy data to an MMIO region. MMIO space accesses are performed
> + * in the sizes indicated in each function's name.
> + */
> +void fastcall __memcpy_toio32(volatile void __iomem *d, const void *s, size_t count)
> +{
> + volatile u32 __iomem *dst = d;
> + const u32 *src = s;
> +
> + while (--count >= 0) {
> + __raw_writel(*src++, dst++);
> +}
Suspicious use of volatile - writel is doing the actual write, this
function never does a dereference. As you've already got private
copies of the pointers already in s and d, it's perfectily reasonable
and idiomatic to do:
while (--count >= 0)
__raw_writel(*s++, d++);
I'd personally write this as:
while (count--)
__raw_writel(*s++, d++);
And as you appear to be using the __raw.. version to avoid repeated
mb()s, you probably ought to tack one on at the end.
--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
