Re: [patch 2/8] Immediate Value - Architecture Independent Code

[Christoph Hellwig]

On Fri, Jul 13, 2007 at 09:24:41PM -0400, Mathieu Desnoyers wrote:
> +#ifdef __KERNEL__

no need for this.  unless you add the header to header-y in the Kbuild
file it's not exported to userspace at all.

> +#ifdef CONFIG_IMMEDIATE
> +#include <asm/immediate.h>
> +#else
> +#include <asm-generic/immediate.h>
> +#endif

Nack on this one. linux/*.h should never include asm-generic headers.

Either put the !CONFIG_IMMEDIATE code directly into this file or let
every arch have a one-liner immediate.h that includes the asm-generic
one.  The first method is probably a lot nicer.

> +/*
> + * modules_mutex nests inside immediate_mutex. immediate_mutex protects builtin
> + * immediates and module immediates.
> + */
> +DEFINE_MUTEX(immediate_mutex);

Why is this non-static?

> +
> +/*
> + * Sets a range of immediates to a enabled state : set the enable bit.
> + */
> +static void _immediate_update_range(
> +	const struct __immediate *begin, const struct __immediate *end)

static void _immediate_update_range(const struct __immediate *begin,
		const struct __immediate *end)

same for a few more functions here.

> +#ifdef CONFIG_MODULES

...

> +#endif
> +
> +#ifdef CONFIG_MODULES

please put all this code into a single ifdef block.


note that the exported functions probably want some kerneldoc documentation.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/