On Tue, 20 Aug 2024 10:58:36 -0500 David Lechner <dlechner@xxxxxxxxxxxx> wrote: > The AD4695 has a calibration feature that allows the user to compensate > for variations in the analog front end. This implements this feature in > the driver using the standard `calibgain` and `calibbias` attributes. > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> Hi David, Whilst some of the messy value manipulation is unavoidable (oh for signed integer zero!), I wonder if we can at least move one case into the core. See below. > + > +static int ad4695_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + unsigned int reg_val; > + int ret; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + switch (mask) { > + case IIO_CHAN_INFO_CALIBSCALE: > + switch (chan->type) { > + case IIO_VOLTAGE: > + if (val < 0 || val2 < 0) > + reg_val = 0; > + else if (val > 1) > + reg_val = U16_MAX; > + else > + reg_val = (val * (1 << 16) + > + mul_u64_u32_div(val2, 1 << 16, > + MICRO)) / 2; Maybe worth extending iio_write_channel_info() to handle IIO_VAL_FRACTIONAL_LOG2()? It'll look much like this and you'll need to provide write_raw_get_fmt() so the core know what to do with the value formatting. I don't really like the mixture here between the read path being able to rely on the core code to deal with the /2^X and the write path not. > + > + return regmap_write(st->regmap16, > + AD4695_REG_GAIN_IN(chan->scan_index), > + reg_val); > + default: > + return -EINVAL;