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]
Date: Wed, 12 Jun 2024 10:57:42 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...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>,
        "Hennerich,
 Michael" <Michael.Hennerich@...log.com>,
        Nuno Sá
	<noname.nuno@...il.com>
Subject: RE: [PATCH v3 1/5] iio: ABI: Generalize ABI documentation for DAC



> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Saturday, June 8, 2024 10:41 PM
> To: Paller, Kim Seer <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>; Hennerich, Michael
> <Michael.Hennerich@...log.com>; Nuno Sá <noname.nuno@...il.com>
> Subject: Re: [PATCH v3 1/5] iio: ABI: Generalize ABI documentation for DAC
> 
> [External]
> 
> 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.

I agree that the description could be clear about the differences between 
autonomous and software toggling. If we were to change the description, would 
this suffice?

Description:
        Toggle enable. Write 1 to enable toggle or 0 to disable it. This
        is useful when one wants to change the DAC output codes. For autonomous toggling, 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.

For software toggling, one can switch to A, modify B, switch to B, modify A, etc.
> 
> > +
> > +        	- disable toggle operation;
> > +        	- change out_voltageY_rawN, where N is the integer value of the
> symbol;
> > +        	- enable toggle operation.