Re: [PATCH v2 2/2] iio: adc: add support for pac1921

[Marius.Cristea]

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