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