From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: William Breathitt Gray <vilhelm.gray@gmail.com>,
linus.walleij@linaro.org, akpm@linux-foundation.org
Cc: linux-gpio@vger.kernel.org, linux-arch@vger.kernel.org,
linux-kernel@vger.kernel.org, andriy.shevchenko@linux.intel.com,
Arnd Bergmann <arnd@arndb.de>
Subject: Re: [RESEND PATCH v4 1/8] bitops: Introduce the for_each_set_clump macro
Date: Tue, 2 Oct 2018 09:42:48 +0200 [thread overview]
Message-ID: <40ecad49-2797-0d30-b52d-a2e6838dc1ab@rasmusvillemoes.dk> (raw)
Message-ID: <20181002074248.dgfx8ZgQ9OrSljKB3UH9PZwAEH4Qyp02r8i6YGyQV6w@z> (raw)
In-Reply-To: <f5250b9c78377e476667be1b616c86cf573b57dc.1538441919.git.vilhelm.gray@gmail.com>
On 2018-10-02 03:13, William Breathitt Gray wrote:
> This macro iterates for each group of bits (clump) with set bits, within
> a bitmap memory region. For each iteration, "clump" is set to the found
> clump index, "index" is set to the word index of the bitmap containing
> the found clump, and "offset" is set to the bit offset of the found
> clump within the respective bitmap word.
I can't say I'm a fan. It seems rather clumsy and ad-hoc - though I do
see how it matches the code you replace in drivers/gpio/. When I
initially read the cover letter, I assumed that one would get a sequence
of 4-bit values, but one has to dig the actual value out of the bitmap
afterwards using all of index, offset and a mask computed from clump_size.
> +
> +/**
> + * find_next_clump - find next clump with set bits in a memory region
> + * @index: location to store bitmap word index of found clump
> + * @offset: bits offset of the found clump within the respective bitmap word
> + * @bits: address to base the search on
> + * @size: bitmap size in number of clumps
That's a rather inconvenient unit, no? And rather easy to get wrong, I
can easily see people passing nbits instead.
I think you could reduce the number of arguments to this helper and the
macro, while getting rid of some confusion: Drop index and offset, let
clump_index be start_index and measured in bit positions (like
find_next_bit et al), and let the return value also be a bit position.
And instead of index and offset, have another unsigned long* output
parameter that gives the actual value at [return value:return
value+clump_size]. IOW, I think the prototype should be close to
find_next_bit, except that in case of "clumps", there's an extra piece
of information to return.
> + * @clump_index: clump index at which to start searching
> + * @clump_size: clump size in bits
> + *
> + * Returns the clump index for the next clump with set bits; the respective
> + * bitmap word index is stored at the location pointed by @index, and the bits
> + * offset of the found clump within the respective bitmap word is stored at the
> + * location pointed by @offset. If no bits are set, returns @size.
> + */
> +size_t find_next_clump(size_t *const index, unsigned int *const offset,
> + const unsigned long *const bits, const size_t size,
> + const size_t clump_index, const unsigned int clump_size)
> +{
> + size_t i;
> + unsigned int bits_offset;
> + unsigned long word_mask;
> + const unsigned long clump_mask = GENMASK(clump_size - 1, 0);
> +
> + for (i = clump_index; i < size; i++) {
> + bits_offset = i * clump_size;
> +
> + *index = BIT_WORD(bits_offset);
> + *offset = bits_offset % BITS_PER_LONG;
> +
> + word_mask = bits[*index] & (clump_mask << *offset);
> + if (!word_mask)
> + continue;
The cover letter says
The clump_size argument can be an arbitrary number of bits and is not
required to be a multiple of 2.
by which I assume you mean "power of 2", but either way, the above code
does not seem to take into account the case where bits_offset +
clump_size straddles a word boundary, so it wouldn't work for a
clump_size that does not divide BITS_PER_LONG.
May I suggest another approach:
unsigned long bitmap_get_value(const unsigned long *bitmap, unsigned
start, unsigned width): Get the value of bitmap[start:start+width] for
1<=width<=BITS_PER_LONG (it's up to the caller to ensure this is within
the defined region). That can almost be an inline
bitmap_get_value(const unsigned long *bitmap, unsigned start, unsigned
width)
{
unsigned idx = BIT_WORD(start);
unsigned offset = start % BITS_PER_LONG;
unsigned long lower = bitmap[idx] >> offset;
unsigned long upper = offset <= BITS_PER_LONG - width ? 0 :
bitmap[idx+1] << (BITS_PER_LONG - offset);
return (lower | upper) & GENMASK(width-1, 0)
}
Then you can implement the for_each_set_clump by a (IMO) more readable
for (i = 0, start = 0; i < num_ports; i++, start += gpio_reg_size) {
word_mask = bitmap_get_value(mask, start, gpio_reg_size);
if (!word_mask)
continue;
...
}
Or, if you do want find_next_clump/for_each_set_clump, that can be
implemented in terms of this.
Rasmus
next prev parent reply other threads:[~2018-10-02 14:24 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-02 1:12 [RESEND PATCH v4 0/8] Introduce the for_each_set_clump macro William Breathitt Gray
2018-10-02 1:12 ` William Breathitt Gray
2018-10-02 1:13 ` [RESEND PATCH v4 1/8] bitops: " William Breathitt Gray
2018-10-02 1:13 ` William Breathitt Gray
2018-10-02 7:42 ` Rasmus Villemoes [this message]
2018-10-02 7:42 ` Rasmus Villemoes
2018-10-02 8:21 ` Andy Shevchenko
2018-10-02 8:21 ` Andy Shevchenko
2018-10-03 11:48 ` Andy Shevchenko
2018-10-03 11:48 ` Andy Shevchenko
2018-10-04 10:36 ` William Breathitt Gray
2018-10-04 10:36 ` William Breathitt Gray
2018-10-04 12:10 ` Andy Shevchenko
2018-10-04 12:10 ` Andy Shevchenko
2018-10-04 10:30 ` William Breathitt Gray
2018-10-04 10:30 ` William Breathitt Gray
2018-10-04 10:03 ` William Breathitt Gray
2018-10-04 10:03 ` William Breathitt Gray
2018-10-02 1:14 ` [RESEND PATCH v4 2/8] lib/test_bitmap.c: Add for_each_set_clump test cases William Breathitt Gray
2018-10-02 1:14 ` William Breathitt Gray
2018-10-02 1:14 ` [RESEND PATCH v4 3/8] gpio: 104-dio-48e: Utilize for_each_set_clump macro William Breathitt Gray
2018-10-02 1:14 ` William Breathitt Gray
2018-10-02 7:00 ` Rasmus Villemoes
2018-10-02 7:00 ` Rasmus Villemoes
2018-10-14 4:19 ` William Breathitt Gray
2018-10-14 4:19 ` William Breathitt Gray
2018-10-15 11:59 ` Rasmus Villemoes
2018-10-15 11:59 ` Rasmus Villemoes
2018-10-17 1:54 ` William Breathitt Gray
2018-10-17 1:54 ` William Breathitt Gray
2018-10-02 1:15 ` [RESEND PATCH v4 4/8] gpio: 104-idi-48: " William Breathitt Gray
2018-10-02 1:15 ` William Breathitt Gray
2018-10-02 1:15 ` [RESEND PATCH v4 5/8] gpio: gpio-mm: " William Breathitt Gray
2018-10-02 1:15 ` William Breathitt Gray
2018-10-02 1:15 ` [RESEND PATCH v4 6/8] gpio: ws16c48: " William Breathitt Gray
2018-10-02 1:15 ` William Breathitt Gray
2018-10-02 1:16 ` [RESEND PATCH v4 7/8] gpio: pci-idio-16: " William Breathitt Gray
2018-10-02 1:16 ` William Breathitt Gray
2018-10-02 1:16 ` [RESEND PATCH v4 8/8] gpio: pcie-idio-24: " William Breathitt Gray
2018-10-02 1:16 ` William Breathitt Gray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40ecad49-2797-0d30-b52d-a2e6838dc1ab@rasmusvillemoes.dk \
--to=linux@rasmusvillemoes.dk \
--cc=akpm@linux-foundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=linus.walleij@linaro.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=vilhelm.gray@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).