Message-ID: <668cf2f3ece62_1f6ba37012@njaxe.notmuch>
Date: Tue, 09 Jul 2024 10:21:07 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
 Lars-Peter Clausen <lars@...afoo.de>, 
 Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski <krzk+dt@...nel.org>, 
 Conor Dooley <conor+dt@...nel.org>, 
 Marius Cristea <marius.cristea@...rochip.com>, 
 Matteo Martelli <matteomartelli3@...il.com>, 
 linux-iio@...r.kernel.org, 
 devicetree@...r.kernel.org, 
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: add support for pac1921

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@...r.kernel.org
> > > > +Description:
> > > > +		List all possible ADC measurement resolutions: "11 14"
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/integration_samples
> > > > +KernelVersion:	6.10
> > > > +Contact:	linux-iio@...r.kernel.org
> > > > +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

Powered by blists - more mailing lists