[PATCH v1 0/3] iio: adc: ad7192: Add sync feature

[PATCH v1 0/3] iio: adc: ad7192: Add sync feature

[Alisa-Dariana Roman]

Dear maintainers,

Here is a little update for the ad7192 driver. I also added myself as
maintainer for the bindings.

Thank you for your attention!

Kind regards,
Alisa-Dariana Roman.

[PATCH v1 1/3] dt-bindings: iio: adc: ad7192: Add maintainer

[Alisa-Dariana Roman]

Add myself as a maintainer for AD7192 devicetree bindings.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 66dd1c549bd3..f70caefdace7 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -9,6 +9,7 @@ title: Analog Devices AD7192 ADC device driver
 
 maintainers:
   - Michael Hennerich <michael.hennerich@analog.com>
+  - Alisa-Dariana Roman <alisa.roman@analog.com>
 
 description: |
   Bindings for the Analog Devices AD7192 ADC device. Datasheet can be
-- 
2.43.0

Re: [PATCH v1 1/3] dt-bindings: iio: adc: ad7192: Add maintainer

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Thu, Nov 28, 2024 at 02:55:01PM +0200, Alisa-Dariana Roman wrote:
> Add myself as a maintainer for AD7192 devicetree bindings.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v1 2/3] dt-bindings: iio: adc: ad7192: Add sync gpio

[Alisa-Dariana Roman]

Add support for the SYNC pin GPIO specification in the devicetree
bindings. This pin allows synchronization of digital filters and analog
modulators when using multiple devices. Update the examples to
demonstrate the usage of the new property.

Also update the interrupt type in the examples to use the proper
IRQ_TYPE_EDGE_FALLING macro instead of the raw value.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../bindings/iio/adc/adi,ad7192.yaml          | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index f70caefdace7..1cd0fd13bc42 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -59,6 +59,17 @@ properties:
   interrupts:
     maxItems: 1
 
+  sync-gpios:
+    description: |
+      Optional GPIO spec for the SYNC pin. The SYNC pin allows synchronization
+      of the digital filters and analog modulators when using multiple AD7192
+      devices. When SYNC is pulled low, it resets the digital filter nodes,
+      filter control logic, calibration control logic, and holds the analog
+      modulator in reset state. Only specify this property if you need to
+      actively control SYNC for multi-device synchronization, otherwise it
+      defaults to HIGH.
+    maxItems: 1
+
   aincom-supply:
     description: |
       AINCOM voltage supply. Analog inputs AINx are referenced to this input
@@ -182,6 +193,8 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -194,8 +207,9 @@ examples:
             spi-cpha;
             clocks = <&ad7192_mclk>;
             clock-names = "mclk";
-            interrupts = <25 0x2>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
             interrupt-parent = <&gpio>;
+            sync-gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
             aincom-supply = <&aincom>;
             dvdd-supply = <&dvdd>;
             avdd-supply = <&avdd>;
@@ -208,6 +222,7 @@ examples:
         };
     };
   - |
+    #include <dt-bindings/interrupt-controller/irq.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -223,7 +238,7 @@ examples:
             spi-cpol;
             spi-cpha;
             #clock-cells = <0>;
-            interrupts = <25 0x2>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
             interrupt-parent = <&gpio>;
             aincom-supply = <&aincom>;
             dvdd-supply = <&dvdd>;
-- 
2.43.0

Re: [PATCH v1 2/3] dt-bindings: iio: adc: ad7192: Add sync gpio

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Thu, Nov 28, 2024 at 02:55:02PM +0200, Alisa-Dariana Roman wrote:
> Add support for the SYNC pin GPIO specification in the devicetree
> bindings. This pin allows synchronization of digital filters and analog
> modulators when using multiple devices. Update the examples to
> demonstrate the usage of the new property.
> 
> Also update the interrupt type in the examples to use the proper
> IRQ_TYPE_EDGE_FALLING macro instead of the raw value.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/3] dt-bindings: iio: adc: ad7192: Add sync gpio

[David Lechner]

On 11/28/24 6:55 AM, Alisa-Dariana Roman wrote:
> Add support for the SYNC pin GPIO specification in the devicetree
> bindings. This pin allows synchronization of digital filters and analog
> modulators when using multiple devices. Update the examples to
> demonstrate the usage of the new property.
> 
> Also update the interrupt type in the examples to use the proper
> IRQ_TYPE_EDGE_FALLING macro instead of the raw value.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7192.yaml          | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index f70caefdace7..1cd0fd13bc42 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -59,6 +59,17 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  sync-gpios:
> +    description: |
> +      Optional GPIO spec for the SYNC pin. The SYNC pin allows synchronization
> +      of the digital filters and analog modulators when using multiple AD7192
> +      devices. When SYNC is pulled low, it resets the digital filter nodes,
> +      filter control logic, calibration control logic, and holds the analog
> +      modulator in reset state. Only specify this property if you need to
> +      actively control SYNC for multi-device synchronization, otherwise it
> +      defaults to HIGH.

The datasheet says that the /SYNC pin can also be used to trigger a conversion.
But I'm not sure we really need all this explanation here. The important parts
are that this is a GPIO that is connected to the /SYNC pin and in the
synchronization use case, the same GPIO might be connected to multiple ADC
chips.

> +    maxItems: 1
> +
>    aincom-supply:
>      description: |
>        AINCOM voltage supply. Analog inputs AINx are referenced to this input
> @@ -182,6 +193,8 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      spi {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -194,8 +207,9 @@ examples:
>              spi-cpha;
>              clocks = <&ad7192_mclk>;
>              clock-names = "mclk";
> -            interrupts = <25 0x2>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
>              interrupt-parent = <&gpio>;
> +            sync-gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;

On the schematic, the SYNC pin is marked as active low, so I would
expect GPIO_ACTIVE_LOW here.

>              aincom-supply = <&aincom>;
>              dvdd-supply = <&dvdd>;
>              avdd-supply = <&avdd>;
> @@ -208,6 +222,7 @@ examples:
>          };
>      };
>    - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      spi {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -223,7 +238,7 @@ examples:
>              spi-cpol;
>              spi-cpha;
>              #clock-cells = <0>;
> -            interrupts = <25 0x2>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
>              interrupt-parent = <&gpio>;
>              aincom-supply = <&aincom>;
>              dvdd-supply = <&dvdd>;

[PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[Alisa-Dariana Roman]

Add support for the SYNC pin of AD719x devices. This pin is controlled
through a GPIO. The pin allows synchronization of digital filters and
analog modulators when using multiple devices.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 112 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 955e9eff0099..542db7280e99 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -196,6 +197,7 @@ struct ad7192_chip_info {
 	u8				num_channels;
 	const struct ad_sigma_delta_info	*sigma_delta_info;
 	const struct iio_info		*info;
+	const struct iio_info		*info_sync;
 	int (*parse_channels)(struct iio_dev *indio_dev);
 };
 
@@ -216,6 +218,8 @@ struct ad7192_state {
 	struct mutex			lock;	/* protect sensor state */
 	u8				syscalib_mode[8];
 
+	struct gpio_desc		*sync_gpio;
+
 	struct ad_sigma_delta		sd;
 };
 
@@ -783,6 +787,36 @@ static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
 	st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
 }
 
+static ssize_t sync_gpio_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad7192_state *st = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%d\n", gpiod_get_value(st->sync_gpio));
+}
+
+static ssize_t sync_gpio_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf,
+			       size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad7192_state *st = iio_priv(indio_dev);
+	int val;
+	int ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	if (st->sync_gpio)
+		gpiod_set_value(st->sync_gpio, val);
+
+	return len;
+}
+
 static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
 		       ad7192_show_bridge_switch, ad7192_set,
 		       AD7192_REG_GPOCON);
@@ -791,25 +825,57 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
 		       ad7192_show_ac_excitation, ad7192_set,
 		       AD7192_REG_CONF);
 
+static IIO_DEVICE_ATTR_RW(sync_gpio, 0);
+
 static struct attribute *ad7192_attributes[] = {
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
 	NULL
 };
 
+static struct attribute *ad7192_attributes_sync[] = {
+	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
+	&iio_dev_attr_sync_gpio.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group ad7192_attribute_group = {
 	.attrs = ad7192_attributes,
 };
 
+static const struct attribute_group ad7192_attribute_group_sync = {
+	.attrs = ad7192_attributes_sync,
+};
+
+static struct attribute *ad7194_attributes_sync[] = {
+	&iio_dev_attr_sync_gpio.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ad7194_attribute_group_sync = {
+	.attrs = ad7194_attributes_sync,
+};
+
 static struct attribute *ad7195_attributes[] = {
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
 	&iio_dev_attr_ac_excitation_en.dev_attr.attr,
 	NULL
 };
 
+static struct attribute *ad7195_attributes_sync[] = {
+	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
+	&iio_dev_attr_ac_excitation_en.dev_attr.attr,
+	&iio_dev_attr_sync_gpio.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group ad7195_attribute_group = {
 	.attrs = ad7195_attributes,
 };
 
+static const struct attribute_group ad7195_attribute_group_sync = {
+	.attrs = ad7195_attributes_sync,
+};
+
 static unsigned int ad7192_get_temp_scale(bool unipolar)
 {
 	return unipolar ? 2815 * 2 : 2815;
@@ -1103,6 +1169,16 @@ static const struct iio_info ad7192_info = {
 	.update_scan_mode = ad7192_update_scan_mode,
 };
 
+static const struct iio_info ad7192_info_sync = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.attrs = &ad7192_attribute_group_sync,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
+};
+
 static const struct iio_info ad7194_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -1111,6 +1187,15 @@ static const struct iio_info ad7194_info = {
 	.validate_trigger = ad_sd_validate_trigger,
 };
 
+static const struct iio_info ad7194_info_sync = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.attrs = &ad7194_attribute_group_sync,
+	.validate_trigger = ad_sd_validate_trigger,
+};
+
 static const struct iio_info ad7195_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -1121,6 +1206,16 @@ static const struct iio_info ad7195_info = {
 	.update_scan_mode = ad7192_update_scan_mode,
 };
 
+static const struct iio_info ad7195_info_sync = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.attrs = &ad7195_attribute_group_sync,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
+};
+
 #define __AD719x_CHANNEL(_si, _channel1, _channel2, _address, _type, \
 	_mask_all, _mask_type_av, _mask_all_av, _ext_info) \
 	{ \
@@ -1292,6 +1387,7 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7192_channels),
 		.sigma_delta_info = &ad7192_sigma_delta_info,
 		.info = &ad7192_info,
+		.info_sync = &ad7192_info_sync,
 	},
 	[ID_AD7192] = {
 		.chip_id = CHIPID_AD7192,
@@ -1300,6 +1396,7 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7192_channels),
 		.sigma_delta_info = &ad7192_sigma_delta_info,
 		.info = &ad7192_info,
+		.info_sync = &ad7192_info_sync,
 	},
 	[ID_AD7193] = {
 		.chip_id = CHIPID_AD7193,
@@ -1308,11 +1405,13 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7193_channels),
 		.sigma_delta_info = &ad7192_sigma_delta_info,
 		.info = &ad7192_info,
+		.info_sync = &ad7192_info_sync,
 	},
 	[ID_AD7194] = {
 		.chip_id = CHIPID_AD7194,
 		.name = "ad7194",
 		.info = &ad7194_info,
+		.info_sync = &ad7194_info_sync,
 		.sigma_delta_info = &ad7194_sigma_delta_info,
 		.parse_channels = ad7194_parse_channels,
 	},
@@ -1323,6 +1422,7 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7192_channels),
 		.sigma_delta_info = &ad7192_sigma_delta_info,
 		.info = &ad7195_info,
+		.info_sync = &ad7195_info_sync,
 	},
 };
 
@@ -1344,6 +1444,11 @@ static int ad7192_probe(struct spi_device *spi)
 
 	mutex_init(&st->lock);
 
+	st->sync_gpio = devm_gpiod_get_optional(dev, "sync", GPIOD_OUT_HIGH);
+	if (IS_ERR(st->sync_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->sync_gpio),
+				     "Failed to get sync gpio.\n");
+
 	/*
 	 * Regulator aincom is optional to maintain compatibility with older DT.
 	 * Newer firmware should provide a zero volt fixed supply if wired to
@@ -1399,7 +1504,12 @@ static int ad7192_probe(struct spi_device *spi)
 
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->info = st->chip_info->info;
+
+	if (st->sync_gpio)
+		indio_dev->info = st->chip_info->info_sync;
+	else
+		indio_dev->info = st->chip_info->info;
+
 	if (st->chip_info->parse_channels) {
 		ret = st->chip_info->parse_channels(indio_dev);
 		if (ret)
-- 
2.43.0

Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[Jonathan Cameron]

On Thu, 28 Nov 2024 14:55:03 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Add support for the SYNC pin of AD719x devices. This pin is controlled
> through a GPIO. The pin allows synchronization of digital filters and
> analog modulators when using multiple devices.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Hi.

Like all userspace ABI, this needs documentation.

It's an unusual feature, so some usecases would help.

It is also cross multiple devices which makes this odd as only one device
can presumably acquire the gpio?

An alternative would be to look at how to do this with a 'wrapper' sort of device
so that we have one instance to which this applies.

I'm not sure that helps that much though as we'd still need some for of
'I'm setup for all channels, now you can go' ABI.

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 112 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 955e9eff0099..542db7280e99 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -10,6 +10,7 @@
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -196,6 +197,7 @@ struct ad7192_chip_info {
>  	u8				num_channels;
>  	const struct ad_sigma_delta_info	*sigma_delta_info;
>  	const struct iio_info		*info;
> +	const struct iio_info		*info_sync;
>  	int (*parse_channels)(struct iio_dev *indio_dev);
>  };
>  
> @@ -216,6 +218,8 @@ struct ad7192_state {
>  	struct mutex			lock;	/* protect sensor state */
>  	u8				syscalib_mode[8];
>  
> +	struct gpio_desc		*sync_gpio;
> +
>  	struct ad_sigma_delta		sd;
>  };
>  
> @@ -783,6 +787,36 @@ static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
>  	st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
>  }
>  
> +static ssize_t sync_gpio_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	return sysfs_emit(buf, "%d\n", gpiod_get_value(st->sync_gpio));
> +}
> +
> +static ssize_t sync_gpio_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf,
> +			       size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	int val;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (st->sync_gpio)
> +		gpiod_set_value(st->sync_gpio, val);
> +
> +	return len;
> +}

Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[David Lechner]

On 11/30/24 12:38 PM, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 14:55:03 +0200
> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> 
>> Add support for the SYNC pin of AD719x devices. This pin is controlled
>> through a GPIO. The pin allows synchronization of digital filters and
>> analog modulators when using multiple devices.
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> Hi.
> 
> Like all userspace ABI, this needs documentation.
> 
> It's an unusual feature, so some usecases would help.
> 
> It is also cross multiple devices which makes this odd as only one device
> can presumably acquire the gpio?
> 
> An alternative would be to look at how to do this with a 'wrapper' sort of device
> so that we have one instance to which this applies.
> 
> I'm not sure that helps that much though as we'd still need some for of
> 'I'm setup for all channels, now you can go' ABI.
> 
> Jonathan
> 

Giving userspace direct control over the /SYNC pin without coordinating
with the rest of the driver does seem like it could be asking for trouble.

It seems like the only time you would want to actually toggle the /SYNC
pin is when starting a buffered read.

1. Deassert /SYNC so that no conversions can be triggered.
2. Enable buffered reads for all chips connected to the same GPIO.
3. Assert /SYNC to start all conversions at the same time.

So it could make sense to integrate this into the buffer pre/post enable
callbacks somehow instead of adding a new sysfs attribute.

For the "wrapper" device, maybe we could do something with configfs to
enable dynamically connecting multiple device instances? We might not
need to actually create a separate device in sysfs, but just do something
so that enabling a buffered read on the first chip will enable buffered
reads on all of the chips in the group.

It seems like we have some other chips that are currently being worked on
that also have the possibility of some sort of multi-chip synchronization
like this so it would be nice to come up with a general solution.

Another use case for a general synchronized buffered read/write between
multiple chips would be the AD3552R DAC. Recently, while adding support
for an IIO backend for this chip, we saw that the AXI DAC backend has a
synchronization feature like this where you set an "arm" bit on all AXI
DAC instances. Then when you enable streaming to the first chip, it also
triggers all of the other AXI DAC blocks to start streaming at the same
time. We ended up not implementing that feature since the IIO subsystem
doesn't really support this yet, but could be a good one to look at as a
similar feature with a different implementation to help us find a general
solution.

Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[Jonathan Cameron]

On Mon, 2 Dec 2024 16:21:43 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/30/24 12:38 PM, Jonathan Cameron wrote:
> > On Thu, 28 Nov 2024 14:55:03 +0200
> > Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> >   
> >> Add support for the SYNC pin of AD719x devices. This pin is controlled
> >> through a GPIO. The pin allows synchronization of digital filters and
> >> analog modulators when using multiple devices.
> >>
> >> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>  
> > Hi.
> > 
> > Like all userspace ABI, this needs documentation.
> > 
> > It's an unusual feature, so some usecases would help.
> > 
> > It is also cross multiple devices which makes this odd as only one device
> > can presumably acquire the gpio?
> > 
> > An alternative would be to look at how to do this with a 'wrapper' sort of device
> > so that we have one instance to which this applies.
> > 
> > I'm not sure that helps that much though as we'd still need some for of
> > 'I'm setup for all channels, now you can go' ABI.
> > 
> > Jonathan
> >   
> 
> Giving userspace direct control over the /SYNC pin without coordinating
> with the rest of the driver does seem like it could be asking for trouble.
> 
> It seems like the only time you would want to actually toggle the /SYNC
> pin is when starting a buffered read.
> 
> 1. Deassert /SYNC so that no conversions can be triggered.
> 2. Enable buffered reads for all chips connected to the same GPIO.
> 3. Assert /SYNC to start all conversions at the same time.
> 
> So it could make sense to integrate this into the buffer pre/post enable
> callbacks somehow instead of adding a new sysfs attribute.
> 
> For the "wrapper" device, maybe we could do something with configfs to
> enable dynamically connecting multiple device instances? We might not
> need to actually create a separate device in sysfs, but just do something
> so that enabling a buffered read on the first chip will enable buffered
> reads on all of the chips in the group.
> 
> It seems like we have some other chips that are currently being worked on
> that also have the possibility of some sort of multi-chip synchronization
> like this so it would be nice to come up with a general solution.

Most of the multichip cases we've had before have been chained, rather
than separate data interfaces, hence I don't recall us needing something
like this before.

> 
> Another use case for a general synchronized buffered read/write between
> multiple chips would be the AD3552R DAC. Recently, while adding support
> for an IIO backend for this chip, we saw that the AXI DAC backend has a
> synchronization feature like this where you set an "arm" bit on all AXI
> DAC instances. Then when you enable streaming to the first chip, it also
> triggers all of the other AXI DAC blocks to start streaming at the same
> time. We ended up not implementing that feature since the IIO subsystem
> doesn't really support this yet, but could be a good one to look at as a
> similar feature with a different implementation to help us find a general
> solution.
> 
This feels like a case where we need a prototype to poke holes in.
It's not totally dissimilar from the hardware trigger stuff that
exists in a few devices. Some of the stm parts for instance where the
triggering is entirely within the chip.  Maybe we could make something
like that work.  So the driver instance that has the sync pin registers
a trigger that the other devices use.   It's a bit ugly though and we'd
still need a dt-binding to let us know 'which' devices are connected
to that sync pin.

Jonathan

Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

[David Lechner]

On 12/8/24 12:43 PM, Jonathan Cameron wrote:
> On Mon, 2 Dec 2024 16:21:43 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 11/30/24 12:38 PM, Jonathan Cameron wrote:
>>> On Thu, 28 Nov 2024 14:55:03 +0200
>>> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
>>>   
>>>> Add support for the SYNC pin of AD719x devices. This pin is controlled
>>>> through a GPIO. The pin allows synchronization of digital filters and
>>>> analog modulators when using multiple devices.
>>>>
>>>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>  
>>> Hi.
>>>
>>> Like all userspace ABI, this needs documentation.
>>>
>>> It's an unusual feature, so some usecases would help.
>>>
>>> It is also cross multiple devices which makes this odd as only one device
>>> can presumably acquire the gpio?
>>>
>>> An alternative would be to look at how to do this with a 'wrapper' sort of device
>>> so that we have one instance to which this applies.
>>>
>>> I'm not sure that helps that much though as we'd still need some for of
>>> 'I'm setup for all channels, now you can go' ABI.
>>>
>>> Jonathan
>>>   
>>
>> Giving userspace direct control over the /SYNC pin without coordinating
>> with the rest of the driver does seem like it could be asking for trouble.
>>
>> It seems like the only time you would want to actually toggle the /SYNC
>> pin is when starting a buffered read.
>>
>> 1. Deassert /SYNC so that no conversions can be triggered.
>> 2. Enable buffered reads for all chips connected to the same GPIO.
>> 3. Assert /SYNC to start all conversions at the same time.
>>
>> So it could make sense to integrate this into the buffer pre/post enable
>> callbacks somehow instead of adding a new sysfs attribute.
>>
>> For the "wrapper" device, maybe we could do something with configfs to
>> enable dynamically connecting multiple device instances? We might not
>> need to actually create a separate device in sysfs, but just do something
>> so that enabling a buffered read on the first chip will enable buffered
>> reads on all of the chips in the group.
>>
>> It seems like we have some other chips that are currently being worked on
>> that also have the possibility of some sort of multi-chip synchronization
>> like this so it would be nice to come up with a general solution.
> 
> Most of the multichip cases we've had before have been chained, rather
> than separate data interfaces, hence I don't recall us needing something
> like this before.
> 
>>
>> Another use case for a general synchronized buffered read/write between
>> multiple chips would be the AD3552R DAC. Recently, while adding support
>> for an IIO backend for this chip, we saw that the AXI DAC backend has a
>> synchronization feature like this where you set an "arm" bit on all AXI
>> DAC instances. Then when you enable streaming to the first chip, it also
>> triggers all of the other AXI DAC blocks to start streaming at the same
>> time. We ended up not implementing that feature since the IIO subsystem
>> doesn't really support this yet, but could be a good one to look at as a
>> similar feature with a different implementation to help us find a general
>> solution.
>>
> This feels like a case where we need a prototype to poke holes in.
> It's not totally dissimilar from the hardware trigger stuff that
> exists in a few devices. Some of the stm parts for instance where the
> triggering is entirely within the chip.  Maybe we could make something
> like that work.  So the driver instance that has the sync pin registers
> a trigger that the other devices use.   It's a bit ugly though and we'd
> still need a dt-binding to let us know 'which' devices are connected
> to that sync pin.
> 
> Jonathan
> 
> 

A shared trigger was one of the ideas that crossed my mind as well.
Are you suggesting that the "main" chip would have the sync-gpios
property in it's .dts node and then the other chips would have a
trigger-sources = <&main_adc>; property instead of the sync-gpios
property? (FYI, trigger-sources/#trigger-source-cells are becoming
are becoming general properties with the SPI offload work I have
been doing, so should be available to use soon-ish).

Now, to try to poke a hole in this idea...

It seems like most (all?) triggers are set up to trigger an individual
sample. But in this case, the /SYNC pin would just be triggering the
start of a buffered read, so triggering multiple samples. Does this
still fit within the definition of a struct iio_trigger?

One way to possibly make it work without a struct iio_trigger could
work like this:
* During probe, "main" chips sets /SYNC so that chips won't sample
  data.
* Set up and enable a buffered read for all non-main chips that have
  the /SYNC pin connected to a common GPIO. Chips don't actually start
  sampling when buffer is enabled due to /SYNC pin state.
* Enable the buffer on the "main" chip last. This changes the /SYNC
  pin state in the buffer pre/post enable and all chips start
  sampling at the same time.
* Read from all buffers for as long as you want.
* Similarly, to stop, the "main" chip should have it's buffer disabled
  first, which changes the /SYNC pin state causing all chips to stop
  sampling.
* Then other buffers can be disabled.

Alternately, we could have a struct iio_trigger and expose control of
the /SYNC pin state as a sysfs attribute on the trigger. This would be
more like what Alisa-Dariana has proposed already.

It could work like this:
* Set up and enable a buffered read for all chips that have the /SYNC pin
  connected to a common GPIO. Chips don't actually start sampling when
  buffer is enabled due to /SYNC pin state.
* Write to a new trigger sysfs attribute to start sampling on all chips at
  the same time.
* Read from all buffers for as long as you want.
* Write to the trigger sysfs attribute to stop sampling on all chips at
  the same time.
* Disable buffers on all chips.