Remove shift/offset macros and instead use the corresponding mask with
FIELD_GET(), FIELD_PREP(), or FIELD_FIT().
In cases where an appropriate mask didn't exist, it was created.
One of the shift/offset macros was used for a convoluted dynamic
bitfield extraction. In its place, a helper function,
adt7316_extract_ad_lsb(), was created so the shift/offset could be
removed.
Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
---
drivers/staging/iio/addac/adt7316.c | 59 ++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 1412808c50c76a68b5771a25c46dd3308c5cbcdb..b8b66f4dd14bb59c3d29fdd569d84f0dd786db9e 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -17,6 +17,7 @@
#include <linux/i2c.h>
#include <linux/rtc.h>
#include <linux/module.h>
+#include <linux/bitfield.h>
#include <linux/iio/iio.h>
#include <linux/iio/events.h>
@@ -31,10 +32,11 @@
#define ADT7316_LSB_IN_TEMP_VDD 0x3
#define ADT7316_LSB_IN_TEMP_MASK 0x3
#define ADT7316_LSB_VDD_MASK 0xC
-#define ADT7316_LSB_VDD_OFFSET 2
#define ADT7316_LSB_EX_TEMP_AIN 0x4
-#define ADT7316_LSB_EX_TEMP_MASK 0x3
-#define ADT7516_LSB_AIN_SHIFT 2
+#define ADT7316_LSB_EX_TEMP_AIN1_MASK GENMASK_U32(1, 0)
+#define ADT7516_LSB_AIN2_MASK GENMASK_U32(3, 2)
+#define ADT7516_LSB_AIN3_MASK GENMASK_U32(5, 4)
+#define ADT7516_LSB_AIN4_MASK GENMASK_U32(7, 6)
#define ADT7316_AD_MSB_DATA_BASE 0x6
#define ADT7316_AD_MSB_DATA_REGS 3
#define ADT7516_AD_MSB_DATA_REGS 6
@@ -46,8 +48,8 @@
#define ADT7516_MSB_AIN3 0xA
#define ADT7516_MSB_AIN4 0xB
#define ADT7316_DA_DATA_BASE 0x10
-#define ADT7316_DA_10_BIT_LSB_SHIFT 6
-#define ADT7316_DA_12_BIT_LSB_SHIFT 4
+#define ADT7316_DA_10_BIT_LSB_MASK GENMASK_U32(7, 6)
+#define ADT7316_DA_12_BIT_LSB_MASK GENMASK_U32(7, 4)
#define ADT7316_DA_MSB_DATA_REGS 4
#define ADT7316_LSB_DAC_A 0x10
#define ADT7316_MSB_DAC_A 0x11
@@ -128,7 +130,6 @@
*/
#define ADT7316_DA_2VREF_CH_MASK 0xF
#define ADT7316_DA_EN_MODE_MASK 0x30
-#define ADT7316_DA_EN_MODE_SHIFT 4
#define ADT7316_DA_EN_MODE_SINGLE 0x00
#define ADT7316_DA_EN_MODE_AB_CD 0x10
#define ADT7316_DA_EN_MODE_ABCD 0x20
@@ -143,7 +144,6 @@
#define ADT7316_DAC_IN_VREF 0x10
#define ADT7516_DAC_AB_IN_VREF 0x10
#define ADT7516_DAC_CD_IN_VREF 0x20
-#define ADT7516_DAC_IN_VREF_OFFSET 4
#define ADT7516_DAC_IN_VREF_MASK 0x30
/*
@@ -155,7 +155,7 @@
* ADT7316 value masks
*/
#define ADT7316_VALUE_MASK 0xfff
-#define ADT7316_T_VALUE_FLOAT_OFFSET 2
+#define ADT7316_AD_MSB_MASK GENMASK_U32(9, 2)
/*
* ADT7316 hardware constants
@@ -873,11 +873,11 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev,
return -EPERM;
ret = kstrtou8(buf, 10, &data);
- if (ret || data > (ADT7316_DA_EN_MODE_MASK >> ADT7316_DA_EN_MODE_SHIFT))
+ if (ret || !FIELD_FIT(ADT7316_DA_EN_MODE_MASK, data))
return -EINVAL;
dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK);
- dac_config |= data << ADT7316_DA_EN_MODE_SHIFT;
+ dac_config |= FIELD_PREP(ADT7316_DA_EN_MODE_MASK, data);
ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
if (ret)
@@ -1038,8 +1038,7 @@ static ssize_t adt7316_show_DAC_internal_Vref(struct device *dev,
if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
return sysfs_emit(buf, "0x%x\n",
- (chip->ldac_config & ADT7516_DAC_IN_VREF_MASK) >>
- ADT7516_DAC_IN_VREF_OFFSET);
+ FIELD_GET(ADT7516_DAC_IN_VREF_MASK, chip->ldac_config));
return sysfs_emit(buf, "%d\n",
!!(chip->ldac_config & ADT7316_DAC_IN_VREF));
}
@@ -1090,6 +1089,22 @@ static IIO_DEVICE_ATTR(DAC_internal_Vref, 0644,
adt7316_store_DAC_internal_Vref,
0);
+static u8 adt7316_extract_ad_lsb(u8 lsb, int channel)
+{
+ switch (channel) {
+ case ADT7316_AD_SINGLE_CH_EX:
+ return FIELD_GET(ADT7316_LSB_EX_TEMP_AIN1_MASK, lsb);
+ case ADT7516_AD_SINGLE_CH_AIN2:
+ return FIELD_GET(ADT7516_LSB_AIN2_MASK, lsb);
+ case ADT7516_AD_SINGLE_CH_AIN3:
+ return FIELD_GET(ADT7516_LSB_AIN3_MASK, lsb);
+ case ADT7516_AD_SINGLE_CH_AIN4:
+ return FIELD_GET(ADT7516_LSB_AIN4_MASK, lsb);
+ default:
+ return 0;
+ }
+}
+
static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
int channel, char *buf)
{
@@ -1113,7 +1128,7 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
if (ret)
return -EIO;
- data = msb << ADT7316_T_VALUE_FLOAT_OFFSET;
+ data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb);
data |= lsb & ADT7316_LSB_IN_TEMP_MASK;
break;
case ADT7316_AD_SINGLE_CH_VDD:
@@ -1128,8 +1143,8 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
if (ret)
return -EIO;
- data = msb << ADT7316_T_VALUE_FLOAT_OFFSET;
- data |= (lsb & ADT7316_LSB_VDD_MASK) >> ADT7316_LSB_VDD_OFFSET;
+ data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb);
+ data |= FIELD_GET(ADT7316_LSB_VDD_MASK, lsb);
return sysfs_emit(buf, "%d\n", data);
default: /* ex_temp and ain */
ret = chip->bus.read(chip->bus.client,
@@ -1142,10 +1157,8 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
if (ret)
return -EIO;
- data = msb << ADT7316_T_VALUE_FLOAT_OFFSET;
- data |= lsb & (ADT7316_LSB_EX_TEMP_MASK <<
- (ADT7516_LSB_AIN_SHIFT * (channel -
- (ADT7316_MSB_EX_TEMP - ADT7316_AD_MSB_DATA_BASE))));
+ data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb);
+ data |= adt7316_extract_ad_lsb(lsb, channel);
if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
return sysfs_emit(buf, "%d\n", data);
@@ -1410,9 +1423,9 @@ static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
return -EIO;
if (chip->dac_bits == 12)
- data = lsb >> ADT7316_DA_12_BIT_LSB_SHIFT;
+ data = FIELD_GET(ADT7316_DA_12_BIT_LSB_MASK, lsb);
else if (chip->dac_bits == 10)
- data = lsb >> ADT7316_DA_10_BIT_LSB_SHIFT;
+ data = FIELD_GET(ADT7316_DA_10_BIT_LSB_MASK, lsb);
data |= msb << offset;
return sysfs_emit(buf, "%d\n", data);
@@ -1441,9 +1454,9 @@ static ssize_t adt7316_store_DAC(struct adt7316_chip_info *chip,
if (chip->dac_bits > 8) {
lsb = data & ((1 << offset) - 1);
if (chip->dac_bits == 12)
- lsb_reg = lsb << ADT7316_DA_12_BIT_LSB_SHIFT;
+ lsb_reg = FIELD_PREP(ADT7316_DA_12_BIT_LSB_MASK, lsb);
else
- lsb_reg = lsb << ADT7316_DA_10_BIT_LSB_SHIFT;
+ lsb_reg = FIELD_PREP(ADT7316_DA_10_BIT_LSB_MASK, lsb);
ret = chip->bus.write(chip->bus.client,
ADT7316_DA_DATA_BASE + channel * 2, lsb_reg);
if (ret)
--
2.53.0On Thu, 05 Mar 2026 23:16:59 -0800 Michael Harris <michaelharriscode@gmail.com> wrote: > Remove shift/offset macros and instead use the corresponding mask with > FIELD_GET(), FIELD_PREP(), or FIELD_FIT(). > > In cases where an appropriate mask didn't exist, it was created. > > One of the shift/offset macros was used for a convoluted dynamic > bitfield extraction. In its place, a helper function, > adt7316_extract_ad_lsb(), was created so the shift/offset could be > removed. > > Signed-off-by: Michael Harris <michaelharriscode@gmail.com> > --- > drivers/staging/iio/addac/adt7316.c | 59 ++++++++++++++++++++++--------------- > 1 file changed, 36 insertions(+), 23 deletions(-) > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c > index 1412808c50c76a68b5771a25c46dd3308c5cbcdb..b8b66f4dd14bb59c3d29fdd569d84f0dd786db9e 100644 > --- a/drivers/staging/iio/addac/adt7316.c > +++ b/drivers/staging/iio/addac/adt7316.c > @@ -17,6 +17,7 @@ > #include <linux/i2c.h> > #include <linux/rtc.h> > #include <linux/module.h> > +#include <linux/bitfield.h> > > #include <linux/iio/iio.h> > #include <linux/iio/events.h> > @@ -31,10 +32,11 @@ > #define ADT7316_LSB_IN_TEMP_VDD 0x3 > #define ADT7316_LSB_IN_TEMP_MASK 0x3 > #define ADT7316_LSB_VDD_MASK 0xC Convert all masks to GENMASK_U32() rather than just the ones where you are removing a shift. That will give us more consistent code and makes sense as part of this patch. > -#define ADT7316_LSB_VDD_OFFSET 2 > #define ADT7316_LSB_EX_TEMP_AIN 0x4 > -#define ADT7316_LSB_EX_TEMP_MASK 0x3 > -#define ADT7516_LSB_AIN_SHIFT 2 > +#define ADT7316_LSB_EX_TEMP_AIN1_MASK GENMASK_U32(1, 0) Why GENMASK_U32()? The registers seem to be 8 bit. I'm not sure we care that much about the extra checks the sized variant brings but if we do want to use it use the U8() variant. Jonathan
On Thu, Mar 05, 2026 at 11:16:59PM -0800, Michael Harris wrote: > Remove shift/offset macros and instead use the corresponding mask with > FIELD_GET(), FIELD_PREP(), or FIELD_FIT(). > > In cases where an appropriate mask didn't exist, it was created. > > One of the shift/offset macros was used for a convoluted dynamic > bitfield extraction. In its place, a helper function, > adt7316_extract_ad_lsb(), was created so the shift/offset could be > removed. ... > #include <linux/i2c.h> > #include <linux/rtc.h> > #include <linux/module.h> > +#include <linux/bitfield.h> Try to squeeze it to make a longest (but sparse AFAICS) ordered list of inclusions. With given context #include <linux/bitfield.h> #include <linux/i2c.h> #include <linux/rtc.h> #include <linux/module.h> gives 3 out of 4 in order. ... > dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK); > - dac_config |= data << ADT7316_DA_EN_MODE_SHIFT; > + dac_config |= FIELD_PREP(ADT7316_DA_EN_MODE_MASK, data); FIELD_MODIFY() ? ... > - data = msb << ADT7316_T_VALUE_FLOAT_OFFSET; > + data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb); > data |= lsb & ADT7316_LSB_IN_TEMP_MASK; Ditto and so on... -- With Best Regards, Andy Shevchenko
On 3/6/26 6:53 AM, Andy Shevchenko wrote: Hi Andy, thanks for reviewing my patch series. >> #include <linux/i2c.h> >> #include <linux/rtc.h> >> #include <linux/module.h> >> +#include <linux/bitfield.h> > > Try to squeeze it to make a longest (but sparse AFAICS) ordered list of > inclusions. With given context > > #include <linux/bitfield.h> > #include <linux/i2c.h> > #include <linux/rtc.h> > #include <linux/module.h> > > gives 3 out of 4 in order. Placing it there would put it right below slab.h, sysfs.h, and list.h. The includes as a whole are fairly messy and unorganized and I don't think there's a clean area where I could insert it. Fully reordering it would be out of scope for this patch series. If you prefer, I can put it at the top of the includes since it would be the highest alphabetically. >> dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK); >> - dac_config |= data << ADT7316_DA_EN_MODE_SHIFT; >> + dac_config |= FIELD_PREP(ADT7316_DA_EN_MODE_MASK, data); > >FIELD_MODIFY() ? > I will apply this. > >> - data = msb << ADT7316_T_VALUE_FLOAT_OFFSET; >> + data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb); >> data |= lsb & ADT7316_LSB_IN_TEMP_MASK; > > Ditto and so on... > The main purpose of this patch was to delete the offset/shift macros, so I only applied the bitfield macros to areas that were using those offsets or shifts. I left that existing bitwise operation there because it wasn't affected by the offsets or shifts. If you'd prefer, I can update it in this case for the sake of symmetry. Thanks, Michael Harris
On Mon, Mar 09, 2026 at 06:32:36PM -0700, Michael Harris wrote: > On 3/6/26 6:53 AM, Andy Shevchenko wrote: ... > >> #include <linux/i2c.h> > >> #include <linux/rtc.h> > >> #include <linux/module.h> > >> +#include <linux/bitfield.h> > > > > Try to squeeze it to make a longest (but sparse AFAICS) ordered list of > > inclusions. With given context > > > > #include <linux/bitfield.h> > > #include <linux/i2c.h> > > #include <linux/rtc.h> > > #include <linux/module.h> > > > > gives 3 out of 4 in order. > > Placing it there would put it right below slab.h, sysfs.h, and list.h. > The includes as a whole are fairly messy and unorganized and I don't > think there's a clean area where I could insert it. Fully reordering it > would be out of scope for this patch series. If you prefer, I can put it > at the top of the includes since it would be the highest alphabetically. As I pointed out "with the given context", meaning that you might find better place. The idea is to have the longest ordered chain even if it's interrupted by some unordered items. ... > >> - data = msb << ADT7316_T_VALUE_FLOAT_OFFSET; > >> + data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb); > >> data |= lsb & ADT7316_LSB_IN_TEMP_MASK; > > > > Ditto and so on... > > The main purpose of this patch was to delete the offset/shift macros, > so I only applied the bitfield macros to areas that were using those > offsets or shifts. I left that existing bitwise operation there because > it wasn't affected by the offsets or shifts. If you'd prefer, I can > update it in this case for the sake of symmetry. The idea is to be consistent. If we change to bitfield,h somewhere else, it probably better to cover the whole driver at the same time. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.