Re: [PATCH v10 09/12] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 5 Jun 2025 09:10:14 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Jun 04, 2025 at 04:37:09PM -0300, 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 the SYNC_IN pin.
> > 
> > Use trigger-sources property to enable device synchronization over SPI
> > and multi-device synchronization while replacing sync-in-gpios property.  
> 
> ...
> 
> > +static int ad7768_trigger_sources_sync_setup(struct device *dev,
> > +					     struct fwnode_handle *fwnode,
> > +					     struct ad7768_state *st)
> > +{
> > +	struct fwnode_reference_args args;
> > +	struct fwnode_handle *ref __free(fwnode_handle) = NULL;

Sorry I was slow to reply to v9 discussion but splitting the destructor
declaration from the constructor isn't an acceptable path.  If
the general form proposed in the previous version isn't acceptable, spin
a local version in this driver doing the same thing.

Jonathan


> > +	int ret;
> > +
> > +	ret = fwnode_property_get_reference_args(fwnode, "trigger-sources",
> > +						 "#trigger-source-cells", 0,
> > +						 AD7768_TRIGGER_SOURCE_SYNC_IDX, &args);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ref = args.fwnode;
> > +	/* First, try getting the GPIO trigger source */
> > +	if (fwnode_device_is_compatible(ref, "gpio-trigger")) {  
> 
> > +		st->gpio_sync_in = devm_fwnode_gpiod_get_index(dev, ref, NULL,
> > +							       0,  
> 
> This 0 is close to NULL semantically, please move it to the above line to make
> the split more logical.
> 
> > +							       GPIOD_OUT_LOW,
> > +							       "sync-in");
> > +		return PTR_ERR_OR_ZERO(st->gpio_sync_in);
> > +	}
> > +
> > +	/*
> > +	 * TODO: Support the other cases when we have a trigger subsystem
> > +	 * to reliably handle other types of devices as trigger sources.
> > +	 *
> > +	 * For now, return an error message. For self triggering, omit the
> > +	 * trigger-sources property.
> > +	 */
> > +	return dev_err_probe(dev, -EOPNOTSUPP, "Invalid synchronization trigger source\n");
> > +}  
>