Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[David Lechner <dlechner@baylibre.com>]

On 12/8/24 12:43 PM, Jonathan Cameron wrote:
> On Mon, 2 Dec 2024 16:21:43 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 11/30/24 12:38 PM, Jonathan Cameron wrote:
>>> On Thu, 28 Nov 2024 14:55:03 +0200
>>> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
>>>   
>>>> Add support for the SYNC pin of AD719x devices. This pin is controlled
>>>> through a GPIO. The pin allows synchronization of digital filters and
>>>> analog modulators when using multiple devices.
>>>>
>>>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>  
>>> Hi.
>>>
>>> Like all userspace ABI, this needs documentation.
>>>
>>> It's an unusual feature, so some usecases would help.
>>>
>>> It is also cross multiple devices which makes this odd as only one device
>>> can presumably acquire the gpio?
>>>
>>> An alternative would be to look at how to do this with a 'wrapper' sort of device
>>> so that we have one instance to which this applies.
>>>
>>> I'm not sure that helps that much though as we'd still need some for of
>>> 'I'm setup for all channels, now you can go' ABI.
>>>
>>> Jonathan
>>>   
>>
>> Giving userspace direct control over the /SYNC pin without coordinating
>> with the rest of the driver does seem like it could be asking for trouble.
>>
>> It seems like the only time you would want to actually toggle the /SYNC
>> pin is when starting a buffered read.
>>
>> 1. Deassert /SYNC so that no conversions can be triggered.
>> 2. Enable buffered reads for all chips connected to the same GPIO.
>> 3. Assert /SYNC to start all conversions at the same time.
>>
>> So it could make sense to integrate this into the buffer pre/post enable
>> callbacks somehow instead of adding a new sysfs attribute.
>>
>> For the "wrapper" device, maybe we could do something with configfs to
>> enable dynamically connecting multiple device instances? We might not
>> need to actually create a separate device in sysfs, but just do something
>> so that enabling a buffered read on the first chip will enable buffered
>> reads on all of the chips in the group.
>>
>> It seems like we have some other chips that are currently being worked on
>> that also have the possibility of some sort of multi-chip synchronization
>> like this so it would be nice to come up with a general solution.
> 
> Most of the multichip cases we've had before have been chained, rather
> than separate data interfaces, hence I don't recall us needing something
> like this before.
> 
>>
>> Another use case for a general synchronized buffered read/write between
>> multiple chips would be the AD3552R DAC. Recently, while adding support
>> for an IIO backend for this chip, we saw that the AXI DAC backend has a
>> synchronization feature like this where you set an "arm" bit on all AXI
>> DAC instances. Then when you enable streaming to the first chip, it also
>> triggers all of the other AXI DAC blocks to start streaming at the same
>> time. We ended up not implementing that feature since the IIO subsystem
>> doesn't really support this yet, but could be a good one to look at as a
>> similar feature with a different implementation to help us find a general
>> solution.
>>
> This feels like a case where we need a prototype to poke holes in.
> It's not totally dissimilar from the hardware trigger stuff that
> exists in a few devices. Some of the stm parts for instance where the
> triggering is entirely within the chip.  Maybe we could make something
> like that work.  So the driver instance that has the sync pin registers
> a trigger that the other devices use.   It's a bit ugly though and we'd
> still need a dt-binding to let us know 'which' devices are connected
> to that sync pin.
> 
> Jonathan
> 
> 

A shared trigger was one of the ideas that crossed my mind as well.
Are you suggesting that the "main" chip would have the sync-gpios
property in it's .dts node and then the other chips would have a
trigger-sources = <&main_adc>; property instead of the sync-gpios
property? (FYI, trigger-sources/#trigger-source-cells are becoming
are becoming general properties with the SPI offload work I have
been doing, so should be available to use soon-ish).

Now, to try to poke a hole in this idea...

It seems like most (all?) triggers are set up to trigger an individual
sample. But in this case, the /SYNC pin would just be triggering the
start of a buffered read, so triggering multiple samples. Does this
still fit within the definition of a struct iio_trigger?

One way to possibly make it work without a struct iio_trigger could
work like this:
* During probe, "main" chips sets /SYNC so that chips won't sample
  data.
* Set up and enable a buffered read for all non-main chips that have
  the /SYNC pin connected to a common GPIO. Chips don't actually start
  sampling when buffer is enabled due to /SYNC pin state.
* Enable the buffer on the "main" chip last. This changes the /SYNC
  pin state in the buffer pre/post enable and all chips start
  sampling at the same time.
* Read from all buffers for as long as you want.
* Similarly, to stop, the "main" chip should have it's buffer disabled
  first, which changes the /SYNC pin state causing all chips to stop
  sampling.
* Then other buffers can be disabled.

Alternately, we could have a struct iio_trigger and expose control of
the /SYNC pin state as a sysfs attribute on the trigger. This would be
more like what Alisa-Dariana has proposed already.

It could work like this:
* Set up and enable a buffered read for all chips that have the /SYNC pin
  connected to a common GPIO. Chips don't actually start sampling when
  buffer is enabled due to /SYNC pin state.
* Write to a new trigger sysfs attribute to start sampling on all chips at
  the same time.
* Read from all buffers for as long as you want.
* Write to the trigger sysfs attribute to stop sampling on all chips at
  the same time.
* Disable buffers on all chips.