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

[PATCH v4 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.

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 out of the scope of this
series.

Following discussion on V2, I have changed the event to be
IIO_EV_TYPE_FAULT and added a direction called IIO_EV_DIR_FAULT_OPENWIRE to
signal the specific fault.

The fault is level triggered (ie: the event will be sent as long as the
fault persists).
There's no event to signal that the fault has been "fixed", an absence
of FAULT event means that the open wire condition is not detected.

Thx,
Guillaume.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
---
Changes in v4:
- Rename IIO_EV_DIR_OPENWIRE to IIO_EV_DIR_FAULT_OPENWIRE to make it
  clearer the OPENWIRE direction is in the FAULT "namespace"
- Removal of the RFC prefix
- Link to v3: https://lore.kernel.org/r/20250116-ad4111_openwire-v3-0-ea9ebf29bd1d@baylibre.com

Changes in v3:
- Rename IIO_EV_TYPE_OPENWIRE to IIO_EV_TYPE_FAULT and add
  IIO_EV_DIR_OPENWIRE.
- Remove per channel open wire threshold configuration interface.
- Link to v2: https://lore.kernel.org/r/20250109-ad4111_openwire-v2-0-0372c2dde0ce@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 FAULT event type
      iio: adc: ad7173: add openwire detection support for single conversions

 drivers/iio/adc/ad7173.c         | 166 +++++++++++++++++++++++++++++++++++++++
 drivers/iio/industrialio-event.c |   2 +
 include/uapi/linux/iio/types.h   |   2 +
 tools/iio/iio_event_monitor.c    |   4 +
 4 files changed, 174 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 v4 1/2] iio: introduce the FAULT event type

[Guillaume Ranquet]

Add a new event type to describe an hardware failure.

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

diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index db06501b0e61a91e3b06345b418504803f4aefb5..06295cfc2da8b1df17061cf58ade38d88020359e 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_FAULT] = "fault",
 };
 
 static const char * const iio_ev_dir_text[] = {
@@ -240,6 +241,7 @@ static const char * const iio_ev_dir_text[] = {
 	[IIO_EV_DIR_FALLING] = "falling",
 	[IIO_EV_DIR_SINGLETAP] = "singletap",
 	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
+	[IIO_EV_DIR_FAULT_OPENWIRE] = "openwire",
 };
 
 static const char * const iio_ev_info_text[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 12886d4465e4896aedce837c2df63c78f83a5496..3eb0821af7a40e29544fbcc67c48e085507e13d0 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_FAULT,
 };
 
 enum iio_event_direction {
@@ -128,6 +129,7 @@ enum iio_event_direction {
 	IIO_EV_DIR_NONE,
 	IIO_EV_DIR_SINGLETAP,
 	IIO_EV_DIR_DOUBLETAP,
+	IIO_EV_DIR_FAULT_OPENWIRE,
 };
 
 #endif /* _UAPI_IIO_TYPES_H_ */
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index cccf62ea2b8f9b55a83a4960c1a60087c7b053f3..eab7b082f19db8703aca55af7dbf4f1d624aa3af 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_FAULT] = "fault",
 };
 
 static const char * const iio_ev_dir_text[] = {
@@ -83,6 +84,7 @@ static const char * const iio_ev_dir_text[] = {
 	[IIO_EV_DIR_FALLING] = "falling",
 	[IIO_EV_DIR_SINGLETAP] = "singletap",
 	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
+	[IIO_EV_DIR_FAULT_OPENWIRE] = "openwire",
 };
 
 static const char * const iio_modifier_names[] = {
@@ -249,6 +251,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_FAULT:
 		break;
 	default:
 		return false;
@@ -260,6 +263,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_EV_DIR_FALLING:
 	case IIO_EV_DIR_SINGLETAP:
 	case IIO_EV_DIR_DOUBLETAP:
+	case IIO_EV_DIR_FAULT_OPENWIRE:
 	case IIO_EV_DIR_NONE:
 		break;
 	default:

-- 
2.47.1

[PATCH v4 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 level fault 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 | 166 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..a2ea8f7ae8e61f1f3cdfba795551de2db96b8d60 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,76 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
 	return 0;
 }
 
+/*
+ * Associative array of channel pairs for open wire detection
+ * The array is indexed by ain and gives the associated channel pair
+ * to perform the open wire 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] = {
+/*	AIN     Single    Differential */
+	[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 int 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 ret;
+
+	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) {
+		dev_err(&indio_dev->dev,
+			"Error running ad_sigma_delta single conversion: %d", ret);
+		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) {
+		dev_err(&indio_dev->dev,
+			"Error running ad_sigma_delta single conversion: %d", ret);
+		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_FAULT, IIO_EV_DIR_FAULT_OPENWIRE),
+			       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);
+	return ret;
+}
+
 static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 			     unsigned int offset, unsigned int *reg,
 			     unsigned int *mask)
@@ -813,6 +892,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 +943,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 +1055,12 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
+		if (ch->openwire_det_en) {
+			ret = ad4111_openwire_event(indio_dev, chan);
+			if (ret < 0)
+				return ret;
+		}
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 
@@ -1112,12 +1205,57 @@ 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];
+
+	switch (type) {
+	case IIO_EV_TYPE_FAULT:
+		adchan->openwire_det_en = state;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7173_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *adchan = &st->channels[chan->address];
+
+	switch (type) {
+	case IIO_EV_TYPE_FAULT:
+		return adchan->openwire_det_en;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_event_spec ad4111_events[] = {
+	{
+		.type = IIO_EV_TYPE_FAULT,
+		.dir = IIO_EV_DIR_FAULT_OPENWIRE,
+		.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,
+	.read_event_config = ad7173_read_event_config,
 };
 
 static const struct iio_scan_type ad4113_scan_type = {
@@ -1321,6 +1459,21 @@ 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 the channel is configured as differential,
+	 * the ad4111 requires specific ains to be used together
+	 */
+	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 +1528,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 +1596,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 +1611,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 v4 1/2] iio: introduce the FAULT event type

[Nuno Sá]

On Mon, 2025-01-20 at 15:10 +0100, Guillaume Ranquet wrote:
> Add a new event type to describe an hardware failure.
> 
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/industrialio-event.c | 2 ++
>  include/uapi/linux/iio/types.h   | 2 ++
>  tools/iio/iio_event_monitor.c    | 4 ++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> event.c
> index
> db06501b0e61a91e3b06345b418504803f4aefb5..06295cfc2da8b1df17061cf58ade38d88020
> 359e 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_FAULT] = "fault",
>  };
>  
>  static const char * const iio_ev_dir_text[] = {
> @@ -240,6 +241,7 @@ static const char * const iio_ev_dir_text[] = {
>  	[IIO_EV_DIR_FALLING] = "falling",
>  	[IIO_EV_DIR_SINGLETAP] = "singletap",
>  	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
> +	[IIO_EV_DIR_FAULT_OPENWIRE] = "openwire",
>  };
>  
>  static const char * const iio_ev_info_text[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index
> 12886d4465e4896aedce837c2df63c78f83a5496..3eb0821af7a40e29544fbcc67c48e085507e
> 13d0 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_FAULT,
>  };
>  
>  enum iio_event_direction {
> @@ -128,6 +129,7 @@ enum iio_event_direction {
>  	IIO_EV_DIR_NONE,
>  	IIO_EV_DIR_SINGLETAP,
>  	IIO_EV_DIR_DOUBLETAP,
> +	IIO_EV_DIR_FAULT_OPENWIRE,
>  };
>  
>  #endif /* _UAPI_IIO_TYPES_H_ */
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index
> cccf62ea2b8f9b55a83a4960c1a60087c7b053f3..eab7b082f19db8703aca55af7dbf4f1d624a
> a3af 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_FAULT] = "fault",
>  };
>  
>  static const char * const iio_ev_dir_text[] = {
> @@ -83,6 +84,7 @@ static const char * const iio_ev_dir_text[] = {
>  	[IIO_EV_DIR_FALLING] = "falling",
>  	[IIO_EV_DIR_SINGLETAP] = "singletap",
>  	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
> +	[IIO_EV_DIR_FAULT_OPENWIRE] = "openwire",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> @@ -249,6 +251,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_FAULT:
>  		break;
>  	default:
>  		return false;
> @@ -260,6 +263,7 @@ static bool event_is_known(struct iio_event_data *event)
>  	case IIO_EV_DIR_FALLING:
>  	case IIO_EV_DIR_SINGLETAP:
>  	case IIO_EV_DIR_DOUBLETAP:
> +	case IIO_EV_DIR_FAULT_OPENWIRE:
>  	case IIO_EV_DIR_NONE:
>  		break;
>  	default:
> 

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

[Nuno Sá]

On Mon, 2025-01-20 at 15:10 +0100, Guillaume Ranquet wrote:
> Some chips of the ad7173 family supports open wire detection.
> 
> Generate a level fault event whenever an external source is disconnected
> from the system input on single conversions.
> 
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> ---

LGTM... Just one small nit. In any case:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7173.c | 166
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 166 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index
> 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..a2ea8f7ae8e61f1f3cdfba795551de2db96b
> 8d60 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 @@
> 

...

> 
> +
>  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  {
>  	struct ad7173_channel *chans_st_arr, *chan_st_priv;
> @@ -1375,6 +1528,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 +1596,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 +1611,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 you need to send another version for some reason, might be worth it to
implement a simple helper for the above to improve code readability.

Maybe is just me but that 'chan_st_priv->cfg.openwire_thrsh_raw =' is fairly
unreadable :)

- Nuno Sá
> 

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

[Jonathan Cameron]

On Mon, 20 Jan 2025 17:07:24 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2025-01-20 at 15:10 +0100, Guillaume Ranquet wrote:
> > Some chips of the ad7173 family supports open wire detection.
> > 
> > Generate a level fault event whenever an external source is disconnected
> > from the system input on single conversions.
> > 
> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> > ---  
> 
> LGTM... Just one small nit. In any case:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> >  drivers/iio/adc/ad7173.c | 166
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 166 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > index
> > 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..a2ea8f7ae8e61f1f3cdfba795551de2db96b
> > 8d60 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 @@
> >   
> 
> ...
> 
> > 
> > +
> >  static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> >  {
> >  	struct ad7173_channel *chans_st_arr, *chan_st_priv;
> > @@ -1375,6 +1528,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 +1596,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 +1611,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 you need to send another version for some reason, might be worth it to
> implement a simple helper for the above to improve code readability.
> 
> Maybe is just me but that 'chan_st_priv->cfg.openwire_thrsh_raw =' is fairly
> unreadable :)
> 
This crossed with a set from David and doesn't apply. Please rebase on top
of my testing branch.  Thanks! If the helper function Nuno suggests makes
sense to you feel free to do that as well.

The two patches I have on this driver that aren't upstream yet are:
iio: adc: ad7173: don't make copy of ad_sigma_delta_info struct 
iio: adc: ad7173: move fwnode_irq_get_byname() call site

Jonathan
> - Nuno Sá
> >