On 12/8/24 12:43 PM, Jonathan Cameron wrote: > On Mon, 2 Dec 2024 16:21:43 -0600 > David Lechner <dlechner@xxxxxxxxxxxx> wrote: > >> On 11/30/24 12:38 PM, Jonathan Cameron wrote: >>> On Thu, 28 Nov 2024 14:55:03 +0200 >>> Alisa-Dariana Roman <alisadariana@xxxxxxxxx> 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@xxxxxxxxxx> >>> 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.
