On 01/27, Jonathan Santos wrote:
> The synchronization method using GPIO requires the generated pulse to be
> truly synchronous with the base MCLK signal. When it is not possible to
> do that in hardware, the datasheet recommends using synchronization over
> SPI, where the generated pulse is already synchronous with MCLK. This
> requires the SYNC_OUT pin to be connected to SYNC_IN pin. In
> multidevices setup, the SYNC_OUT from other devices can be used as
> synchronization source.
>
> Use trigger-sources property to enable device synchronization over SPI
> for single and multiple devices.
>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> ---
> v2 Changes:
> * Synchronization via SPI is enabled when the Sync GPIO is not defined.
> * now trigger-sources property indicates the synchronization provider or
> main device. The main device will be used to drive the SYNC_IN when
> requested (via GPIO or SPI).
> ---
> drivers/iio/adc/ad7768-1.c | 81 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 7686556c7808..01ccbe0aa708 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -193,6 +193,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>
> struct ad7768_state {
> struct spi_device *spi;
> + struct spi_device *sync_source_spi;
> struct regmap *regmap;
> struct regulator *vref;
> struct mutex lock;
> @@ -206,6 +207,7 @@ struct ad7768_state {
> struct iio_trigger *trig;
> struct gpio_desc *gpio_sync_in;
> struct gpio_desc *gpio_reset;
> + bool en_spi_sync;
> const char *labels[ARRAY_SIZE(ad7768_channels)];
> /*
> * DMA (thus cache coherency maintenance) may require the
> @@ -264,6 +266,21 @@ static int ad7768_spi_reg_write(void *context,
> return spi_write(st->spi, st->data.d8, 2);
> }
>
> +static int ad7768_send_sync_pulse(struct ad7768_state *st)
> +{
> + if (st->en_spi_sync) {
> + st->data.d8[0] = AD7768_WR_FLAG_MSK(AD7768_REG_SYNC_RESET);
> + st->data.d8[1] = 0x00;
> +
> + return spi_write(st->sync_source_spi, st->data.d8, 2);
Hmm, is it really OK to do something like that? I mean, is that safe?
I wonder if that can lead to race conditions if, for example, sync_source_spi
is already registered as an IIO device doing buffered capture.
Or maybe sync_source_spi is in the middle of some read-modify-write cycle
and we sneak in this transfer. I think we also don't know from here if
sync_source_spi device is expecting to have it's AD7768_REG_SYNC_RESET changed.
For multi-device synchronization, I'm still biased towards something like we
had for for quad-adaq7768-1 [1] with one device managing the others.
I would like to try something like that along with the component infrastructure
(include/linux/component.h, drivers/base/component.c), but I already have
a bunch of pending stuff.
Basic idea would be to treat multiple ADCs that are intended for synchronized
read as a single aggregated device (that would have and manage all individual
SPI devices) with component infrastructure.
I suspect that might be useful for the AD7192 [2] case as well.
[1]: https://github.com/analogdevicesinc/linux/tree/adaq7768-aggregate-dev
[2]: https://lore.kernel.org/linux-iio/20241128125811.11913-4-alisa.roman@xxxxxxxxxx/
Anyway, I guess better to just do what David suggested and check the
trigger-source device is the same as this SPI device and then write to our own
registers or error out otherwise.
> + } else if (st->gpio_sync_in) {
> + gpiod_set_value_cansleep(st->gpio_sync_in, 1);
> + gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> + }
> +
> + return 0;
> +}
> +
...
