correct macro issues

[PATCH 2/2] staging: iio: adt7316: convert magic numbers to BIT() and GENMASK()

Posted by Michael Harris 1 week, 1 day ago

Improve readability by converting raw hex macros to use BIT() or GENMASK()
instead.

Update a few sysfs_emit() string formats to expect the unsigned long type
given by BIT() and GENMASK() and prevent compiler errors.

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

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 4173c8822fff495e8c69d9cf6c11be9e9227a8c1..0af7e18ff9684993622a268110f0a17c0bff3616 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -29,11 +29,11 @@
 #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(1, 0)
+#define ADT7316_LSB_VDD_MASK		GENMASK(3, 2)
 #define ADT7316_LSB_VDD_OFFSET		2
 #define ADT7316_LSB_EX_TEMP_AIN		0x4
-#define ADT7316_LSB_EX_TEMP_MASK	0x3
+#define ADT7316_LSB_EX_TEMP_MASK	GENMASK(1, 0)
 #define ADT7516_LSB_AIN_SHIFT		2
 #define ADT7316_AD_MSB_DATA_BASE        0x6
 #define ADT7316_AD_MSB_DATA_REGS        3
@@ -88,19 +88,19 @@
 /*
  * 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_EN			BIT(0)
+#define ADT7516_SEL_EX_TEMP		BIT(2)
+#define ADT7516_SEL_AIN1_2_EX_TEMP_MASK	GENMASK(2, 1)
+#define ADT7516_SEL_AIN3		BIT(3)
+#define ADT7316_INT_EN			BIT(5)
+#define ADT7316_INT_POLARITY		BIT(6)
+#define ADT7316_PD			BIT(7)
 
 /*
  * ADT7316 config2
  */
-#define ADT7316_AD_SINGLE_CH_MASK	0x3
-#define ADT7516_AD_SINGLE_CH_MASK	0x7
+#define ADT7316_AD_SINGLE_CH_MASK	GENMASK(1, 0)
+#define ADT7516_AD_SINGLE_CH_MASK	GENMASK(2, 0)
 #define ADT7316_AD_SINGLE_CH_VDD	0
 #define ADT7316_AD_SINGLE_CH_IN		1
 #define ADT7316_AD_SINGLE_CH_EX		2
@@ -108,54 +108,54 @@
 #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_AD_SINGLE_CH_MODE	BIT(4)
+#define ADT7316_DISABLE_AVERAGING	BIT(5)
+#define ADT7316_EN_SMBUS_TIMEOUT	BIT(6)
 
 /*
  * 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_ADCLK_22_5		BIT(0)
+#define ADT7316_DA_HIGH_RESOLUTION	BIT(1)
+#define ADT7316_DA_EN_VIA_DAC_LDAC	BIT(3)
+#define ADT7516_AIN_IN_VREF		BIT(4)
+#define ADT7316_EN_IN_TEMP_PROP_DACA	BIT(5)
+#define ADT7316_EN_EX_TEMP_PROP_DACB	BIT(6)
 
 /*
  * ADT7316 DAC config
  */
-#define ADT7316_DA_2VREF_CH_MASK	0xF
-#define ADT7316_DA_EN_MODE_MASK		0x30
+#define ADT7316_DA_2VREF_CH_MASK	GENMASK(3, 0)
+#define ADT7316_DA_EN_MODE_MASK		GENMASK(5, 4)
 #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
 #define ADT7316_DA_EN_MODE_LDAC		0x30
-#define ADT7316_VREF_BYPASS_DAC_AB	0x40
-#define ADT7316_VREF_BYPASS_DAC_CD	0x80
+#define ADT7316_VREF_BYPASS_DAC_AB	BIT(6)
+#define ADT7316_VREF_BYPASS_DAC_CD	BIT(7)
 
 /*
  * 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 ADT7316_LDAC_EN_DA_MASK		GENMASK(3, 0)
+#define ADT7316_DAC_IN_VREF		BIT(4)
+#define ADT7516_DAC_AB_IN_VREF		BIT(4)
+#define ADT7516_DAC_CD_IN_VREF		BIT(5)
 #define ADT7516_DAC_IN_VREF_OFFSET	4
-#define ADT7516_DAC_IN_VREF_MASK	0x30
+#define ADT7516_DAC_IN_VREF_MASK	GENMASK(5, 4)
 
 /*
  * ADT7316 INT_MASK2
  */
-#define ADT7316_INT_MASK2_VDD		0x10
+#define ADT7316_INT_MASK2_VDD		BIT(4)
 
 /*
  * ADT7316 value masks
  */
-#define ADT7316_T_VALUE_SIGN		0x400
+#define ADT7316_T_VALUE_SIGN		BIT(10)
 #define ADT7316_T_VALUE_FLOAT_OFFSET	2
-#define ADT7316_T_VALUE_FLOAT_MASK	0x2
+#define ADT7316_T_VALUE_FLOAT_MASK	BIT(1)
 
 /*
  * Chip ID
@@ -167,7 +167,7 @@
 #define ID_ADT7517		0x12
 #define ID_ADT7519		0x14
 
-#define ID_FAMILY_MASK		0xF0
+#define ID_FAMILY_MASK		GENMASK(7, 4)
 #define ID_ADT73XX		0x0
 #define ID_ADT75XX		0x10
 
@@ -192,9 +192,9 @@ struct adt7316_chip_info {
  * Logic interrupt mask for user application to enable
  * interrupts.
  */
-#define ADT7316_VDD_INT_MASK		0x100
-#define ADT7316_TEMP_INT_MASK		0x1F
-#define ADT7516_AIN_INT_MASK		0xE0
+#define ADT7316_VDD_INT_MASK		BIT(8)
+#define ADT7316_TEMP_INT_MASK		GENMASK(4, 0)
+#define ADT7516_AIN_INT_MASK		GENMASK(7, 5)
 #define ADT7316_TEMP_AIN_INT_MASK	\
 	(ADT7316_TEMP_INT_MASK)
 
@@ -783,7 +783,7 @@ static ssize_t adt7316_show_DAC_2Vref_ch_mask(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, "0x%x\n",
+	return sysfs_emit(buf, "0x%lx\n",
 		chip->dac_config & ADT7316_DA_2VREF_CH_MASK);
 }
 
@@ -1023,7 +1023,7 @@ static ssize_t adt7316_show_DAC_internal_Vref(struct device *dev,
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
 	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-		return sysfs_emit(buf, "0x%x\n",
+		return sysfs_emit(buf, "0x%lx\n",
 			(chip->ldac_config & ADT7516_DAC_IN_VREF_MASK) >>
 			ADT7516_DAC_IN_VREF_OFFSET);
 	return sysfs_emit(buf, "%d\n",
@@ -1146,7 +1146,7 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
 		sign = '-';
 	}
 
-	return sysfs_emit(buf, "%c%d.%.2d\n", sign,
+	return sysfs_emit(buf, "%c%d.%.2ld\n", sign,
 		(data >> ADT7316_T_VALUE_FLOAT_OFFSET),
 		(data & ADT7316_T_VALUE_FLOAT_MASK) * 25);
 }

-- 
2.52.0

Re: [PATCH 2/2] staging: iio: adt7316: convert magic numbers to BIT() and GENMASK()

Posted by Jonathan Cameron 1 week, 1 day ago

On Fri, 30 Jan 2026 23:38:28 -0800
Michael Harris <michaelharriscode@gmail.com> wrote:

> Improve readability by converting raw hex macros to use BIT() or GENMASK()
> instead.
> 
> Update a few sysfs_emit() string formats to expect the unsigned long type
> given by BIT() and GENMASK() and prevent compiler errors.
> 
> Signed-off-by: Michael Harris <michaelharriscode@gmail.com>
> ---
>  drivers/staging/iio/addac/adt7316.c | 80 ++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 4173c8822fff495e8c69d9cf6c11be9e9227a8c1..0af7e18ff9684993622a268110f0a17c0bff3616 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -29,11 +29,11 @@


> @@ -88,19 +88,19 @@
>  /*
>   * 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_EN			BIT(0)
Arguably its a separate change but I'd much prefer if these defines
included what register they were found in. That is things like

#define ADT7316_CONFIG1_EN
etc. It makes it a lot easier to spot mismatches in the code.

Also this define sounds like it would be general enable / disable of capture
but it seems to only be about monitoring modes.

> +#define ADT7516_SEL_EX_TEMP		BIT(2)
That's not a mask.  It is a value and it should clearly placed
in the relevant field, so you can do things like
FIELD_PREP(ADT7516_SEL_AIN1_2_EX_TEMP_MASK, ADT7516...TEMP)
So it should take the value 2 == b10 not BIT(2).

> +#define ADT7516_SEL_AIN1_2_EX_TEMP_MASK	GENMASK(2, 1)
I'd put the masks before the values. 

> +#define ADT7516_SEL_AIN3		BIT(3)
> +#define ADT7316_INT_EN			BIT(5)
> +#define ADT7316_INT_POLARITY		BIT(6)
> +#define ADT7316_PD			BIT(7)
>  
>  /*
>   * ADT7316 config2
>   */
> -#define ADT7316_AD_SINGLE_CH_MASK	0x3
> -#define ADT7516_AD_SINGLE_CH_MASK	0x7
> +#define ADT7316_AD_SINGLE_CH_MASK	GENMASK(1, 0)
> +#define ADT7516_AD_SINGLE_CH_MASK	GENMASK(2, 0)
>  #define ADT7316_AD_SINGLE_CH_VDD	0
>  #define ADT7316_AD_SINGLE_CH_IN		1
>  #define ADT7316_AD_SINGLE_CH_EX		2
> @@ -108,54 +108,54 @@
>  #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_AD_SINGLE_CH_MODE	BIT(4)
> +#define ADT7316_DISABLE_AVERAGING	BIT(5)
> +#define ADT7316_EN_SMBUS_TIMEOUT	BIT(6)
As commented on previous patch I'd prefer a full register definition
even if a few bits aren't used. Makes it easier to spot when they
are being accidentally cleared.

>  
>  /*
>   * 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_ADCLK_22_5		BIT(0)
> +#define ADT7316_DA_HIGH_RESOLUTION	BIT(1)
> +#define ADT7316_DA_EN_VIA_DAC_LDAC	BIT(3)
> +#define ADT7516_AIN_IN_VREF		BIT(4)
> +#define ADT7316_EN_IN_TEMP_PROP_DACA	BIT(5)
> +#define ADT7316_EN_EX_TEMP_PROP_DACB	BIT(6)
>  
>  /*
>   * ADT7316 DAC config
>   */
> -#define ADT7316_DA_2VREF_CH_MASK	0xF
> -#define ADT7316_DA_EN_MODE_MASK		0x30
> +#define ADT7316_DA_2VREF_CH_MASK	GENMASK(3, 0)
> +#define ADT7316_DA_EN_MODE_MASK		GENMASK(5, 4)
>  #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
>  #define ADT7316_DA_EN_MODE_LDAC		0x30
> -#define ADT7316_VREF_BYPASS_DAC_AB	0x40
> -#define ADT7316_VREF_BYPASS_DAC_CD	0x80
> +#define ADT7316_VREF_BYPASS_DAC_AB	BIT(6)
> +#define ADT7316_VREF_BYPASS_DAC_CD	BIT(7)
>  
>  /*
>   * 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 ADT7316_LDAC_EN_DA_MASK		GENMASK(3, 0)
> +#define ADT7316_DAC_IN_VREF		BIT(4)
> +#define ADT7516_DAC_AB_IN_VREF		BIT(4)
> +#define ADT7516_DAC_CD_IN_VREF		BIT(5)
>  #define ADT7516_DAC_IN_VREF_OFFSET	4
Try and get rid of separate shifts / offsets.  (I think this
is just a shift?)  Replace them with use of FIELD_GET()/ FIELD_PREP() and
the mask.

A precursor patch doing that might make this patch easier to
read as all the shifts would likely be gone.

> -#define ADT7516_DAC_IN_VREF_MASK	0x30
> +#define ADT7516_DAC_IN_VREF_MASK	GENMASK(5, 4)
>  
>  /*
>   * ADT7316 INT_MASK2
>   */
> -#define ADT7316_INT_MASK2_VDD		0x10
> +#define ADT7316_INT_MASK2_VDD		BIT(4)
>  
>  /*
>   * ADT7316 value masks
>   */
> -#define ADT7316_T_VALUE_SIGN		0x400
> +#define ADT7316_T_VALUE_SIGN		BIT(10)

From a quick glance, these are all 2's complement.
As such, using sign_extend32() or similar would be much nicer
than the custom sign manipulation in here and the need for
this macro would probably go away.

>  #define ADT7316_T_VALUE_FLOAT_OFFSET	2
> -#define ADT7316_T_VALUE_FLOAT_MASK	0x2
> +#define ADT7316_T_VALUE_FLOAT_MASK	BIT(1)
The usage of these is also rather strange and more about
a fixed point maths conversion than anything related to registers.
I'd expect that to get cleaned up by reporting temperature as _raw
and providing an appropriate scale.

>  
>  /*
>   * Chip ID
> @@ -167,7 +167,7 @@
>  #define ID_ADT7517		0x12
>  #define ID_ADT7519		0x14
>  
> -#define ID_FAMILY_MASK		0xF0
> +#define ID_FAMILY_MASK		GENMASK(7, 4)
>  #define ID_ADT73XX		0x0
>  #define ID_ADT75XX		0x10
>  
> @@ -192,9 +192,9 @@ struct adt7316_chip_info {
>   * Logic interrupt mask for user application to enable
>   * interrupts.
>   */
> -#define ADT7316_VDD_INT_MASK		0x100
> -#define ADT7316_TEMP_INT_MASK		0x1F
> -#define ADT7516_AIN_INT_MASK		0xE0
> +#define ADT7316_VDD_INT_MASK		BIT(8)
> +#define ADT7316_TEMP_INT_MASK		GENMASK(4, 0)
> +#define ADT7516_AIN_INT_MASK		GENMASK(7, 5)
>  #define ADT7316_TEMP_AIN_INT_MASK	\
>  	(ADT7316_TEMP_INT_MASK)
>  
> @@ -783,7 +783,7 @@ static ssize_t adt7316_show_DAC_2Vref_ch_mask(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, "0x%x\n",
> +	return sysfs_emit(buf, "0x%lx\n",
>  		chip->dac_config & ADT7316_DA_2VREF_CH_MASK);
Is the compiler complaining about these? It really should be able to tell that the masks
are small enough that the original can always print the right thing.

>  }
>  
> @@ -1023,7 +1023,7 @@ static ssize_t adt7316_show_DAC_internal_Vref(struct device *dev,
>  	struct adt7316_chip_info *chip = iio_priv(dev_info);
>  
>  	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
> -		return sysfs_emit(buf, "0x%x\n",
> +		return sysfs_emit(buf, "0x%lx\n",
>  			(chip->ldac_config & ADT7516_DAC_IN_VREF_MASK) >>
>  			ADT7516_DAC_IN_VREF_OFFSET);
>  	return sysfs_emit(buf, "%d\n",
> @@ -1146,7 +1146,7 @@ static ssize_t adt7316_show_ad(struct adt7316_chip_info *chip,
>  		sign = '-';
>  	}
>  
> -	return sysfs_emit(buf, "%c%d.%.2d\n", sign,
> +	return sysfs_emit(buf, "%c%d.%.2ld\n", sign,
>  		(data >> ADT7316_T_VALUE_FLOAT_OFFSET),
>  		(data & ADT7316_T_VALUE_FLOAT_MASK) * 25);
I mention this above.  This calculation is odd.  I'd just expect the maths to be done
with a multiplier, probably 100 such that the 1/4 per bit after the decimal point
gets buried in that and you can do a simple % 100 here to get the decimal places.

I've asked for a couple of different types of change in here. It is likely
that some of them should be in separate patches.

Thanks,

Jonathan

>  }
>

Re: [PATCH 2/2] staging: iio: adt7316: convert magic numbers to BIT() and GENMASK()

Posted by David Laight 4 days, 12 hours ago

On Sat, 31 Jan 2026 16:21:40 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 30 Jan 2026 23:38:28 -0800
> Michael Harris <michaelharriscode@gmail.com> wrote:
.. 
> > -	return sysfs_emit(buf, "0x%x\n",
> > +	return sysfs_emit(buf, "0x%lx\n",
> >  		chip->dac_config & ADT7316_DA_2VREF_CH_MASK);  
> Is the compiler complaining about these? It really should be able to tell that the masks
> are small enough that the original can always print the right thing.

That will be because BIT() and GENMASK() generate 'unsigned long' values.
Sort of silly when they are used for hardware registers that are fixed width.
You can use BIT_U32() and GENMASK_U32() instead - they will be
'unsigned int'.

	David

Re: [PATCH 2/2] staging: iio: adt7316: convert magic numbers to BIT() and GENMASK()

Posted by Michael Harris 4 days, 16 hours ago

On 1/31/26 8:21 AM, Jonathan Cameron wrote:
> I've asked for a couple of different types of change in here. It is likely
> that some of them should be in separate patches.
> 
> Thanks,
> 
> Jonathan
Thank you for the detailed review. I am working on a v2 series to
address your feedback and will send it when ready.

Thanks,
Michael

[PATCH 1/2] staging: iio: adt7316: remove unused macros
[PATCH 2/2] staging: iio: adt7316: convert magic numbers to BIT() and GENMASK()