Message-ID: <20240506151725.10cf025e@jic23-huawei>
Date: Mon, 6 May 2024 15:17:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Julien Stephan <jstephan@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, Mark Brown
 <broonie@...nel.org>, kernel test robot <lkp@...el.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling
 average oversampling mode

On Wed, 01 May 2024 16:55:42 +0200
Julien Stephan <jstephan@...libre.com> wrote:

> Adds support for rolling average oversampling mode.
> 
> Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> the most recent samples in the averaging calculation, allowing the ADC
> throughput rate and output data rate to stay the same, since we only need
> to take only one sample for each new conversion.
> 
> The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> in this mode (vs 1,  2, 4, 8, 16, 32 for the normal average)

Ah. I should have read on!

> 
> In order to be able to change the averaging mode, this commit also adds
> the new "oversampling_mode" and "oversampling_mode_available" custom
> attributes along with the according documentation file in
> Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> attributes correspond to this use case.

This comes to the comment I stuck in the previous patch.

To most people this is not a form of oversampling because the data rate
remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at:
https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter

in_voltage_low_pass_3db_frequency would be the most appropriate standard
ABI for this if we do treat it as a low pass filter control.

I'm not necessarily saying we don't want new ABI for this, but I would
like to consider the pros and cons of just using the 3db frequency.

So would that work for this part or am I missing something?

> 
> Signed-off-by: Julien Stephan <jstephan@...libre.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 |  38 ++++++
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/ad7380.c                           | 143 +++++++++++++++++++--
>  3 files changed, 174 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> new file mode 100644
> index 000000000000..0a560ef3e32a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> @@ -0,0 +1,38 @@
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <michael.hennerich@...log.com>
> +Description:
> +    Writing this attribute sets the oversampling average mode.
> +    Reading it, shows the configured mode.
> +    Available modes can be displayed using the oversampling_mode_available
> +    attribute.
> +    When writing this attribute to change the oversampling mode, this will
> +    have the following side effects:
Where possible, write ABI docs with the assumption we will generalize
them in future. Annoyingly the documentation system doesn't allow for
multiple descriptions. As such, additional information like this doesn't
belong in the ABI docs.

> +
> +      - soft reset the ADC to flush the oversampling block and FIFO
I think this was already picked up on in another review, but my inclination is
make this something you can't change with the buffer enabled. The results
will be rather unpredictable anyway and it will simplify the handling a little
to just block that corner with a claim (or failure to claim) direct mode
when setting this.

> +
> +      - the available oversampling ratios depend on the oversampling mode
> +        configured so to avoid misconfiguration, changing the mode will disable
> +        the oversampling by setting the ratio to 1.

Better to get a close as possible.  If they've configured it to something we can't
do then it's user error. If they have picked a value that is still possible then
allowing that is a nice usability improvement.

> +
> +      - the list of available ratios (displayed by reading the
> +        oversampling_ratio_available attribute) will be updated when changing
> +        the oversampling mode.

In general an ABI element is allowed to modify any other (because this sort of
chaining is common.)  As such I don't think this needs to be in the ABI docs
but would be a useful detail to add to a chip specific main document elsewhere.

> +
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <michael.hennerich@...log.com>
> +Description:
> +    Display the available oversampling average modes. The two available modes
> +    are "normal" and "rolling" where "normal" average mode is the default one.
> +
> +      - normal averaging involves taking a number of samples, adding them
> +        together, and dividing the result by the number of samples taken.
> +        This result is then output from the device. The sample data is cleared
> +        when the process completes. Because we need more samples to output a
> +        value, the data output rate decrease with the oversampling ratio.
> +
> +      - rolling oversampling mode uses a first in, first out (FIFO) buffer of
> +        the most recent samples in the averaging calculation, allowing the ADC
> +        throughput rate and output data rate to stay the same, since we only need
> +        to take only one sample for each new conversion.

If we keep this, I wonder if "moving" or "rolling" is the more common term for this.


> +
> +static IIO_DEVICE_ATTR_RW(oversampling_mode, 0);
> +static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0);
> +
> +static struct attribute *ad7380_attributes[] = {
> +	&iio_dev_attr_oversampling_mode.dev_attr.attr,
> +	&iio_dev_attr_oversampling_mode_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ad7380_attribute_group = {
> +	.attrs = ad7380_attributes,
> +};

Bring the sysfs includes in here rather than in the original driver patch.

Thanks,

Jonathan

Powered by blists - more mailing lists