Re: [PATCH v2 14/16] iio: adc: ad7768-1: add support for Synchronization over SPI

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

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@analog.com>
> ---
> 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@analog.com/

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;
> +}
> +
...