>
> > +static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
> > + const struct iio_chan_spec *chan,
> > + unsigned int osr)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + guard(mutex)(&st->lock);
> > +
> > + if (osr == 1) {
> > + ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > + AD4851_OS_EN_MSK, 0);
> > + if (ret)
> > + return ret;
> > + } else {
> > + ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > + AD4851_OS_EN_MSK,
> AD4851_OS_EN_MSK);
> > + if (ret)
> > + return ret;
>
> regmap_clear_bits() and regmap_set_bits() would make this a bit
> less verbose and consistent with the effort started in [1].
>
> [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240617-review-v3-0-
> 88d1338c4cca@xxxxxxxxxxxx/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VY
> nCyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rv3yGMDWs$
>
Will do in v5.
>
> > +
> > + val = ad4851_osr_to_regval(osr);
> > + if (val < 0)
> > + return -EINVAL;
> > +
> > + ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > + AD4851_OS_RATIO_MSK, val);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + switch (chan->scan_type.realbits) {
> > + case 20:
> > + switch (osr) {
> > + case 1:
> > + val = 20;
> > + break;
> > + default:
> > + val = 24;
> > + break;
> > + }
> > + break;
> > + case 16:
> > + val = 16;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = iio_backend_data_size_set(st->back, val);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > + AD4851_PACKET_FORMAT_MASK, (osr == 1)
> ? 0 : 1);
> > +}
> > +
> > +static int ad4851_get_oversampling_ratio(struct ad4851_state *st,
> unsigned int *val)
> > +{
> > + unsigned int osr;
> > + int ret;
> > +
> > + 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)];
>
> Why is 1 not in the table?
Because there is no 1 in the OS_RATIO table from datasheet. 1 means you disable the OS via OS_EN bit.
>
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4851_setup(struct ad4851_state *st)
> > +{
> > + unsigned int product_id;
> > + int ret;
> > +
>
> Would be nice to do a hard reset here if possible using st->pd_gpio
> (datasheet says to cycle this twice and then wait 1 ms).
Sure, will do in v5.
> > + 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;
> > +
...
>
> > + .scan_index = index, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = real, \
> > + .storagebits = storage, \
>
> Since enabling oversampling can change realbits, this driver will likely
> need to implement scan_type_ext so that userspace is aware of the
> difference when oversampling is enabled. (Adding support for oversampling
> could always be a followup patch instead of trying to do everything
> all at once.)
Will do in v5.
>
> See the ad7380 driver as an example of how to impelemt this. [2]
>
> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
> cbc4acea2cfa@xxxxxxxxxxxx/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
> CyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
>
> Also, I would expect the .sign value to depend on how the
> input is being used. If it is differential or single-ended
> bipolar, then it is signed, but if it is signle-ended unipoloar
> then it is unsiged.
>
> Typically, this is coming from the devicetree because it
> depends on what is wired up to the input.
This topic is mentioned in the cover letter, maybe not argued enough there.
Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
But this is a request from the actual users of the driver: to have the softspan fully
controlled from userspace. That's why the offset and scale implementations were added.
Both these attributes are influencing the softspan.
> > + }, \
> > +}
>
> ...
