On Wed, 22 Apr 2026 15:45:45 +0100 Rodrigo Alencar via B4 Relay <[email protected]> wrote:
> From: Rodrigo Alencar <[email protected]> > > 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 <[email protected]> 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. > }
