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

[David Lechner <dlechner@baylibre.com>]

On 1/27/25 9:14 AM, 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);
> +	} else if (st->gpio_sync_in) {

Redundant else after return.

> +		gpiod_set_value_cansleep(st->gpio_sync_in, 1);
> +		gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {
> @@ -348,10 +365,7 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
>  		return ret;
>  
>  	/* A sync-in pulse is required every time the filter dec rate changes */
> -	gpiod_set_value(st->gpio_sync_in, 1);
> -	gpiod_set_value(st->gpio_sync_in, 0);
> -
> -	return 0;
> +	return ad7768_send_sync_pulse(st);
>  }
>  
>  static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> @@ -638,6 +652,58 @@ static const struct iio_info ad7768_info = {
>  	.debugfs_reg_access = &ad7768_reg_access,
>  };
>  
> +static int ad7768_parse_trigger_sources(struct device *dev, struct ad7768_state *st)
> +{
> +	struct fwnode_reference_args args;
> +	int ret;
> +
> +	/*
> +	 * The AD7768-1 allows two primary methods for driving the SYNC_IN pin to synchronize
> +	 * one or more devices:
> +	 * 1. Using a GPIO to directly drive the SYNC_IN pin.
> +	 * 2. Using a SPI command, where the SYNC_OUT pin generates a synchronization pulse
> +	 *    that loops back to the SYNC_IN pin.
> +	 *
> +	 * In multi-device setups, the SYNC_IN pin can also be driven by the SYNC_OUT pin of
> +	 * another device. To support this, we use the "trigger-source" property to get a
> +	 * reference to the "main" device responsible for generating the synchronization pulse.
> +	 * In a single-device setup, the "trigger-source" property should reference the device's
> +	 * own node.
> +	 */
> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "trigger-sources",
> +						 "#trigger-source-cells", 0, 0, &args);

The trigger-sources property is optional, so we should have a special case for
ret == -EINVAL here. Otherwise existing users that use the gpio binding will
fail here.

> +	if (ret) {
> +		dev_err(dev, "Failed to get trigger-sources reference: %d\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	st->gpio_sync_in = devm_gpiod_get_optional(args.fwnode->dev, "adi,sync-in",
> +						   GPIOD_OUT_LOW);

I would put this in the -EINVAL special case mentioned above, then we don't
nee to use the _optional variant.

> +	if (IS_ERR(st->gpio_sync_in))
> +		return PTR_ERR(st->gpio_sync_in);

This leaks args.fwnode reference on return (but would be fixed if making the
above suggested change).

> +
> +	/*
> +	 * If the SYNC_IN GPIO is not defined, fall back to synchronization over SPI.
> +	 * Use the trigger-source reference to identify the main SPI device for generating
> +	 * the synchronization pulse.
> +	 */
> +	if (!st->gpio_sync_in) {
> +		st->sync_source_spi = to_spi_device(args.fwnode->dev);

This seems unsafe. The DT could be pointing to something other than a SPI device.
to_spi_device() calls container_of() so would cause undefined behavior if dev
was something else. Also, even if it is a spi device, we don't know what state
it is in and if it is safe to use it.

I think it would be sufficient for now to just check that args.fw_node is the
the same one as this SPI device since that is the supported case. 

> +		if (!st->sync_source_spi) {
> +			dev_err(dev,
> +				"Synchronization setup failed. GPIO is undefined and trigger-source reference is invalid\n");
> +			return -EINVAL;

Leaks args.fwnode on return. Also, dev_err_probe().

> +		}
> +
> +		st->en_spi_sync = true;
> +	}
> +
> +	/* Release the fwnode reference after use */
> +	fwnode_handle_put(args.fwnode);
> +
> +	return 0;
> +}
> +
>  static int ad7768_setup(struct iio_dev *indio_dev)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> @@ -668,10 +734,9 @@ static int ad7768_setup(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> -	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
> -					  GPIOD_OUT_LOW);
> -	if (IS_ERR(st->gpio_sync_in))
> -		return PTR_ERR(st->gpio_sync_in);
> +	ret = ad7768_parse_trigger_sources(&st->spi->dev, st);
> +	if (ret)
> +		return ret;
>  
>  	/* Only create a Chip GPIO if flagged for it */
>  	if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {