Re: [PATCH v4 10/13] iio: dac: ad5686: remove powerdown mask magic number

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 02:07:40PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Define macro AD5686_PD_MSK to hold powerdown mask value.

...

> -	return ((st->pwr_down_mode >> shift) & 0x3) - 1;
> +	return ((st->pwr_down_mode >> shift) & AD5686_PD_MSK) - 1;

I believe you want two macros that shifts that accordingly.

#define AD5686_PD_MSK			GENMASK(1, 0)
#define AD5686_GET_PD_MSK(shift)	(AD5686_PD_MSK << shift)
#define AD5686_GET_PD_BITS(mode, shift)	(((mode) >> shift) & AD5686_PD_MSK)

But this all seems like reinventing a wheel, id est
FIELD_GET()/FIELD_PREP()/FIELD_MODIFY() from bitfield.h.

...

> -	st->pwr_down_mode &= ~(0x3 << shift);
> -	st->pwr_down_mode |= (mode + 1) << shift;
> +	st->pwr_down_mode &= ~(AD5686_PD_MSK << shift);
> +	st->pwr_down_mode |= ((mode + 1) & AD5686_PD_MSK) << shift;

This is FIELD_MODIFY()

...

> -	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask & (0x3 << shift)));
> +	return sysfs_emit(buf, "%d\n", !!(st->pwr_down_mask & (AD5686_PD_MSK << shift)));

FIELD_GET()

...

>  	if (readin)
> -		st->pwr_down_mask |= 0x3 << ad5686_pd_mask_shift(chan);
> +		st->pwr_down_mask |= AD5686_PD_MSK << ad5686_pd_mask_shift(chan);
>  	else
> -		st->pwr_down_mask &= ~(0x3 << ad5686_pd_mask_shift(chan));
> +		st->pwr_down_mask &= ~(AD5686_PD_MSK << ad5686_pd_mask_shift(chan));

FIELD_MODIFY()

...

>  		shift = ad5686_pd_mask_shift(&st->chip_info->channels[i]);
> -		st->pwr_down_mask &= ~(0x3 << shift); /* powered up state */
> -		st->pwr_down_mode &= ~(0x3 << shift);
> +		st->pwr_down_mask &= ~(AD5686_PD_MSK << shift); /* powered up state */
> +		st->pwr_down_mode &= ~(AD5686_PD_MSK << shift);

FIELD_MODIFY()

>  	}

...

Since these require some shifts at run-time perhaps better to have helpers
that will do all needed stuff in one call.

-- 
With Best Regards,
Andy Shevchenko