LinuxLists.cc - [PATCH v17 00/14] Introduce the for_each_set

[permalink] [raw]

Subject: [PATCH v17 05/14] gpio: gpio-mm: Utilize for_each_set_clump8 macro

Replace verbose implementation in get_multiple/set_multiple callbacks
with for_each_set_clump8 macro to simplify code and improve clarity.

Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/gpio/gpio-gpio-mm.c | 73 +++++++++++--------------------------
1 file changed, 21 insertions(+), 52 deletions(-)

diff --git a/drivers/gpio/gpio-gpio-mm.c b/drivers/gpio/gpio-gpio-mm.c
index 78a1db24e931..72196ea36358 100644
--- a/drivers/gpio/gpio-gpio-mm.c
+++ b/drivers/gpio/gpio-gpio-mm.c
@@ -164,46 +164,25 @@ static int gpiomm_gpio_get(struct gpio_chip *chip, unsigned int offset)
return !!(port_state & mask);
}

+static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
+
static int gpiomm_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
unsigned long *bits)
{
struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
- size_t i;
- static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
- const unsigned int gpio_reg_size = 8;
- unsigned int bits_offset;
- size_t word_index;
- unsigned int word_offset;
- unsigned long word_mask;
- const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+ unsigned long offset;
+ unsigned long gpio_mask;
+ unsigned int port_addr;
unsigned long port_state;

/* clear bits array to a clean slate */
bitmap_zero(bits, chip->ngpio);

- /* get bits are evaluated a gpio port register at a time */
- for (i = 0; i < ARRAY_SIZE(ports); i++) {
- /* gpio offset in bits array */
- bits_offset = i * gpio_reg_size;
-
- /* word index for bits array */
- word_index = BIT_WORD(bits_offset);
-
- /* gpio offset within current word of bits array */
- word_offset = bits_offset % BITS_PER_LONG;
-
- /* mask of get bits for current gpio within current word */
- word_mask = mask[word_index] & (port_mask << word_offset);
- if (!word_mask) {
- /* no get bits in this port so skip to next one */
- continue;
- }
-
- /* read bits from current gpio port */
- port_state = inb(gpiommgpio->base + ports[i]);
+ for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+ port_addr = gpiommgpio->base + ports[offset / 8];
+ port_state = inb(port_addr) & gpio_mask;

- /* store acquired bits at respective bits array offset */
- bits[word_index] |= (port_state << word_offset) & word_mask;
+ bitmap_set_value8(bits, port_state, offset);
}

return 0;
@@ -234,37 +213,27 @@ static void gpiomm_gpio_set_multiple(struct gpio_chip *chip,
unsigned long *mask, unsigned long *bits)
{
struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
- unsigned int i;
- const unsigned int gpio_reg_size = 8;
- unsigned int port;
- unsigned int out_port;
- unsigned int bitmask;
+ unsigned long offset;
+ unsigned long gpio_mask;
+ size_t index;
+ unsigned int port_addr;
+ unsigned long bitmask;
unsigned long flags;

- /* set bits are evaluated a gpio register size at a time */
- for (i = 0; i < chip->ngpio; i += gpio_reg_size) {
- /* no more set bits in this mask word; skip to the next word */
- if (!mask[BIT_WORD(i)]) {
- i = (BIT_WORD(i) + 1) * BITS_PER_LONG - gpio_reg_size;
- continue;
- }
+ for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+ index = offset / 8;
+ port_addr = gpiommgpio->base + ports[index];

- port = i / gpio_reg_size;
- out_port = (port > 2) ? port + 1 : port;
- bitmask = mask[BIT_WORD(i)] & bits[BIT_WORD(i)];
+ bitmask = bitmap_get_value8(bits, offset) & gpio_mask;

spin_lock_irqsave(&gpiommgpio->lock, flags);

/* update output state data and set device gpio register */
- gpiommgpio->out_state[port] &= ~mask[BIT_WORD(i)];
- gpiommgpio->out_state[port] |= bitmask;
- outb(gpiommgpio->out_state[port], gpiommgpio->base + out_port);
+ gpiommgpio->out_state[index] &= ~gpio_mask;
+ gpiommgpio->out_state[index] |= bitmask;
+ outb(gpiommgpio->out_state[index], port_addr);

spin_unlock_irqrestore(&gpiommgpio->lock, flags);
-
- /* prepare for next gpio register set */
- mask[BIT_WORD(i)] >>= gpio_reg_size;
- bits[BIT_WORD(i)] >>= gpio_reg_size;
}
}

--
2.23.0

2019-10-09 15:31:21

[permalink] [raw]

Subject: [PATCH v17 11/14] thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro

Utilize for_each_set_clump8 macro, and the bitmap_set_value8 and
bitmap_get_value8 functions, where appropriate. In addition, remove the
now unnecessary temp_mask and temp_shift members of the
intel_soc_dts_sensor_entry structure.

Suggested-by: Andy Shevchenko <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/thermal/intel/intel_soc_dts_iosf.c | 31 +++++++++++++---------
drivers/thermal/intel/intel_soc_dts_iosf.h | 2 --
2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
index 5716b62e0f73..f75271b669c6 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.c
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
@@ -6,6 +6,7 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/bitops.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
@@ -103,6 +104,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
int status;
u32 temp_out;
u32 out;
+ unsigned long update_ptps;
u32 store_ptps;
u32 store_ptmc;
u32 store_te_out;
@@ -120,8 +122,10 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
if (status)
return status;

- out = (store_ptps & ~(0xFF << (thres_index * 8)));
- out |= (temp_out & 0xFF) << (thres_index * 8);
+ update_ptps = store_ptps;
+ bitmap_set_value8(&update_ptps, temp_out & 0xFF, thres_index * 8);
+ out = update_ptps;
+
status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
SOC_DTS_OFFSET_PTPS, out);
if (status)
@@ -223,6 +227,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
u32 out;
struct intel_soc_dts_sensor_entry *dts;
struct intel_soc_dts_sensors *sensors;
+ unsigned long raw;

dts = tzd->devdata;
sensors = dts->sensors;
@@ -231,8 +236,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
if (status)
return status;

- out = (out & dts->temp_mask) >> dts->temp_shift;
- out -= SOC_DTS_TJMAX_ENCODING;
+ raw = out;
+ out = bitmap_get_value8(&raw, dts->id * 8) - SOC_DTS_TJMAX_ENCODING;
*temp = sensors->tj_max - out * 1000;

return 0;
@@ -280,11 +285,14 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
int read_only_trip_cnt)
{
char name[10];
+ unsigned long trip;
int trip_count = 0;
int trip_mask = 0;
+ int writable_trip_cnt = 0;
+ unsigned long ptps;
u32 store_ptps;
+ unsigned long i;
int ret;
- int i;

/* Store status to restor on exit */
ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
@@ -293,11 +301,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
goto err_ret;

dts->id = id;
- dts->temp_mask = 0x00FF << (id * 8);
- dts->temp_shift = id * 8;
if (notification_support) {
trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
- trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
+ writable_trip_cnt = trip_count - read_only_trip_cnt;
+ trip_mask = GENMASK(writable_trip_cnt - 1, 0);
}

/* Check if the writable trip we provide is not used by BIOS */
@@ -306,11 +313,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
if (ret)
trip_mask = 0;
else {
- for (i = 0; i < trip_count; ++i) {
- if (trip_mask & BIT(i))
- if (store_ptps & (0xff << (i * 8)))
- trip_mask &= ~BIT(i);
- }
+ ptps = store_ptps;
+ for_each_set_clump8(i, trip, &ptps, writable_trip_cnt * 8)
+ trip_mask &= ~BIT(i / 8);
}
dts->trip_mask = trip_mask;
dts->trip_count = trip_count;
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
index adfb09af33fc..c54945748200 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.h
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
@@ -24,8 +24,6 @@ struct intel_soc_dts_sensors;

struct intel_soc_dts_sensor_entry {
int id;
- u32 temp_mask;
- u32 temp_shift;
u32 store_status;
u32 trip_mask;
u32 trip_count;
--
2.23.0

2019-10-09 15:32:18

[permalink] [raw]

Subject: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

This macro iterates for each 8-bit group of bits (clump) with set bits,
within a bitmap memory region. For each iteration, "start" is set to the
bit offset of the found clump, while the respective clump value is
stored to the location pointed by "clump". Additionally, the
bitmap_get_value8 and bitmap_set_value8 functions are introduced to
respectively get and set an 8-bit value in a bitmap memory region.

Suggested-by: Andy Shevchenko <[email protected]>
Suggested-by: Rasmus Villemoes <[email protected]>
Suggested-by: Lukas Wunner <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
include/asm-generic/bitops/find.h | 17 +++++++++++++++
include/linux/bitmap.h | 35 +++++++++++++++++++++++++++++++
include/linux/bitops.h | 5 +++++
lib/find_bit.c | 14 +++++++++++++
4 files changed, 71 insertions(+)

diff --git a/include/asm-generic/bitops/find.h b/include/asm-generic/bitops/find.h
index 8a1ee10014de..9fdf21302fdf 100644
--- a/include/asm-generic/bitops/find.h
+++ b/include/asm-generic/bitops/find.h
@@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,

#endif /* CONFIG_GENERIC_FIND_FIRST_BIT */

+/**
+ * find_next_clump8 - find next 8-bit clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump8(unsigned long *clump,
+ const unsigned long *addr,
+ unsigned long size, unsigned long offset);
+
+#define find_first_clump8(clump, bits, size) \
+ find_next_clump8((clump), (bits), (size), 0)
+
#endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 90528f12bdfa..761fab5b60a7 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -66,6 +66,8 @@
* bitmap_allocate_region(bitmap, pos, order) Allocate specified bit region
* bitmap_from_arr32(dst, buf, nbits) Copy nbits from u32[] buf to dst
* bitmap_to_arr32(buf, src, nbits) Copy nbits from buf to u32[] dst
+ * bitmap_get_value8(map, start) Get 8bit value from map at start
+ * bitmap_set_value8(map, value, start) Set 8bit value to map at start
*
* Note, bitmap_zero() and bitmap_fill() operate over the region of
* unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
dst[1] = mask >> 32;
}

+/**
+ * bitmap_get_value8 - get an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ *
+ * Returns the 8-bit value located at the @start bit offset within the @src
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value8(const unsigned long *map,
+ unsigned long start)
+{
+ const size_t index = BIT_WORD(start);
+ const unsigned long offset = start % BITS_PER_LONG;
+
+ return (map[index] >> offset) & 0xFF;
+}
+
+/**
+ * bitmap_set_value8 - set an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ */
+static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
+ unsigned long start)
+{
+ const size_t index = BIT_WORD(start);
+ const unsigned long offset = start % BITS_PER_LONG;
+
+ map[index] &= ~(0xFF << offset);
+ map[index] |= value << offset;
+}
+
#endif /* __ASSEMBLY__ */

#endif /* __LINUX_BITMAP_H */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index cf074bce3eb3..fb94a10f7853 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
(bit) < (size); \
(bit) = find_next_zero_bit((addr), (size), (bit) + 1))

+#define for_each_set_clump8(start, clump, bits, size) \
+ for ((start) = find_first_clump8(&(clump), (bits), (size)); \
+ (start) < (size); \
+ (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
+
static inline int get_bitmask_order(unsigned int count)
{
int order;
diff --git a/lib/find_bit.c b/lib/find_bit.c
index 5c51eb45178a..e35a76b291e6 100644
--- a/lib/find_bit.c
+++ b/lib/find_bit.c
@@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
#endif

#endif /* __BIG_ENDIAN */
+
+unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
+ unsigned long size, unsigned long offset)
+{
+ offset = find_next_bit(addr, size, offset);
+ if (offset == size)
+ return size;
+
+ offset = round_down(offset, 8);
+ *clump = bitmap_get_value8(addr, offset);
+
+ return offset;
+}
+EXPORT_SYMBOL(find_next_clump8);
--
2.23.0

2019-10-09 16:29:33

by Masahiro Yamada

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
<[email protected]> wrote:
>
> This macro iterates for each 8-bit group of bits (clump) with set bits,
> within a bitmap memory region. For each iteration, "start" is set to the
> bit offset of the found clump, while the respective clump value is
> stored to the location pointed by "clump". Additionally, the
> bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> respectively get and set an 8-bit value in a bitmap memory region.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Rasmus Villemoes <[email protected]>
> Suggested-by: Lukas Wunner <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Signed-off-by: William Breathitt Gray <[email protected]>
> ---
> include/asm-generic/bitops/find.h | 17 +++++++++++++++
> include/linux/bitmap.h | 35 +++++++++++++++++++++++++++++++
> include/linux/bitops.h | 5 +++++
> lib/find_bit.c | 14 +++++++++++++
> 4 files changed, 71 insertions(+)
>
> diff --git a/include/asm-generic/bitops/find.h b/include/asm-generic/bitops/find.h
> index 8a1ee10014de..9fdf21302fdf 100644
> --- a/include/asm-generic/bitops/find.h
> +++ b/include/asm-generic/bitops/find.h
> @@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
>
> #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
>
> +/**
> + * find_next_clump8 - find next 8-bit clump with set bits in a memory region
> + * @clump: location to store copy of found clump
> + * @addr: address to base the search on
> + * @size: bitmap size in number of bits
> + * @offset: bit offset at which to start searching
> + *
> + * Returns the bit offset for the next set clump; the found clump value is
> + * copied to the location pointed by @clump. If no bits are set, returns @size.
> + */
> +extern unsigned long find_next_clump8(unsigned long *clump,
> + const unsigned long *addr,
> + unsigned long size, unsigned long offset);
> +
> +#define find_first_clump8(clump, bits, size) \
> + find_next_clump8((clump), (bits), (size), 0)
> +
> #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 90528f12bdfa..761fab5b60a7 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -66,6 +66,8 @@
> * bitmap_allocate_region(bitmap, pos, order) Allocate specified bit region
> * bitmap_from_arr32(dst, buf, nbits) Copy nbits from u32[] buf to dst
> * bitmap_to_arr32(buf, src, nbits) Copy nbits from buf to u32[] dst
> + * bitmap_get_value8(map, start) Get 8bit value from map at start
> + * bitmap_set_value8(map, value, start) Set 8bit value to map at start
> *
> * Note, bitmap_zero() and bitmap_fill() operate over the region of
> * unsigned longs, that is, bits behind bitmap till the unsigned long
> @@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
> dst[1] = mask >> 32;
> }
>
> +/**
> + * bitmap_get_value8 - get an 8-bit value within a memory region
> + * @map: address to the bitmap memory region
> + * @start: bit offset of the 8-bit value; must be a multiple of 8
> + *
> + * Returns the 8-bit value located at the @start bit offset within the @src
> + * memory region.
> + */
> +static inline unsigned long bitmap_get_value8(const unsigned long *map,
> + unsigned long start)

Why is the return type "unsigned long" where you know
it return the 8-bit value ?

u8?

> +{
> + const size_t index = BIT_WORD(start);
> + const unsigned long offset = start % BITS_PER_LONG;
> +
> + return (map[index] >> offset) & 0xFF;
> +}
> +
> +/**
> + * bitmap_set_value8 - set an 8-bit value within a memory region
> + * @map: address to the bitmap memory region
> + * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
> + * @start: bit offset of the 8-bit value; must be a multiple of 8
> + */
> +static inline void bitmap_set_value8(unsigned long *map, unsigned long value,

Same here, "u8 value"

> + unsigned long start)
> +{
> + const size_t index = BIT_WORD(start);
> + const unsigned long offset = start % BITS_PER_LONG;
> +
> + map[index] &= ~(0xFF << offset);
> + map[index] |= value << offset;
> +}
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* __LINUX_BITMAP_H */
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index cf074bce3eb3..fb94a10f7853 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
> (bit) < (size); \
> (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
>
> +#define for_each_set_clump8(start, clump, bits, size) \
> + for ((start) = find_first_clump8(&(clump), (bits), (size)); \
> + (start) < (size); \
> + (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
> +
> static inline int get_bitmask_order(unsigned int count)
> {
> int order;
> diff --git a/lib/find_bit.c b/lib/find_bit.c
> index 5c51eb45178a..e35a76b291e6 100644
> --- a/lib/find_bit.c
> +++ b/lib/find_bit.c
> @@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
> #endif
>
> #endif /* __BIG_ENDIAN */
> +
> +unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,

Ditto. "u8 *clump"

> + unsigned long size, unsigned long offset)
> +{
> + offset = find_next_bit(addr, size, offset);
> + if (offset == size)
> + return size;
> +
> + offset = round_down(offset, 8);
> + *clump = bitmap_get_value8(addr, offset);
> +
> + return offset;
> +}
> +EXPORT_SYMBOL(find_next_clump8);
> --
> 2.23.0
>

--
Best Regards

Masahiro Yamada

2019-10-09 17:04:28

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> <[email protected]> wrote:
> >
> > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > within a bitmap memory region. For each iteration, "start" is set to the
> > bit offset of the found clump, while the respective clump value is
> > stored to the location pointed by "clump". Additionally, the
> > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > respectively get and set an 8-bit value in a bitmap memory region.
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Suggested-by: Rasmus Villemoes <[email protected]>
> > Suggested-by: Lukas Wunner <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Linus Walleij <[email protected]>
> > Signed-off-by: William Breathitt Gray <[email protected]>
> > ---
> > include/asm-generic/bitops/find.h | 17 +++++++++++++++
> > include/linux/bitmap.h | 35 +++++++++++++++++++++++++++++++
> > include/linux/bitops.h | 5 +++++
> > lib/find_bit.c | 14 +++++++++++++
> > 4 files changed, 71 insertions(+)
> >
> > diff --git a/include/asm-generic/bitops/find.h b/include/asm-generic/bitops/find.h
> > index 8a1ee10014de..9fdf21302fdf 100644
> > --- a/include/asm-generic/bitops/find.h
> > +++ b/include/asm-generic/bitops/find.h
> > @@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
> >
> > #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
> >
> > +/**
> > + * find_next_clump8 - find next 8-bit clump with set bits in a memory region
> > + * @clump: location to store copy of found clump
> > + * @addr: address to base the search on
> > + * @size: bitmap size in number of bits
> > + * @offset: bit offset at which to start searching
> > + *
> > + * Returns the bit offset for the next set clump; the found clump value is
> > + * copied to the location pointed by @clump. If no bits are set, returns @size.
> > + */
> > +extern unsigned long find_next_clump8(unsigned long *clump,
> > + const unsigned long *addr,
> > + unsigned long size, unsigned long offset);
> > +
> > +#define find_first_clump8(clump, bits, size) \
> > + find_next_clump8((clump), (bits), (size), 0)
> > +
> > #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 90528f12bdfa..761fab5b60a7 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -66,6 +66,8 @@
> > * bitmap_allocate_region(bitmap, pos, order) Allocate specified bit region
> > * bitmap_from_arr32(dst, buf, nbits) Copy nbits from u32[] buf to dst
> > * bitmap_to_arr32(buf, src, nbits) Copy nbits from buf to u32[] dst
> > + * bitmap_get_value8(map, start) Get 8bit value from map at start
> > + * bitmap_set_value8(map, value, start) Set 8bit value to map at start
> > *
> > * Note, bitmap_zero() and bitmap_fill() operate over the region of
> > * unsigned longs, that is, bits behind bitmap till the unsigned long
> > @@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
> > dst[1] = mask >> 32;
> > }
> >
> > +/**
> > + * bitmap_get_value8 - get an 8-bit value within a memory region
> > + * @map: address to the bitmap memory region
> > + * @start: bit offset of the 8-bit value; must be a multiple of 8
> > + *
> > + * Returns the 8-bit value located at the @start bit offset within the @src
> > + * memory region.
> > + */
> > +static inline unsigned long bitmap_get_value8(const unsigned long *map,
> > + unsigned long start)
>
> Why is the return type "unsigned long" where you know
> it return the 8-bit value ?
>
> u8?

The primary reason is to be consistent with the datatype of the bitmap:
https://lkml.org/lkml/2019/1/12/26

This should also make it easier to extent to other sizes in the future
since we won't have to change the interface in order to support 16-bit
or 32-bit values -- they should easily fit within an unsigned long.

William Breathitt Gray

>
>
>
> > +{
> > + const size_t index = BIT_WORD(start);
> > + const unsigned long offset = start % BITS_PER_LONG;
> > +
> > + return (map[index] >> offset) & 0xFF;
> > +}
> > +
> > +/**
> > + * bitmap_set_value8 - set an 8-bit value within a memory region
> > + * @map: address to the bitmap memory region
> > + * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
> > + * @start: bit offset of the 8-bit value; must be a multiple of 8
> > + */
> > +static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
>
>
> Same here, "u8 value"
>
>
>
> > + unsigned long start)
> > +{
> > + const size_t index = BIT_WORD(start);
> > + const unsigned long offset = start % BITS_PER_LONG;
> > +
> > + map[index] &= ~(0xFF << offset);
> > + map[index] |= value << offset;
> > +}
> > +
> > #endif /* __ASSEMBLY__ */
> >
> > #endif /* __LINUX_BITMAP_H */
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index cf074bce3eb3..fb94a10f7853 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
> > (bit) < (size); \
> > (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
> >
> > +#define for_each_set_clump8(start, clump, bits, size) \
> > + for ((start) = find_first_clump8(&(clump), (bits), (size)); \
> > + (start) < (size); \
> > + (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
> > +
> > static inline int get_bitmask_order(unsigned int count)
> > {
> > int order;
> > diff --git a/lib/find_bit.c b/lib/find_bit.c
> > index 5c51eb45178a..e35a76b291e6 100644
> > --- a/lib/find_bit.c
> > +++ b/lib/find_bit.c
> > @@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
> > #endif
> >
> > #endif /* __BIG_ENDIAN */
> > +
> > +unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
>
>
> Ditto. "u8 *clump"
>
>
>
>
> > + unsigned long size, unsigned long offset)
> > +{
> > + offset = find_next_bit(addr, size, offset);
> > + if (offset == size)
> > + return size;
> > +
> > + offset = round_down(offset, 8);
> > + *clump = bitmap_get_value8(addr, offset);
> > +
> > + return offset;
> > +}
> > +EXPORT_SYMBOL(find_next_clump8);
> > --
> > 2.23.0
> >
>
>
> --
> Best Regards
>
> Masahiro Yamada

2019-10-09 17:09:55

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

2019-10-09 18:58:06

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > <[email protected]> wrote:
> > >
> > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > within a bitmap memory region. For each iteration, "start" is set to the
> > > bit offset of the found clump, while the respective clump value is
> > > stored to the location pointed by "clump". Additionally, the
> > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > respectively get and set an 8-bit value in a bitmap memory region.
>
> > Why is the return type "unsigned long" where you know
> > it return the 8-bit value ?
>
> Because bitmap API operates on unsigned long type. This is not only
> consistency, but for sake of flexibility in case we would like to introduce
> more calls like clump16 or so.

TBH, that doesn't convince me: those functions explicitly take/return an
8-bit value, and have "8" in their name. The 8-bit value is never
really related to, retrieved from, or stored in a full "unsigned long"
element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an
"unsigned long" value, too.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-10-10 02:30:10

by Masahiro Yamada

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Andy,
>
> On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > <[email protected]> wrote:
> > > >
> > > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > > within a bitmap memory region. For each iteration, "start" is set to the
> > > > bit offset of the found clump, while the respective clump value is
> > > > stored to the location pointed by "clump". Additionally, the
> > > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > > respectively get and set an 8-bit value in a bitmap memory region.
> >
> > > Why is the return type "unsigned long" where you know
> > > it return the 8-bit value ?
> >
> > Because bitmap API operates on unsigned long type. This is not only
> > consistency, but for sake of flexibility in case we would like to introduce
> > more calls like clump16 or so.
>
> TBH, that doesn't convince me: those functions explicitly take/return an
> 8-bit value, and have "8" in their name. The 8-bit value is never
> really related to, retrieved from, or stored in a full "unsigned long"
> element of a bitmap, only to/from/in a part (byte) of it.
>
> Following your rationale, all of iowrite{8,16,32,64}*() should take an
> "unsigned long" value, too.
>

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

If you want to see more examples from other parts,

int of_property_read_u8(const struct device_node *np,
const char *propname,
u8 *out_value)

int of_property_read_u16(const struct device_node *np,
const char *propname,
u16 *out_value)

--
Best Regards
Masahiro Yamada

2019-10-10 05:49:52

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
<[email protected]> wrote:
> On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > <[email protected]> wrote:
> > > > >
> > > > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > > > within a bitmap memory region. For each iteration, "start" is set to the
> > > > > bit offset of the found clump, while the respective clump value is
> > > > > stored to the location pointed by "clump". Additionally, the
> > > > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > > > respectively get and set an 8-bit value in a bitmap memory region.
> > >
> > > > Why is the return type "unsigned long" where you know
> > > > it return the 8-bit value ?
> > >
> > > Because bitmap API operates on unsigned long type. This is not only
> > > consistency, but for sake of flexibility in case we would like to introduce
> > > more calls like clump16 or so.
> >
> > TBH, that doesn't convince me: those functions explicitly take/return an
> > 8-bit value, and have "8" in their name. The 8-bit value is never
> > really related to, retrieved from, or stored in a full "unsigned long"
> > element of a bitmap, only to/from/in a part (byte) of it.
> >
> > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > "unsigned long" value, too.
> >
>
> +1
>
> Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
>
> void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> int nbits);
> void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
>
>
>
> If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about
fixed-width properies when we know that is the part of protocol.
Here we have no protocol which stricts us to the mentioned fixed-width types.

So, I can tell an opposite, your arguments didn't convince me.

Imagine the function which does an or / and / xor operation on bitmap.
Now, when I supply unsigned long, I will see
operations on one type in _one_ function independently of the size.
Your proposal will make an unneded churn.

--
With Best Regards,
Andy Shevchenko

2019-10-10 06:30:11

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> <[email protected]> wrote:
> > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > > > > within a bitmap memory region. For each iteration, "start" is set to the
> > > > > > bit offset of the found clump, while the respective clump value is
> > > > > > stored to the location pointed by "clump". Additionally, the
> > > > > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > > > > respectively get and set an 8-bit value in a bitmap memory region.
> > > >
> > > > > Why is the return type "unsigned long" where you know
> > > > > it return the 8-bit value ?
> > > >
> > > > Because bitmap API operates on unsigned long type. This is not only
> > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > more calls like clump16 or so.
> > >
> > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > 8-bit value, and have "8" in their name. The 8-bit value is never
> > > really related to, retrieved from, or stored in a full "unsigned long"
> > > element of a bitmap, only to/from/in a part (byte) of it.
> > >
> > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > "unsigned long" value, too.
> > >
> >
> > +1
> >
> > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> >
> > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > int nbits);
> > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> >
> >
> >
> > If you want to see more examples from other parts,
>
> Geert's and yours examples both are not related. They are about
> fixed-width properies when we know that is the part of protocol.
> Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from
the middle of the bitmap, which is reflected in their names ("clump8",
"value8").
The input/output value is clearly separated from the actual bitmap,
which is referenced by the "unsigned long *".

If you add new "value16" functions, they will be intended to store/retrieve
16-bit values.

Besides, if retrieving an 8-bit value requires passing an
"unsigned long *", the caller needs two variables: one unsigned long to
pass the address of, and one u8 to copy the returned value into.

> So, I can tell an opposite, your arguments didn't convince me.
>
> Imagine the function which does an or / and / xor operation on bitmap.
> Now, when I supply unsigned long, I will see
> operations on one type in _one_ function independently of the size.
> Your proposal will make an unneded churn.

Depends on what kind of value you will use to do the logical operation
with the bitmap:
- Full bitmap => unsigned long * + size,
- Single bitmap "word" => unsigned long,
- 8-bit value => u8,
- 16-bit value => u16

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-10-10 07:42:31

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 9:29 AM Geert Uytterhoeven <[email protected]> wrote:
> On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> > <[email protected]> wrote:
> > > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > > > > > within a bitmap memory region. For each iteration, "start" is set to the
> > > > > > > bit offset of the found clump, while the respective clump value is
> > > > > > > stored to the location pointed by "clump". Additionally, the
> > > > > > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > > > > > respectively get and set an 8-bit value in a bitmap memory region.
> > > > >
> > > > > > Why is the return type "unsigned long" where you know
> > > > > > it return the 8-bit value ?
> > > > >
> > > > > Because bitmap API operates on unsigned long type. This is not only
> > > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > > more calls like clump16 or so.
> > > >
> > > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > > 8-bit value, and have "8" in their name. The 8-bit value is never
> > > > really related to, retrieved from, or stored in a full "unsigned long"
> > > > element of a bitmap, only to/from/in a part (byte) of it.
> > > >
> > > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > > "unsigned long" value, too.
> > > >
> > >
> > > +1
> > >
> > > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> > >
> > > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > > int nbits);
> > > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> > >
> > >
> > >
> > > If you want to see more examples from other parts,
> >
> > Geert's and yours examples both are not related. They are about
> > fixed-width properies when we know that is the part of protocol.
> > Here we have no protocol which stricts us to the mentioned fixed-width types.
>
> Yes you have: they are functions to store/retrieve an 8-bit value from
> the middle of the bitmap, which is reflected in their names ("clump8",
> "value8").
> The input/output value is clearly separated from the actual bitmap,
> which is referenced by the "unsigned long *".
>
> If you add new "value16" functions, they will be intended to store/retrieve
> 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

> Besides, if retrieving an 8-bit value requires passing an
> "unsigned long *", the caller needs two variables: one unsigned long to
> pass the address of, and one u8 to copy the returned value into.

Why do you need a temporary variable? In some cases it might make
sense, but in general simple cases I don't see what you may achieve
with it.

I looked at bitmap.h and see few functions may have benefited of
actually eliminating a use of long -> u8 -> long conversion.

Here is the question what we are mostly doing after we got a clump out
of bitmap.

> > So, I can tell an opposite, your arguments didn't convince me.
> >
> > Imagine the function which does an or / and / xor operation on bitmap.
> > Now, when I supply unsigned long, I will see
> > operations on one type in _one_ function independently of the size.
> > Your proposal will make an unneded churn.
>
> Depends on what kind of value you will use to do the logical operation
> with the bitmap:
> - Full bitmap => unsigned long * + size,
> - Single bitmap "word" => unsigned long,
> - 8-bit value => u8,
> - 16-bit value => u16

--
With Best Regards,
Andy Shevchenko

2019-10-10 07:50:32

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

On Thu, Oct 10, 2019 at 9:42 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Oct 10, 2019 at 9:29 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> > > <[email protected]> wrote:
> > > > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > > > <[email protected]> wrote:
> > > > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > This macro iterates for each 8-bit group of bits (clump) with set bits,
> > > > > > > > within a bitmap memory region. For each iteration, "start" is set to the
> > > > > > > > bit offset of the found clump, while the respective clump value is
> > > > > > > > stored to the location pointed by "clump". Additionally, the
> > > > > > > > bitmap_get_value8 and bitmap_set_value8 functions are introduced to
> > > > > > > > respectively get and set an 8-bit value in a bitmap memory region.
> > > > > >
> > > > > > > Why is the return type "unsigned long" where you know
> > > > > > > it return the 8-bit value ?
> > > > > >
> > > > > > Because bitmap API operates on unsigned long type. This is not only
> > > > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > > > more calls like clump16 or so.
> > > > >
> > > > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > > > 8-bit value, and have "8" in their name. The 8-bit value is never
> > > > > really related to, retrieved from, or stored in a full "unsigned long"
> > > > > element of a bitmap, only to/from/in a part (byte) of it.
> > > > >
> > > > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > > > "unsigned long" value, too.
> > > > >
> > > >
> > > > +1
> > > >
> > > > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> > > >
> > > > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > > > int nbits);
> > > > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > > > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> > > >
> > > >
> > > >
> > > > If you want to see more examples from other parts,
> > >
> > > Geert's and yours examples both are not related. They are about
> > > fixed-width properies when we know that is the part of protocol.
> > > Here we have no protocol which stricts us to the mentioned fixed-width types.
> >
> > Yes you have: they are functions to store/retrieve an 8-bit value from
> > the middle of the bitmap, which is reflected in their names ("clump8",
> > "value8").
> > The input/output value is clearly separated from the actual bitmap,
> > which is referenced by the "unsigned long *".
> >
> > If you add new "value16" functions, they will be intended to store/retrieve
> > 16-bit values.
>
> And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Whatever is needed to store that?
I agree "unsigned long" is appropriate for a generic function to extract a
bit field of 1 to BITS_PER_LONG bits.

> > Besides, if retrieving an 8-bit value requires passing an
> > "unsigned long *", the caller needs two variables: one unsigned long to
> > pass the address of, and one u8 to copy the returned value into.
>
> Why do you need a temporary variable? In some cases it might make
> sense, but in general simple cases I don't see what you may achieve
> with it.

Because find_next_clump8() takes a pointer to store the output value.

> I looked at bitmap.h and see few functions may have benefited of
> actually eliminating a use of long -> u8 -> long conversion.
>
> Here is the question what we are mostly doing after we got a clump out
> of bitmap.

If I call find_next_clump8() to extract a byte, I guess I want to process an u8
aftwerwards?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-10-10 08:10:32

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

On Thu, Oct 10, 2019 at 09:49:51AM +0200, Geert Uytterhoeven wrote:
> On Thu, Oct 10, 2019 at 9:42 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Oct 10, 2019 at 9:29 AM Geert Uytterhoeven <[email protected]> wrote:
> > > On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> > > > <[email protected]> wrote:
> > > > > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > > > > <[email protected]> wrote:
> > > > > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > > > > <[email protected]> wrote:

> > > > > > > > Why is the return type "unsigned long" where you know
> > > > > > > > it return the 8-bit value ?
> > > > > > >
> > > > > > > Because bitmap API operates on unsigned long type. This is not only
> > > > > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > > > > more calls like clump16 or so.
> > > > > >
> > > > > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > > > > 8-bit value, and have "8" in their name. The 8-bit value is never
> > > > > > really related to, retrieved from, or stored in a full "unsigned long"
> > > > > > element of a bitmap, only to/from/in a part (byte) of it.
> > > > > >
> > > > > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > > > > "unsigned long" value, too.
> > > > >
> > > > > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> > > > >
> > > > > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > > > > int nbits);
> > > > > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > > > > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> > > > >
> > > > > If you want to see more examples from other parts,
> > > >
> > > > Geert's and yours examples both are not related. They are about
> > > > fixed-width properies when we know that is the part of protocol.
> > > > Here we have no protocol which stricts us to the mentioned fixed-width types.
> > >
> > > Yes you have: they are functions to store/retrieve an 8-bit value from
> > > the middle of the bitmap, which is reflected in their names ("clump8",
> > > "value8").
> > > The input/output value is clearly separated from the actual bitmap,
> > > which is referenced by the "unsigned long *".
> > >
> > > If you add new "value16" functions, they will be intended to store/retrieve
> > > 16-bit values.
> >
> > And if I add 4-bit, 12-bit or 24-bit values, what should I use?
>
> Whatever is needed to store that?
> I agree "unsigned long" is appropriate for a generic function to extract a
> bit field of 1 to BITS_PER_LONG bits.
>
> > > Besides, if retrieving an 8-bit value requires passing an
> > > "unsigned long *", the caller needs two variables: one unsigned long to
> > > pass the address of, and one u8 to copy the returned value into.
> >
> > Why do you need a temporary variable? In some cases it might make
> > sense, but in general simple cases I don't see what you may achieve
> > with it.
>
> Because find_next_clump8() takes a pointer to store the output value.

So does regmap_read().

8 appeared there during review when it has been proposed to optimize to 8-bit
clumps as most of the current users utilize it. The initial idea was to be
bit-width agnostic. And with current API it's possible to easy convert to other
formats later if we need.

> > I looked at bitmap.h and see few functions may have benefited of
> > actually eliminating a use of long -> u8 -> long conversion.
> >
> > Here is the question what we are mostly doing after we got a clump out
> > of bitmap.
>
> If I call find_next_clump8() to extract a byte, I guess I want to process an u8
> aftwerwards?

Some functions may expect a width-(semi-)dependent types, like regmap_write().
Yes, it's possible to supply u8 there and have an implicit type cast.

--
With Best Regards,
Andy Shevchenko

2019-10-10 08:25:31

[permalink] [raw]

Subject: Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

On Thu, Oct 10, 2019 at 10:08 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Oct 10, 2019 at 09:49:51AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Oct 10, 2019 at 9:42 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Thu, Oct 10, 2019 at 9:29 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Thu, Oct 10, 2019 at 7:49 AM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Thu, Oct 10, 2019 at 5:31 AM Masahiro Yamada
> > > > > <[email protected]> wrote:
> > > > > > On Thu, Oct 10, 2019 at 3:54 AM Geert Uytterhoeven <[email protected]> wrote:
> > > > > > > On Wed, Oct 9, 2019 at 7:09 PM Andy Shevchenko
> > > > > > > <[email protected]> wrote:
> > > > > > > > On Thu, Oct 10, 2019 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > > > > > > > > On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray
> > > > > > > > > <[email protected]> wrote:
>
> > > > > > > > > Why is the return type "unsigned long" where you know
> > > > > > > > > it return the 8-bit value ?
> > > > > > > >
> > > > > > > > Because bitmap API operates on unsigned long type. This is not only
> > > > > > > > consistency, but for sake of flexibility in case we would like to introduce
> > > > > > > > more calls like clump16 or so.
> > > > > > >
> > > > > > > TBH, that doesn't convince me: those functions explicitly take/return an
> > > > > > > 8-bit value, and have "8" in their name. The 8-bit value is never
> > > > > > > really related to, retrieved from, or stored in a full "unsigned long"
> > > > > > > element of a bitmap, only to/from/in a part (byte) of it.
> > > > > > >
> > > > > > > Following your rationale, all of iowrite{8,16,32,64}*() should take an
> > > > > > > "unsigned long" value, too.
> > > > > >
> > > > > > Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.
> > > > > >
> > > > > > void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned
> > > > > > int nbits);
> > > > > > void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits);
> > > > > > static inline void bitmap_from_u64(unsigned long *dst, u64 mask);
> > > > > >
> > > > > > If you want to see more examples from other parts,
> > > > >
> > > > > Geert's and yours examples both are not related. They are about
> > > > > fixed-width properies when we know that is the part of protocol.
> > > > > Here we have no protocol which stricts us to the mentioned fixed-width types.
> > > >
> > > > Yes you have: they are functions to store/retrieve an 8-bit value from
> > > > the middle of the bitmap, which is reflected in their names ("clump8",
> > > > "value8").
> > > > The input/output value is clearly separated from the actual bitmap,
> > > > which is referenced by the "unsigned long *".
> > > >
> > > > If you add new "value16" functions, they will be intended to store/retrieve
> > > > 16-bit values.
> > >
> > > And if I add 4-bit, 12-bit or 24-bit values, what should I use?
> >
> > Whatever is needed to store that?
> > I agree "unsigned long" is appropriate for a generic function to extract a
> > bit field of 1 to BITS_PER_LONG bits.
> >
> > > > Besides, if retrieving an 8-bit value requires passing an
> > > > "unsigned long *", the caller needs two variables: one unsigned long to
> > > > pass the address of, and one u8 to copy the returned value into.
> > >
> > > Why do you need a temporary variable? In some cases it might make
> > > sense, but in general simple cases I don't see what you may achieve
> > > with it.
> >
> > Because find_next_clump8() takes a pointer to store the output value.
>
> So does regmap_read().

I believe that one is different, as it is a generic function, and the
width of the
returned value depends on the regmap config.

> 8 appeared there during review when it has been proposed to optimize to 8-bit
> clumps as most of the current users utilize it. The initial idea was to be
> bit-width agnostic. And with current API it's possible to easy convert to other
> formats later if we need.

"optimized for 8-bit clumps" and "out-of-line function that takes an
unsigned long pointer for an output parameter" don't match well, IMHO.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2019-10-10 13:15:45