| Message ID | 1824412519cb8791ab428065116927ee7b77cf35.1738329459.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Non-const bitfield helpers | expand |
On 31/01/2025 14:46:51+0100, Geert Uytterhoeven wrote: > The existing FIELD_{GET,PREP}() macros are limited to compile-time > constants. However, it is very common to prepare or extract bitfield > elements where the bitfield mask is not a compile-time constant. > > To avoid this limitation, the AT91 clock driver and several other > drivers already have their own non-const field_{prep,get}() macros. > Make them available for general use by consolidating them in > <linux/bitfield.h>, and improve them slightly: > 1. Avoid evaluating macro parameters more than once, > 2. Replace "ffs() - 1" by "__ffs()", > 3. Support 64-bit use on 32-bit architectures. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > v2: > - Cast val resp. reg to the mask type, > - Fix 64-bit use on 32-bit architectures, > - Convert new upstream users: > - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > - drivers/gpio/gpio-aspeed.c > - drivers/iio/temperature/mlx90614.c > - drivers/pinctrl/nuvoton/pinctrl-ma35.c > - sound/usb/mixer_quirks.c > - Convert new user queued in renesas-devel for v6.15: > - drivers/soc/renesas/rz-sysc.c > --- > drivers/clk/at91/clk-peripheral.c | 1 + > drivers/clk/at91/pmc.h | 3 -- > .../qat/qat_common/adf_gen4_pm_debugfs.c | 8 +---- > drivers/gpio/gpio-aspeed.c | 5 +-- > drivers/iio/temperature/mlx90614.c | 5 +-- > drivers/pinctrl/nuvoton/pinctrl-ma35.c | 4 --- > drivers/soc/renesas/rz-sysc.c | 3 +- > include/linux/bitfield.h | 34 +++++++++++++++++++ > sound/usb/mixer_quirks.c | 4 --- > 9 files changed, 39 insertions(+), 28 deletions(-) > > diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c > index c173a44c800aa8cc..60208bdc3fe4797e 100644 > --- a/drivers/clk/at91/clk-peripheral.c > +++ b/drivers/clk/at91/clk-peripheral.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com> > */ > > +#include <linux/bitfield.h> > #include <linux/bitops.h> > #include <linux/clk-provider.h> > #include <linux/clkdev.h> > diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h > index 4fb29ca111f7d427..3838e4f7df2d4a70 100644 > --- a/drivers/clk/at91/pmc.h > +++ b/drivers/clk/at91/pmc.h > @@ -116,9 +116,6 @@ struct at91_clk_pms { > unsigned int parent; > }; > > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > #define ndck(a, s) (a[s - 1].id + 1) > #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1) > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > index 2e4095c4c12c94f9..ebaa59e934178309 100644 > --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2023 Intel Corporation */ > +#include <linux/bitfield.h> > #include <linux/dma-mapping.h> > #include <linux/kernel.h> > #include <linux/string_helpers.h> > @@ -11,13 +12,6 @@ > #include "adf_gen4_pm.h" > #include "icp_qat_fw_init_admin.h" > > -/* > - * This is needed because a variable is used to index the mask at > - * pm_scnprint_table(), making it not compile time constant, so the compile > - * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled. > - */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > - > #define PM_INFO_MEMBER_OFF(member) \ > (offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32)) > > diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c > index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644 > --- a/drivers/gpio/gpio-aspeed.c > +++ b/drivers/gpio/gpio-aspeed.c > @@ -5,6 +5,7 @@ > * Joel Stanley <joel@jms.id.au> > */ > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/gpio/aspeed.h> > #include <linux/gpio/driver.h> > @@ -30,10 +31,6 @@ > #include <linux/gpio/consumer.h> > #include "gpiolib.h" > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > #define GPIO_G7_IRQ_STS_BASE 0x100 > #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4) > #define GPIO_G7_CTRL_REG_BASE 0x180 > diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c > index 740018d4b3dfb35e..c58dc59d4f570831 100644 > --- a/drivers/iio/temperature/mlx90614.c > +++ b/drivers/iio/temperature/mlx90614.c > @@ -22,6 +22,7 @@ > * the "wakeup" GPIO is not given, power management will be disabled. > */ > > +#include <linux/bitfield.h> > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/gpio/consumer.h> > @@ -68,10 +69,6 @@ > #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */ > #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */ > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > struct mlx_chip_info { > /* EEPROM offsets with 16-bit data, MSB first */ > /* emissivity correction coefficient */ > diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c > index 59c4e7c6cddea127..3ba28faa8e1418a9 100644 > --- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c > +++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c > @@ -81,10 +81,6 @@ > #define MVOLT_1800 0 > #define MVOLT_3300 1 > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > static const char * const gpio_group_name[] = { > "gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog", > "gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion", > diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c > index 1c98da37b7d18745..917a029d849585cd 100644 > --- a/drivers/soc/renesas/rz-sysc.c > +++ b/drivers/soc/renesas/rz-sysc.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2024 Renesas Electronics Corp. > */ > > +#include <linux/bitfield.h> > #include <linux/io.h> > #include <linux/of.h> > #include <linux/platform_device.h> > @@ -12,8 +13,6 @@ > > #include "rz-sysc.h" > > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > - > /** > * struct rz_sysc - RZ SYSC private data structure > * @base: SYSC base address > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 63928f1732230700..c62324a9fcc81241 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -203,4 +203,38 @@ __MAKE_OP(64) > #undef __MAKE_OP > #undef ____MAKE_OP > > +/** > + * field_prep() - prepare a bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_val: value to put in the field > + * > + * field_prep() masks and shifts up the value. The result should be > + * combined with other fields of the bitfield using logical OR. > + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant. > + */ > +#define field_prep(_mask, _val) \ > + ({ \ > + typeof(_mask) __mask = (_mask); \ > + unsigned int __shift = sizeof(_mask) <= 4 ? \ > + __ffs(__mask) : __ffs64(__mask); \ > + (((typeof(_mask))(_val) << __shift) & (__mask)); \ > + }) > + > +/** > + * field_get() - extract a bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_reg: value of entire bitfield > + * > + * field_get() extracts the field specified by @_mask from the > + * bitfield passed in as @_reg by masking and shifting it down. > + * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant. > + */ > +#define field_get(_mask, _reg) \ > + ({ \ > + typeof(_mask) __mask = _mask; \ > + unsigned int __shift = sizeof(_mask) <= 4 ? \ > + __ffs(__mask) : __ffs64(__mask); \ > + (typeof(_mask))(((_reg) & (__mask)) >> __shift); \ > + }) > + > #endif > diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c > index 23fcd680167d0298..00ab811e4b11a573 100644 > --- a/sound/usb/mixer_quirks.c > +++ b/sound/usb/mixer_quirks.c > @@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer) > #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask)) > #define RME_DIGIFACE_INVERT BIT(31) > > -/* Nonconst helpers */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val) > { > struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol); > -- > 2.43.0 >
On Fri, 31 Jan 2025 14:46:51 +0100 Geert Uytterhoeven <geert+renesas@glider.be> wrote: > The existing FIELD_{GET,PREP}() macros are limited to compile-time > constants. However, it is very common to prepare or extract bitfield > elements where the bitfield mask is not a compile-time constant. > > To avoid this limitation, the AT91 clock driver and several other > drivers already have their own non-const field_{prep,get}() macros. > Make them available for general use by consolidating them in > <linux/bitfield.h>, and improve them slightly: > 1. Avoid evaluating macro parameters more than once, > 2. Replace "ffs() - 1" by "__ffs()", > 3. Support 64-bit use on 32-bit architectures. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> For the IIO one. Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v2: > - Cast val resp. reg to the mask type, > - Fix 64-bit use on 32-bit architectures, > - Convert new upstream users: > - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > - drivers/gpio/gpio-aspeed.c > - drivers/iio/temperature/mlx90614.c > - drivers/pinctrl/nuvoton/pinctrl-ma35.c > - sound/usb/mixer_quirks.c > - Convert new user queued in renesas-devel for v6.15: > - drivers/soc/renesas/rz-sysc.c > --- > drivers/clk/at91/clk-peripheral.c | 1 + > drivers/clk/at91/pmc.h | 3 -- > .../qat/qat_common/adf_gen4_pm_debugfs.c | 8 +---- > drivers/gpio/gpio-aspeed.c | 5 +-- > drivers/iio/temperature/mlx90614.c | 5 +-- > drivers/pinctrl/nuvoton/pinctrl-ma35.c | 4 --- > drivers/soc/renesas/rz-sysc.c | 3 +- > include/linux/bitfield.h | 34 +++++++++++++++++++ > sound/usb/mixer_quirks.c | 4 --- > 9 files changed, 39 insertions(+), 28 deletions(-) > > diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c > index c173a44c800aa8cc..60208bdc3fe4797e 100644 > --- a/drivers/clk/at91/clk-peripheral.c > +++ b/drivers/clk/at91/clk-peripheral.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com> > */ > > +#include <linux/bitfield.h> > #include <linux/bitops.h> > #include <linux/clk-provider.h> > #include <linux/clkdev.h> > diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h > index 4fb29ca111f7d427..3838e4f7df2d4a70 100644 > --- a/drivers/clk/at91/pmc.h > +++ b/drivers/clk/at91/pmc.h > @@ -116,9 +116,6 @@ struct at91_clk_pms { > unsigned int parent; > }; > > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > #define ndck(a, s) (a[s - 1].id + 1) > #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1) > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > index 2e4095c4c12c94f9..ebaa59e934178309 100644 > --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2023 Intel Corporation */ > +#include <linux/bitfield.h> > #include <linux/dma-mapping.h> > #include <linux/kernel.h> > #include <linux/string_helpers.h> > @@ -11,13 +12,6 @@ > #include "adf_gen4_pm.h" > #include "icp_qat_fw_init_admin.h" > > -/* > - * This is needed because a variable is used to index the mask at > - * pm_scnprint_table(), making it not compile time constant, so the compile > - * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled. > - */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > - > #define PM_INFO_MEMBER_OFF(member) \ > (offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32)) > > diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c > index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644 > --- a/drivers/gpio/gpio-aspeed.c > +++ b/drivers/gpio/gpio-aspeed.c > @@ -5,6 +5,7 @@ > * Joel Stanley <joel@jms.id.au> > */ > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/gpio/aspeed.h> > #include <linux/gpio/driver.h> > @@ -30,10 +31,6 @@ > #include <linux/gpio/consumer.h> > #include "gpiolib.h" > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > #define GPIO_G7_IRQ_STS_BASE 0x100 > #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4) > #define GPIO_G7_CTRL_REG_BASE 0x180 > diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c > index 740018d4b3dfb35e..c58dc59d4f570831 100644 > --- a/drivers/iio/temperature/mlx90614.c > +++ b/drivers/iio/temperature/mlx90614.c > @@ -22,6 +22,7 @@ > * the "wakeup" GPIO is not given, power management will be disabled. > */ > > +#include <linux/bitfield.h> > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/gpio/consumer.h> > @@ -68,10 +69,6 @@ > #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */ > #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */ > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > struct mlx_chip_info { > /* EEPROM offsets with 16-bit data, MSB first */ > /* emissivity correction coefficient */ > diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c > index 59c4e7c6cddea127..3ba28faa8e1418a9 100644 > --- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c > +++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c > @@ -81,10 +81,6 @@ > #define MVOLT_1800 0 > #define MVOLT_3300 1 > > -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > static const char * const gpio_group_name[] = { > "gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog", > "gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion", > diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c > index 1c98da37b7d18745..917a029d849585cd 100644 > --- a/drivers/soc/renesas/rz-sysc.c > +++ b/drivers/soc/renesas/rz-sysc.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2024 Renesas Electronics Corp. > */ > > +#include <linux/bitfield.h> > #include <linux/io.h> > #include <linux/of.h> > #include <linux/platform_device.h> > @@ -12,8 +13,6 @@ > > #include "rz-sysc.h" > > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > - > /** > * struct rz_sysc - RZ SYSC private data structure > * @base: SYSC base address > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 63928f1732230700..c62324a9fcc81241 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -203,4 +203,38 @@ __MAKE_OP(64) > #undef __MAKE_OP > #undef ____MAKE_OP > > +/** > + * field_prep() - prepare a bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_val: value to put in the field > + * > + * field_prep() masks and shifts up the value. The result should be > + * combined with other fields of the bitfield using logical OR. > + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant. > + */ > +#define field_prep(_mask, _val) \ > + ({ \ > + typeof(_mask) __mask = (_mask); \ > + unsigned int __shift = sizeof(_mask) <= 4 ? \ > + __ffs(__mask) : __ffs64(__mask); \ > + (((typeof(_mask))(_val) << __shift) & (__mask)); \ > + }) > + > +/** > + * field_get() - extract a bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_reg: value of entire bitfield > + * > + * field_get() extracts the field specified by @_mask from the > + * bitfield passed in as @_reg by masking and shifting it down. > + * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant. > + */ > +#define field_get(_mask, _reg) \ > + ({ \ > + typeof(_mask) __mask = _mask; \ > + unsigned int __shift = sizeof(_mask) <= 4 ? \ > + __ffs(__mask) : __ffs64(__mask); \ > + (typeof(_mask))(((_reg) & (__mask)) >> __shift); \ > + }) > + > #endif > diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c > index 23fcd680167d0298..00ab811e4b11a573 100644 > --- a/sound/usb/mixer_quirks.c > +++ b/sound/usb/mixer_quirks.c > @@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer) > #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask)) > #define RME_DIGIFACE_INVERT BIT(31) > > -/* Nonconst helpers */ > -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) > -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) > - > static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val) > { > struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
On Fri, 31 Jan 2025 14:46:51 +0100 Geert Uytterhoeven <geert+renesas@glider.be> wrote: > The existing FIELD_{GET,PREP}() macros are limited to compile-time > constants. However, it is very common to prepare or extract bitfield > elements where the bitfield mask is not a compile-time constant. > > To avoid this limitation, the AT91 clock driver and several other > drivers already have their own non-const field_{prep,get}() macros. > Make them available for general use by consolidating them in > <linux/bitfield.h>, and improve them slightly: > 1. Avoid evaluating macro parameters more than once, > 2. Replace "ffs() - 1" by "__ffs()", > 3. Support 64-bit use on 32-bit architectures. ... > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 63928f1732230700..c62324a9fcc81241 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -203,4 +203,38 @@ __MAKE_OP(64) > #undef __MAKE_OP > #undef ____MAKE_OP > > +/** > + * field_prep() - prepare a bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_val: value to put in the field > + * > + * field_prep() masks and shifts up the value. The result should be > + * combined with other fields of the bitfield using logical OR. > + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant. > + */ > +#define field_prep(_mask, _val) \ You don't need an _ prefix on the 'parameters' - it doesn't gain anything. > + ({ \ > + typeof(_mask) __mask = (_mask); \ Use: __auto_type __mask = (_mask); > + unsigned int __shift = sizeof(_mask) <= 4 ? \ > + __ffs(__mask) : __ffs64(__mask); \ > + (((typeof(_mask))(_val) << __shift) & (__mask)); \ There are a lot of () in that line, perhaps: __auto_type(__mask) = (_mask); typeof (__mask) __val = (_val); unsigned int __shift = ...; (__val << __shift) & __mask; Note the typeof (__mask) - avoids line-length 'bloat' when the arguments are non-trivial. David
On 31/01/2025 at 22:46, Geert Uytterhoeven wrote: > The existing FIELD_{GET,PREP}() macros are limited to compile-time > constants. However, it is very common to prepare or extract bitfield > elements where the bitfield mask is not a compile-time constant. Why is it that the existing FIELD_{GET,PREP}() macros must be limited to compile time constants? Instead of creating another variant for non-constant bitfields, wouldn't it be better to make the existing macro accept both? As far as I can see, only __BUILD_BUG_ON_NOT_POWER_OF_2() and __BF_FIELD_CHECK() need to be adjusted. I am thinking of this: diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 63928f173223..c6bedab862d1 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -8,6 +8,7 @@ #define _LINUX_BITFIELD_H #include <linux/build_bug.h> +#include <linux/compiler.h> #include <asm/byteorder.h> /* @@ -62,15 +63,13 @@ #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ ({ \ - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ - _pfx "mask is not constant"); \ - BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ - ~((_mask) >> __bf_shf(_mask)) & \ - (0 + (_val)) : 0, \ + BUILD_BUG_ON_MSG(statically_true((_mask) == 0), \ + _pfx "mask is zero"); \ + BUILD_BUG_ON_MSG(statically_true(~((_mask) >> __bf_shf(_mask)) & \ + (0 + (_val))), \ _pfx "value too large for the field"); \ - BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ - __bf_cast_unsigned(_reg, ~0ull), \ + BUILD_BUG_ON_MSG(statically_true(__bf_cast_unsigned(_mask, _mask) > \ + __bf_cast_unsigned(_reg, ~0ull)), \ _pfx "type of reg too small for mask"); \ __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ (1ULL << __bf_shf(_mask))); \ diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h index 3aa3640f8c18..3b8055ebb55f 100644 --- a/include/linux/build_bug.h +++ b/include/linux/build_bug.h @@ -18,9 +18,9 @@ /* Force a compilation error if a constant expression is not a power of 2 */ #define __BUILD_BUG_ON_NOT_POWER_OF_2(n) \ - BUILD_BUG_ON(((n) & ((n) - 1)) != 0) + BUILD_BUG_ON(statically_true((n) & ((n) - 1))) #define BUILD_BUG_ON_NOT_POWER_OF_2(n) \ - BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0)) + BUILD_BUG_ON(statically_true(!(n) || ((n) & ((n) - 1)))) /* * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the > To avoid this limitation, the AT91 clock driver and several other > drivers already have their own non-const field_{prep,get}() macros. > Make them available for general use by consolidating them in > <linux/bitfield.h>, and improve them slightly: > 1. Avoid evaluating macro parameters more than once, > 2. Replace "ffs() - 1" by "__ffs()", > 3. Support 64-bit use on 32-bit architectures. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> (...) Yours sincerely, Vincent Mailhol
On Sun, Feb 02, 2025 at 05:26:04PM +0900, Vincent Mailhol wrote: > On 31/01/2025 at 22:46, Geert Uytterhoeven wrote: > > The existing FIELD_{GET,PREP}() macros are limited to compile-time > > constants. However, it is very common to prepare or extract bitfield > > elements where the bitfield mask is not a compile-time constant. > > Why is it that the existing FIELD_{GET,PREP}() macros must be limited to > compile time constants? I guess, for historical reasons? > Instead of creating another variant for > non-constant bitfields, wouldn't it be better to make the existing macro > accept both? Yes, it would definitely be better IMO. > As far as I can see, only __BUILD_BUG_ON_NOT_POWER_OF_2() and > __BF_FIELD_CHECK() need to be adjusted. I am thinking of this: > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 63928f173223..c6bedab862d1 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -8,6 +8,7 @@ > #define _LINUX_BITFIELD_H > > #include <linux/build_bug.h> > +#include <linux/compiler.h> > #include <asm/byteorder.h> > > /* > @@ -62,15 +63,13 @@ > > #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ > ({ \ > - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ > - _pfx "mask is not constant"); \ > - BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ > - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > - ~((_mask) >> __bf_shf(_mask)) & \ > - (0 + (_val)) : 0, \ > + BUILD_BUG_ON_MSG(statically_true((_mask) == 0), \ > + _pfx "mask is zero"); \ > + BUILD_BUG_ON_MSG(statically_true(~((_mask) >> This should be a const_true(), because statically_true() may be OK with something like: ((runtime_var << 1) & 1 == 0) I think it's your own patch that adds const_true(): 4f3d1be4c2f8a :) Thanks, Yury
On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote: > > > Instead of creating another variant for > > non-constant bitfields, wouldn't it be better to make the existing macro > > accept both? > > Yes, it would definitely be better IMO. On the flip side, there have been discussions in the past (though I think not all, if any, on the list(s)) about the argument order. Since the value is typically not a constant, requiring the mask to be a constant has ensured that the argument order isn't as easily mixed up as otherwise. With a non-constant mask there can also be no validation that the mask is contiguous etc. Now that doesn't imply a strong objection - personally I've come to prefer the lower-case typed versions anyway - but something to keep in mind when doing this. However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost certainly shouldn't be done for the same reason - not compiling for non- constant values is [IMHO] part of the API contract for that macro. This can be important for the same reasons. (Obviously, doing that change now doesn't invalidate existing code, but it does remove checks that may have been intended to be present in the code.) johannes
On 03/02/2025 at 16:44, Johannes Berg wrote: > On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote: >> >>> Instead of creating another variant for >>> non-constant bitfields, wouldn't it be better to make the existing macro >>> accept both? >> >> Yes, it would definitely be better IMO. > > On the flip side, there have been discussions in the past (though I > think not all, if any, on the list(s)) about the argument order. Since > the value is typically not a constant, requiring the mask to be a > constant has ensured that the argument order isn't as easily mixed up as > otherwise. If this is a concern, then it can be checked with: BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) && __builtin_constant_p(_val), _pfx "mask is not constant"); It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow any other combination. > With a non-constant mask there can also be no validation that the mask > is contiguous etc. > > Now that doesn't imply a strong objection - personally I've come to > prefer the lower-case typed versions anyway - but something to keep in > mind when doing this. > > However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost > certainly shouldn't be done for the same reason - not compiling for non- > constant values is [IMHO] part of the API contract for that macro. This > can be important for the same reasons. Your point is fair enough. But I do not see this as a killer argument. We can instead just add below helper: BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2() But, for the same reason why I would rather not have both the FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not have a BUILD_BUG_ON_NOT_POWER_OF_2() and a BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2(). If your concern is the wording of the contract, the description can just be updated. Yours sincerely, Vincent Mailhol
Hi Vincent, On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > On 03/02/2025 at 16:44, Johannes Berg wrote: > > On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote: > >>> Instead of creating another variant for > >>> non-constant bitfields, wouldn't it be better to make the existing macro > >>> accept both? > >> > >> Yes, it would definitely be better IMO. > > > > On the flip side, there have been discussions in the past (though I > > think not all, if any, on the list(s)) about the argument order. Since > > the value is typically not a constant, requiring the mask to be a > > constant has ensured that the argument order isn't as easily mixed up as > > otherwise. > > If this is a concern, then it can be checked with: > > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) && > __builtin_constant_p(_val), > _pfx "mask is not constant"); > > It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow > any other combination. Even that case looks valid to me. Actually there is already such a user in drivers/iio/temperature/mlx90614.c: ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR); So if you want enhanced safety, having both the safer/const upper-case variants and the less-safe/non-const lower-case variants makes sense. Gr{oetje,eeting}s, Geert
On Mon, 2025-02-03 at 22:36 +0900, Vincent Mailhol wrote: > > On the flip side, there have been discussions in the past (though I > > think not all, if any, on the list(s)) about the argument order. Since > > the value is typically not a constant, requiring the mask to be a > > constant has ensured that the argument order isn't as easily mixed up as > > otherwise. > > If this is a concern, then it can be checked with: > > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) && > __builtin_constant_p(_val), > _pfx "mask is not constant"); > > It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow > any other combination. There almost certainly will be users who want both to be non-constant though, and anyway I don't understand how that helps - if you want to write the value 0x7 to the (variable) mask 0xF then this won't catch anything? > > However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost > > certainly shouldn't be done for the same reason - not compiling for non- > > constant values is [IMHO] part of the API contract for that macro. This > > can be important for the same reasons. > > Your point is fair enough. But I do not see this as a killer argument. > We can instead just add below helper: > > BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2() > > But, for the same reason why I would rather not have both the > FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not > have a BUILD_BUG_ON_NOT_POWER_OF_2() and a > BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2(). > > If your concern is the wording of the contract, the description can just > be updated. No, I just think in both cases it's really bad form to silently update the contract removing negative assertions that other people may have been relying on. Not because these trigger today, of course, but because they may not have added additional checks, or similar. So arguably then you should have BUILD_BUG_ON_CONST_NOT_POWER_OF_2() or so instead, so that all existing users are unaffected by the updates, and similarly that's an argument for leaving FIELD_* versions intact. Or I guess one could change all existing users to new ones accordingly, say FIELD_*_CONST_MASK(), but that's pretty annoying too. johannes
On 03/02/2025 at 22:59, Geert Uytterhoeven wrote: > Hi Vincent, > > On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: >> On 03/02/2025 at 16:44, Johannes Berg wrote: >>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote: >>>>> Instead of creating another variant for >>>>> non-constant bitfields, wouldn't it be better to make the existing macro >>>>> accept both? >>>> >>>> Yes, it would definitely be better IMO. >>> >>> On the flip side, there have been discussions in the past (though I >>> think not all, if any, on the list(s)) about the argument order. Since >>> the value is typically not a constant, requiring the mask to be a >>> constant has ensured that the argument order isn't as easily mixed up as >>> otherwise. >> >> If this is a concern, then it can be checked with: >> >> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) && >> __builtin_constant_p(_val), >> _pfx "mask is not constant"); >> >> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow >> any other combination. > > Even that case looks valid to me. Actually there is already such a user > in drivers/iio/temperature/mlx90614.c: > > ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR); > > So if you want enhanced safety, having both the safer/const upper-case > variants and the less-safe/non-const lower-case variants makes sense. So, we are scared of people calling FIELD_PREP() with the arguments in the wrong order: FIELD_PREP(val, mask) thus adding the check that mask must be a compile time constant. But if we introduce a second function, don't we introduce the risk of having people use the lower case variant instead of the upper case variant? field_prep(incorrect_const_mask, val) I am not sure to follow the logic of why having two functions is the safer choice. Whatever the solution you propose, there will be a way to misuse it. Let me ask, what is the most likely to happen: 1. wrong parameter order 2. wrong function name ? If you have the conviction that people more often do mistake 1. then I am fine with your solution. Otherwise, if 1. and 2. have an equally likelihood, then I would argue to go with the simplicity of the single function. Yours sincerely, Vincent Mailhol
On Tue, Feb 04, 2025 at 12:41:55AM +0900, Vincent Mailhol wrote: > On 03/02/2025 at 22:59, Geert Uytterhoeven wrote: > > Hi Vincent, > > > > On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > >> On 03/02/2025 at 16:44, Johannes Berg wrote: > >>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote: > >>>>> Instead of creating another variant for > >>>>> non-constant bitfields, wouldn't it be better to make the existing macro > >>>>> accept both? > >>>> > >>>> Yes, it would definitely be better IMO. > >>> > >>> On the flip side, there have been discussions in the past (though I > >>> think not all, if any, on the list(s)) about the argument order. Since > >>> the value is typically not a constant, requiring the mask to be a > >>> constant has ensured that the argument order isn't as easily mixed up as > >>> otherwise. > >> > >> If this is a concern, then it can be checked with: > >> > >> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) && > >> __builtin_constant_p(_val), > >> _pfx "mask is not constant"); > >> > >> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow > >> any other combination. > > > > Even that case looks valid to me. Actually there is already such a user > > in drivers/iio/temperature/mlx90614.c: > > > > ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR); > > > > So if you want enhanced safety, having both the safer/const upper-case > > variants and the less-safe/non-const lower-case variants makes sense. I agree with that. I just don't want the same shift-and operation to be opencoded again and again. What I actually meant is that I'm OK with whatever number of field_prep() macro flavors, if we make sure that they don't duplicate each other. So for me, something like this would be the best solution: #define field_prep(mask, val) \ (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) #define FIELD_PREP(mask, val) \ ( \ FIELD_PREP_INPUT_CHECK(_mask, _val,); \ field_prep(mask, val); \ ) #define FIELD_PREP_CONST(_mask, _val) \ ( \ FIELD_PREP_CONST_INPUT_CHECK(mask, val); FIELD_PREP(mask, val); // or field_prep() ) We have a similar macro GENMASK() in linux/bits.h. It is implemented like this: #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) #define GENMASK(h, l) \ (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) And it works just well. Can we end up with a similar approach here? > So, we are scared of people calling FIELD_PREP() with the arguments in > the wrong order: > > FIELD_PREP(val, mask) > > thus adding the check that mask must be a compile time constant. Don't be scared. Kernel coding implies that people get used to read function declarations and comments on top of them before using something. Thansk, Yury
On Sun, 2 Feb 2025 17:26:04 +0900 Vincent Mailhol wrote: > On 31/01/2025 at 22:46, Geert Uytterhoeven wrote: > > The existing FIELD_{GET,PREP}() macros are limited to compile-time > > constants. However, it is very common to prepare or extract bitfield > > elements where the bitfield mask is not a compile-time constant. > > Why is it that the existing FIELD_{GET,PREP}() macros must be limited to > compile time constants? Hard no, some high performance networking drivers use this on the fastpath. We want to make sure that the compiler doesn't do anything stupid, and decomposes the masks at build time. The macros work just fine for a *lot* of code: $ git grep -E 'FIELD_(PREP|GET)\(' | wc -l 22407 BTW aren't u32_get_bits(), u32_replace_bits() etc. not what you need in the first place? I think people don't know about those, with all due respect the way they are coded up looks like an IOCCC submission..
diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c index c173a44c800aa8cc..60208bdc3fe4797e 100644 --- a/drivers/clk/at91/clk-peripheral.c +++ b/drivers/clk/at91/clk-peripheral.c @@ -3,6 +3,7 @@ * Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com> */ +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/clk-provider.h> #include <linux/clkdev.h> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h index 4fb29ca111f7d427..3838e4f7df2d4a70 100644 --- a/drivers/clk/at91/pmc.h +++ b/drivers/clk/at91/pmc.h @@ -116,9 +116,6 @@ struct at91_clk_pms { unsigned int parent; }; -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) - #define ndck(a, s) (a[s - 1].id + 1) #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1) diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c index 2e4095c4c12c94f9..ebaa59e934178309 100644 --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2023 Intel Corporation */ +#include <linux/bitfield.h> #include <linux/dma-mapping.h> #include <linux/kernel.h> #include <linux/string_helpers.h> @@ -11,13 +12,6 @@ #include "adf_gen4_pm.h" #include "icp_qat_fw_init_admin.h" -/* - * This is needed because a variable is used to index the mask at - * pm_scnprint_table(), making it not compile time constant, so the compile - * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled. - */ -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) - #define PM_INFO_MEMBER_OFF(member) \ (offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32)) diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644 --- a/drivers/gpio/gpio-aspeed.c +++ b/drivers/gpio/gpio-aspeed.c @@ -5,6 +5,7 @@ * Joel Stanley <joel@jms.id.au> */ +#include <linux/bitfield.h> #include <linux/clk.h> #include <linux/gpio/aspeed.h> #include <linux/gpio/driver.h> @@ -30,10 +31,6 @@ #include <linux/gpio/consumer.h> #include "gpiolib.h" -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) - #define GPIO_G7_IRQ_STS_BASE 0x100 #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4) #define GPIO_G7_CTRL_REG_BASE 0x180 diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c index 740018d4b3dfb35e..c58dc59d4f570831 100644 --- a/drivers/iio/temperature/mlx90614.c +++ b/drivers/iio/temperature/mlx90614.c @@ -22,6 +22,7 @@ * the "wakeup" GPIO is not given, power management will be disabled. */ +#include <linux/bitfield.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/gpio/consumer.h> @@ -68,10 +69,6 @@ #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */ #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */ -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) - struct mlx_chip_info { /* EEPROM offsets with 16-bit data, MSB first */ /* emissivity correction coefficient */ diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c index 59c4e7c6cddea127..3ba28faa8e1418a9 100644 --- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c +++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c @@ -81,10 +81,6 @@ #define MVOLT_1800 0 #define MVOLT_3300 1 -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */ -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) - static const char * const gpio_group_name[] = { "gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog", "gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion", diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c index 1c98da37b7d18745..917a029d849585cd 100644 --- a/drivers/soc/renesas/rz-sysc.c +++ b/drivers/soc/renesas/rz-sysc.c @@ -5,6 +5,7 @@ * Copyright (C) 2024 Renesas Electronics Corp. */ +#include <linux/bitfield.h> #include <linux/io.h> #include <linux/of.h> #include <linux/platform_device.h> @@ -12,8 +13,6 @@ #include "rz-sysc.h" -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) - /** * struct rz_sysc - RZ SYSC private data structure * @base: SYSC base address diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 63928f1732230700..c62324a9fcc81241 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -203,4 +203,38 @@ __MAKE_OP(64) #undef __MAKE_OP #undef ____MAKE_OP +/** + * field_prep() - prepare a bitfield element + * @_mask: shifted mask defining the field's length and position + * @_val: value to put in the field + * + * field_prep() masks and shifts up the value. The result should be + * combined with other fields of the bitfield using logical OR. + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant. + */ +#define field_prep(_mask, _val) \ + ({ \ + typeof(_mask) __mask = (_mask); \ + unsigned int __shift = sizeof(_mask) <= 4 ? \ + __ffs(__mask) : __ffs64(__mask); \ + (((typeof(_mask))(_val) << __shift) & (__mask)); \ + }) + +/** + * field_get() - extract a bitfield element + * @_mask: shifted mask defining the field's length and position + * @_reg: value of entire bitfield + * + * field_get() extracts the field specified by @_mask from the + * bitfield passed in as @_reg by masking and shifting it down. + * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant. + */ +#define field_get(_mask, _reg) \ + ({ \ + typeof(_mask) __mask = _mask; \ + unsigned int __shift = sizeof(_mask) <= 4 ? \ + __ffs(__mask) : __ffs64(__mask); \ + (typeof(_mask))(((_reg) & (__mask)) >> __shift); \ + }) + #endif diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index 23fcd680167d0298..00ab811e4b11a573 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer) #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask)) #define RME_DIGIFACE_INVERT BIT(31) -/* Nonconst helpers */ -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1)) -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask)) - static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val) { struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
The existing FIELD_{GET,PREP}() macros are limited to compile-time constants. However, it is very common to prepare or extract bitfield elements where the bitfield mask is not a compile-time constant. To avoid this limitation, the AT91 clock driver and several other drivers already have their own non-const field_{prep,get}() macros. Make them available for general use by consolidating them in <linux/bitfield.h>, and improve them slightly: 1. Avoid evaluating macro parameters more than once, 2. Replace "ffs() - 1" by "__ffs()", 3. Support 64-bit use on 32-bit architectures. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v2: - Cast val resp. reg to the mask type, - Fix 64-bit use on 32-bit architectures, - Convert new upstream users: - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c - drivers/gpio/gpio-aspeed.c - drivers/iio/temperature/mlx90614.c - drivers/pinctrl/nuvoton/pinctrl-ma35.c - sound/usb/mixer_quirks.c - Convert new user queued in renesas-devel for v6.15: - drivers/soc/renesas/rz-sysc.c --- drivers/clk/at91/clk-peripheral.c | 1 + drivers/clk/at91/pmc.h | 3 -- .../qat/qat_common/adf_gen4_pm_debugfs.c | 8 +---- drivers/gpio/gpio-aspeed.c | 5 +-- drivers/iio/temperature/mlx90614.c | 5 +-- drivers/pinctrl/nuvoton/pinctrl-ma35.c | 4 --- drivers/soc/renesas/rz-sysc.c | 3 +- include/linux/bitfield.h | 34 +++++++++++++++++++ sound/usb/mixer_quirks.c | 4 --- 9 files changed, 39 insertions(+), 28 deletions(-)