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
