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.
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Created macro for the SYNC index from trigger-sources.
* Check trigger source by the compatible string (and the dev node for the
self triggering).
* Check nargs before accessing the args array.
* Use `trigger-sources` as an alternative to `adi,sync-in-gpios`
(now optional), instead of replacing it.
v5 Changes:
* Allow omitting trigger-sources property.
* include gpio-trigger to trigger-sources to replace adi,sync-in-gpios
property.
* Read trigger-sources cell value to differentiate the trigger type.
v4 Changes:
* None.
v3 Changes:
* Fixed args.fwnode leakage in the trigger-sources parsing.
* Synchronization over spi is enabled when the trigger-sources
references the own device.
* Synchronization is kept within the device, and return error if the
gpio is not defined and the trigger-sources reference does not match
the current device.
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 | 126 +++++++++++++++++++++++++++++++++++--
1 file changed, 120 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 087478afb5bf..c00571f17254 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -28,6 +28,8 @@
#include <linux/iio/triggered_buffer.h>
#include <linux/iio/trigger_consumer.h>
+#include <dt-bindings/iio/adc/adi,ad7768-1.h>
+
/* AD7768 registers definition */
#define AD7768_REG_CHIP_TYPE 0x3
#define AD7768_REG_PROD_ID_L 0x4
@@ -100,6 +102,8 @@
#define AD7768_VCM_OFF 0x07
+#define AD7768_TRIGGER_SOURCE_SYNC_IDX 0
+
enum ad7768_conv_mode {
AD7768_CONTINUOUS,
AD7768_ONE_SHOT,
@@ -209,6 +213,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
@@ -295,6 +300,19 @@ static const struct regmap_config ad7768_regmap24_config = {
.max_register = AD7768_REG24_COEFF_DATA,
};
+static int ad7768_send_sync_pulse(struct ad7768_state *st)
+{
+ if (st->en_spi_sync)
+ return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00);
+
+ 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;
+}
+
static int ad7768_set_mode(struct ad7768_state *st,
enum ad7768_conv_mode mode)
{
@@ -391,10 +409,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)
@@ -671,6 +686,94 @@ static const struct iio_info ad7768_info = {
.debugfs_reg_access = &ad7768_reg_access,
};
+static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
+ struct fwnode_handle *fwnode)
+{
+ const char *value;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "compatible", &value);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (strcmp("gpio-trigger", value))
+ return ERR_PTR(-EINVAL);
+
+ return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
+ GPIOD_OUT_LOW, "sync-in");
+}
+
+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:
+ ret = dev_err_probe(dev, -EOPNOTSUPP,
+ "Invalid synchronization trigger source");
+out_put_node:
+ fwnode_handle_put(args.fwnode);
+ return ret;
+}
+
static int ad7768_setup(struct iio_dev *indio_dev)
{
struct ad7768_state *st = iio_priv(indio_dev);
@@ -701,11 +804,22 @@ 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);
+ /* For backwards compatibility, try the adi,sync-in-gpios property */
+ st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in",
+ GPIOD_OUT_LOW);
if (IS_ERR(st->gpio_sync_in))
return PTR_ERR(st->gpio_sync_in);
+ /*
+ * If the synchronization is not defined by adi,sync-in-gpios, try the
+ * trigger-sources.
+ */
+ if (!st->gpio_sync_in) {
+ ret = ad7768_trigger_sources_get_sync(&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")) {
ret = ad7768_gpio_init(indio_dev);
--
2.34.1On 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;
> +}
> +
On 4/27/25 7:13 PM, 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.
>
> Use trigger-sources property to enable device synchronization over SPI
> and multi-device synchronization, as an alternative to adi,sync-in-gpios
> property.
>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
...
> +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)) {
Normally, having error be the indented path like this is preferred, but I think
this case is an exception since the following is "normal" code path, not error
return path.
I would understand this better as:
st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
if (!IS_ERR(st->gpio_sync_in))
/* The trigger is a GPIO, our job is done here. */
goto out_put_node;
/* Second, ... */
> + /*
> + * 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) {
As Andy pointed out, this is a bit odd. Technically, we should be allowing any
trigger provider with the right capabilities. But we don't have a trigger
subsystem yet to make that easy. Since we can already handle the SYNC_IN is
wired to SYNC_OUT of the same chip above by omitting the trigger-sources
property, I would just make a TODO here and always return the not supported
error for now.
> + st->en_spi_sync = true;
> + goto out_put_node;
> + }
> +
> + goto err_not_supp;
> + }
> +
> + goto out_put_node;
> +
> +err_not_supp:
> + ret = dev_err_probe(dev, -EOPNOTSUPP,
> + "Invalid synchronization trigger source");
> +out_put_node:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
> +
On Mon, Apr 28, 2025 at 3:14 AM 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.
to the SYNC_IN
> 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_send_sync_pulse(struct ad7768_state *st)
> +{
> + if (st->en_spi_sync)
> + return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00);
> + if (st->gpio_sync_in) {
Dup check, the following have already it.
> + gpiod_set_value_cansleep(st->gpio_sync_in, 1);
Yes, I see the original code, but still the Q is why no delay. Perhaps
a comment explaining that the GPIO op is slow enough (?) to add.
> + gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> + }
> +
> + return 0;
> +}
...
> +static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
> + struct fwnode_handle *fwnode)
> +{
> + const char *value;
> + int ret;
> +
> + ret = fwnode_property_read_string(fwnode, "compatible", &value);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (strcmp("gpio-trigger", value))
> + return ERR_PTR(-EINVAL);
Reinvention of fwnode_device_is_compatible().
> + return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
> + GPIOD_OUT_LOW, "sync-in");
> +}
...
> +static int ad7768_trigger_sources_get_sync(struct device *dev,
> + struct ad7768_state *st)
> +{
> + struct fwnode_reference_args args;
> + struct fwnode_handle *fwnode = NULL;
Redundant assignment AFAICS.
> + 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),
In this case the above is better to be also fwnode_property_present().
You save a double call to dev_fwnode().
> + "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) {
?!?! What is this?!
For the reference:
https://elixir.bootlin.com/linux/v6.15-rc3/source/include/linux/fwnode.h#L51
> + st->en_spi_sync = true;
> + goto out_put_node;
> + }
> +
> + goto err_not_supp;
> + }
> +
> + goto out_put_node;
> +
> +err_not_supp:
> + ret = dev_err_probe(dev, -EOPNOTSUPP,
> + "Invalid synchronization trigger source");
Missing \n, and can be one line anyway (we don't complain about long
strings ending with string literals for ages, way before the 100
character limit).
> +out_put_node:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
On 04/28, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 3:14 AM 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.
>
> to the SYNC_IN
>
> > 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_send_sync_pulse(struct ad7768_state *st)
> > +{
> > + if (st->en_spi_sync)
> > + return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00);
>
> > + if (st->gpio_sync_in) {
>
> Dup check, the following have already it.
>
> > + gpiod_set_value_cansleep(st->gpio_sync_in, 1);
>
> Yes, I see the original code, but still the Q is why no delay. Perhaps
> a comment explaining that the GPIO op is slow enough (?) to add.
>
Datasheet specifies a minimum of 1.5*Tmclk pulse width. For the
recommended mclk of 16.384 MHz, it usually takes 4 times the minimum
pulse width. If it can be less than that for other plataforms I can add
this delay.
> > + gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
> > + struct fwnode_handle *fwnode)
> > +{
> > + const char *value;
> > + int ret;
> > +
> > + ret = fwnode_property_read_string(fwnode, "compatible", &value);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + if (strcmp("gpio-trigger", value))
> > + return ERR_PTR(-EINVAL);
>
> Reinvention of fwnode_device_is_compatible().
>
Thanks!
> > + return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
> > + GPIOD_OUT_LOW, "sync-in");
> > +}
>
> ...
>
> > +static int ad7768_trigger_sources_get_sync(struct device *dev,
> > + struct ad7768_state *st)
> > +{
> > + struct fwnode_reference_args args;
> > + struct fwnode_handle *fwnode = NULL;
>
> Redundant assignment AFAICS.
>
> > + 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),
>
> In this case the above is better to be also fwnode_property_present().
> You save a double call to dev_fwnode().
>
> > + "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) {
>
> ?!?! What is this?!
>
> For the reference:
> https://elixir.bootlin.com/linux/v6.15-rc3/source/include/linux/fwnode.h#L51
>
I see, my bad. I will follow David's suggestion, so we will avoid this.
> > + st->en_spi_sync = true;
> > + goto out_put_node;
> > + }
> > +
> > + goto err_not_supp;
> > + }
> > +
> > + goto out_put_node;
> > +
> > +err_not_supp:
> > + ret = dev_err_probe(dev, -EOPNOTSUPP,
> > + "Invalid synchronization trigger source");
>
> Missing \n, and can be one line anyway (we don't complain about long
> strings ending with string literals for ages, way before the 100
> character limit).
>
> > +out_put_node:
> > + fwnode_handle_put(args.fwnode);
> > + return ret;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.