Re: [patch 03/36] Hexagon: Add bitops support

[Arnd Bergmann]

On Wednesday 17 August 2011 11:35:00 Richard Kuo wrote:

> +
> +#ifdef CONFIG_HEXAGON_BITOPS
> +#include <asm/bitops/ffz.h>
> +#include <asm/bitops/fls.h>
> +#include <asm/bitops/ffs.h>
> +#include <asm/bitops/__ffs.h>
> +#include <asm/bitops/__fls.h>
> +#else
> +#include <asm-generic/bitops/ffz.h>
> +#include <asm-generic/bitops/fls.h>
> +#include <asm-generic/bitops/ffs.h>
> +#include <asm-generic/bitops/__ffs.h>
> +#define __fls(x) (fls(x) - 1)
> +#endif

Is there a particular reason why this is configurable? Also, I see no reaons
to split these up into lots of files. Just put all of them into one file.

> +/*
> + * __ffs - find first bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + *
> + * bits_per_long assumed to be 32
> + * numbering starts at 0 I think (instead of 1 like ffs)
> + */
> +static inline unsigned long __ffs(unsigned long word)
> +{
> +	int num;
> +
> +	asm volatile("%0 = ct0(%1);\n"
> +		: "=&r" (num)
> +		: "r" (word));
> +
> +	return num;
> +}

I think this should not be marked volatile: If the result is unused,
there is no need to compute the value either. Same for __fls, ffs,
ffz, and fls.

> +/*
> + * ffs - find first bit set
> + * @x: the word to search
> + *
> + * This is defined the same way as
> + * the libc and compiler builtin ffs routines, therefore
> + * differs in spirit from the above ffz (man ffs).
> + */
> +static inline int ffs(int x)
> +{

The type is normally 'long', not 'int'. They are obviously the
same length on 32 bit machines, but you might get bogus compiler
warnings from this. Same for ffz and fls.

	Arnd
--
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/