On Wed, 22 Sep 2021 09:53:58 +0200
Olivier MOYSAN <olivier.moysan@xxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> On 9/18/21 8:42 PM, Jonathan Cameron wrote:
> > On Wed, 15 Sep 2021 12:02:45 +0200
> > Olivier MOYSAN <olivier.moysan@xxxxxxxxxxx> wrote:
> >
> >> Hi Jonathan,
> >>
> >> On 9/11/21 6:28 PM, Jonathan Cameron wrote:
> >>> On Wed, 8 Sep 2021 17:54:51 +0200
> >>> Olivier Moysan <olivier.moysan@xxxxxxxxxxx> wrote:
> >>>
> >>>> Add support of vrefint calibration.
> >>>> If a channel is labeled as vrefint, get vrefint calibration
> >>>> from non volatile memory for this channel.
> >>>> A conversion on vrefint channel allows to update scale
> >>>> factor according to vrefint deviation, compared to vrefint
> >>>> calibration value.
> >>>
> >>> As I mention inline, whilst technically the ABI doesn't demand it
> >>> the expectation of much of userspace software is that _scale is
> >>> pseudo constant - that is it doesn't tend to change very often and when
> >>> it does it's normally because someone deliberately made it change.
> >>> As such most software reads it just once.
> >>>
> >>> Normally we work around this by applying the maths in kernel and
> >>> not exposing the scale at all. Is this something that could be done here?
> >>>
> >>> Jonathan
> >>>
> >>>>
> >>>> Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
> >>>> ---
> >>>> drivers/iio/adc/stm32-adc.c | 88 ++++++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 82 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> >>>> index ef3d2af98025..9e52a7de9b16 100644
> >>>> --- a/drivers/iio/adc/stm32-adc.c
> >>>> +++ b/drivers/iio/adc/stm32-adc.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include <linux/io.h>
> >>>> #include <linux/iopoll.h>
> >>>> #include <linux/module.h>
> >>>> +#include <linux/nvmem-consumer.h>
> >>>> #include <linux/platform_device.h>
> >>>> #include <linux/pm_runtime.h>
> >>>> #include <linux/of.h>
> >>>> @@ -42,6 +43,7 @@
> >>>> #define STM32_ADC_TIMEOUT (msecs_to_jiffies(STM32_ADC_TIMEOUT_US / 1000))
> >>>> #define STM32_ADC_HW_STOP_DELAY_MS 100
> >>>> #define STM32_ADC_CHAN_NONE -1
> >>>> +#define STM32_ADC_VREFINT_VOLTAGE 3300
> >>>>
> >>>> #define STM32_DMA_BUFFER_SIZE PAGE_SIZE
> >>>>
> >>>> @@ -79,6 +81,7 @@ enum stm32_adc_extsel {
> >>>> };
> >>>>
> >>>> enum stm32_adc_int_ch {
> >>>> + STM32_ADC_INT_CH_NONE = -1,
> >>>> STM32_ADC_INT_CH_VDDCORE,
> >>>> STM32_ADC_INT_CH_VREFINT,
> >>>> STM32_ADC_INT_CH_VBAT,
> >>>> @@ -137,6 +140,16 @@ struct stm32_adc_regs {
> >>>> int shift;
> >>>> };
> >>>>
> >>>> +/**
> >>>> + * struct stm32_adc_vrefint - stm32 ADC internal reference voltage data
> >>>> + * @vrefint_cal: vrefint calibration value from nvmem
> >>>> + * @vrefint_data: vrefint actual value
> >>>> + */
> >>>> +struct stm32_adc_vrefint {
> >>>> + u32 vrefint_cal;
> >>>> + u32 vrefint_data;
> >>>> +};
> >>>> +
> >>>> /**
> >>>> * struct stm32_adc_regspec - stm32 registers definition
> >>>> * @dr: data register offset
> >>>> @@ -186,6 +199,7 @@ struct stm32_adc;
> >>>> * @unprepare: optional unprepare routine (disable, power-down)
> >>>> * @irq_clear: routine to clear irqs
> >>>> * @smp_cycles: programmable sampling time (ADC clock cycles)
> >>>> + * @ts_vrefint_ns: vrefint minimum sampling time in ns
> >>>> */
> >>>> struct stm32_adc_cfg {
> >>>> const struct stm32_adc_regspec *regs;
> >>>> @@ -199,6 +213,7 @@ struct stm32_adc_cfg {
> >>>> void (*unprepare)(struct iio_dev *);
> >>>> void (*irq_clear)(struct iio_dev *indio_dev, u32 msk);
> >>>> const unsigned int *smp_cycles;
> >>>> + const unsigned int ts_vrefint_ns;
> >>>> };
> >>>>
> >>>> /**
> >>>> @@ -223,6 +238,7 @@ struct stm32_adc_cfg {
> >>>> * @pcsel: bitmask to preselect channels on some devices
> >>>> * @smpr_val: sampling time settings (e.g. smpr1 / smpr2)
> >>>> * @cal: optional calibration data on some devices
> >>>> + * @vrefint: internal reference voltage data
> >>>> * @chan_name: channel name array
> >>>> * @num_diff: number of differential channels
> >>>> * @int_ch: internal channel indexes array
> >>>> @@ -248,6 +264,7 @@ struct stm32_adc {
> >>>> u32 pcsel;
> >>>> u32 smpr_val[2];
> >>>> struct stm32_adc_calib cal;
> >>>> + struct stm32_adc_vrefint vrefint;
> >>>> char chan_name[STM32_ADC_CH_MAX][STM32_ADC_CH_SZ];
> >>>> u32 num_diff;
> >>>> int int_ch[STM32_ADC_INT_CH_NB];
> >>>> @@ -1331,15 +1348,35 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
> >>>> ret = stm32_adc_single_conv(indio_dev, chan, val);
> >>>> else
> >>>> ret = -EINVAL;
> >>>> +
> >>>> + /* If channel mask corresponds to vrefint, store data */
> >>>> + if (adc->int_ch[STM32_ADC_INT_CH_VREFINT] == chan->channel)
> >>>> + adc->vrefint.vrefint_data = *val;
> >>>> +
> >>>> iio_device_release_direct_mode(indio_dev);
> >>>> return ret;
> >>>>
> >>>> case IIO_CHAN_INFO_SCALE:
> >>>> if (chan->differential) {
> >>>> - *val = adc->common->vref_mv * 2;
> >>>> + if (adc->vrefint.vrefint_data &&
> >>>> + adc->vrefint.vrefint_cal) {
> >>>> + *val = STM32_ADC_VREFINT_VOLTAGE * 2 *
> >>>> + adc->vrefint.vrefint_cal /
> >>>> + adc->vrefint.vrefint_data;
> >>>
> >>> Ah.. Dynamic scale. This is always awkward when it occurs.
> >>> Given most / possibly all userspace software assumes a pseudo static scale
> >>> (not data dependent) we normally hide this by doing the maths internal to the
> >>> driver - sometimes meaning we need to present the particular channel as processed
> >>> not raw.
> >>>
> >>> Is the expectation here that vrefint_data is actually very nearly constant? If
> >>> so then what you have here may be fine as anyone not aware the scale might change
> >>> will get very nearly the right value anyway.
> >>>
> >>
> >> The need here is to compare the measured value of vrefint with the
> >> calibrated value saved in non volatile memory. The ratio between these
> >> two values can be used as a correction factor for the acquisitions on
> >> all other channels.
> >>
> >> The vrefint data is expected to be close to the saved vrefint
> >> calibration value, and it should not vary strongly over time.
> >> So, yes, we can indeed consider the scale as a pseudo constant. If the
> >> scale is not updated, the deviation with actual value should remain
> >> limited, as well.
> >
> > Ok, so in that case we could probably get away with having it as you have
> > here, though for maximum precision we'd need userspace to occasionally check
> > the scale.
> >
> >>
> >> You suggest above to hide scale tuning through processed channels.
> >> If I follow this logic, when vrefint channel is available, all channels
> >> should be defined as processed channels (excepted vrefint channel)
> >> In this case no scale is exposed for these channels, and the vrefint
> >> calibration ratio can be used to provide converted data directly.
> >> Do you prefer this implementation ?
> >
> >>
> >> In this case I wonder how buffered data have to be managed. These data
> >> are still provided as raw data, but the scale factor is not more
> >> available to convert them. I guess that these data have to be converted
> >> internally also, either in dma callback or irq handler.
> >> Is this correct ?
> >
> > This is one of the holes in what IIO does today. Without meta data in the
> > buffer (which is hard to define in a clean fashion) it is hard to have
> > a compact representation of the data in the presence of dynamic scaling.
> > The vast majority of devices don't inherently support such scaling so
> > this is only occasionally a problem.
> >
> > To support this at the moment you would indeed need to scale the data
> > before pushing it to the buffer which is obviously really ugly.
> >
> > My gut feeling here is there are three possible approaches.
> >
> > 1) Ignore the dynamic nature of the calibration and pretend it's static.
> > 2) Add an explicit 'calibration' sysfs attribute.
> > This is a fairly common model for other sensor types which don't do
> > dynamic calibration but instead require to you to start some special
> > calibration sequence.
> > As the calibration is not updated, except on explicit userspace action
> > we can assume that the scale is static unless userspace is aware of
> > the dynamic aspect.
> > 3) Add a userspace control to turn on dynamic calibration. That makes it
> > opt in. Everything will work reasonably well without it turned on
> > as we'll hopefully have a static estimate of scale which is good enough.
> > If aware software is using the device, it can enable this mode and
> > sample the scale as often as it wants to.
> >
> > I slightly favour option 3. What do you think? If we ever figure out
> > the meta data question for buffered case then we can make that work on top
> > of this.
> >
> > Jonathan
>
> This discussion made me revisit the calibration aspects in the ADC driver.
>
> We have three types of calibration in ADC:
>
> - Linear calibration: this calibration is not voltage or temperature
> dependent. So, it can be done once at boot time, as this is done currently.
>
> - offset calibration: this calibration has a voltage and temperature
> dependency. This calibration is currently done once at boot. But it
> would be relevant to allow application to request a new offset
> calibration, when supply or temperature change over time.
> Here the 'calibration' sysfs attribute you suggested in option 2, would
> be convenient I think. I plan to submit this improvement in a separate
> patch.
>
> - vref compensation: the vrefint channel offers a way to evaluate vref
> deviation. Here I need to change a bit the logic. I think that putting
> intelligence in the driver is not the best way at the end. This hides
> voltage deviation information, where it could be useful to check if an
> offset calibration is needed. Moreover we get a lack of consistency
> between raw and buffered data.
> It looks that processed type is the good way to expose vrefint channel.
> This allows to provide the actual vref voltage. And we can let the
> application decide how to manage this information. (trigger offset
> calibration / compensate raw data)
> I'm going to send a v2 serie with this change for vrefint support
> (plus the corrections for other comments)
Sounds like a good compromise solution to me. It's not great that we
end up with a 'somewhat' device specific solution, but as I've not previously
seen this particular form of calibration I am not that bothered by it
being device specific.
As ever, these stm32 parts continue to cut a new trail!
Thanks,
Jonathan
>
> Regards
> Olivier
>
> >>
> >> Regards
> >> Olivier
> >>
> >>>> + } else {
> >>>> + *val = adc->common->vref_mv * 2;
> >>>> + }
> >>>> *val2 = chan->scan_type.realbits;
> >>>> } else {
> >>>> - *val = adc->common->vref_mv;
> >>>> + /* Use vrefint data if available */
> >>>> + if (adc->vrefint.vrefint_data &&
> >>>> + adc->vrefint.vrefint_cal) {
> >>>> + *val = STM32_ADC_VREFINT_VOLTAGE *
> >>>> + adc->vrefint.vrefint_cal /
> >>>> + adc->vrefint.vrefint_data;
> >>>> + } else {
> >>>> + *val = adc->common->vref_mv;
> >>>> + }
> >>>> *val2 = chan->scan_type.realbits;
> >>>> }
> >>>> return IIO_VAL_FRACTIONAL_LOG2;
> >>>> @@ -1907,6 +1944,35 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev,
> >>>> return scan_index;
> >>>> }
> >>>>
> >>>> +static int stm32_adc_get_int_ch(struct iio_dev *indio_dev, const char *ch_name,
> >>>> + int chan)
> >>>
> >>> Naming would suggest to me that it would return a channel rather than setting it
> >>> inside adc->int_ch[i] Perhaps something like st32_adc_populate_int_ch() ?
> >>>
> >>>
> >>>> +{
> >>>> + struct stm32_adc *adc = iio_priv(indio_dev);
> >>>> + u16 vrefint;
> >>>> + int i, ret;
> >>>> +
> >>>> + for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
> >>>> + if (!strncmp(stm32_adc_ic[i].name, ch_name, STM32_ADC_CH_SZ)) {
> >>>> + adc->int_ch[i] = chan;
> >>>> + /* If channel is vrefint get calibration data. */
> >>>> + if (stm32_adc_ic[i].idx == STM32_ADC_INT_CH_VREFINT) {
> >>>
> >>> I would reduce indentation by reversing the logic.
> >>>
> >>> if (stm32_adc_ic[i].idx != STM32_ADC_INT_CH_VREFINT)
> >>> continue;
> >>>
> >>> ret =
> >>>> + ret = nvmem_cell_read_u16(&indio_dev->dev, "vrefint", &vrefint);
> >>>> + if (ret && ret != -ENOENT && ret != -EOPNOTSUPP) {
> >>>> + dev_err(&indio_dev->dev, "nvmem access error %d\n", ret);
> >>>> + return ret;
> >>>> + }
> >>>> + if (ret == -ENOENT)
> >>>> + dev_dbg(&indio_dev->dev,
> >>>> + "vrefint calibration not found\n");
> >>>> + else
> >>>> + adc->vrefint.vrefint_cal = vrefint;
> >>>> + }
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
> >>>> struct stm32_adc *adc,
> >>>> struct iio_chan_spec *channels)
> >>>> @@ -1938,10 +2004,9 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
> >>>> return -EINVAL;
> >>>> }
> >>>> strncpy(adc->chan_name[val], name, STM32_ADC_CH_SZ);
> >>>> - for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
> >>>> - if (!strncmp(stm32_adc_ic[i].name, name, STM32_ADC_CH_SZ))
> >>>> - adc->int_ch[i] = val;
> >>>> - }
> >>>> + ret = stm32_adc_get_int_ch(indio_dev, name, val);
> >>>> + if (ret)
> >>>> + goto err;
> >>>> } else if (ret != -EINVAL) {
> >>>> dev_err(&indio_dev->dev, "Invalid label %d\n", ret);
> >>>> goto err;
> >>>> @@ -2044,6 +2109,16 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping)
> >>>> */
> >>>> of_property_read_u32_index(node, "st,min-sample-time-nsecs",
> >>>> i, &smp);
> >>>> +
> >>>> + /*
> >>>> + * For vrefint channel, ensure that the sampling time cannot
> >>>> + * be lower than the one specified in the datasheet
> >>>> + */
> >>>> + if (channels[i].channel == adc->int_ch[STM32_ADC_INT_CH_VREFINT] &&
> >>>> + smp < adc->cfg->ts_vrefint_ns) {
> >>>> + smp = adc->cfg->ts_vrefint_ns;
> >>>> + }
> >>>
> >>> if (channels[i].channel == adc->int_ch[STM32_ADC_INT_CH_VREFINT])
> >>> smp = max(smp, adc->cfg->ts_vrefint_ns);
> >>>
> >>>> +
> >>>> /* Prepare sampling time settings */
> >>>> stm32_adc_smpr_init(adc, channels[i].channel, smp);
> >>>> }
> >>>> @@ -2350,6 +2425,7 @@ static const struct stm32_adc_cfg stm32mp1_adc_cfg = {
> >>>> .unprepare = stm32h7_adc_unprepare,
> >>>> .smp_cycles = stm32h7_adc_smp_cycles,
> >>>> .irq_clear = stm32h7_adc_irq_clear,
> >>>> + .ts_vrefint_ns = 4300,
> >>>> };
> >>>>
> >>>> static const struct of_device_id stm32_adc_of_match[] = {
> >>>
> >
