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

[Jonathan Cameron]

On Wed, 12 Jun 2024 10:57:42 +0000
"Paller, Kim Seer" <KimSeer.Paller@xxxxxxxxxx> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Sent: Saturday, June 8, 2024 10:41 PM
> > To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; David Lechner <dlechner@xxxxxxxxxxxx>; Lars-
> > Peter Clausen <lars@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>;
> > Mark Brown <broonie@xxxxxxxxxx>; Dimitri Fedrau <dima.fedrau@xxxxxxxxx>;
> > Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> > Conor Dooley <conor+dt@xxxxxxxxxx>; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>; Nuno Sá <noname.nuno@xxxxxxxxx>
> > 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>  
> > 
> > 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@xxxxxxxxxxxxxxx
> > > +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@xxxxxxxxxxxxxxx
> > > +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@xxxxxxxxxxxxxxx
> > > +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@xxxxxxxxxxxxxxx
> > > +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.

To here is good as focuses on the use case.

> 
> For software toggling, one can switch to A, modify B, switch to B, modify A, etc.

I'd not mention this part (not sure if you were intending to though given the formatting!)

Jonathan


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