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

[Jonathan Cameron]

On Wed, 22 Apr 2026 15:45:45 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> 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.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
Hi Rodrigo,

Just one minor thing inline.

Thanks,

> @@ -65,8 +85,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev *indio_dev,
>  	bool readin;
>  	int ret;
>  	struct ad5686_state *st = iio_priv(indio_dev);
> -	unsigned int val, ref_bit_msk;
> -	u8 shift, address = 0;
> +	unsigned int val;
> +	u8 address = 0;
>  
>  	ret = kstrtobool(buf, &readin);
>  	if (ret)
> @@ -79,30 +99,26 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev *indio_dev,
>  
>  	switch (st->chip_info->regmap_type) {
>  	case AD5310_REGMAP:
> -		shift = 9;
> -		ref_bit_msk = AD5310_REF_BIT_MSK;
> +		ret = ad5310_control_sync(st);
I'd add explicit error check and return here.
		if (ret)
			return ret;
>  		break;
>  	case AD5683_REGMAP:
> -		shift = 13;
> -		ref_bit_msk = AD5683_REF_BIT_MSK;
> +		ret = ad5683_control_sync(st);
and here.
>  		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 {
> +			val = lower_16_bits(val);
> +		}
> +		ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, address, val);
and here.
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	val = ((st->pwr_down_mask & st->pwr_down_mode) << shift);
> -	if (!st->use_internal_vref)
> -		val |= ref_bit_msk;
> -
> -	ret = st->write(st, AD5686_CMD_POWERDOWN_DAC,
> -			address, val >> (address * 2));
>  
>  	return ret ? ret : len;
Then I think this just ends up as
	return len;

Adds a few lines of code, but makes it clear when we are exiting
early due to error and there is nothing else to do.

>  }