[PATCH v2 0/4] correct macro issues

[PATCH v2 0/4] correct macro issues

[Michael Harris]

This patch series fixes various issues with the macros definitions, such
as:
- Config macro names that don't indicate which config it's for
- Magic numbers
- Mask values being before the mask itself

Additionally, some macros are outright removed due to having better
modern standards:
- Offset/shift macros are replaced with the use of FIELD_GET(),
  FIELD_PREP(), or FIELD_FIT()
- Macros used for custom sign manipulation are replaced with
  sign_extend32()

---
Changes in v2:
- Delete "remove unused macros" patch
- Add a patch that fixes manual temperature calculation and deletes the
  associated macros
- Add a patch that removes offset/shift macros
- Add a patch that adds the associated config to
  macro names and adjusts ordering so that mask values are below the
  mask itself
- Use GENMASK_U32() and BIT_U32() instead of GENMASK() and BIT(), to
  prevent sysfs_emit() from expecting unsigned long (thanks to David
  Laight for this suggestion)
- Corrected macros that were incorrectly using BIT() or GENMASK()
- Change the cover letter description to better fit all the new patches
- Link to v1: https://lore.kernel.org/r/20260130-adt7316-correct-macros-v1-0-8a71df1e42f1@gmail.com

---
Michael Harris (4):
      staging: iio: adt7316: refactor temperature calculation logic
      staging: iio: adt7316: remove shift/offset macros
      staging: iio: adt7316: add config names to registers and reorder
      staging: iio: adt7316: convert magic numbers to BIT_U32() or GENMASK_U32()

 drivers/staging/iio/addac/adt7316.c | 384 +++++++++++++++++++-----------------
 1 file changed, 203 insertions(+), 181 deletions(-)
---
base-commit: 5d2905f2c6acdd1c553f9b022dee730ab2cced71
change-id: 20260130-adt7316-correct-macros-b6801b4ae1b1

Best regards,
-- 
Michael Harris <michaelharriscode@gmail.com>

[PATCH v2 1/4] staging: iio: adt7316: refactor temperature calculation logic

[Michael Harris]

Replace the manual sign manipulation with sign_extend32() and change the
affected variable from u16 to s32 to properly handle negative values.

Resolve a logic error where the sign bit was being checked at bit 10
instead of bit 9 for a 10-bit value.

Convert the data variable to be in centidegrees celsius (0.25 C per bit)
so we can use simple division and modulo for sysfs_emit() instead of the
convoluted bit shifting and masking.

Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 8a9a8262c2bec34f3c3e79d8174f492b9a23fb70..1412808c50c76a68b5771a25c46dd3308c5cbcdb 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -155,9 +155,12 @@
  * ADT7316 value masks
  */
 #define ADT7316_VALUE_MASK		0xfff
-#define ADT7316_T_VALUE_SIGN		0x400
 #define ADT7316_T_VALUE_FLOAT_OFFSET	2
-#define ADT7316_T_VALUE_FLOAT_MASK	0x2
+
+/*
+ * ADT7316 hardware constants
+ */
+#define ADT7316_TEMP_CENTIDEG_PER_BIT	25
 
 /*
  * Chip ID
@@ -1090,9 +1093,8 @@ static IIO_DEVICE_ATTR(DAC_internal_Vref, 0644,
 static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
 			       int channel, char *buf)
 {
-	u16 data;
+	s32 data;
 	u8 msb, lsb;
-	char sign = ' ';
 	int ret;
 
 	if ((chip->config2 & ADT7316_AD_SINGLE_CH_MODE) &&
@@ -1151,15 +1153,13 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
 		break;
 	}
 
-	if (data & ADT7316_T_VALUE_SIGN) {
-		/* convert supplement to positive value */
-		data = (ADT7316_T_VALUE_SIGN << 1) - data;
-		sign = '-';
-	}
+	data = sign_extend32(data, 9);
+	data *= ADT7316_TEMP_CENTIDEG_PER_BIT;
 
-	return sysfs_emit(buf, "%c%d.%.2d\n", sign,
-		(data >> ADT7316_T_VALUE_FLOAT_OFFSET),
-		(data & ADT7316_T_VALUE_FLOAT_MASK) * 25);
+	return sysfs_emit(buf, "%s%d.%02u\n",
+			  (data < 0 ? "-" : ""),
+			  abs(data / 100),
+			  abs(data % 100));
 }
 
 static ssize_t adt7316_show_VDD(struct device *dev,

-- 
2.53.0

Re: [PATCH v2 1/4] staging: iio: adt7316: refactor temperature calculation logic

[Andy Shevchenko]

On Thu, Mar 05, 2026 at 11:16:58PM -0800, Michael Harris wrote:
> Replace the manual sign manipulation with sign_extend32() and change the
> affected variable from u16 to s32 to properly handle negative values.
> 
> Resolve a logic error where the sign bit was being checked at bit 10
> instead of bit 9 for a 10-bit value.
> 
> Convert the data variable to be in centidegrees celsius (0.25 C per bit)
> so we can use simple division and modulo for sysfs_emit() instead of the
> convoluted bit shifting and masking.

Sounds like a candidate for Fixes tag.

...

> -	return sysfs_emit(buf, "%c%d.%.2d\n", sign,
> -		(data >> ADT7316_T_VALUE_FLOAT_OFFSET),
> -		(data & ADT7316_T_VALUE_FLOAT_MASK) * 25);
> +	return sysfs_emit(buf, "%s%d.%02u\n",
> +			  (data < 0 ? "-" : ""),
> +			  abs(data / 100),
> +			  abs(data % 100));

Hmm... If you create a temporary variable for abs(data), this might give
slightly different code generation, presumably better on (some) architectures.
/ % combined maybe a single assembly instruction.

Or keep sign in a char as it was in the original code and just move data to be
abs(data).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] staging: iio: adt7316: refactor temperature calculation logic

[Jonathan Cameron]

On Thu, 05 Mar 2026 23:16:58 -0800
Michael Harris <michaelharriscode@gmail.com> wrote:

> Replace the manual sign manipulation with sign_extend32() and change the
> affected variable from u16 to s32 to properly handle negative values.
> 
> Resolve a logic error where the sign bit was being checked at bit 10
> instead of bit 9 for a 10-bit value.
> 
> Convert the data variable to be in centidegrees celsius (0.25 C per bit)
> so we can use simple division and modulo for sysfs_emit() instead of the
> convoluted bit shifting and masking.
> 
> Signed-off-by: Michael Harris <michaelharriscode@gmail.com>

As Andy noted needs a fixes tag.

> ---
>  drivers/staging/iio/addac/adt7316.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 8a9a8262c2bec34f3c3e79d8174f492b9a23fb70..1412808c50c76a68b5771a25c46dd3308c5cbcdb 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -155,9 +155,12 @@
>   * ADT7316 value masks
>   */
>  #define ADT7316_VALUE_MASK		0xfff
> -#define ADT7316_T_VALUE_SIGN		0x400
>  #define ADT7316_T_VALUE_FLOAT_OFFSET	2
> -#define ADT7316_T_VALUE_FLOAT_MASK	0x2
> +
> +/*
> + * ADT7316 hardware constants
> + */
> +#define ADT7316_TEMP_CENTIDEG_PER_BIT	25

As it's only used in one place, I'd think just use the numeric value down there
and if you feel it needs more background, add a comment there.

[PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Michael Harris]

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.0

Re: [PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Andy Shevchenko]

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

Re: [PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Michael Harris]

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

Re: [PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Andy Shevchenko]

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

Re: [PATCH v2 2/4] staging: iio: adt7316: remove shift/offset macros

[Jonathan Cameron]

On 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

[PATCH v2 3/4] staging: iio: adt7316: add config names to registers and reorder

[Michael Harris]

Add config names to macros to make it more clear which register they're
affecting. Also renamed ADT7316_EN to further clarify what
it's enabling.

Some macros were reordered, so that mask values were below the actual
mask.

Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 299 +++++++++++++++++++-----------------
 1 file changed, 154 insertions(+), 145 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index b8b66f4dd14bb59c3d29fdd569d84f0dd786db9e..ea7d97bb0f378f3a3ce1225d8d13af2c5955ca56 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -90,61 +90,61 @@
 /*
  * ADT7316 config1
  */
-#define ADT7316_EN			0x1
-#define ADT7516_SEL_EX_TEMP		0x4
-#define ADT7516_SEL_AIN1_2_EX_TEMP_MASK	0x6
-#define ADT7516_SEL_AIN3		0x8
-#define ADT7316_INT_EN			0x20
-#define ADT7316_INT_POLARITY		0x40
-#define ADT7316_PD			0x80
+#define ADT7316_CONFIG1_MONITOR_EN		0x1
+#define ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK	0x6
+#define ADT7516_CONFIG1_SEL_EX_TEMP		0x4
+#define ADT7516_CONFIG1_SEL_AIN3		0x8
+#define ADT7316_CONFIG1_INT_EN			0x20
+#define ADT7316_CONFIG1_INT_POLARITY		0x40
+#define ADT7316_CONFIG1_PD			0x80
 
 /*
  * ADT7316 config2
  */
-#define ADT7316_AD_SINGLE_CH_MASK	0x3
-#define ADT7516_AD_SINGLE_CH_MASK	0x7
-#define ADT7316_AD_SINGLE_CH_VDD	0
-#define ADT7316_AD_SINGLE_CH_IN		1
-#define ADT7316_AD_SINGLE_CH_EX		2
-#define ADT7516_AD_SINGLE_CH_AIN1	2
-#define ADT7516_AD_SINGLE_CH_AIN2	3
-#define ADT7516_AD_SINGLE_CH_AIN3	4
-#define ADT7516_AD_SINGLE_CH_AIN4	5
-#define ADT7316_AD_SINGLE_CH_MODE	0x10
-#define ADT7316_DISABLE_AVERAGING	0x20
-#define ADT7316_EN_SMBUS_TIMEOUT	0x40
-#define ADT7316_RESET			0x80
+#define ADT7316_CONFIG2_AD_SINGLE_CH_MASK	0x3
+#define ADT7516_CONFIG2_AD_SINGLE_CH_MASK	0x7
+#define ADT7316_CONFIG2_AD_SINGLE_CH_VDD	0
+#define ADT7316_CONFIG2_AD_SINGLE_CH_IN		1
+#define ADT7316_CONFIG2_AD_SINGLE_CH_EX		2
+#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN1	2
+#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN2	3
+#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN3	4
+#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN4	5
+#define ADT7316_CONFIG2_AD_SINGLE_CH_MODE	0x10
+#define ADT7316_CONFIG2_DISABLE_AVERAGING	0x20
+#define ADT7316_CONFIG2_EN_SMBUS_TIMEOUT	0x40
+#define ADT7316_CONFIG2_RESET			0x80
 
 /*
  * ADT7316 config3
  */
-#define ADT7316_ADCLK_22_5		0x1
-#define ADT7316_DA_HIGH_RESOLUTION	0x2
-#define ADT7316_DA_EN_VIA_DAC_LDAC	0x8
-#define ADT7516_AIN_IN_VREF		0x10
-#define ADT7316_EN_IN_TEMP_PROP_DACA	0x20
-#define ADT7316_EN_EX_TEMP_PROP_DACB	0x40
+#define ADT7316_CONFIG3_ADCLK_22_5		0x1
+#define ADT7316_CONFIG3_DA_HIGH_RESOLUTION	0x2
+#define ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC	0x8
+#define ADT7516_CONFIG3_AIN_IN_VREF		0x10
+#define ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA	0x20
+#define ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB	0x40
 
 /*
  * ADT7316 DAC config
  */
-#define ADT7316_DA_2VREF_CH_MASK	0xF
-#define ADT7316_DA_EN_MODE_MASK		0x30
-#define ADT7316_DA_EN_MODE_SINGLE	0x00
-#define ADT7316_DA_EN_MODE_AB_CD	0x10
-#define ADT7316_DA_EN_MODE_ABCD		0x20
-#define ADT7316_DA_EN_MODE_LDAC		0x30
-#define ADT7316_VREF_BYPASS_DAC_AB	0x40
-#define ADT7316_VREF_BYPASS_DAC_CD	0x80
+#define ADT7316_DAC_CONFIG_2VREF_CH_MASK	0xF
+#define ADT7316_DAC_CONFIG_EN_MODE_MASK		0x30
+#define ADT7316_DAC_CONFIG_EN_MODE_SINGLE	0x00
+#define ADT7316_DAC_CONFIG_EN_MODE_AB_CD	0x10
+#define ADT7316_DAC_CONFIG_EN_MODE_ABCD		0x20
+#define ADT7316_DAC_CONFIG_EN_MODE_LDAC		0x30
+#define ADT7316_DAC_CONFIG_VREF_BYPASS_AB	0x40
+#define ADT7316_DAC_CONFIG_VREF_BYPASS_CD	0x80
 
 /*
  * ADT7316 LDAC config
  */
-#define ADT7316_LDAC_EN_DA_MASK		0xF
-#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_MASK	0x30
+#define ADT7316_LDAC_CONFIG_EN_DA_MASK		0xF
+#define ADT7316_LDAC_CONFIG_DAC_IN_VREF		0x10
+#define ADT7516_LDAC_CONFIG_DAC_AB_IN_VREF	0x10
+#define ADT7516_LDAC_CONFIG_DAC_CD_IN_VREF	0x20
+#define ADT7516_LDAC_CONFIG_DAC_IN_VREF_MASK	0x30
 
 /*
  * ADT7316 INT_MASK2
@@ -219,7 +219,8 @@ static ssize_t adt7316_show_enabled(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return sysfs_emit(buf, "%d\n", !!(chip->config1 & ADT7316_EN));
+	return sysfs_emit(buf, "%d\n",
+			  !!(chip->config1 & ADT7316_CONFIG1_MONITOR_EN));
 }
 
 static ssize_t _adt7316_store_enabled(struct adt7316_chip_info *chip,
@@ -229,9 +230,9 @@ static ssize_t _adt7316_store_enabled(struct adt7316_chip_info *chip,
 	int ret;
 
 	if (enable)
-		config1 = chip->config1 | ADT7316_EN;
+		config1 = chip->config1 | ADT7316_CONFIG1_MONITOR_EN;
 	else
-		config1 = chip->config1 & ~ADT7316_EN;
+		config1 = chip->config1 & ~ADT7316_CONFIG1_MONITOR_EN;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
 	if (ret)
@@ -277,7 +278,8 @@ static ssize_t adt7316_show_select_ex_temp(struct device *dev,
 	if ((chip->id & ID_FAMILY_MASK) != ID_ADT75XX)
 		return -EPERM;
 
-	return sysfs_emit(buf, "%d\n", !!(chip->config1 & ADT7516_SEL_EX_TEMP));
+	return sysfs_emit(buf, "%d\n",
+			  !!(chip->config1 & ADT7516_CONFIG1_SEL_EX_TEMP));
 }
 
 static ssize_t adt7316_store_select_ex_temp(struct device *dev,
@@ -293,9 +295,9 @@ static ssize_t adt7316_store_select_ex_temp(struct device *dev,
 	if ((chip->id & ID_FAMILY_MASK) != ID_ADT75XX)
 		return -EPERM;
 
-	config1 = chip->config1 & (~ADT7516_SEL_EX_TEMP);
+	config1 = chip->config1 & (~ADT7516_CONFIG1_SEL_EX_TEMP);
 	if (buf[0] == '1')
-		config1 |= ADT7516_SEL_EX_TEMP;
+		config1 |= ADT7516_CONFIG1_SEL_EX_TEMP;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
 	if (ret)
@@ -318,7 +320,7 @@ static ssize_t adt7316_show_mode(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (chip->config2 & ADT7316_AD_SINGLE_CH_MODE)
+	if (chip->config2 & ADT7316_CONFIG2_AD_SINGLE_CH_MODE)
 		return sysfs_emit(buf, "single_channel\n");
 
 	return sysfs_emit(buf, "round_robin\n");
@@ -334,9 +336,9 @@ static ssize_t adt7316_store_mode(struct device *dev,
 	u8 config2;
 	int ret;
 
-	config2 = chip->config2 & (~ADT7316_AD_SINGLE_CH_MODE);
+	config2 = chip->config2 & (~ADT7316_CONFIG2_AD_SINGLE_CH_MODE);
 	if (!memcmp(buf, "single_channel", 14))
-		config2 |= ADT7316_AD_SINGLE_CH_MODE;
+		config2 |= ADT7316_CONFIG2_AD_SINGLE_CH_MODE;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
 	if (ret)
@@ -368,31 +370,31 @@ static ssize_t adt7316_show_ad_channel(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (!(chip->config2 & ADT7316_AD_SINGLE_CH_MODE))
+	if (!(chip->config2 & ADT7316_CONFIG2_AD_SINGLE_CH_MODE))
 		return -EPERM;
 
-	switch (chip->config2 & ADT7516_AD_SINGLE_CH_MASK) {
-	case ADT7316_AD_SINGLE_CH_VDD:
+	switch (chip->config2 & ADT7516_CONFIG2_AD_SINGLE_CH_MASK) {
+	case ADT7316_CONFIG2_AD_SINGLE_CH_VDD:
 		return sysfs_emit(buf, "0 - VDD\n");
-	case ADT7316_AD_SINGLE_CH_IN:
+	case ADT7316_CONFIG2_AD_SINGLE_CH_IN:
 		return sysfs_emit(buf, "1 - Internal Temperature\n");
-	case ADT7316_AD_SINGLE_CH_EX:
+	case ADT7316_CONFIG2_AD_SINGLE_CH_EX:
 		if (((chip->id & ID_FAMILY_MASK) == ID_ADT75XX) &&
-		    (chip->config1 & ADT7516_SEL_AIN1_2_EX_TEMP_MASK) == 0)
+		    (chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
 			return sysfs_emit(buf, "2 - AIN1\n");
 
 		return sysfs_emit(buf, "2 - External Temperature\n");
-	case ADT7516_AD_SINGLE_CH_AIN2:
-		if ((chip->config1 & ADT7516_SEL_AIN1_2_EX_TEMP_MASK) == 0)
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN2:
+		if ((chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
 			return sysfs_emit(buf, "3 - AIN2\n");
 
 		return sysfs_emit(buf, "N/A\n");
-	case ADT7516_AD_SINGLE_CH_AIN3:
-		if (chip->config1 & ADT7516_SEL_AIN3)
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN3:
+		if (chip->config1 & ADT7516_CONFIG1_SEL_AIN3)
 			return sysfs_emit(buf, "4 - AIN3\n");
 
 		return sysfs_emit(buf, "N/A\n");
-	case ADT7516_AD_SINGLE_CH_AIN4:
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN4:
 		return sysfs_emit(buf, "5 - AIN4\n");
 	default:
 		return sysfs_emit(buf, "N/A\n");
@@ -410,7 +412,7 @@ static ssize_t adt7316_store_ad_channel(struct device *dev,
 	u8 data;
 	int ret;
 
-	if (!(chip->config2 & ADT7316_AD_SINGLE_CH_MODE))
+	if (!(chip->config2 & ADT7316_CONFIG2_AD_SINGLE_CH_MODE))
 		return -EPERM;
 
 	ret = kstrtou8(buf, 10, &data);
@@ -421,12 +423,12 @@ static ssize_t adt7316_store_ad_channel(struct device *dev,
 		if (data > 5)
 			return -EINVAL;
 
-		config2 = chip->config2 & (~ADT7516_AD_SINGLE_CH_MASK);
+		config2 = chip->config2 & (~ADT7516_CONFIG2_AD_SINGLE_CH_MASK);
 	} else {
 		if (data > 2)
 			return -EINVAL;
 
-		config2 = chip->config2 & (~ADT7316_AD_SINGLE_CH_MASK);
+		config2 = chip->config2 & (~ADT7316_CONFIG2_AD_SINGLE_CH_MASK);
 	}
 
 	config2 |= data;
@@ -452,7 +454,7 @@ static ssize_t adt7316_show_all_ad_channels(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (!(chip->config2 & ADT7316_AD_SINGLE_CH_MODE))
+	if (!(chip->config2 & ADT7316_CONFIG2_AD_SINGLE_CH_MODE))
 		return -EPERM;
 
 	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
@@ -474,7 +476,7 @@ static ssize_t adt7316_show_disable_averaging(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->config2 & ADT7316_DISABLE_AVERAGING));
+		!!(chip->config2 & ADT7316_CONFIG2_DISABLE_AVERAGING));
 }
 
 static ssize_t adt7316_store_disable_averaging(struct device *dev,
@@ -487,9 +489,9 @@ static ssize_t adt7316_store_disable_averaging(struct device *dev,
 	u8 config2;
 	int ret;
 
-	config2 = chip->config2 & (~ADT7316_DISABLE_AVERAGING);
+	config2 = chip->config2 & (~ADT7316_CONFIG2_DISABLE_AVERAGING);
 	if (buf[0] == '1')
-		config2 |= ADT7316_DISABLE_AVERAGING;
+		config2 |= ADT7316_CONFIG2_DISABLE_AVERAGING;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
 	if (ret)
@@ -513,7 +515,7 @@ static ssize_t adt7316_show_enable_smbus_timeout(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->config2 & ADT7316_EN_SMBUS_TIMEOUT));
+		!!(chip->config2 & ADT7316_CONFIG2_EN_SMBUS_TIMEOUT));
 }
 
 static ssize_t adt7316_store_enable_smbus_timeout(struct device *dev,
@@ -526,9 +528,9 @@ static ssize_t adt7316_store_enable_smbus_timeout(struct device *dev,
 	u8 config2;
 	int ret;
 
-	config2 = chip->config2 & (~ADT7316_EN_SMBUS_TIMEOUT);
+	config2 = chip->config2 & (~ADT7316_CONFIG2_EN_SMBUS_TIMEOUT);
 	if (buf[0] == '1')
-		config2 |= ADT7316_EN_SMBUS_TIMEOUT;
+		config2 |= ADT7316_CONFIG2_EN_SMBUS_TIMEOUT;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG2, config2);
 	if (ret)
@@ -551,7 +553,7 @@ static ssize_t adt7316_show_powerdown(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return sysfs_emit(buf, "%d\n", !!(chip->config1 & ADT7316_PD));
+	return sysfs_emit(buf, "%d\n", !!(chip->config1 & ADT7316_CONFIG1_PD));
 }
 
 static ssize_t adt7316_store_powerdown(struct device *dev,
@@ -564,9 +566,9 @@ static ssize_t adt7316_store_powerdown(struct device *dev,
 	u8 config1;
 	int ret;
 
-	config1 = chip->config1 & (~ADT7316_PD);
+	config1 = chip->config1 & (~ADT7316_CONFIG1_PD);
 	if (buf[0] == '1')
-		config1 |= ADT7316_PD;
+		config1 |= ADT7316_CONFIG1_PD;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
 	if (ret)
@@ -589,7 +591,8 @@ static ssize_t adt7316_show_fast_ad_clock(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return sysfs_emit(buf, "%d\n", !!(chip->config3 & ADT7316_ADCLK_22_5));
+	return sysfs_emit(buf, "%d\n",
+			  !!(chip->config3 & ADT7316_CONFIG3_ADCLK_22_5));
 }
 
 static ssize_t adt7316_store_fast_ad_clock(struct device *dev,
@@ -602,9 +605,9 @@ static ssize_t adt7316_store_fast_ad_clock(struct device *dev,
 	u8 config3;
 	int ret;
 
-	config3 = chip->config3 & (~ADT7316_ADCLK_22_5);
+	config3 = chip->config3 & (~ADT7316_CONFIG3_ADCLK_22_5);
 	if (buf[0] == '1')
-		config3 |= ADT7316_ADCLK_22_5;
+		config3 |= ADT7316_CONFIG3_ADCLK_22_5;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
 	if (ret)
@@ -627,7 +630,7 @@ static ssize_t adt7316_show_da_high_resolution(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (chip->config3 & ADT7316_DA_HIGH_RESOLUTION) {
+	if (chip->config3 & ADT7316_CONFIG3_DA_HIGH_RESOLUTION) {
 		if (chip->id != ID_ADT7318 && chip->id != ID_ADT7519)
 			return sysfs_emit(buf, "1 (10 bits)\n");
 	}
@@ -648,9 +651,9 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
 	if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519)
 		return -EPERM;
 
-	config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
+	config3 = chip->config3 & (~ADT7316_CONFIG3_DA_HIGH_RESOLUTION);
 	if (buf[0] == '1')
-		config3 |= ADT7316_DA_HIGH_RESOLUTION;
+		config3 |= ADT7316_CONFIG3_DA_HIGH_RESOLUTION;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
 	if (ret)
@@ -677,7 +680,7 @@ static ssize_t adt7316_show_AIN_internal_Vref(struct device *dev,
 		return -EPERM;
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->config3 & ADT7516_AIN_IN_VREF));
+		!!(chip->config3 & ADT7516_CONFIG3_AIN_IN_VREF));
 }
 
 static ssize_t adt7316_store_AIN_internal_Vref(struct device *dev,
@@ -694,9 +697,9 @@ static ssize_t adt7316_store_AIN_internal_Vref(struct device *dev,
 		return -EPERM;
 
 	if (buf[0] != '1')
-		config3 = chip->config3 & (~ADT7516_AIN_IN_VREF);
+		config3 = chip->config3 & (~ADT7516_CONFIG3_AIN_IN_VREF);
 	else
-		config3 = chip->config3 | ADT7516_AIN_IN_VREF;
+		config3 = chip->config3 | ADT7516_CONFIG3_AIN_IN_VREF;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
 	if (ret)
@@ -720,7 +723,7 @@ static ssize_t adt7316_show_enable_prop_DACA(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->config3 & ADT7316_EN_IN_TEMP_PROP_DACA));
+		!!(chip->config3 & ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA));
 }
 
 static ssize_t adt7316_store_enable_prop_DACA(struct device *dev,
@@ -733,9 +736,9 @@ static ssize_t adt7316_store_enable_prop_DACA(struct device *dev,
 	u8 config3;
 	int ret;
 
-	config3 = chip->config3 & (~ADT7316_EN_IN_TEMP_PROP_DACA);
+	config3 = chip->config3 & (~ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA);
 	if (buf[0] == '1')
-		config3 |= ADT7316_EN_IN_TEMP_PROP_DACA;
+		config3 |= ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
 	if (ret)
@@ -759,7 +762,7 @@ static ssize_t adt7316_show_enable_prop_DACB(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->config3 & ADT7316_EN_EX_TEMP_PROP_DACB));
+		!!(chip->config3 & ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB));
 }
 
 static ssize_t adt7316_store_enable_prop_DACB(struct device *dev,
@@ -772,9 +775,9 @@ static ssize_t adt7316_store_enable_prop_DACB(struct device *dev,
 	u8 config3;
 	int ret;
 
-	config3 = chip->config3 & (~ADT7316_EN_EX_TEMP_PROP_DACB);
+	config3 = chip->config3 & (~ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB);
 	if (buf[0] == '1')
-		config3 |= ADT7316_EN_EX_TEMP_PROP_DACB;
+		config3 |= ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
 	if (ret)
@@ -798,7 +801,7 @@ static ssize_t adt7316_show_DAC_2Vref_ch_mask(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "0x%x\n",
-		chip->dac_config & ADT7316_DA_2VREF_CH_MASK);
+		chip->dac_config & ADT7316_DAC_CONFIG_2VREF_CH_MASK);
 }
 
 static ssize_t adt7316_store_DAC_2Vref_ch_mask(struct device *dev,
@@ -813,10 +816,10 @@ static ssize_t adt7316_store_DAC_2Vref_ch_mask(struct device *dev,
 	int ret;
 
 	ret = kstrtou8(buf, 16, &data);
-	if (ret || data > ADT7316_DA_2VREF_CH_MASK)
+	if (ret || data > ADT7316_DAC_CONFIG_2VREF_CH_MASK)
 		return -EINVAL;
 
-	dac_config = chip->dac_config & (~ADT7316_DA_2VREF_CH_MASK);
+	dac_config = chip->dac_config & (~ADT7316_DAC_CONFIG_2VREF_CH_MASK);
 	dac_config |= data;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
@@ -840,20 +843,20 @@ static ssize_t adt7316_show_DAC_update_mode(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC))
+	if (!(chip->config3 & ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC))
 		return sysfs_emit(buf, "manual\n");
 
-	switch (chip->dac_config & ADT7316_DA_EN_MODE_MASK) {
-	case ADT7316_DA_EN_MODE_SINGLE:
+	switch (chip->dac_config & ADT7316_DAC_CONFIG_EN_MODE_MASK) {
+	case ADT7316_DAC_CONFIG_EN_MODE_SINGLE:
 		return sysfs_emit(buf,
 			"0 - auto at any MSB DAC writing\n");
-	case ADT7316_DA_EN_MODE_AB_CD:
+	case ADT7316_DAC_CONFIG_EN_MODE_AB_CD:
 		return sysfs_emit(buf,
 			"1 - auto at MSB DAC AB and CD writing\n");
-	case ADT7316_DA_EN_MODE_ABCD:
+	case ADT7316_DAC_CONFIG_EN_MODE_ABCD:
 		return sysfs_emit(buf,
 			"2 - auto at MSB DAC ABCD writing\n");
-	default: /* ADT7316_DA_EN_MODE_LDAC */
+	default: /* ADT7316_DAC_CONFIG_EN_MODE_LDAC */
 		return sysfs_emit(buf, "3 - manual\n");
 	}
 }
@@ -869,15 +872,15 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev,
 	u8 data;
 	int ret;
 
-	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC))
+	if (!(chip->config3 & ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC))
 		return -EPERM;
 
 	ret = kstrtou8(buf, 10, &data);
-	if (ret || !FIELD_FIT(ADT7316_DA_EN_MODE_MASK, data))
+	if (ret || !FIELD_FIT(ADT7316_DAC_CONFIG_EN_MODE_MASK, data))
 		return -EINVAL;
 
-	dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK);
-	dac_config |= FIELD_PREP(ADT7316_DA_EN_MODE_MASK, data);
+	dac_config = chip->dac_config & (~ADT7316_DAC_CONFIG_EN_MODE_MASK);
+	dac_config |= FIELD_PREP(ADT7316_DAC_CONFIG_EN_MODE_MASK, data);
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
 	if (ret)
@@ -900,7 +903,7 @@ static ssize_t adt7316_show_all_DAC_update_modes(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC)
+	if (chip->config3 & ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC)
 		return sysfs_emit(buf, "0 - auto at any MSB DAC writing\n"
 				"1 - auto at MSB DAC AB and CD writing\n"
 				"2 - auto at MSB DAC ABCD writing\n"
@@ -922,16 +925,17 @@ static ssize_t adt7316_store_update_DAC(struct device *dev,
 	u8 data;
 	int ret;
 
-	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC) {
-		if ((chip->dac_config & ADT7316_DA_EN_MODE_MASK) !=
-			ADT7316_DA_EN_MODE_LDAC)
+	if (chip->config3 & ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC) {
+		if ((chip->dac_config & ADT7316_DAC_CONFIG_EN_MODE_MASK) !=
+			ADT7316_DAC_CONFIG_EN_MODE_LDAC)
 			return -EPERM;
 
 		ret = kstrtou8(buf, 16, &data);
-		if (ret || data > ADT7316_LDAC_EN_DA_MASK)
+		if (ret || data > ADT7316_LDAC_CONFIG_EN_DA_MASK)
 			return -EINVAL;
 
-		ldac_config = chip->ldac_config & (~ADT7316_LDAC_EN_DA_MASK);
+		ldac_config = chip->ldac_config &
+			(~ADT7316_LDAC_CONFIG_EN_DA_MASK);
 		ldac_config |= data;
 
 		ret = chip->bus.write(chip->bus.client, ADT7316_LDAC_CONFIG,
@@ -959,7 +963,7 @@ static ssize_t adt7316_show_DA_AB_Vref_bypass(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->dac_config & ADT7316_VREF_BYPASS_DAC_AB));
+		!!(chip->dac_config & ADT7316_DAC_CONFIG_VREF_BYPASS_AB));
 }
 
 static ssize_t adt7316_store_DA_AB_Vref_bypass(struct device *dev,
@@ -972,9 +976,9 @@ static ssize_t adt7316_store_DA_AB_Vref_bypass(struct device *dev,
 	u8 dac_config;
 	int ret;
 
-	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_AB);
+	dac_config = chip->dac_config & (~ADT7316_DAC_CONFIG_VREF_BYPASS_AB);
 	if (buf[0] == '1')
-		dac_config |= ADT7316_VREF_BYPASS_DAC_AB;
+		dac_config |= ADT7316_DAC_CONFIG_VREF_BYPASS_AB;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
 	if (ret)
@@ -998,7 +1002,7 @@ static ssize_t adt7316_show_DA_CD_Vref_bypass(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	return sysfs_emit(buf, "%d\n",
-		!!(chip->dac_config & ADT7316_VREF_BYPASS_DAC_CD));
+		!!(chip->dac_config & ADT7316_DAC_CONFIG_VREF_BYPASS_CD));
 }
 
 static ssize_t adt7316_store_DA_CD_Vref_bypass(struct device *dev,
@@ -1011,9 +1015,9 @@ static ssize_t adt7316_store_DA_CD_Vref_bypass(struct device *dev,
 	u8 dac_config;
 	int ret;
 
-	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_CD);
+	dac_config = chip->dac_config & (~ADT7316_DAC_CONFIG_VREF_BYPASS_CD);
 	if (buf[0] == '1')
-		dac_config |= ADT7316_VREF_BYPASS_DAC_CD;
+		dac_config |= ADT7316_DAC_CONFIG_VREF_BYPASS_CD;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
 	if (ret)
@@ -1038,9 +1042,10 @@ 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",
-			FIELD_GET(ADT7516_DAC_IN_VREF_MASK, chip->ldac_config));
+			FIELD_GET(ADT7516_LDAC_CONFIG_DAC_IN_VREF_MASK,
+				  chip->ldac_config));
 	return sysfs_emit(buf, "%d\n",
-		       !!(chip->ldac_config & ADT7316_DAC_IN_VREF));
+		       !!(chip->ldac_config & ADT7316_LDAC_CONFIG_DAC_IN_VREF));
 }
 
 static ssize_t adt7316_store_DAC_internal_Vref(struct device *dev,
@@ -1059,19 +1064,22 @@ static ssize_t adt7316_store_DAC_internal_Vref(struct device *dev,
 		if (ret || data > 3)
 			return -EINVAL;
 
-		ldac_config = chip->ldac_config & (~ADT7516_DAC_IN_VREF_MASK);
+		ldac_config = chip->ldac_config &
+			(~ADT7516_LDAC_CONFIG_DAC_IN_VREF_MASK);
 		if (data & 0x1)
-			ldac_config |= ADT7516_DAC_AB_IN_VREF;
+			ldac_config |= ADT7516_LDAC_CONFIG_DAC_AB_IN_VREF;
 		if (data & 0x2)
-			ldac_config |= ADT7516_DAC_CD_IN_VREF;
+			ldac_config |= ADT7516_LDAC_CONFIG_DAC_CD_IN_VREF;
 	} else {
 		ret = kstrtou8(buf, 16, &data);
 		if (ret)
 			return -EINVAL;
 
-		ldac_config = chip->ldac_config & (~ADT7316_DAC_IN_VREF);
+		ldac_config = chip->ldac_config &
+			(~ADT7316_LDAC_CONFIG_DAC_IN_VREF);
 		if (data)
-			ldac_config = chip->ldac_config | ADT7316_DAC_IN_VREF;
+			ldac_config = chip->ldac_config |
+				ADT7316_LDAC_CONFIG_DAC_IN_VREF;
 	}
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_LDAC_CONFIG,
@@ -1092,13 +1100,13 @@ static IIO_DEVICE_ATTR(DAC_internal_Vref, 0644,
 static u8 adt7316_extract_ad_lsb(u8 lsb, int channel)
 {
 	switch (channel) {
-	case ADT7316_AD_SINGLE_CH_EX:
+	case ADT7316_CONFIG2_AD_SINGLE_CH_EX:
 		return FIELD_GET(ADT7316_LSB_EX_TEMP_AIN1_MASK, lsb);
-	case ADT7516_AD_SINGLE_CH_AIN2:
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN2:
 		return FIELD_GET(ADT7516_LSB_AIN2_MASK, lsb);
-	case ADT7516_AD_SINGLE_CH_AIN3:
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN3:
 		return FIELD_GET(ADT7516_LSB_AIN3_MASK, lsb);
-	case ADT7516_AD_SINGLE_CH_AIN4:
+	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN4:
 		return FIELD_GET(ADT7516_LSB_AIN4_MASK, lsb);
 	default:
 		return 0;
@@ -1112,12 +1120,12 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
 	u8 msb, lsb;
 	int ret;
 
-	if ((chip->config2 & ADT7316_AD_SINGLE_CH_MODE) &&
-	    channel != (chip->config2 & ADT7516_AD_SINGLE_CH_MASK))
+	if ((chip->config2 & ADT7316_CONFIG2_AD_SINGLE_CH_MODE) &&
+	    channel != (chip->config2 & ADT7516_CONFIG2_AD_SINGLE_CH_MASK))
 		return -EPERM;
 
 	switch (channel) {
-	case ADT7316_AD_SINGLE_CH_IN:
+	case ADT7316_CONFIG2_AD_SINGLE_CH_IN:
 		ret = chip->bus.read(chip->bus.client,
 			ADT7316_LSB_IN_TEMP_VDD, &lsb);
 		if (ret)
@@ -1131,7 +1139,7 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
 		data = FIELD_PREP(ADT7316_AD_MSB_MASK, msb);
 		data |= lsb & ADT7316_LSB_IN_TEMP_MASK;
 		break;
-	case ADT7316_AD_SINGLE_CH_VDD:
+	case ADT7316_CONFIG2_AD_SINGLE_CH_VDD:
 		ret = chip->bus.read(chip->bus.client,
 			ADT7316_LSB_IN_TEMP_VDD, &lsb);
 		if (ret)
@@ -1182,7 +1190,7 @@ static ssize_t adt7316_show_VDD(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7316_AD_SINGLE_CH_VDD, buf);
+	return adt7316_show_ad(chip, ADT7316_CONFIG2_AD_SINGLE_CH_VDD, buf);
 }
 static IIO_DEVICE_ATTR(VDD, 0444, adt7316_show_VDD, NULL, 0);
 
@@ -1193,7 +1201,7 @@ static ssize_t adt7316_show_in_temp(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7316_AD_SINGLE_CH_IN, buf);
+	return adt7316_show_ad(chip, ADT7316_CONFIG2_AD_SINGLE_CH_IN, buf);
 }
 
 static IIO_DEVICE_ATTR(in_temp, 0444, adt7316_show_in_temp, NULL, 0);
@@ -1205,7 +1213,7 @@ static ssize_t adt7316_show_ex_temp_AIN1(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7316_AD_SINGLE_CH_EX, buf);
+	return adt7316_show_ad(chip, ADT7316_CONFIG2_AD_SINGLE_CH_EX, buf);
 }
 
 static IIO_DEVICE_ATTR(ex_temp_AIN1, 0444, adt7316_show_ex_temp_AIN1,
@@ -1219,7 +1227,7 @@ static ssize_t adt7316_show_AIN2(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7516_AD_SINGLE_CH_AIN2, buf);
+	return adt7316_show_ad(chip, ADT7516_CONFIG2_AD_SINGLE_CH_AIN2, buf);
 }
 static IIO_DEVICE_ATTR(AIN2, 0444, adt7316_show_AIN2, NULL, 0);
 
@@ -1230,7 +1238,7 @@ static ssize_t adt7316_show_AIN3(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7516_AD_SINGLE_CH_AIN3, buf);
+	return adt7316_show_ad(chip, ADT7516_CONFIG2_AD_SINGLE_CH_AIN3, buf);
 }
 static IIO_DEVICE_ATTR(AIN3, 0444, adt7316_show_AIN3, NULL, 0);
 
@@ -1241,7 +1249,7 @@ static ssize_t adt7316_show_AIN4(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return adt7316_show_ad(chip, ADT7516_AD_SINGLE_CH_AIN4, buf);
+	return adt7316_show_ad(chip, ADT7516_CONFIG2_AD_SINGLE_CH_AIN4, buf);
 }
 static IIO_DEVICE_ATTR(AIN4, 0444, adt7316_show_AIN4, NULL, 0);
 
@@ -1403,9 +1411,9 @@ static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
 
 	if (channel >= ADT7316_DA_MSB_DATA_REGS ||
 	    (channel == 0 &&
-	    (chip->config3 & ADT7316_EN_IN_TEMP_PROP_DACA)) ||
+	    (chip->config3 & ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA)) ||
 	    (channel == 1 &&
-	    (chip->config3 & ADT7316_EN_EX_TEMP_PROP_DACB)))
+	    (chip->config3 & ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB)))
 		return -EPERM;
 
 	offset = chip->dac_bits - 8;
@@ -1440,9 +1448,9 @@ static ssize_t adt7316_store_DAC(struct adt7316_chip_info *chip,
 
 	if (channel >= ADT7316_DA_MSB_DATA_REGS ||
 	    (channel == 0 &&
-	    (chip->config3 & ADT7316_EN_IN_TEMP_PROP_DACA)) ||
+	    (chip->config3 & ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA)) ||
 	    (channel == 1 &&
-	    (chip->config3 & ADT7316_EN_EX_TEMP_PROP_DACB)))
+	    (chip->config3 & ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB)))
 		return -EPERM;
 
 	offset = chip->dac_bits - 8;
@@ -1834,7 +1842,7 @@ static int adt7316_setup_irq(struct iio_dev *indio_dev)
 	}
 
 	if (irq_type & IRQF_TRIGGER_HIGH)
-		chip->config1 |= ADT7316_INT_POLARITY;
+		chip->config1 |= ADT7316_CONFIG1_INT_POLARITY;
 
 	return 0;
 }
@@ -1918,7 +1926,7 @@ static inline ssize_t adt7316_show_ad_bound(struct device *dev,
 	data = (int)val;
 
 	if (!((chip->id & ID_FAMILY_MASK) == ID_ADT75XX &&
-	      (chip->config1 & ADT7516_SEL_AIN1_2_EX_TEMP_MASK) == 0)) {
+	      (chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)) {
 		if (data & 0x80)
 			data -= 256;
 	}
@@ -1947,7 +1955,7 @@ static inline ssize_t adt7316_set_ad_bound(struct device *dev,
 		return -EINVAL;
 
 	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX &&
-	    (chip->config1 & ADT7516_SEL_AIN1_2_EX_TEMP_MASK) == 0) {
+	    (chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0) {
 		if (data > 255 || data < 0)
 			return -EINVAL;
 	} else {
@@ -1974,7 +1982,8 @@ static ssize_t adt7316_show_int_enabled(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	return sysfs_emit(buf, "%d\n", !!(chip->config1 & ADT7316_INT_EN));
+	return sysfs_emit(buf, "%d\n",
+			  !!(chip->config1 & ADT7316_CONFIG1_INT_EN));
 }
 
 static ssize_t adt7316_set_int_enabled(struct device *dev,
@@ -1987,9 +1996,9 @@ static ssize_t adt7316_set_int_enabled(struct device *dev,
 	u8 config1;
 	int ret;
 
-	config1 = chip->config1 & (~ADT7316_INT_EN);
+	config1 = chip->config1 & (~ADT7316_CONFIG1_INT_EN);
 	if (buf[0] == '1')
-		config1 |= ADT7316_INT_EN;
+		config1 |= ADT7316_CONFIG1_INT_EN;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, config1);
 	if (ret)
@@ -2167,9 +2176,9 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 	}
 
 	if (!chip->ldac_pin) {
-		chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDAC;
+		chip->config3 |= ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC;
 		if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-			chip->config1 |= ADT7516_SEL_AIN3;
+			chip->config1 |= ADT7516_CONFIG1_SEL_AIN3;
 	}
 	chip->int_mask = ADT7316_TEMP_INT_MASK | ADT7316_VDD_INT_MASK;
 	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)

-- 
2.53.0

Re: [PATCH v2 3/4] staging: iio: adt7316: add config names to registers and reorder

[Andy Shevchenko]

On Thu, Mar 05, 2026 at 11:17:00PM -0800, Michael Harris wrote:
> Add config names to macros to make it more clear which register they're
> affecting. Also renamed ADT7316_EN to further clarify what
> it's enabling.
> 
> Some macros were reordered, so that mask values were below the actual
> mask.

...

> +#define ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK	0x6

GENMASK*()

...

> +#define ADT7516_CONFIG1_SEL_EX_TEMP		0x4
> +#define ADT7516_CONFIG1_SEL_AIN3		0x8

Bits? Values?

...

> +#define ADT7316_CONFIG1_INT_EN			0x20
> +#define ADT7316_CONFIG1_INT_POLARITY		0x40

These looks like bits, why not BIT*()?

> +#define ADT7316_CONFIG1_PD			0x80

A bit?

...

> +#define ADT7316_CONFIG2_AD_SINGLE_CH_MASK	0x3
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_MASK	0x7

GENMASK()

> +#define ADT7316_CONFIG2_AD_SINGLE_CH_VDD	0
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_IN		1
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_EX		2
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN1	2
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN2	3
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN3	4
> +#define ADT7516_CONFIG2_AD_SINGLE_CH_AIN4	5
> +#define ADT7316_CONFIG2_AD_SINGLE_CH_MODE	0x10

...

> +#define ADT7316_CONFIG2_DISABLE_AVERAGING	0x20
> +#define ADT7316_CONFIG2_EN_SMBUS_TIMEOUT	0x40
> +#define ADT7316_CONFIG2_RESET			0x80

Bits?
And so on...

...

Are you going to have this conversion in a separate patch?

...

> +	switch (chip->config2 & ADT7516_CONFIG2_AD_SINGLE_CH_MASK) {
> +	case ADT7316_CONFIG2_AD_SINGLE_CH_VDD:
>  		return sysfs_emit(buf, "0 - VDD\n");
> +	case ADT7316_CONFIG2_AD_SINGLE_CH_IN:
>  		return sysfs_emit(buf, "1 - Internal Temperature\n");
> +	case ADT7316_CONFIG2_AD_SINGLE_CH_EX:
>  		if (((chip->id & ID_FAMILY_MASK) == ID_ADT75XX) &&
> +		    (chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
>  			return sysfs_emit(buf, "2 - AIN1\n");
>  
>  		return sysfs_emit(buf, "2 - External Temperature\n");
> +	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN2:
> +		if ((chip->config1 & ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK) == 0)
>  			return sysfs_emit(buf, "3 - AIN2\n");
>  
>  		return sysfs_emit(buf, "N/A\n");
> +	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN3:
> +		if (chip->config1 & ADT7516_CONFIG1_SEL_AIN3)
>  			return sysfs_emit(buf, "4 - AIN3\n");
>  
>  		return sysfs_emit(buf, "N/A\n");
> +	case ADT7516_CONFIG2_AD_SINGLE_CH_AIN4:
>  		return sysfs_emit(buf, "5 - AIN4\n");
>  	default:
>  		return sysfs_emit(buf, "N/A\n");

Side note:
Instead of this long switch I would rather introduce  a clear string array per
each of the possible variant. Note, that linker will eliminate string duplication,
so it won't be a problem.

...

> +	switch (chip->dac_config & ADT7316_DAC_CONFIG_EN_MODE_MASK) {
> +	case ADT7316_DAC_CONFIG_EN_MODE_SINGLE:
>  		return sysfs_emit(buf,
>  			"0 - auto at any MSB DAC writing\n");
> +	case ADT7316_DAC_CONFIG_EN_MODE_AB_CD:
>  		return sysfs_emit(buf,
>  			"1 - auto at MSB DAC AB and CD writing\n");
> +	case ADT7316_DAC_CONFIG_EN_MODE_ABCD:
>  		return sysfs_emit(buf,
>  			"2 - auto at MSB DAC ABCD writing\n");
> +	default: /* ADT7316_DAC_CONFIG_EN_MODE_LDAC */
>  		return sysfs_emit(buf, "3 - manual\n");
>  	}

Also can be just a string array with indexed access and a single call to
sysfs_emit(buf, "%s\n");


...

>  static IIO_DEVICE_ATTR(ex_temp_AIN1, 0444, adt7316_show_ex_temp_AIN1,

Side note:
Can IIO_DEVICE_ATTR_RO() be used here (and in other similar cases)?


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/4] staging: iio: adt7316: convert magic numbers to BIT_U32() or GENMASK_U32()

[Michael Harris]

Improve readability by converting raw hex macros to use BIT_U32() or
GENMASK_U32() instead.

Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 72 ++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index ea7d97bb0f378f3a3ce1225d8d13af2c5955ca56..824801c022475c35b8dfd4626820a533fbb68a17 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -30,8 +30,8 @@
 #define ADT7316_INT_STAT1		0x0
 #define ADT7316_INT_STAT2		0x1
 #define ADT7316_LSB_IN_TEMP_VDD		0x3
-#define ADT7316_LSB_IN_TEMP_MASK	0x3
-#define ADT7316_LSB_VDD_MASK		0xC
+#define ADT7316_LSB_IN_TEMP_MASK	GENMASK_U32(1, 0)
+#define ADT7316_LSB_VDD_MASK		GENMASK_U32(3, 2)
 #define ADT7316_LSB_EX_TEMP_AIN		0x4
 #define ADT7316_LSB_EX_TEMP_AIN1_MASK	GENMASK_U32(1, 0)
 #define ADT7516_LSB_AIN2_MASK		GENMASK_U32(3, 2)
@@ -90,19 +90,19 @@
 /*
  * ADT7316 config1
  */
-#define ADT7316_CONFIG1_MONITOR_EN		0x1
-#define ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK	0x6
-#define ADT7516_CONFIG1_SEL_EX_TEMP		0x4
-#define ADT7516_CONFIG1_SEL_AIN3		0x8
-#define ADT7316_CONFIG1_INT_EN			0x20
-#define ADT7316_CONFIG1_INT_POLARITY		0x40
-#define ADT7316_CONFIG1_PD			0x80
+#define ADT7316_CONFIG1_MONITOR_EN		BIT_U32(0)
+#define ADT7516_CONFIG1_SEL_AIN1_2_EX_TEMP_MASK	GENMASK_U32(2, 1)
+#define ADT7516_CONFIG1_SEL_EX_TEMP		BIT_U32(2)
+#define ADT7516_CONFIG1_SEL_AIN3		BIT_U32(3)
+#define ADT7316_CONFIG1_INT_EN			BIT_U32(5)
+#define ADT7316_CONFIG1_INT_POLARITY		BIT_U32(6)
+#define ADT7316_CONFIG1_PD			BIT_U32(7)
 
 /*
  * ADT7316 config2
  */
-#define ADT7316_CONFIG2_AD_SINGLE_CH_MASK	0x3
-#define ADT7516_CONFIG2_AD_SINGLE_CH_MASK	0x7
+#define ADT7316_CONFIG2_AD_SINGLE_CH_MASK	GENMASK_U32(1, 0)
+#define ADT7516_CONFIG2_AD_SINGLE_CH_MASK	GENMASK_U32(2, 0)
 #define ADT7316_CONFIG2_AD_SINGLE_CH_VDD	0
 #define ADT7316_CONFIG2_AD_SINGLE_CH_IN		1
 #define ADT7316_CONFIG2_AD_SINGLE_CH_EX		2
@@ -110,51 +110,51 @@
 #define ADT7516_CONFIG2_AD_SINGLE_CH_AIN2	3
 #define ADT7516_CONFIG2_AD_SINGLE_CH_AIN3	4
 #define ADT7516_CONFIG2_AD_SINGLE_CH_AIN4	5
-#define ADT7316_CONFIG2_AD_SINGLE_CH_MODE	0x10
-#define ADT7316_CONFIG2_DISABLE_AVERAGING	0x20
-#define ADT7316_CONFIG2_EN_SMBUS_TIMEOUT	0x40
-#define ADT7316_CONFIG2_RESET			0x80
+#define ADT7316_CONFIG2_AD_SINGLE_CH_MODE	BIT_U32(4)
+#define ADT7316_CONFIG2_DISABLE_AVERAGING	BIT_U32(5)
+#define ADT7316_CONFIG2_EN_SMBUS_TIMEOUT	BIT_U32(6)
+#define ADT7316_CONFIG2_RESET			BIT_U32(7)
 
 /*
  * ADT7316 config3
  */
-#define ADT7316_CONFIG3_ADCLK_22_5		0x1
-#define ADT7316_CONFIG3_DA_HIGH_RESOLUTION	0x2
-#define ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC	0x8
-#define ADT7516_CONFIG3_AIN_IN_VREF		0x10
-#define ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA	0x20
-#define ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB	0x40
+#define ADT7316_CONFIG3_ADCLK_22_5		BIT_U32(0)
+#define ADT7316_CONFIG3_DA_HIGH_RESOLUTION	BIT_U32(1)
+#define ADT7316_CONFIG3_DA_EN_VIA_DAC_LDAC	BIT_U32(3)
+#define ADT7516_CONFIG3_AIN_IN_VREF		BIT_U32(4)
+#define ADT7316_CONFIG3_EN_IN_TEMP_PROP_DACA	BIT_U32(5)
+#define ADT7316_CONFIG3_EN_EX_TEMP_PROP_DACB	BIT_U32(6)
 
 /*
  * ADT7316 DAC config
  */
-#define ADT7316_DAC_CONFIG_2VREF_CH_MASK	0xF
-#define ADT7316_DAC_CONFIG_EN_MODE_MASK		0x30
+#define ADT7316_DAC_CONFIG_2VREF_CH_MASK	GENMASK_U32(3, 0)
+#define ADT7316_DAC_CONFIG_EN_MODE_MASK		GENMASK_U32(5, 4)
 #define ADT7316_DAC_CONFIG_EN_MODE_SINGLE	0x00
 #define ADT7316_DAC_CONFIG_EN_MODE_AB_CD	0x10
 #define ADT7316_DAC_CONFIG_EN_MODE_ABCD		0x20
 #define ADT7316_DAC_CONFIG_EN_MODE_LDAC		0x30
-#define ADT7316_DAC_CONFIG_VREF_BYPASS_AB	0x40
-#define ADT7316_DAC_CONFIG_VREF_BYPASS_CD	0x80
+#define ADT7316_DAC_CONFIG_VREF_BYPASS_AB	BIT_U32(6)
+#define ADT7316_DAC_CONFIG_VREF_BYPASS_CD	BIT_U32(7)
 
 /*
  * ADT7316 LDAC config
  */
-#define ADT7316_LDAC_CONFIG_EN_DA_MASK		0xF
-#define ADT7316_LDAC_CONFIG_DAC_IN_VREF		0x10
-#define ADT7516_LDAC_CONFIG_DAC_AB_IN_VREF	0x10
-#define ADT7516_LDAC_CONFIG_DAC_CD_IN_VREF	0x20
-#define ADT7516_LDAC_CONFIG_DAC_IN_VREF_MASK	0x30
+#define ADT7316_LDAC_CONFIG_EN_DA_MASK		GENMASK_U32(3, 0)
+#define ADT7316_LDAC_CONFIG_DAC_IN_VREF		BIT_U32(4)
+#define ADT7516_LDAC_CONFIG_DAC_AB_IN_VREF	BIT_U32(4)
+#define ADT7516_LDAC_CONFIG_DAC_CD_IN_VREF	BIT_U32(5)
+#define ADT7516_LDAC_CONFIG_DAC_IN_VREF_MASK	GENMASK_U32(5, 4)
 
 /*
  * ADT7316 INT_MASK2
  */
-#define ADT7316_INT_MASK2_VDD		0x10
+#define ADT7316_INT_MASK2_VDD		BIT_U32(4)
 
 /*
  * ADT7316 value masks
  */
-#define ADT7316_VALUE_MASK		0xfff
+#define ADT7316_VALUE_MASK		GENMASK_U32(11, 0)
 #define ADT7316_AD_MSB_MASK		GENMASK_U32(9, 2)
 
 /*
@@ -172,7 +172,7 @@
 #define ID_ADT7517		0x12
 #define ID_ADT7519		0x14
 
-#define ID_FAMILY_MASK		0xF0
+#define ID_FAMILY_MASK		GENMASK_U32(7, 4)
 #define ID_ADT73XX		0x0
 #define ID_ADT75XX		0x10
 
@@ -206,9 +206,9 @@ struct adt7316_chip_info {
 #define ADT7516_AIN2_INT_MASK		0x20
 #define ADT7516_AIN3_INT_MASK		0x40
 #define ADT7516_AIN4_INT_MASK		0x80
-#define ADT7316_VDD_INT_MASK		0x100
-#define ADT7316_TEMP_INT_MASK		0x1F
-#define ADT7516_AIN_INT_MASK		0xE0
+#define ADT7316_VDD_INT_MASK		BIT_U32(8)
+#define ADT7316_TEMP_INT_MASK		GENMASK_U32(4, 0)
+#define ADT7516_AIN_INT_MASK		GENMASK_U32(7, 5)
 #define ADT7316_TEMP_AIN_INT_MASK	\
 	(ADT7316_TEMP_INT_MASK)
 

-- 
2.53.0

Re: [PATCH v2 4/4] staging: iio: adt7316: convert magic numbers to BIT_U32() or GENMASK_U32()

[Andy Shevchenko]

On Thu, Mar 05, 2026 at 11:17:01PM -0800, Michael Harris wrote:
> Improve readability by converting raw hex macros to use BIT_U32() or
> GENMASK_U32() instead.

Ah, cool, so one patch to rename, the other to convert to BIT/GENMASK.
Do you need to include bits.h or bitops.h (depending on the code)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] staging: iio: adt7316: convert magic numbers to BIT_U32() or GENMASK_U32()

[Jonathan Cameron]

On Thu, 05 Mar 2026 23:17:01 -0800
Michael Harris <michaelharriscode@gmail.com> wrote:

> Improve readability by converting raw hex macros to use BIT_U32() or
> GENMASK_U32() instead.
> 
> Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
> ---

Ah. I should have read on.  Fine to have the patch doing the rest separate.
Maybe a forwards reference from the earlier patch description might make
it more obvious that you've just delayed them.


Same thing on using GENMASK_U8() applies here.

Thanks,

Jonathan