[PATCH RFC v2 0/2] iio: adc: ad7173: add ad4111 openwire detection support

[PATCH RFC v2 0/2] iio: adc: ad7173: add ad4111 openwire detection support

[Guillaume Ranquet]

Hi.

This patch adds the openwire detection support for the ad4111 chip.

The openwire detection is done in software and relies on comparing the
results of two conversions on different channels.

Following the discussion in the RFC V1, I'm taking the easy route of
adding a new event type  called IIO_EV_TYPE_OPENWIRE to signal the
condition to the userland.

I have chosen this option rather than the generic TRACE_EVENT() solution
exposed by Jonathan because this change is more contained and only
impacts the iio subsystem.

The TRACE_EVENT() option, although more generic, will require a lot of
discussion between involved parties and is probably easier to tackle in
person than through the mailing list?

The openwire detection on ad4111 is triggered automatically when a
single conversion is requested.
Due to the way openwire detection works on ad4111, implementing openwire
detection for continuous conversion mode is more complicated.

Thx,
Guillaume.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
---
Changes in v2:
- Introduce IIO_EV_TYPE_OPENWIRE instead of misusing the IIO_EV_THRESH
  event.
- Link to v1: https://lore.kernel.org/r/20241115-ad4111_openwire-v1-1-db97ac8bf250@baylibre.com

---
Guillaume Ranquet (2):
      iio: introduce the OPENWIRE event type
      iio: adc: ad7173: add openwire detection support for single conversions

 drivers/iio/adc/ad7173.c         | 164 +++++++++++++++++++++++++++++++++++++++
 drivers/iio/industrialio-event.c |   1 +
 include/uapi/linux/iio/types.h   |   1 +
 tools/iio/iio_event_monitor.c    |   2 +
 4 files changed, 168 insertions(+)
---
base-commit: c849f534b9ea4688304f80f4571af75931dda7c1
change-id: 20241115-ad4111_openwire-e55deba8297f
prerequisite-message-id: <20241115-ad411x_calibration-v1-1-5f820dfb5c80@baylibre.com>
prerequisite-patch-id: 26241903b8fee8c4243e73d11fb2872cd9f52a15

Best regards,
-- 
Guillaume Ranquet <granquet@baylibre.com>

[PATCH RFC v2 1/2] iio: introduce the OPENWIRE event type

[Guillaume Ranquet]

Add a new event type to describe an hardware failure related to a
disconnected line on a sensor.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
---
 drivers/iio/industrialio-event.c | 1 +
 include/uapi/linux/iio/types.h   | 1 +
 tools/iio/iio_event_monitor.c    | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index db06501b0e61a91e3b06345b418504803f4aefb5..a3a3e448bd9fe7f768db6a37d7ddd78402bec0c2 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -232,6 +232,7 @@ static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_CHANGE] = "change",
 	[IIO_EV_TYPE_MAG_REFERENCED] = "mag_referenced",
 	[IIO_EV_TYPE_GESTURE] = "gesture",
+	[IIO_EV_TYPE_OPENWIRE] = "openwire",
 };
 
 static const char * const iio_ev_dir_text[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 12886d4465e4896aedce837c2df63c78f83a5496..46a63a16a16e40f2cecd14f1db65a1a1ad7b3814 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -119,6 +119,7 @@ enum iio_event_type {
 	IIO_EV_TYPE_CHANGE,
 	IIO_EV_TYPE_MAG_REFERENCED,
 	IIO_EV_TYPE_GESTURE,
+	IIO_EV_TYPE_OPENWIRE,
 };
 
 enum iio_event_direction {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index cccf62ea2b8f9b55a83a4960c1a60087c7b053f3..ee1bf4fe405f6c9e3619a990baa8c6347b80ca77 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -75,6 +75,7 @@ static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_CHANGE] = "change",
 	[IIO_EV_TYPE_MAG_REFERENCED] = "mag_referenced",
 	[IIO_EV_TYPE_GESTURE] = "gesture",
+	[IIO_EV_TYPE_OPENWIRE] = "openwire",
 };
 
 static const char * const iio_ev_dir_text[] = {
@@ -249,6 +250,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_EV_TYPE_MAG_ADAPTIVE:
 	case IIO_EV_TYPE_CHANGE:
 	case IIO_EV_TYPE_GESTURE:
+	case IIO_EV_TYPE_OPENWIRE:
 		break;
 	default:
 		return false;

-- 
2.47.1

[PATCH RFC v2 2/2] iio: adc: ad7173: add openwire detection support for single conversions

[Guillaume Ranquet]

Some chips of the ad7173 family supports open wire detection.

Generate a threshold event whenever an external source is disconnected
from the system input on single conversions.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
---
 drivers/iio/adc/ad7173.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..2f1b506c439249462b9740ff69e7553a37c45943 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -35,6 +35,7 @@
 #include <linux/units.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
@@ -102,6 +103,7 @@
 
 #define AD7173_GPIO_PDSW	BIT(14)
 #define AD7173_GPIO_OP_EN2_3	BIT(13)
+#define AD4111_GPIO_GP_OW_EN	BIT(12)
 #define AD7173_GPIO_MUX_IO	BIT(12)
 #define AD7173_GPIO_SYNC_EN	BIT(11)
 #define AD7173_GPIO_ERR_EN	BIT(10)
@@ -149,6 +151,7 @@
 
 #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
 #define AD7173_MAX_CONFIGS		8
+#define AD4111_OW_DET_THRSH_MV		300
 
 #define AD7173_MODE_CAL_INT_ZERO		0x4 /* Internal Zero-Scale Calibration */
 #define AD7173_MODE_CAL_INT_FULL		0x5 /* Internal Full-Scale Calibration */
@@ -181,11 +184,15 @@ struct ad7173_device_info {
 	bool has_int_ref;
 	bool has_ref2;
 	bool has_internal_fs_calibration;
+	bool has_openwire_det;
 	bool higher_gpio_bits;
 	u8 num_gpios;
 };
 
 struct ad7173_channel_config {
+	/* Openwire detection threshold */
+	unsigned int openwire_thrsh_raw;
+	int openwire_comp_chan;
 	u8 cfg_slot;
 	bool live;
 
@@ -202,6 +209,7 @@ struct ad7173_channel {
 	unsigned int chan_reg;
 	unsigned int ain;
 	struct ad7173_channel_config cfg;
+	bool openwire_det_en;
 };
 
 struct ad7173_state {
@@ -280,6 +288,7 @@ static const struct ad7173_device_info ad4111_device_info = {
 	.has_current_inputs = true,
 	.has_int_ref = true,
 	.has_internal_fs_calibration = true,
+	.has_openwire_det = true,
 	.clock = 2 * HZ_PER_MHZ,
 	.sinc5_data_rates = ad7173_sinc5_data_rates,
 	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -616,6 +625,69 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
 	return 0;
 }
 
+/* Associative array of channel pairs for openwire detection
+ * The array is indexed by ain and gives the associated channel pair
+ * to perform the openwire detection with
+ * the channel pair [0] is for non differential and pair [1]
+ * is for differential inputs
+ */
+static int openwire_ain_to_channel_pair[][2][2] = {
+	[0] = { {0, 15},  {1, 2}   },
+	[1] = { {1, 2},   {2, 1}   },
+	[2] = { {3, 4},   {5, 6}   },
+	[3] = { {5, 6},   {6, 5}   },
+	[4] = { {7, 8},   {9, 10}  },
+	[5] = { {9, 10},  {10, 9}  },
+	[6] = { {11, 12}, {13, 14} },
+	[7] = { {13, 14}, {14, 13} },
+};
+
+/* Openwire detection on ad4111 works by running the same input measurement
+ * on two different channels and compare if the difference between the two
+ * measurements exceeds a certain value (typical 300mV)
+ */
+static void ad4111_openwire_event(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *adchan = &st->channels[chan->address];
+	struct ad7173_channel_config *cfg = &adchan->cfg;
+	int ret, val1, val2;
+
+	ret = regmap_set_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
+	if (ret)
+		return;
+
+	adchan->cfg.openwire_comp_chan =
+		openwire_ain_to_channel_pair[chan->channel][chan->differential][0];
+
+	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val1);
+	if (ret < 0)
+		goto out;
+
+	adchan->cfg.openwire_comp_chan =
+		openwire_ain_to_channel_pair[chan->channel][chan->differential][1];
+
+	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val2);
+	if (ret < 0)
+		goto out;
+
+	if (abs(val1 - val2) > cfg->openwire_thrsh_raw)
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
+						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_RISING),
+			       iio_get_time_ns(indio_dev));
+	else
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
+						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_FALLING),
+			       iio_get_time_ns(indio_dev));
+
+out:
+	adchan->cfg.openwire_comp_chan = -1;
+	regmap_clear_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
+}
+
 static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 			     unsigned int offset, unsigned int *reg,
 			     unsigned int *mask)
@@ -813,6 +885,9 @@ static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
 	      FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, st->channels[channel].cfg.cfg_slot) |
 	      st->channels[channel].ain;
 
+	if (st->channels[channel].cfg.openwire_comp_chan >= 0)
+		channel = st->channels[channel].cfg.openwire_comp_chan;
+
 	return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
 }
 
@@ -861,6 +936,11 @@ static int ad7173_disable_all(struct ad_sigma_delta *sd)
 
 static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
 {
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+
+	if (st->channels[chan].cfg.openwire_comp_chan >= 0)
+		chan = st->channels[chan].cfg.openwire_comp_chan;
+
 	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
 }
 
@@ -968,6 +1048,9 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
+		if (ch->openwire_det_en)
+			ad4111_openwire_event(indio_dev, chan);
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 
@@ -1112,12 +1195,71 @@ static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
 }
 
+static int ad7173_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     bool state)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *adchan = &st->channels[chan->address];
+
+	adchan->openwire_det_en = state;
+
+	return 0;
+}
+
+static int ad7173_write_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				    enum iio_event_type type, enum iio_event_direction dir,
+				    enum iio_event_info info, int val, int val2)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *adchan = &st->channels[chan->address];
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		adchan->cfg.openwire_thrsh_raw = val;
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7173_read_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				   enum iio_event_type type, enum iio_event_direction dir,
+				   enum iio_event_info info, int *val, int *val2)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *adchan = &st->channels[chan->address];
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		*val = adchan->cfg.openwire_thrsh_raw;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_event_spec ad4111_events[] = {
+	{
+		.type = IIO_EV_TYPE_OPENWIRE,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 static const struct iio_info ad7173_info = {
 	.read_raw = &ad7173_read_raw,
 	.write_raw = &ad7173_write_raw,
 	.debugfs_reg_access = &ad7173_debug_reg_access,
 	.validate_trigger = ad_sd_validate_trigger,
 	.update_scan_mode = ad7173_update_scan_mode,
+	.write_event_config = ad7173_write_event_config,
+	.write_event_value = ad7173_write_event_value,
+	.read_event_value = ad7173_read_event_value,
 };
 
 static const struct iio_scan_type ad4113_scan_type = {
@@ -1321,6 +1463,15 @@ static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
 	return 0;
 }
 
+static int ad7173_validate_openwire_ain_inputs(struct ad7173_state *st, bool differential,
+					       unsigned int ain0, unsigned int ain1)
+{
+	if (differential)
+		return (ain0 % 2) ? (ain0 - 1) == ain1 : (ain0 + 1) == ain1;
+
+	return ain1 == AD4111_VINCOM_INPUT;
+}
+
 static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_channel *chans_st_arr, *chan_st_priv;
@@ -1375,6 +1526,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		chan_st_priv->cfg.bipolar = false;
 		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
 		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
+		chan_st_priv->cfg.openwire_comp_chan = -1;
 		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
 		if (st->info->data_reg_only_16bit)
 			chan_arr[chan_index].scan_type = ad4113_scan_type;
@@ -1442,6 +1594,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		chan_st_priv->chan_reg = chan_index;
 		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
 		chan_st_priv->cfg.odr = 0;
+		chan_st_priv->cfg.openwire_comp_chan = -1;
 
 		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
 		if (chan_st_priv->cfg.bipolar)
@@ -1456,6 +1609,17 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 			chan_st_priv->cfg.input_buf = st->info->has_input_buf;
 			chan->channel2 = ain[1];
 			chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+			if (st->info->has_openwire_det &&
+			    ad7173_validate_openwire_ain_inputs(st, chan->differential, ain[0], ain[1])) {
+				chan->event_spec = ad4111_events;
+				chan->num_event_specs = ARRAY_SIZE(ad4111_events);
+				chan_st_priv->cfg.openwire_thrsh_raw =
+					BIT(chan->scan_type.realbits - !!(chan_st_priv->cfg.bipolar))
+					* AD4111_OW_DET_THRSH_MV
+					/ ad7173_get_ref_voltage_milli(st, chan_st_priv->cfg.ref_sel);
+				if (chan->channel < st->info->num_voltage_in_div)
+					chan_st_priv->cfg.openwire_thrsh_raw /= AD4111_DIVIDER_RATIO;
+			}
 		}
 
 		if (st->info->data_reg_only_16bit)

-- 
2.47.1

Re: [PATCH RFC v2 2/2] iio: adc: ad7173: add openwire detection support for single conversions

[David Lechner]

On 1/9/25 7:55 AM, Guillaume Ranquet wrote:
> Some chips of the ad7173 family supports open wire detection.
> 
> Generate a threshold event whenever an external source is disconnected

s/threshold/"openwire"/

> from the system input on single conversions.

And also an event when reconnected.

What about during buffered reads? Is the recommendation that we should do a
single conversion, check for event, do buffered read, do a single conversion,
check for event to ensure that a wire did not become disconnected while doing a
buffered read?

> 
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> ---
>  drivers/iio/adc/ad7173.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..2f1b506c439249462b9740ff69e7553a37c45943 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -35,6 +35,7 @@
>  #include <linux/units.h>
>  
>  #include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> @@ -102,6 +103,7 @@
>  
>  #define AD7173_GPIO_PDSW	BIT(14)
>  #define AD7173_GPIO_OP_EN2_3	BIT(13)
> +#define AD4111_GPIO_GP_OW_EN	BIT(12)
>  #define AD7173_GPIO_MUX_IO	BIT(12)
>  #define AD7173_GPIO_SYNC_EN	BIT(11)
>  #define AD7173_GPIO_ERR_EN	BIT(10)
> @@ -149,6 +151,7 @@
>  
>  #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
>  #define AD7173_MAX_CONFIGS		8
> +#define AD4111_OW_DET_THRSH_MV		300
>  
>  #define AD7173_MODE_CAL_INT_ZERO		0x4 /* Internal Zero-Scale Calibration */
>  #define AD7173_MODE_CAL_INT_FULL		0x5 /* Internal Full-Scale Calibration */
> @@ -181,11 +184,15 @@ struct ad7173_device_info {
>  	bool has_int_ref;
>  	bool has_ref2;
>  	bool has_internal_fs_calibration;
> +	bool has_openwire_det;
>  	bool higher_gpio_bits;
>  	u8 num_gpios;
>  };
>  
>  struct ad7173_channel_config {
> +	/* Openwire detection threshold */
> +	unsigned int openwire_thrsh_raw;
> +	int openwire_comp_chan;
>  	u8 cfg_slot;
>  	bool live;
>  
> @@ -202,6 +209,7 @@ struct ad7173_channel {
>  	unsigned int chan_reg;
>  	unsigned int ain;
>  	struct ad7173_channel_config cfg;
> +	bool openwire_det_en;
>  };
>  
>  struct ad7173_state {
> @@ -280,6 +288,7 @@ static const struct ad7173_device_info ad4111_device_info = {
>  	.has_current_inputs = true,
>  	.has_int_ref = true,
>  	.has_internal_fs_calibration = true,
> +	.has_openwire_det = true,
>  	.clock = 2 * HZ_PER_MHZ,
>  	.sinc5_data_rates = ad7173_sinc5_data_rates,
>  	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -616,6 +625,69 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
>  	return 0;
>  }
>  
> +/* Associative array of channel pairs for openwire detection
> + * The array is indexed by ain and gives the associated channel pair
> + * to perform the openwire detection with
> + * the channel pair [0] is for non differential and pair [1]
> + * is for differential inputs

Maybe just me, but it seem like this would be a bit easier to understand if this
was two separate tables even if the code that uses it gets a bit more verbose.

> + */
> +static int openwire_ain_to_channel_pair[][2][2] = {
> +	[0] = { {0, 15},  {1, 2}   },

Not sure if it makes a difference, but the order in the datasheet is 15, 0.

> +	[1] = { {1, 2},   {2, 1}   },
> +	[2] = { {3, 4},   {5, 6}   },
> +	[3] = { {5, 6},   {6, 5}   },
> +	[4] = { {7, 8},   {9, 10}  },
> +	[5] = { {9, 10},  {10, 9}  },
> +	[6] = { {11, 12}, {13, 14} },
> +	[7] = { {13, 14}, {14, 13} },
> +};
> +
> +/* Openwire detection on ad4111 works by running the same input measurement

s/Openwire/Open wire/

> + * on two different channels and compare if the difference between the two
> + * measurements exceeds a certain value (typical 300mV)
> + */
> +static void ad4111_openwire_event(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *adchan = &st->channels[chan->address];
> +	struct ad7173_channel_config *cfg = &adchan->cfg;
> +	int ret, val1, val2;
> +
> +	ret = regmap_set_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
> +	if (ret)

Probably best to return errors to userspace. Or at least log errors instead of
silently ignoring them. Otherwise troubleshooting failures won't be possible
without modifying the kernel.

> +		return;
> +
> +	adchan->cfg.openwire_comp_chan =
> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][0];
> +
> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val1);
> +	if (ret < 0)
> +		goto out;
> +
> +	adchan->cfg.openwire_comp_chan =
> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][1];
> +
> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val2);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (abs(val1 - val2) > cfg->openwire_thrsh_raw)
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_RISING),
> +			       iio_get_time_ns(indio_dev));
> +	else
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_FALLING),
> +			       iio_get_time_ns(indio_dev));

Hmm... rising and falling doesn't make so much sense for an openwire event. It
is either "open" or "closed" and doesn't really have a direction. (The threshold
is just an internal implementation detail of how it is detected.)

So maybe we need new IIO_EV_DIR_ members for this too?

I guess we could call it "opening" and "closing" to make it direction-like?

Also, this implementation makes it a level event instead of an edge event.
In other words, on every single conversion, we will get an event. Usually, we
would keep track of the previous value and only emit an event when the state
changes.

> +
> +out:
> +	adchan->cfg.openwire_comp_chan = -1;
> +	regmap_clear_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
> +}
> +
>  static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
>  			     unsigned int offset, unsigned int *reg,
>  			     unsigned int *mask)
> @@ -813,6 +885,9 @@ static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
>  	      FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, st->channels[channel].cfg.cfg_slot) |
>  	      st->channels[channel].ain;
>  
> +	if (st->channels[channel].cfg.openwire_comp_chan >= 0)
> +		channel = st->channels[channel].cfg.openwire_comp_chan;
> +
>  	return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
>  }
>  
> @@ -861,6 +936,11 @@ static int ad7173_disable_all(struct ad_sigma_delta *sd)
>  
>  static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
>  {
> +	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
> +
> +	if (st->channels[chan].cfg.openwire_comp_chan >= 0)
> +		chan = st->channels[chan].cfg.openwire_comp_chan;
> +
>  	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
>  }
>  
> @@ -968,6 +1048,9 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> +		if (ch->openwire_det_en)
> +			ad4111_openwire_event(indio_dev, chan);
> +
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  
> @@ -1112,12 +1195,71 @@ static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
>  }
>  
> +static int ad7173_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *adchan = &st->channels[chan->address];
> +
> +	adchan->openwire_det_en = state;
> +
> +	return 0;
> +}
> +
> +static int ad7173_write_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> +				    enum iio_event_type type, enum iio_event_direction dir,
> +				    enum iio_event_info info, int val, int val2)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *adchan = &st->channels[chan->address];
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		adchan->cfg.openwire_thrsh_raw = val;

This difference threshold seems like an internal implementation detail of how
this specific chip handles open wire detection. I'm not sure that writing
this value to the events/in_voltageY_openwire_value attribute makes much sense
from an IIO ABI point of view.

It would be OK with me to just always use 300mV for now. And if we find an
application where that doesn't work, consider adding an adjustment somewhere.
For example, we might find that it makes more sense to put this in the
devictree because it depends on electrical properties of the application.

> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad7173_read_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> +				   enum iio_event_type type, enum iio_event_direction dir,
> +				   enum iio_event_info info, int *val, int *val2)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *adchan = &st->channels[chan->address];
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = adchan->cfg.openwire_thrsh_raw;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_event_spec ad4111_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_OPENWIRE,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
>  static const struct iio_info ad7173_info = {
>  	.read_raw = &ad7173_read_raw,
>  	.write_raw = &ad7173_write_raw,
>  	.debugfs_reg_access = &ad7173_debug_reg_access,
>  	.validate_trigger = ad_sd_validate_trigger,
>  	.update_scan_mode = ad7173_update_scan_mode,

No read_event_config ?

> +	.write_event_config = ad7173_write_event_config,
> +	.write_event_value = ad7173_write_event_value,
> +	.read_event_value = ad7173_read_event_value,
>  };
>  
>  static const struct iio_scan_type ad4113_scan_type = {
> @@ -1321,6 +1463,15 @@ static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
>  	return 0;
>  }
>  
> +static int ad7173_validate_openwire_ain_inputs(struct ad7173_state *st, bool differential,
> +					       unsigned int ain0, unsigned int ain1)
> +{
> +	if (differential)

A comment here explaining why this check would be helpful.

> +		return (ain0 % 2) ? (ain0 - 1) == ain1 : (ain0 + 1) == ain1;
> +
> +	return ain1 == AD4111_VINCOM_INPUT;
> +}
> +
>  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7173_channel *chans_st_arr, *chan_st_priv;
> @@ -1375,6 +1526,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  		chan_st_priv->cfg.bipolar = false;
>  		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>  		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		chan_st_priv->cfg.openwire_comp_chan = -1;
>  		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>  		if (st->info->data_reg_only_16bit)
>  			chan_arr[chan_index].scan_type = ad4113_scan_type;
> @@ -1442,6 +1594,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  		chan_st_priv->chan_reg = chan_index;
>  		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>  		chan_st_priv->cfg.odr = 0;
> +		chan_st_priv->cfg.openwire_comp_chan = -1;

Setting the same value twice?

>  
>  		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
>  		if (chan_st_priv->cfg.bipolar)
> @@ -1456,6 +1609,17 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  			chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>  			chan->channel2 = ain[1];
>  			chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> +			if (st->info->has_openwire_det &&
> +			    ad7173_validate_openwire_ain_inputs(st, chan->differential, ain[0], ain[1])) {
> +				chan->event_spec = ad4111_events;
> +				chan->num_event_specs = ARRAY_SIZE(ad4111_events);
> +				chan_st_priv->cfg.openwire_thrsh_raw =
> +					BIT(chan->scan_type.realbits - !!(chan_st_priv->cfg.bipolar))
> +					* AD4111_OW_DET_THRSH_MV
> +					/ ad7173_get_ref_voltage_milli(st, chan_st_priv->cfg.ref_sel);
> +				if (chan->channel < st->info->num_voltage_in_div)
> +					chan_st_priv->cfg.openwire_thrsh_raw /= AD4111_DIVIDER_RATIO;
> +			}
>  		}
>  
>  		if (st->info->data_reg_only_16bit)
> 

Overall, I think the way this is implemented looks reasonable. We'll see how
much Jonathan agrees or disagrees my comments on the ABI stuff. :-)

Re: [PATCH RFC v2 2/2] iio: adc: ad7173: add openwire detection support for single conversions

[Guillaume Ranquet]

On Thu, 09 Jan 2025 22:53, David Lechner <dlechner@baylibre.com> wrote:
>On 1/9/25 7:55 AM, Guillaume Ranquet wrote:
>> Some chips of the ad7173 family supports open wire detection.
>>
>> Generate a threshold event whenever an external source is disconnected
>
>s/threshold/"openwire"/
>
>> from the system input on single conversions.
>
>And also an event when reconnected.
>
>What about during buffered reads? Is the recommendation that we should do a
>single conversion, check for event, do buffered read, do a single conversion,
>check for event to ensure that a wire did not become disconnected while doing a
>buffered read?

With the current design of the ad7173/ad_sigma_delta, the buffered
read enables continuous mode.

Continuous mode makes the ADC cycle through all enabled channels
automatically.

So, enabling open wire detection during buffered reads has its own
challenges:
* Assigning channels gets more complicated in the current state of the
	driver
* We divide the number of possible channels by two (or even three
	depending on the gymnastic we did when assigning channels).
* Timing performance will suffer as twice as many conversions needs to
	happen
* Accuracy might also suffer as we would need to keep
	AD4111_GPIO_GP_OW_EN enabled at all times.

So, I don't know what the recommendation is, but having open wire
detection enabled during buffered reads seems complex (and is it worth
the performance penalties?).

Another way of doing it would be to decouple open wire detection from
single conversions and create a new channel property instead of an
event.

The property would trigger the two conversions needed for open wire
detection and return synchronously the current status of the channel.

What happens when the user reads the "open wire" property when a
buffered read is in progress?

* Pause the buffered read, do the open wire detection and resume?
* Return an error signaling openwire detection doesn't work when a
	concurrent buffered read is in progress?
>
>>
>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 164 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..2f1b506c439249462b9740ff69e7553a37c45943 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/units.h>
>>
>>  #include <linux/iio/buffer.h>
>> +#include <linux/iio/events.h>
>>  #include <linux/iio/iio.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>> @@ -102,6 +103,7 @@
>>
>>  #define AD7173_GPIO_PDSW	BIT(14)
>>  #define AD7173_GPIO_OP_EN2_3	BIT(13)
>> +#define AD4111_GPIO_GP_OW_EN	BIT(12)
>>  #define AD7173_GPIO_MUX_IO	BIT(12)
>>  #define AD7173_GPIO_SYNC_EN	BIT(11)
>>  #define AD7173_GPIO_ERR_EN	BIT(10)
>> @@ -149,6 +151,7 @@
>>
>>  #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
>>  #define AD7173_MAX_CONFIGS		8
>> +#define AD4111_OW_DET_THRSH_MV		300
>>
>>  #define AD7173_MODE_CAL_INT_ZERO		0x4 /* Internal Zero-Scale Calibration */
>>  #define AD7173_MODE_CAL_INT_FULL		0x5 /* Internal Full-Scale Calibration */
>> @@ -181,11 +184,15 @@ struct ad7173_device_info {
>>  	bool has_int_ref;
>>  	bool has_ref2;
>>  	bool has_internal_fs_calibration;
>> +	bool has_openwire_det;
>>  	bool higher_gpio_bits;
>>  	u8 num_gpios;
>>  };
>>
>>  struct ad7173_channel_config {
>> +	/* Openwire detection threshold */
>> +	unsigned int openwire_thrsh_raw;
>> +	int openwire_comp_chan;
>>  	u8 cfg_slot;
>>  	bool live;
>>
>> @@ -202,6 +209,7 @@ struct ad7173_channel {
>>  	unsigned int chan_reg;
>>  	unsigned int ain;
>>  	struct ad7173_channel_config cfg;
>> +	bool openwire_det_en;
>>  };
>>
>>  struct ad7173_state {
>> @@ -280,6 +288,7 @@ static const struct ad7173_device_info ad4111_device_info = {
>>  	.has_current_inputs = true,
>>  	.has_int_ref = true,
>>  	.has_internal_fs_calibration = true,
>> +	.has_openwire_det = true,
>>  	.clock = 2 * HZ_PER_MHZ,
>>  	.sinc5_data_rates = ad7173_sinc5_data_rates,
>>  	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> @@ -616,6 +625,69 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
>>  	return 0;
>>  }
>>
>> +/* Associative array of channel pairs for openwire detection
>> + * The array is indexed by ain and gives the associated channel pair
>> + * to perform the openwire detection with
>> + * the channel pair [0] is for non differential and pair [1]
>> + * is for differential inputs
>
>Maybe just me, but it seem like this would be a bit easier to understand if this
>was two separate tables even if the code that uses it gets a bit more verbose.
>
>> + */
>> +static int openwire_ain_to_channel_pair[][2][2] = {
>> +	[0] = { {0, 15},  {1, 2}   },
>
>Not sure if it makes a difference, but the order in the datasheet is 15, 0.
>
>> +	[1] = { {1, 2},   {2, 1}   },
>> +	[2] = { {3, 4},   {5, 6}   },
>> +	[3] = { {5, 6},   {6, 5}   },
>> +	[4] = { {7, 8},   {9, 10}  },
>> +	[5] = { {9, 10},  {10, 9}  },
>> +	[6] = { {11, 12}, {13, 14} },
>> +	[7] = { {13, 14}, {14, 13} },
>> +};
>> +
>> +/* Openwire detection on ad4111 works by running the same input measurement
>
>s/Openwire/Open wire/
>
>> + * on two different channels and compare if the difference between the two
>> + * measurements exceeds a certain value (typical 300mV)
>> + */
>> +static void ad4111_openwire_event(struct iio_dev *indio_dev,
>> +				  const struct iio_chan_spec *chan)
>> +{
>> +	struct ad7173_state *st = iio_priv(indio_dev);
>> +	struct ad7173_channel *adchan = &st->channels[chan->address];
>> +	struct ad7173_channel_config *cfg = &adchan->cfg;
>> +	int ret, val1, val2;
>> +
>> +	ret = regmap_set_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
>> +	if (ret)
>
>Probably best to return errors to userspace. Or at least log errors instead of
>silently ignoring them. Otherwise troubleshooting failures won't be possible
>without modifying the kernel.
>
>> +		return;
>> +
>> +	adchan->cfg.openwire_comp_chan =
>> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][0];
>> +
>> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val1);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	adchan->cfg.openwire_comp_chan =
>> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][1];
>> +
>> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val2);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	if (abs(val1 - val2) > cfg->openwire_thrsh_raw)
>> +		iio_push_event(indio_dev,
>> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
>> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_RISING),
>> +			       iio_get_time_ns(indio_dev));
>> +	else
>> +		iio_push_event(indio_dev,
>> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
>> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_FALLING),
>> +			       iio_get_time_ns(indio_dev));
>
>Hmm... rising and falling doesn't make so much sense for an openwire event. It
>is either "open" or "closed" and doesn't really have a direction. (The threshold
>is just an internal implementation detail of how it is detected.)
>
>So maybe we need new IIO_EV_DIR_ members for this too?
>
>I guess we could call it "opening" and "closing" to make it direction-like?
>
>Also, this implementation makes it a level event instead of an edge event.
>In other words, on every single conversion, we will get an event. Usually, we
>would keep track of the previous value and only emit an event when the state
>changes.
>
>> +
>> +out:
>> +	adchan->cfg.openwire_comp_chan = -1;
>> +	regmap_clear_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
>> +}
>> +
>>  static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
>>  			     unsigned int offset, unsigned int *reg,
>>  			     unsigned int *mask)
>> @@ -813,6 +885,9 @@ static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
>>  	      FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, st->channels[channel].cfg.cfg_slot) |
>>  	      st->channels[channel].ain;
>>
>> +	if (st->channels[channel].cfg.openwire_comp_chan >= 0)
>> +		channel = st->channels[channel].cfg.openwire_comp_chan;
>> +
>>  	return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
>>  }
>>
>> @@ -861,6 +936,11 @@ static int ad7173_disable_all(struct ad_sigma_delta *sd)
>>
>>  static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
>>  {
>> +	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
>> +
>> +	if (st->channels[chan].cfg.openwire_comp_chan >= 0)
>> +		chan = st->channels[chan].cfg.openwire_comp_chan;
>> +
>>  	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
>>  }
>>
>> @@ -968,6 +1048,9 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
>>  		if (ret < 0)
>>  			return ret;
>>
>> +		if (ch->openwire_det_en)
>> +			ad4111_openwire_event(indio_dev, chan);
>> +
>>  		return IIO_VAL_INT;
>>  	case IIO_CHAN_INFO_SCALE:
>>
>> @@ -1112,12 +1195,71 @@ static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>>  	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
>>  }
>>
>> +static int ad7173_write_event_config(struct iio_dev *indio_dev,
>> +				     const struct iio_chan_spec *chan,
>> +				     enum iio_event_type type,
>> +				     enum iio_event_direction dir,
>> +				     bool state)
>> +{
>> +	struct ad7173_state *st = iio_priv(indio_dev);
>> +	struct ad7173_channel *adchan = &st->channels[chan->address];
>> +
>> +	adchan->openwire_det_en = state;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad7173_write_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
>> +				    enum iio_event_type type, enum iio_event_direction dir,
>> +				    enum iio_event_info info, int val, int val2)
>> +{
>> +	struct ad7173_state *st = iio_priv(indio_dev);
>> +	struct ad7173_channel *adchan = &st->channels[chan->address];
>> +
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		adchan->cfg.openwire_thrsh_raw = val;
>
>This difference threshold seems like an internal implementation detail of how
>this specific chip handles open wire detection. I'm not sure that writing
>this value to the events/in_voltageY_openwire_value attribute makes much sense
>from an IIO ABI point of view.
>
>It would be OK with me to just always use 300mV for now. And if we find an
>application where that doesn't work, consider adding an adjustment somewhere.
>For example, we might find that it makes more sense to put this in the
>devictree because it depends on electrical properties of the application.
>

The datasheet suggests 300mV, I don't know what factors could make this
threshold change.
I agree we could hardcode it as a first approach.

>> +		return 0;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ad7173_read_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
>> +				   enum iio_event_type type, enum iio_event_direction dir,
>> +				   enum iio_event_info info, int *val, int *val2)
>> +{
>> +	struct ad7173_state *st = iio_priv(indio_dev);
>> +	struct ad7173_channel *adchan = &st->channels[chan->address];
>> +
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		*val = adchan->cfg.openwire_thrsh_raw;
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_event_spec ad4111_events[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_OPENWIRE,
>> +		.dir = IIO_EV_DIR_EITHER,
>> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
>> +	},
>> +};
>> +
>>  static const struct iio_info ad7173_info = {
>>  	.read_raw = &ad7173_read_raw,
>>  	.write_raw = &ad7173_write_raw,
>>  	.debugfs_reg_access = &ad7173_debug_reg_access,
>>  	.validate_trigger = ad_sd_validate_trigger,
>>  	.update_scan_mode = ad7173_update_scan_mode,
>
>No read_event_config ?
>
>> +	.write_event_config = ad7173_write_event_config,
>> +	.write_event_value = ad7173_write_event_value,
>> +	.read_event_value = ad7173_read_event_value,
>>  };
>>
>>  static const struct iio_scan_type ad4113_scan_type = {
>> @@ -1321,6 +1463,15 @@ static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
>>  	return 0;
>>  }
>>
>> +static int ad7173_validate_openwire_ain_inputs(struct ad7173_state *st, bool differential,
>> +					       unsigned int ain0, unsigned int ain1)
>> +{
>> +	if (differential)
>
>A comment here explaining why this check would be helpful.
>
>> +		return (ain0 % 2) ? (ain0 - 1) == ain1 : (ain0 + 1) == ain1;
>> +
>> +	return ain1 == AD4111_VINCOM_INPUT;
>> +}
>> +
>>  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>>  {
>>  	struct ad7173_channel *chans_st_arr, *chan_st_priv;
>> @@ -1375,6 +1526,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>>  		chan_st_priv->cfg.bipolar = false;
>>  		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>>  		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
>> +		chan_st_priv->cfg.openwire_comp_chan = -1;
>>  		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>>  		if (st->info->data_reg_only_16bit)
>>  			chan_arr[chan_index].scan_type = ad4113_scan_type;
>> @@ -1442,6 +1594,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>>  		chan_st_priv->chan_reg = chan_index;
>>  		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>>  		chan_st_priv->cfg.odr = 0;
>> +		chan_st_priv->cfg.openwire_comp_chan = -1;
>
>Setting the same value twice?
>
>>
>>  		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
>>  		if (chan_st_priv->cfg.bipolar)
>> @@ -1456,6 +1609,17 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>>  			chan_st_priv->cfg.input_buf = st->info->has_input_buf;
>>  			chan->channel2 = ain[1];
>>  			chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
>> +			if (st->info->has_openwire_det &&
>> +			    ad7173_validate_openwire_ain_inputs(st, chan->differential, ain[0], ain[1])) {
>> +				chan->event_spec = ad4111_events;
>> +				chan->num_event_specs = ARRAY_SIZE(ad4111_events);
>> +				chan_st_priv->cfg.openwire_thrsh_raw =
>> +					BIT(chan->scan_type.realbits - !!(chan_st_priv->cfg.bipolar))
>> +					* AD4111_OW_DET_THRSH_MV
>> +					/ ad7173_get_ref_voltage_milli(st, chan_st_priv->cfg.ref_sel);
>> +				if (chan->channel < st->info->num_voltage_in_div)
>> +					chan_st_priv->cfg.openwire_thrsh_raw /= AD4111_DIVIDER_RATIO;
>> +			}
>>  		}
>>
>>  		if (st->info->data_reg_only_16bit)
>>
>
>Overall, I think the way this is implemented looks reasonable. We'll see how
>much Jonathan agrees or disagrees my comments on the ABI stuff. :-)

Thx for the review,
Guillaume.

Re: [PATCH RFC v2 2/2] iio: adc: ad7173: add openwire detection support for single conversions

[Jonathan Cameron]

On Fri, 10 Jan 2025 05:50:45 -0500
Guillaume Ranquet <granquet@baylibre.com> wrote:

> On Thu, 09 Jan 2025 22:53, David Lechner <dlechner@baylibre.com> wrote:
> >On 1/9/25 7:55 AM, Guillaume Ranquet wrote:  
> >> Some chips of the ad7173 family supports open wire detection.
> >>
> >> Generate a threshold event whenever an external source is disconnected  
> >
> >s/threshold/"openwire"/
> >  
> >> from the system input on single conversions.  
> >
> >And also an event when reconnected.
> >
> >What about during buffered reads? Is the recommendation that we should do a
> >single conversion, check for event, do buffered read, do a single conversion,
> >check for event to ensure that a wire did not become disconnected while doing a
> >buffered read?  
> 
> With the current design of the ad7173/ad_sigma_delta, the buffered
> read enables continuous mode.
> 
> Continuous mode makes the ADC cycle through all enabled channels
> automatically.
> 
> So, enabling open wire detection during buffered reads has its own
> challenges:
> * Assigning channels gets more complicated in the current state of the
> 	driver
> * We divide the number of possible channels by two (or even three
> 	depending on the gymnastic we did when assigning channels).
> * Timing performance will suffer as twice as many conversions needs to
> 	happen
> * Accuracy might also suffer as we would need to keep
> 	AD4111_GPIO_GP_OW_EN enabled at all times.
> 
> So, I don't know what the recommendation is, but having open wire
> detection enabled during buffered reads seems complex (and is it worth
> the performance penalties?).

Whilst I'm not sure on usecases for this part, I'd go with it not being
worth it.

> 
> Another way of doing it would be to decouple open wire detection from
> single conversions and create a new channel property instead of an
> event.
> 
> The property would trigger the two conversions needed for open wire
> detection and return synchronously the current status of the channel.

That would result in some ABI that isn't particularly general,
so I'd rather not go that way.  It probably also interferes with
continuous mode anyway so doesn't gain us anything.

> 
> What happens when the user reads the "open wire" property when a
> buffered read is in progress?
> 
> * Pause the buffered read, do the open wire detection and resume?
> * Return an error signaling openwire detection doesn't work when a
> 	concurrent buffered read is in progress?

Assume openwire events are rare and just check before enabling and
after disabling buffered mode. We could do that in driver if it
turns out to be useful.  Otherwise don't bother and make it a userspace
problem.

I thought about suggesting we poll for this like we'd do for other
events where the device doesn't self clock, but it makes little sense.
We don't really care about a wire being out until we want to read.


> >> +		return;
> >> +
> >> +	adchan->cfg.openwire_comp_chan =
> >> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][0];
> >> +
> >> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val1);
> >> +	if (ret < 0)
> >> +		goto out;
> >> +
> >> +	adchan->cfg.openwire_comp_chan =
> >> +		openwire_ain_to_channel_pair[chan->channel][chan->differential][1];
> >> +
> >> +	ret = ad_sigma_delta_single_conversion(indio_dev, chan, &val2);
> >> +	if (ret < 0)
> >> +		goto out;
> >> +
> >> +	if (abs(val1 - val2) > cfg->openwire_thrsh_raw)
> >> +		iio_push_event(indio_dev,
> >> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
> >> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_RISING),
> >> +			       iio_get_time_ns(indio_dev));
> >> +	else
> >> +		iio_push_event(indio_dev,
> >> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, chan->address,
> >> +						    IIO_EV_TYPE_OPENWIRE, IIO_EV_DIR_FALLING),
> >> +			       iio_get_time_ns(indio_dev));  
> >
> >Hmm... rising and falling doesn't make so much sense for an openwire event. It
> >is either "open" or "closed" and doesn't really have a direction. (The threshold
> >is just an internal implementation detail of how it is detected.)

True.

> >
> >So maybe we need new IIO_EV_DIR_ members for this too?

Silly question. Does a close wire event make any sense?  What is the usecase?
I can see short detection being useful just not 'wire plugged back in'.

I was thinking of this as detection for nasty events - repairing them is an engineer
problem who can check the actual data makes sense.  We can provide debugfs
for that if it is useful.  Everyone wants to know there is a problem but the
set of people and generality of interface needed for repair is a different
question!

We probably do need multiple "types" even so,  Open wire / short etc.

Could do
IIO_EV_TYPE_OPENWIRE
IIO_EV_TYPE_SHORT...

Or go down the gesture route and
IIO_EV_TYPE_FAULT
+
IIO_EV_DIR_OPENWIRE
IIO_EV_DIR_SHORT


(only implement what we need here though).



> >
> >I guess we could call it "opening" and "closing" to make it direction-like?
> >
> >Also, this implementation makes it a level event instead of an edge event.
> >In other words, on every single conversion, we will get an event. Usually, we
> >would keep track of the previous value and only emit an event when the state
> >changes.
For open wire, I'm fine with just squirting events out on every read.  Any detection
of this event should cause userspace to stop polling it anyway.

However, if you want to make it edge based that's fine too as long as it will
trigger on first enabling of error checking followed by a read.

> >  
> >> +
> >> +out:
> >> +	adchan->cfg.openwire_comp_chan = -1;
> >> +	regmap_clear_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, AD4111_GPIO_GP_OW_EN);
> >> +}
> >> +

> >> +static int ad7173_write_event_value(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> >> +				    enum iio_event_type type, enum iio_event_direction dir,
> >> +				    enum iio_event_info info, int val, int val2)
> >> +{
> >> +	struct ad7173_state *st = iio_priv(indio_dev);
> >> +	struct ad7173_channel *adchan = &st->channels[chan->address];
> >> +
> >> +	switch (info) {
> >> +	case IIO_EV_INFO_VALUE:
> >> +		adchan->cfg.openwire_thrsh_raw = val;  
> >
> >This difference threshold seems like an internal implementation detail of how
> >this specific chip handles open wire detection. I'm not sure that writing
> >this value to the events/in_voltageY_openwire_value attribute makes much sense
> >from an IIO ABI point of view.
> >
> >It would be OK with me to just always use 300mV for now. And if we find an
> >application where that doesn't work, consider adding an adjustment somewhere.
> >For example, we might find that it makes more sense to put this in the
> >devictree because it depends on electrical properties of the application.
> >  
> 
> The datasheet suggests 300mV, I don't know what factors could make this
> threshold change.
> I agree we could hardcode it as a first approach.

Agreed.

> 
> >> +		return 0;
> >> +
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +
...

> >
> >Overall, I think the way this is implemented looks reasonable. We'll see how
> >much Jonathan agrees or disagrees my comments on the ABI stuff. :-)  
You spotted the problem which was the hard bit! :)  
> 
> Thx for the review,
> Guillaume.

Re: [PATCH RFC v2 2/2] iio: adc: ad7173: add openwire detection support for single conversions

[Jonathan Cameron]

> @@ -616,6 +625,69 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
>  	return 0;
>  }
>  
> +/* Associative array of channel pairs for openwire detection

/*
 * Associative ...

Same for all other multiline comments.