Re: [PATCH v2 7/7] Documentation: ABI: testing: ad485x: add ABI docs

[Nuno Sá]

On Sat, 2024-10-05 at 18:36 +0100, Jonathan Cameron wrote:
> On Fri, 4 Oct 2024 17:07:56 +0300
> Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote:
> 
> > Add documentation for the packet size.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > ---
> > changes in v2:
> >  - improve description for packet_format
> >  - add kernel version
> >  .../ABI/testing/sysfs-bus-iio-adc-ad485x         | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> > b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> > new file mode 100644
> > index 000000000000..5d69a8d30383
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> > @@ -0,0 +1,16 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/packet_format_available
> > +KernelVersion:	6.13
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Packet sizes on the CMOS or LVDS conversion data output
> > bus.
> > +		Reading this returns the valid values that can be written
> > to the
> > +		packet_format.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/packet_format
> > +KernelVersion:	6.13
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		This attribute configures the frame size on conversion data
> > +		output bus. See packet_format_available for available sizes
> > +		based on the device used.
> > +		Reading returns the actual size used.
> This needs to give some guidance to the user on 'why' they might pick a
> particular
> format.
> 
> I'm also inclined to suggest that for now we pick a sensible default dependent
> on the other options enabled (oversampling etc) and don't expose it to the
> user. 
> 

Just for documentations and if someone does not want to check the datasheet :):

- Nonoversampling Packet Formats:

			  -------------------
20bit (packet_size = 0) - |20 bit conversion|
			  -------------------

                          -------------------------------------------
24bit (packet_size = 1) - |20 bit conversion| OR/UR | 3 bit chan_id |
			  -------------------------------------------

                          --------------------------------------------------------------------
32bit (packet_size = 2) - |20 bit conversion| OR/UR | 3 bit chan_id | 4 bit softspan | 0s... |
			  --------------------------------------------------------------------

- Oversampling Packet Formats

			  -------------------
20bit (packet_size = 0) - |20 bit conversion|
			  -------------------

                          --------------------
24bit (packet_size = 1) - |24 bit conversion |
			  --------------------

                          ------------------------------------------------------------
32bit (packet_size = 2) - |24 bit conversion| OR/UR | 3 bit chan_id | 4 bit softspan |
			  ------------------------------------------------------------

> Eventually it looks like we may have to figure out a solution to describe
> metadata packed alongside the channel readings but that may take a while
> and I don't want to stall this driver on that discussion.
> 

There's something still not fully clear to me. So, oversampling would be easy to
deal with (for arguing about some packet size). OR/UR is the more tricky case
because of having metadata. But I'm trying to understand on how it could look
because we still need a way to enable/disable OR/UR. 

Do you have in mind to have a form of metadata description in 'struct
iio_scan_types' plus additional IIO_METADATA channels to enable/disable those
bits? For this usecase and as OR/UR actually depends on the packet format we
could flip things with something like in_metadataX_enable and then argue about
the packet size settings.

But things get messier if we look closer to the packets as we can see that for
non oversampled samples, the softspan info is optional. Now, I'm not convinced
about that information being useful in the sample as we already have the scale
attribute and I'm not expecting people to change it during buffering... But for
the fun of things, how could we handle it if we cared (or if we actually care)
about it? Custom ABI like in_metadataX_scan_enable?

Other thing that came to mind and that might be a sneaky way of bypassing the
metadata stuff (for now) is to use events. AFAIU, we have status registers were
we can check the OR/UR and push those as normal events. But we would need to
rely on an external trigger as hrtimer or something like that. So we could have
this "slowpath" for checking the channel status but still use the events ABI
(and this is the sneaky part) to argue about the packet_size and whether or not
that info will be available in the sample through DMA. Not sure if it's worth it
though...

- Nuno Sá