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

Jonathan Cameron
2026-04-23T17:56:42Z

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

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>
> 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@analog.com>
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.

> }