On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add scaling support to STM32 DFSDM.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
> ---
Just some general comments...
Acked-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> drivers/iio/adc/Kconfig | 1 +
> drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f3dfdaa80678..858ae8161fa4 100644
>
...
> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
> if (adc->hwc)
> iio_hw_consumer_disable(adc->hwc);
> + if (adc->backend[idx])
> + iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
> if (ret < 0) {
> dev_err(&indio_dev->dev,
> "%s: Conversion failed (channel %d)\n",
> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> *val = adc->sample_freq;
>
> return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + /*
> + * Scale is expressed in mV.
> + * When fast mode is disabled, actual resolution may be lower
> + * than 2^n, where n=realbits-1.
> + * This leads to underestimating input voltage. To
> + * compensate this deviation, the voltage reference can be
> + * corrected with a factor = realbits resolution / actual max
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
This is something that I've been thinking about since the beginning of backends
support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
interfaces. For example, in your usecase I think it would make more sense to have
dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.
OTOH, iio_backend_read_raw() could be useful in cases where frontends call
iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
And so, in the default: statement this "catch all" API could be used.
Anyways, this are really minor (likely pedantic) details and you kind of came first
and made the decision for me. As I don't really have strong feelings about it, I'm
fine with it as-is.
> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
> max);
> + *val2 = chan->scan_type.realbits;
> + if (chan->differential)
> + *val *= 2;
> + }
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_OFFSET:
> + /*
> + * DFSDM output data are in the range [-2^n,2^n],
> + * with n=realbits-1.
> + * - Differential modulator:
> + * Offset correspond to SD modulator offset.
> + * - Single ended modulator:
> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
> Vref to 2^n.
> + * Add 2^n to offset. (i.e. middle of input range)
> + * offset = offset(sd) * vref / res(sd) * max / vref.
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
> + if (!chan->differential)
> + *val += max;
> + }
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
> *indio_dev, struct iio_c
> * IIO_CHAN_INFO_RAW: used to compute regular conversion
> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> */
> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + if (child) {
> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET);
Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
you could maybe use iio_backend_extend_chan_spec().
- Nuno Sá
