Re: [PATCH v6 08/11] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 27 Apr 2025 21:13:47 -0300
Jonathan Santos <Jonathan.Santos@analog.com> 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.
> 
> Use trigger-sources property to enable device synchronization over SPI
> and multi-device synchronization, as an alternative to adi,sync-in-gpios
> property.
> 

> +static int ad7768_trigger_sources_get_sync(struct device *dev,
> +					   struct ad7768_state *st)
> +{
> +	struct fwnode_reference_args args;
> +	struct fwnode_handle *fwnode = NULL;
> +	int ret;
> +
> +	/*
> +	 * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
> +	 * to synchronize one or more devices:
> +	 * 1. Using an external GPIO.
> +	 * 2. Using a SPI command, where the SYNC_OUT pin generates a
> +	 *    synchronization pulse that drives the SYNC_IN pin.
> +	 */
> +	if (!device_property_present(dev, "trigger-sources")) {
> +		/*
> +		 * In the absence of trigger-sources property, enable self
> +		 * synchronization over SPI (SYNC_OUT).
> +		 */
> +		st->en_spi_sync = true;
> +		return 0;
> +	}
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells",
> +						 0,
> +						 AD7768_TRIGGER_SOURCE_SYNC_IDX,
> +						 &args);
> +	if (ret)
> +		return ret;
> +
> +	fwnode = args.fwnode;
> +	/*
> +	 * First, try getting the GPIO trigger source and fallback to
> +	 * synchronization over SPI in case of failure.
> +	 */
> +	st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
> +	if (IS_ERR(st->gpio_sync_in)) {
> +		/*
> +		 * For this case, it requires one argument, which indicates the
> +		 * output pin referenced.
> +		 */
> +		if (args.nargs < 1)
> +			goto err_not_supp;
> +
> +		if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
> +			goto err_not_supp;
> +
> +		/*
> +		 * Only self trigger is supported for now, i.e.,
> +		 * external SYNC_OUT is not allowed.
> +		 */
> +		if (fwnode->dev == dev) {
> +			st->en_spi_sync = true;
> +			goto out_put_node;
> +		}
> +
> +		goto err_not_supp;
> +	}
> +
> +	goto out_put_node;
> +
> +err_not_supp:

Split the good and bad paths here so we don't have good paths jumping to
mid way through the block. If the good paths need to jump then I'd
suggest factoring out the stuff with the node held into a separate function.
Superficially it just looks like one condition flip and you are refactoring
this anyway based on other feedback so maybe this comment becomes irrelevant!


> +	ret = dev_err_probe(dev, -EOPNOTSUPP,
> +			    "Invalid synchronization trigger source");
> +out_put_node:
> +	fwnode_handle_put(args.fwnode);
> +	return ret;
> +}
> +