Date:   Fri, 12 Nov 2021 17:56:25 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Antoniu Miclaus <antoniu.miclaus@analog.com>
Cc:     <robh+dt@kernel.org>, <linux-iio@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation
Message-ID: <20211112175625.4a9f393d@jic23-huawei>
In-Reply-To: <20211109123127.96399-5-antoniu.miclaus@analog.com>
References: <20211109123127.96399-1-antoniu.miclaus@analog.com>
        <20211109123127.96399-5-antoniu.miclaus@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 9 Nov 2021 14:31:27 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add initial ABI documentation for admv8818 filter sysfs interfaces.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818 b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> new file mode 100644
> index 000000000000..7fa5b0819055
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> @@ -0,0 +1,60 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The cut-off frequency of the ADMV8818 high pass filter. The value is scaled using
> +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> +		The accepted range of values for the frequencies is between 1.75GHz and 19.9GHz.
> +
> +		The default value for the scale is 1000000, therefore MHz frequency values are
> +		passed as input.

I don't think this ABI really works unfortunately.  What we are talking here is a bunch of
selectable filters and one high pass + one low pass filter max can be enabled at a time.

So two options, we either have simply a single
out_altvoltage_filter_low_pass_3db_frequency
out_altvoltage_filter_high_pass_3db_frequency
Probably both with index 0 and index free channels are a silly idea given it's fine to just have
one with index 0.

or if there is sufficient reason to setup a selectable set of options then
we could look at indexed filters and a _symbol type selection which may seem
odd but generalises fairly well from Phase Shift Keying type symbol stuff we
have had before (though still in staging because no one has cleaned the drivers
up yet).


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The cut-off frequency of the ADMV8818 low pass filter. The value is scaled using
> +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> +		The accepted range of values for the frequencies is between 2.05GHz and 18.85GHz.
> +
> +		The default value for the scale is 1000000, therefore MHz frequency values are
> +		passed as input.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Scale high pass and lowpass filter frequency values to Hz.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading this returns the valid values that can be written to the
> +		on_altvoltage0_mode attribute:
> +
> +		- auto -> enable/register the clock rate notifier

Hmm I'm wondering about the usecases of this.

If this is being used with a clk device, then I think only the notifier option makes much
sense.  If it's not a clk that linux is aware of then manual makes more sense.

> +		- manual -> disable/unregister the clock rate notifier
> +		- bypass -> bypass LPF/HPF and disable/unregister the clock rate notifier

This should be separate enable for the two filters though I think we've use the value 0
to mean this in the past.  The bypasses look to be per filter anyway, so a single
mode is insufficiently flexible.

In the vast majority of cases, mode attributes are not used because they are always device
specific and hence generic code has no idea what to do with them.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute configures the filter mode.
> +		Reading returns the actual mode.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_bandwidth_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Store the band pass bandwidth frequency value applied.
> +		Reading returns the bandwidth frequency scaled.

The device has no concept of bandpass that I can find so why are we introducing it?
Let the user set the two filters to achieve this result.  Userspace can do the maths for us :)

> +
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_center_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Store the band pass center frequency value applied.
> +		Reading returns the center frequency scaled.