Re: [PATCH v6 11/11] iio: adc: ti-ads1015: Replace custom shift macros with FIELD_PREP/GET

[Jonathan Cameron <jic23@kernel.org>]

On Mon,  9 Nov 2026 22:28:05 +0530
Archit Anant <architanant5@gmail.com> wrote:

> The driver currently uses custom _SHIFT macros and manual bitwise
> operations to read and write to the configuration register.
> 
> Modernize the bitwise logic by replacing all manual shifts with the
> standard FIELD_PREP() and FIELD_GET() macros from <linux/bitfield.h>.
> This improves readability and prevents potential bitwise errors.
> 
> Remove the now-unused _SHIFT macros from the top of the file.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Archit Anant <architanant5@gmail.com>
> ---
>  drivers/iio/adc/ti-ads1015.c | 79 ++++++++++++++++++++----------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 2c3ba1b91aac4..8f7ec61314363 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -11,6 +11,7 @@
>   *	* 0x4B - ADDR connected to SCL
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> @@ -38,15 +39,6 @@
>  #define ADS1015_LO_THRESH_REG	0x02
>  #define ADS1015_HI_THRESH_REG	0x03
>  
> -#define ADS1015_CFG_COMP_QUE_SHIFT	0
> -#define ADS1015_CFG_COMP_LAT_SHIFT	2
> -#define ADS1015_CFG_COMP_POL_SHIFT	3
> -#define ADS1015_CFG_COMP_MODE_SHIFT	4
> -#define ADS1015_CFG_DR_SHIFT	5
> -#define ADS1015_CFG_MOD_SHIFT	8
> -#define ADS1015_CFG_PGA_SHIFT	9
> -#define ADS1015_CFG_MUX_SHIFT	12
> -
>  #define ADS1015_CFG_COMP_QUE_MASK	GENMASK(1, 0)
>  #define ADS1015_CFG_COMP_LAT_MASK	BIT(2)
>  #define ADS1015_CFG_COMP_POL_MASK	BIT(3)
> @@ -382,17 +374,24 @@ int ads1015_get_adc_result(struct ads1015_data *data, int chan, int *val)
>  
>  	pga = data->channel_data[chan].pga;
>  	dr = data->channel_data[chan].data_rate;
> -	mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> -		ADS1015_CFG_DR_MASK;
> -	cfg = chan << ADS1015_CFG_MUX_SHIFT | pga << ADS1015_CFG_PGA_SHIFT |
> -		dr << ADS1015_CFG_DR_SHIFT;
>  

I'm probably being inconsistent across versions, but at least
looking at it this time, I prefer the building up common cfg
and mask first, then adding the extra bits in the following.

>  	if (ads1015_event_channel_enabled(data)) {
> -		mask |= ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK;
> -		cfg |= data->thresh_data[chan].comp_queue <<
> -				ADS1015_CFG_COMP_QUE_SHIFT |
> -			data->comp_mode <<
> -				ADS1015_CFG_COMP_MODE_SHIFT;
> +		mask = ADS1015_CFG_COMP_QUE_MASK | ADS1015_CFG_COMP_MODE_MASK |
> +		       ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> +		       ADS1015_CFG_DR_MASK;
> +		cfg = FIELD_PREP(ADS1015_CFG_MUX_MASK, chan) |
> +		      FIELD_PREP(ADS1015_CFG_PGA_MASK, pga) |
> +		      FIELD_PREP(ADS1015_CFG_DR_MASK, dr) |
> +		      FIELD_PREP(ADS1015_CFG_COMP_QUE_MASK,
> +				 data->thresh_data[chan].comp_queue) |
> +		      FIELD_PREP(ADS1015_CFG_COMP_MODE_MASK,
> +				 data->comp_mode);
> +	} else {
> +		mask = ADS1015_CFG_MUX_MASK | ADS1015_CFG_PGA_MASK |
> +			ADS1015_CFG_DR_MASK;
> +		cfg = FIELD_PREP(ADS1015_CFG_MUX_MASK, chan) |
> +		      FIELD_PREP(ADS1015_CFG_PGA_MASK, pga) |
> +		      FIELD_PREP(ADS1015_CFG_DR_MASK, dr);
>  	}