Jonathan Cameron wrote:
...
> > I could add the shunt-resistor controls to allow calibration as Marius
> > suggested, but that's also a custom ABI, what are your thoughts on this?
>
> This would actually be a generalization of existing device specific ABI
> that has been through review in the past.
> See Documentation/ABI/testing/sysfs-bus-iio-adc-pac1934
> for example (similar in other places).
> So if you want to do this move that ABI up a level to cover multiple devices
> (removing the entries in specific files as you do so).
>
I would do this in a separate commit, would you prefer it in this same patch
set or in another separate patch?
...
> >
> > > > +
> > > > +What: /sys/bus/iio/devices/iio:deviceX/resolution_bits_available
> > > > +KernelVersion: 6.10
> > > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > > +Description:
> > > > + List all possible ADC measurement resolutions: "11 14"
> > > > +
> > > > +What: /sys/bus/iio/devices/iio:deviceX/integration_samples
> > > > +KernelVersion: 6.10
> > > > +Contact: linux-iio@xxxxxxxxxxxxxxx
> > > > +Description:
> > > > + Number of samples taken during a full integration period. Can be
> > > > + set to any power of 2 value from 1 (default) to 2048.
> > > > + This attribute affects the integration time: higher the number
> > > > + of samples, longer the integration time. See Table 4-5 in device
> > > > + datasheet for details.
> > >
> > > Sounds like oversampling_ratio which is standards ABI. So use that or explain
> > > why you can't here.
> >
> > I am not sure that this is an oversampling ratio but correct me if I am wrong:
> > generally by increasing the oversampling you would have additional samples in a
> > fixed time period, while in this case by increasing the number of samples you
> > would still have the same number of samples in a fixed time period, but you
> > would have a longer integration period. So maybe the comment is not very
> > clear since this parameter actually means "the number of samples required to
> > complete the integration period".
>
> No. Oversampling is independent of the sampling period in general (though
> here the 'integration time' is very confusing terminology. You may
> have to have sampling_frequency (if provided) updated to incorporate that
> the device can't deliver data as quickly.
>
> >
> > Initially I thought to let the user edit this by writing the integration_time
> > control (which is currently read-only), but since the integration period
> > depends also on the resolution and whether filters are enabled or not, it would
> > have introduced some confusion: what parameter is being changed upon
> > integretion_time write? Maybe after removing resolution and filter controls
> > there would be no such confusion anymore.
>
> Hmm. The documentation seems to have an unusual definition of 'integration' time.
> That looks like 1/sampling_frequency. In an oversampling device integration time
> is normally about a single sample, not the aggregate of sampling and read out
> etc.
>
> I guess here the complexity is that integration time isn't about the time
> taken for a capacitor to charge, but more the time over which power is computed.
> But then the value is divided by number of samples so I'm even more confused.
>
> If we just read 'integration time' as data acquisition time, it makes a lot
> more sense.
>
I think I now get what you are suggesting, please correct me otherwise:
1. Let's consider the sampling frequency as how often the device provides
computed ("integrated") measurements to the host, so this would be
1/"integration period". This is not the internal ADC sampling rate.
2. I will expose sampling_frequency (RO), oversampling_ratio (R/W) and
oversampling_ratio_available (RO) to the user, where oversampling_ratio
corresponds to what the datasheet refers to as the "number of ADC samples to
complete an integration".
3. When the user writes the oversampling_ratio, the sampling_frequency gets
updated accordingly.
4. With two real examples:
4.1. The user writes 16 to oversampling_ratio, then reads 43.478 from
sampling_frequency: with 16 samples the "integration period" is 23ms
(from Table 4-5) so 1/0.023 => 43.478 Hz
4.2. The user writes 2048 to oversampling_ratio, then reads 0.34 from
sampling_frequency: with 2048 samples the "integration period" is 2941ms
(from Table 4-5) so 1/2.941 => 0.34 Hz
5. Do not expose the integration_time control to avoid confusion: the so called
"integration period" can be derived from the sampling frequency as
1/sampling_frequency.
...
> > > > +static int pac1921_update_cfg_reg(struct pac1921_priv *priv, unsigned int reg,
> > > > + unsigned int mask, unsigned int val)
> > > > +{
> > > > + /* Enter READ state before configuration */
> > > > + int ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
> > > > + PAC1921_INT_CFG_INTEN, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* Update configuration value */
> > > > + ret = regmap_update_bits(priv->regmap, reg, mask, val);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* Re-enable integration and reset start time */
> > > > + ret = regmap_update_bits(priv->regmap, PAC1921_REG_INT_CFG,
> > > > + PAC1921_INT_CFG_INTEN, PAC1921_INT_CFG_INTEN);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + priv->integr_start_time = jiffies;
> > >
> > > Add a comment for why this value.
> > >
> > Could you elaborate what's confusing here? The comment above states "reset
> > start time", maybe I should move it above the assignment of
> > priv->integr_start_time? Or it's the use of jiffies that it's confusing?
>
> Why is it jiffies? Why not jiffies * 42?
> I'm looking for a datasheet reference for why the particular value is used.
>
I used jiffies just to track the elapsed time between readings. Something I am
not considering here? Of course jiffies granularity might be larger than the
minimum sampling frequency. Is there a common better approach?
...
> For future reference, no need to acknowledge stuff you agree
> with. Much better to crop to the places where there are questions or responses
> as it saves time for the next step of the discussion!
Ok....oops!
>
> Thanks,
>
> Jonathan
>
Thanks,
Matteo
