Re: [PATCH 11/22] iio: dac: ad5686: fix ref bit initialization for single-channel parts

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Apr 22, 2026 at 03:45:45PM +0100, Rodrigo Alencar via B4 Relay wrote:

> For single-channel parts the control register is used to enable the
> internal voltage reference and perform powerdown control. The reference
> enable bit position was ignored when writing the register at the probe
> function. This patch adds a control_sync() function that properly consumes
> the mask definitions with FIELD_PREP(). As further cleanup, the created
> functions and definitions are also used in ad5686_write_dac_powerdown().
> Some local variables ended up being unused (so removed) and
> st->use_internal_vref is initialized earlier in probe.

...

> +static int ad5310_control_sync(struct ad5686_state *st)
> +{
> +	unsigned int pd_val = st->pwr_down_mask & st->pwr_down_mode;
> +
> +	return st->write(st, AD5686_CMD_CONTROL_REG, 0,
> +			 FIELD_PREP(AD5310_PD_MSK, pd_val) |
> +			 FIELD_PREP(AD5310_REF_BIT_MSK, !st->use_internal_vref));

I prefer to see explicit integers in use instead of implicit boolean promotions
in FIELD_*() that might lead to subtle mistakes.

st_use_internal_vref ? 0 : 1

In the code compiler will optimise this to the current state anyway.

> +}

...

>  	switch (st->chip_info->regmap_type) {
>  	case AD5310_REGMAP:
> -		shift = 9;
> -		ref_bit_msk = AD5310_REF_BIT_MSK;
> +		ret = ad5310_control_sync(st);
>  		break;
>  	case AD5683_REGMAP:
> -		shift = 13;
> -		ref_bit_msk = AD5683_REF_BIT_MSK;
> +		ret = ad5683_control_sync(st);
>  		break;
>  	case AD5686_REGMAP:
> -		shift = 0;
> -		ref_bit_msk = 0;
>  		/* AD5674R/AD5679R have 16 channels and 2 powerdown registers */
> -		if (chan->channel > 0x7)
> +		val = st->pwr_down_mask & st->pwr_down_mode;
> +		if (chan->channel > 0x7) {
>  			address = 0x8;
> +			val = upper_16_bits(val);
> +		} else {

Would be nice to see address to be assigned here explicitly if it's a constant.

> +			val = lower_16_bits(val);
> +		}
> +		ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, address, val);
>  		break;
>  	default:
>  		return -EINVAL;
>  	}

-- 
With Best Regards,
Andy Shevchenko