On Tue, 2024-07-09 at 10:21 +0200, Matteo Martelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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.
Let me came with some clarifications. Internal sampling frequency for
this device has a fixed value. Based on the number of samples the PAC
will internally accumulate the read values and then the average is
calculated.
I think the "best" approach is to use the oversampling_ratio and the
frequency gets updated accordingly and it will be RO. We will also need
the oversampling_ratio_available.
>
> 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
>
Thanks,
Marius
