linux-kernel - Re: [PATCH v3 1/5] iio: ABI: Generalize ABI documentation for DAC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240608154053.1cf1097e@jic23-huawei>
Date: Sat, 8 Jun 2024 15:40:53 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Kim Seer Paller <kimseer.paller@...log.com>
Cc: <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, David Lechner <dlechner@...libre.com>,
 Lars-Peter Clausen <lars@...afoo.de>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Dimitri Fedrau <dima.fedrau@...il.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, "Rob Herring" <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, "Michael Hennerich"
 <michael.hennerich@...log.com>, Nuno Sá
 <noname.nuno@...il.com>
Subject: Re: [PATCH v3 1/5] iio: ABI: Generalize ABI documentation for DAC

On Mon, 3 Jun 2024 09:21:56 +0800
Kim Seer Paller <kimseer.paller@...log.com> wrote:

> Introduces a more generalized ABI documentation for DAC. Instead of
> having separate ABI files for each DAC, we now have a single ABI file
> that covers the common sysfs interface for all DAC.
> 
> Co-developed-by: Michael Hennerich <michael.hennerich@...log.com>
> Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>

A few comments inline.

I wondered if it made sense to combine voltage and current entries of each type
in single block, but I think the docs would become too complicated with lots
of wild cards etc.  Hence I think the duplication is fine.

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio-dac   | 61 +++++++++++++++++++
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 31 ----------
>  2 files changed, 61 insertions(+), 31 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac b/Documentation/ABI/testing/sysfs-bus-iio-dac
> new file mode 100644
> index 000000000000..36d316bb75f6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac
> @@ -0,0 +1,61 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_currentY_toggle_en
> +KernelVersion:	5.18
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +       		Toggle enable. Write 1 to enable toggle or 0 to disable it. This
Tab vs space issue - see below.

> +		is useful when one wants to change the DAC output codes. The way
> +		it should be done is:
> +
> +        	- disable toggle operation;
> +        	- change out_currentY_rawN, where N is the integer value of the symbol;
> +        	- enable toggle operation.
Same question as below on whether this is accurate - Maybe it just needs to mention
this scheme needs to be used for autonomous toggling (out of software control).
It works for software toggling but may be overkill!

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_currentY_rawN
> +KernelVersion:	5.18
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		This attribute has the same meaning as out_currentY_raw. It is
> +		specific to toggle enabled channels and refers to the DAC output
> +		code in INPUT_N (_rawN), where N is the integer value of the symbol.
> +		The same scale and offset as in out_currentY_raw applies.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_currentY_symbol
> +KernelVersion:	5.18
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		Performs a SW switch to a predefined output symbol. This attribute
> +		is specific to toggle enabled channels and allows switching between
> +		multiple predefined symbols. Each symbol corresponds to a different
> +		output, denoted as out_currentY_rawN, where N is the integer value
> +		of the symbol. Writing an integer value N will select out_currentY_rawN.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
> +KernelVersion:	5.18
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +       		Toggle enable. Write 1 to enable toggle or 0 to disable it. This

Mix of spacing and tabs is inconsistent. Hence the odd indent in this reply version.

> +		is useful when one wants to change the DAC output codes. The way
> +		it should be done is:

Hmm. Is this true?  If we are doing autonomous toggling on a clock or similar than agreed.
If we are using the out_current_symbol software control it would be common to switch
to A, modify B, switch to B, modify A etc.

I think our interface has probably evolved and so this might need an update.

> +
> +        	- disable toggle operation;
> +        	- change out_voltageY_rawN, where N is the integer value of the symbol;
> +        	- enable toggle operation.