> > + if (osr == 1) {
> > + ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > + AD4851_PACKET_FORMAT_MASK,
> 0);
>
> regmap_clear_bits()
>
> > + if (ret)
> > + return ret;
> > +
> > + st->resolution_boost_enabled = false;
> > + } else {
> > + ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > + AD4851_PACKET_FORMAT_MASK,
> 1);
>
> regmap_set_bits()
Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap set_bits
Should I do 2 separate masks?
>
> > + if (ret)
> > + return ret;
> > +
> > + st->resolution_boost_enabled = true;
>
> Technically speaking, 16-bit chips don't have resolution boost. And
> selecting PACKET_FORMAT = 1 here would enable extra status bits that
> we aren't using. I don't think that is what we want.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4851_get_oversampling_ratio(struct ad4851_state *st,
> unsigned int *val)
> > +{
> > + unsigned int osr;
> > + int ret;
>
> guard(mutex)(&st->lock);
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_OVERSAMPLE, &osr);
> > + if (ret)
> > + return ret;
> > +
> > + if (!FIELD_GET(AD4851_OS_EN_MSK, osr))
> > + *val = 1;
> > + else
> > + *val =
> ad4851_oversampling_ratios[FIELD_GET(AD4851_OS_RATIO_MSK, osr)];
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static void ad4851_reg_disable(void *data)
> > +{
> > + regulator_disable(data);
> > +}
> > +
> > +static int ad4851_setup(struct ad4851_state *st)
> > +{
> > + unsigned int product_id;
> > + int ret;
> > +
> > + if (st->pd_gpio) {
> > + gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > + fsleep(1);
> > + gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> > + fsleep(1);
> > + gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > + fsleep(1);
> > + gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
>
> The GPIOD_OUT_* macros are not valid here. Just use 0 and 1.
> Also, we can use gpiod_set_value_cansleep() here.
>
> > + fsleep(1000);
> > + }
> > +
> > + if (!IS_ERR(st->vrefbuf)) {
> > + ret = regmap_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > + AD4851_REFBUF_PD,
> AD4851_REFBUF_PD);
>
> Can be simplified to regmap_set_bits().
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_enable(st->vrefbuf);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&st->spi->dev,
> ad4851_reg_disable,
> > + st->vrefbuf);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (!IS_ERR(st->vrefio)) {
> > + ret = regmap_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > + AD4851_REFSEL_PD,
> AD4851_REFSEL_PD);
>
> Can be simplified to regmap_set_bits().
>
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_enable(st->vrefio);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&st->spi->dev,
> ad4851_reg_disable,
> > + st->vrefio);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > + AD4851_SW_RESET);
> > + if (ret)
> > + return ret;
>
> Probably need a time delay after reset. Also we should not need to
> call this if we were able to reset using the PD gpio since the chip
> was already reset.
>
> Also, also, this needs to be moved before other register writes
> above, otherwise it will reset those registers.
>
> > +
> > + ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_B,
> > + AD4851_SINGLE_INSTRUCTION);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > + AD4851_SDO_ENABLE);
> > + if (ret)
> > + return ret;
>
> This one also needs to be done before any regmap reads (including
> update_bits, set_bits, clear_bits that implicitly read).
>
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_PRODUCT_ID_L,
> &product_id);
> > + if (ret)
> > + return ret;
> > +
> > + if (product_id != st->info->product_id)
> > + dev_info(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> > + product_id);
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_DEVICE_CTRL,
> > + AD4851_ECHO_CLOCK_MODE);
>
> Doing regmap_write here will set all other bits to 0, which will
> clear previous config done above. Should be regmap_set_bits().
>
> Other regmap_write() calls seem OK, but it's always nice to have a
> comment explaining why it is OK, otherwise it looks suspicios, like
> it could be hiding a bug like we have here.
>
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> > +}
> > +
> > +static int ad4851_find_opt(bool *field, u32 size, u32 *ret_start)
> > +{
> > + unsigned int i, cnt = 0, max_cnt = 0, max_start = 0;
> > + int start;
> > +
> > + for (i = 0, start = -1; i < size; i++) {
> > + if (field[i] == 0) {
> > + if (start == -1)
> > + start = i;
> > + cnt++;
> > + } else {
> > + if (cnt > max_cnt) {
> > + max_cnt = cnt;
> > + max_start = start;
> > + }
> > + start = -1;
> > + cnt = 0;
> > + }
> > + }
> > + /*
> > + * Find the longest consecutive sequence of false values from field
> > + * and return starting index.
> > + */
> > + if (cnt > max_cnt) {
> > + max_cnt = cnt;
> > + max_start = start;
> > + }
> > +
> > + if (!max_cnt)
> > + return -ENOENT;
> > +
> > + *ret_start = max_start;
> > +
> > + return max_cnt;
> > +}
> > +
> > +static int ad4851_calibrate(struct ad4851_state *st)
> > +{
> > + unsigned int opt_delay, lane_num, delay, i, s, c;
> > + enum iio_backend_interface_type interface_type;
> > + DECLARE_BITMAP(pn_status, AD4851_MAX_LANES *
> AD4851_MAX_IODELAY);
> > + bool status;
> > + int ret;
> > +
> > + ret = iio_backend_interface_type_get(st->back, &interface_type);
> > + if (ret)
> > + return ret;
> > +
> > + switch (interface_type) {
> > + case IIO_BACKEND_INTERFACE_SERIAL_CMOS:
> > + lane_num = st->info->num_channels;
> > + break;
> > + case IIO_BACKEND_INTERFACE_SERIAL_LVDS:
> > + lane_num = 1;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }> +
> > + if (st->info->resolution == 16) {
> > + ret = iio_backend_data_size_set(st->back, 24);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> > + AD4851_TEST_PAT |
> AD4857_PACKET_SIZE_24);
> > + if (ret)
> > + return ret;
> > + } else {
> > + ret = iio_backend_data_size_set(st->back, 32);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> > + AD4851_TEST_PAT |
> AD4858_PACKET_SIZE_32);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < st->info->num_channels; i++) {
> > + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_0(i),
> > + AD4851_TESTPAT_0_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_1(i),
> > + AD4851_TESTPAT_1_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_2(i),
> > + AD4851_TESTPAT_2_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_3(i),
> > + AD4851_TESTPAT_3_DEFAULT(i));
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_backend_chan_enable(st->back, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < lane_num; i++) {
> > + for (delay = 0; delay < AD4851_MAX_IODELAY; delay++) {
> > + ret = iio_backend_iodelay_set(st->back, i, delay);
> > + if (ret)
> > + return ret;
> > + ret = iio_backend_chan_status(st->back, i, &status);
> > + if (ret)
> > + return ret;
> > +
> > + if (status)
> > + set_bit(i * AD4851_MAX_IODELAY + delay,
> pn_status);
> > + else
> > + clear_bit(i * AD4851_MAX_IODELAY + delay,
> pn_status);
> > + }
> > + }
> > +
> > + for (i = 0; i < lane_num; i++) {
> > + status = test_bit(i * AD4851_MAX_IODELAY, pn_status);
> > + c = ad4851_find_opt(&status, AD4851_MAX_IODELAY, &s);
> > + if (c < 0)
> > + return c;
> > +
> > + opt_delay = s + c / 2;
> > + ret = iio_backend_iodelay_set(st->back, i, opt_delay);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < st->info->num_channels; i++) {
> > + ret = iio_backend_chan_disable(st->back, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = iio_backend_data_size_set(st->back, 20);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> > +}
> > +
> > +static int ad4851_get_calibscale(struct ad4851_state *st, int ch, int *val, int
> *val2)
> > +{
> > + unsigned int reg_val;
> > + int gain;
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> > + ®_val);
> > + if (ret)
> > + return ret;
> > +
> > + gain = reg_val << 8;
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> > + ®_val);
> > + if (ret)
> > + return ret;
> > +
> > + gain |= reg_val;
> > +
> > + *val = gain;
> > + *val2 = 32768;
> > +
> > + return IIO_VAL_FRACTIONAL;
> > +}
> > +
> > +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> > + int val2)
> > +{
> > + u64 gain;
> > + u8 buf[0];
> > + int ret;
> > +
> > + if (val < 0 || val2 < 0)
> > + return -EINVAL;
> > +
> > + gain = val * MICRO + val2;
> > + gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
> > +
> > + put_unaligned_be16(gain, buf);
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> > + buf[0]);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> > + buf[1]);
> > +}
> > +
>
> I'm pretty sure that calibscale and calibbias also need to take into
> account if resolution boost is enabled or not.
Can you please detail a bit on this topic? I am not sure what I should do.
> > +static int ad4851_get_calibbias(struct ad4851_state *st, int ch, int *val)
> > +{
> > + unsigned int lsb, mid, msb;
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MSB(ch),
> > + &msb);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
> > + &mid);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
> > + &lsb);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->info->resolution == 16) {
> > + *val = msb << 8;
> > + *val |= mid;
> > + *val = sign_extend32(*val, 15);
> > + } else {
> > + *val = msb << 12;
> > + *val |= mid << 4;
> > + *val |= lsb >> 4;
> > + *val = sign_extend32(*val, 19);
> > + }
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4851_set_calibbias(struct ad4851_state *st, int ch, int val)
> > +{
> > + u8 buf[3] = { 0 };
> > + int ret;
> > +
> > + if (val < 0)
> > + return -EINVAL;
> > +
> > + if (st->info->resolution == 16)
> > + put_unaligned_be16(val, buf);
> > + else
> > + put_unaligned_be24(val << 4, buf);
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
> buf[2]);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
> buf[1]);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_write(st->regmap,
> AD4851_REG_CHX_OFFSET_MSB(ch), buf[0]);
> > +}
> > +
>
> nit: a comment mentioning this is mapping voltage ranges to register
> values would be helpful
>
> > +static const unsigned int ad4851_scale_table[][2] = {
> > + { 2500, 0x0 },
> > + { 5000, 0x1 },
> > + { 5000, 0x2 },
> > + { 10000, 0x3 },
> > + { 6250, 0x04 },
>
> nit: drop the extra 0
>
> > + { 12500, 0x5 },
> > + { 10000, 0x6 },
> > + { 20000, 0x7 },
> > + { 12500, 0x8 },
> > + { 25000, 0x9 },
> > + { 20000, 0xA },
> > + { 40000, 0xB },
> > + { 25000, 0xC },
> > + { 50000, 0xD },
> > + { 40000, 0xE },
> > + { 80000, 0xF },
> > +};
>
> I'm not sure how this table is supposed to work since there are
> multiple entries with the same voltage value. Probably better
> would be to just have the entries for the unipolar/unsigned ranges.
> Then if applying this to a differential/signed channel, just add
> 1 to resulting register value before writing it to the register.
> Or make two different tables, one for unsigned and one for signed
> channels.
It is stated in the set_scale function comment how this table works.
This table contains range-register value pair.
Always the second value corresponds to the single ended mode.
>
> > +
> > +static const int ad4857_offset_table[] = {
> > + 0, -32768,
> > +};
> > +
> > +static const int ad4858_offset_table[] = {
> > + 0, -524288,
> > +};
>
> These are no longer used.
>
> > +
> > +static const unsigned int ad4851_scale_avail[] = {
> > + 2500, 5000,
> > + 10000, 6250,
> > + 12500, 20000,
> > + 25000, 40000,
> > + 50000, 80000,
> > +};
>
> This should only go up to 40000. Or we need two different tables, one
> for signed channels and one for unsigned channels. Although it would
> probably be simpler to just multiply values from this table by 2 if
> .differential = 1 rather than having a second table.
>
> > +
> > +static void __ad4851_get_scale(struct ad4851_state *st, int scale_tbl,
> > + unsigned int *val, unsigned int *val2)
> > +{
> > + const struct ad4851_chip_info *info = st->info;
> > + const struct iio_chan_spec *chan = &info->channels[0];
> > + unsigned int tmp;
> > +
> > + tmp = ((unsigned long long)scale_tbl * MICRO) >> chan-
> >scan_type.realbits;
> > + *val = tmp / MICRO;
> > + *val2 = tmp % MICRO;
> > +}
> > +
> > +static int ad4851_set_scale(struct ad4851_state *st,
> > + const struct iio_chan_spec *chan, int val, int val2)
> > +{
> > + unsigned int scale_val[2];
> > + unsigned int i;
> > + bool single_ended = false;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
> > + __ad4851_get_scale(st, ad4851_scale_table[i][0],
> > + &scale_val[0], &scale_val[1]);
> > + if (scale_val[0] != val || scale_val[1] != val2)
> > + continue;
> > +
> > + /*
> > + * Adjust the softspan value (differential or single ended)
> > + * based on the scale value selected channel type.
> > + *
> > + * If the channel is not differential then continue iterations
> > + * until the next matching scale value which always
> corresponds
> > + * to the single ended mode.
> > + */
> > + if (!chan->differential && !single_ended) {
> > + single_ended = true;
> > + continue;
> > + }
> > +
> > + return regmap_write(st->regmap,
> > + AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > + ad4851_scale_table[i][1]);
>
> Since we don't know if we are using single-ended or differential
> until we actually read data, we should not be setting the softspan
> register here. Instead, we should just save the selected scale
> to st->scale. Then when we do a buffered read, we can set the
> softspan correctly for each channel depending on which one is
> enabled.
>
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ad4851_get_scale(struct ad4851_state *st,
> > + const struct iio_chan_spec *chan, int *val,
> > + int *val2)
> > +{
> > + int i, softspan_val;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > + &softspan_val);
> > + if (ret)
> > + return ret;
>
> As above, we can just return the value save in st->scale instead of
> reading the register.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
> > + if (softspan_val == ad4851_scale_table[i][1])
> > + break;
> > + }
> > +
> > + if (i == ARRAY_SIZE(ad4851_scale_table))
> > + return -EIO;
> > +
> > + __ad4851_get_scale(st, ad4851_scale_table[i][0], val, val2);
>
> If resolution boost is in effect because of oversampling, we need
> to take that into account here as well.
>
> > +
> > + return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int ad4851_scale_fill(struct ad4851_state *st)
> > +{
> > + unsigned int i, val1, val2;
> > +
> > + st->scales = devm_kmalloc_array(&st->spi->dev,
> ARRAY_SIZE(ad4851_scale_avail),
> > + sizeof(*st->scales), GFP_KERNEL);
> > + if (!st->scales)
> > + return -ENOMEM;
>
> It looks like this is a fixed size, so we could just include that
> size in struct ad4851_state instead of doing a kmalloc here.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail); i++) {
> > + __ad4851_get_scale(st, ad4851_scale_avail[i], &val1, &val2);
> > + st->scales[i][0] = val1;
> > + st->scales[i][1] = val2;
> > + }
>
> Maybe simpler to make st->scales a 1-dimintional array and just
> store the index of the values in ad4851_scale_avail instead of
> copying the values?
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4851_read_raw(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + int *val, int *val2, long info)
> > +{
> > + struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = st->sampling_freq;
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_CALIBSCALE:
> > + return ad4851_get_calibscale(st, chan->channel, val, val2);
> > + case IIO_CHAN_INFO_SCALE:
> > + return ad4851_get_scale(st, chan, val, val2);
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + return ad4851_get_calibbias(st, chan->channel, val);
> > + return IIO_VAL_INT;
>
> Unreachable return.
>
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + return ad4851_get_oversampling_ratio(st, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4851_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long info)
> > +{
> > + struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + return ad4851_set_sampling_freq(st, val);
> > + case IIO_CHAN_INFO_SCALE:
> > + return ad4851_set_scale(st, chan, val, val2);
> > + case IIO_CHAN_INFO_CALIBSCALE:
> > + return ad4851_set_calibscale(st, chan->channel, val, val2);
> > + case IIO_CHAN_INFO_CALIBBIAS:
> > + return ad4851_set_calibbias(st, chan->channel, val);
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + return ad4851_set_oversampling_ratio(st, chan, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4851_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask)
> > +{
> > + struct ad4851_state *st = iio_priv(indio_dev);
> > + unsigned int c;
> > + int ret;
> > +
>
> This is where we should also write the SoftSpan register to ensure
> the correct unipolar or bipolar range is selected depending on
> which channels are enabled.
>
> Also, we will need to check here and return error if both the
> single-ended and differential channel for any physical input
> is enabled.
>
> > + for (c = 0; c < st->info->num_channels; c++) {
> > + if (test_bit(c, scan_mask))
> > + ret = iio_backend_chan_enable(st->back, c);
> > + else
> > + ret = iio_backend_chan_disable(st->back, c);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4851_read_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long mask)
> > +{
> > + struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + *vals = (const int *)st->scales;
> > + *type = IIO_VAL_INT_PLUS_MICRO;
> > + /* Values are stored in a 2D matrix */
> > + *length = ARRAY_SIZE(ad4851_scale_avail) * 2;
> > + return IIO_AVAIL_LIST;
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + *vals = ad4851_oversampling_ratios;
> > + *length = ARRAY_SIZE(ad4851_oversampling_ratios);
> > + *type = IIO_VAL_INT;
> > + return IIO_AVAIL_LIST;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_scan_type ad4851_scan_type_16[] = {
> > + [AD4851_SCAN_TYPE_NORMAL] = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
>
> Is storagebits really 16 here? The HDL engineers mentioned that on
> other projects, they were trying to standardize on 32-bit storage
> size everywhere, even when data is <= 16 bit.
>
> > + },
> > + [AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + },
> > +};
>
> NORMAL and RESOLUTION_BOOST are the same on 16-bit chips, so we
> don't actually need to implement ext_scan_type for those chips.
>
> > +
> > +static const struct iio_scan_type ad4851_scan_type_20[] = {
> > + [AD4851_SCAN_TYPE_NORMAL] = {
> > + .sign = 's',
> > + .realbits = 20,
> > + .storagebits = 32,
> > + },
> > + [AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > + .sign = 's',
> > + .realbits = 24,
> > + .storagebits = 32,
> > + },
> > +};
>
> The single-ended channels (differential = 0) have unsigned data, so we
> will need different structs to handle that case.
>
> > +
> > +static int ad4851_get_current_scan_type(const struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > + return st->resolution_boost_enabled ?
> AD4851_SCAN_TYPE_RESOLUTION_BOOST
> > + : AD4851_SCAN_TYPE_NORMAL;
> > +}
> > +
> > +#define AD4851_IIO_CHANNEL(index, diff, real)
> \
>
> nit: "bits" would make more sense to me than "real"
>
> > +{ \
> > + .type = IIO_VOLTAGE, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |
> \
> > + BIT(IIO_CHAN_INFO_CALIBBIAS) |
> \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> \
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> \
> > + .info_mask_shared_by_type_available =
> \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
>
> Scale needs to be info_mask_separate_available to match
> IIO_CHAN_INFO_SCALE
> flag on info_mask_separate.
>
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> \
> > + .indexed = 1, \
> > + .differential = diff, \
> > + .channel = index, \
> > + .channel2 = index + diff * 8, \
> > + .scan_index = index + diff * 8, \
> > + .ext_scan_type = ad4851_scan_type_##real, \
> > + .num_ext_scan_type = \
> > + ARRAY_SIZE(ad4851_scan_type_##real), \
> > +}
> > +
> > +static const struct iio_chan_spec ad4858_channels[] = {
> > + AD4851_IIO_CHANNEL(0, 0, 20),
> > + AD4851_IIO_CHANNEL(1, 0, 20),
> > + AD4851_IIO_CHANNEL(2, 0, 20),
> > + AD4851_IIO_CHANNEL(3, 0, 20),
> > + AD4851_IIO_CHANNEL(4, 0, 20),
> > + AD4851_IIO_CHANNEL(5, 0, 20),
> > + AD4851_IIO_CHANNEL(6, 0, 20),
> > + AD4851_IIO_CHANNEL(7, 0, 20),
> > + AD4851_IIO_CHANNEL(0, 1, 20),
> > + AD4851_IIO_CHANNEL(1, 1, 20),
> > + AD4851_IIO_CHANNEL(2, 1, 20),
> > + AD4851_IIO_CHANNEL(3, 1, 20),
> > + AD4851_IIO_CHANNEL(4, 1, 20),
> > + AD4851_IIO_CHANNEL(5, 1, 20),
> > + AD4851_IIO_CHANNEL(6, 1, 20),
> > + AD4851_IIO_CHANNEL(7, 1, 20),
> > +};
> > +
> > +static const struct iio_chan_spec ad4857_channels[] = {
> > + AD4851_IIO_CHANNEL(0, 0, 16),
> > + AD4851_IIO_CHANNEL(1, 0, 16),
> > + AD4851_IIO_CHANNEL(2, 0, 16),
> > + AD4851_IIO_CHANNEL(3, 0, 16),
> > + AD4851_IIO_CHANNEL(4, 0, 16),
> > + AD4851_IIO_CHANNEL(5, 0, 16),
> > + AD4851_IIO_CHANNEL(6, 0, 16),
> > + AD4851_IIO_CHANNEL(7, 0, 16),
> > + AD4851_IIO_CHANNEL(0, 1, 16),
> > + AD4851_IIO_CHANNEL(1, 1, 16),
> > + AD4851_IIO_CHANNEL(2, 1, 16),
> > + AD4851_IIO_CHANNEL(3, 1, 16),
> > + AD4851_IIO_CHANNEL(4, 1, 16),
> > + AD4851_IIO_CHANNEL(5, 1, 16),
> > + AD4851_IIO_CHANNEL(6, 1, 16),
> > + AD4851_IIO_CHANNEL(7, 1, 16),
> > +};
>
> I don't think it is valid for two channels to have the same scan_index.
> And since this is simultaneous sampling and we don't have control over
> the order in which the data is received from the backend, to get the
> ordering correct, we will likely have to make this:
>
I am not sure which of these channels have the same index.
scan_index is index + diff * 8 in the channel definition.
> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
> ...
>
> AD4851_IIO_CHANNEL(0, 0, 0, 16),
> AD4851_IIO_CHANNEL(1, 0, 1, 16),
> AD4851_IIO_CHANNEL(2, 1, 0, 16),
> AD4851_IIO_CHANNEL(3, 1, 1, 16),
> AD4851_IIO_CHANNEL(4, 2, 0, 16),
> AD4851_IIO_CHANNEL(5, 2, 1, 16),
> AD4851_IIO_CHANNEL(6, 3, 0, 16),
> AD4851_IIO_CHANNEL(7, 3, 1, 16),
> AD4851_IIO_CHANNEL(8, 4, 0, 16),
> AD4851_IIO_CHANNEL(9, 4, 1, 16),
> AD4851_IIO_CHANNEL(10, 5, 0, 16),
> AD4851_IIO_CHANNEL(11, 5, 1, 16),
> AD4851_IIO_CHANNEL(12, 6, 0, 16),
> AD4851_IIO_CHANNEL(13, 6, 1, 16),
> AD4851_IIO_CHANNEL(14, 7, 0, 16),
> AD4851_IIO_CHANNEL(15, 7, 1, 16),
>
>
> > +
> > +static const struct ad4851_chip_info ad4851_info = {
> > + .name = "ad4851",
> > + .product_id = 0x67,
> > + .channels = ad4857_channels,
> > + .num_channels = ARRAY_SIZE(ad4857_channels),
> > + .throughput = 250 * KILO,
