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>
---
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.

As presented here, the code sends a THRESH Rising event tied to the
in_voltageX_raw channel on which it happened.

We think this is not correct but we can't seem to find an implementation
that would be elegant.

The main idea would be to add a specific channel for openwire detection
but we still would need to have the openwire enablement and threshold
tied to the voltage channel.

Any thought on this?

Thx,
Guillaume.
---
 drivers/iio/adc/ad7173.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..f87f002cbd99cfabac2754559b3c6fc0a69f5b70 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,59 @@ static int ad7173_calibrate_all(struct ad7173_state *st, struct iio_dev *indio_d
 	return 0;
 }
 
+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} },
+};
+
+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_THRESH, 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_THRESH, 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 +875,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 +926,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 +1038,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 +1185,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,
+				     int 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_THRESH,
+		.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 +1453,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 +1516,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 +1584,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 +1599,15 @@ 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 (st->info->data_reg_only_16bit)

---
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>

On 11/15/24 4:34 AM, Guillaume Ranquet wrote:
> 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>
> ---
> 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.
> 
> As presented here, the code sends a THRESH Rising event tied to the
> in_voltageX_raw channel on which it happened.
> 
> We think this is not correct but we can't seem to find an implementation
> that would be elegant.
> 
> The main idea would be to add a specific channel for openwire detection
> but we still would need to have the openwire enablement and threshold
> tied to the voltage channel.
> 
> Any thought on this?
> 
Just to spell this out in a bit more detail, my understanding is that
the procedure works like this...

To detect an open wire on a single-ended input, we measure the voltage
on pin VIN0 using channel register 15, the we read the voltage on the
same pin again using channel register 0. The datasheet isn't clear on
the details, but on one or the other or both of these, the ADC chip is
applying some kind of pull up or pull down on the input pin so that
each measurement will be nearly the same if there is a wire attached
with a 0-10V signal on it. Or if the wire is detached and the pins are
left floating, the two measurements will be different (> 300 mV).

So this threshold value (the 300 mV) is measured in terms of the
difference between two voltage measurements, but of the same input pin.

My suggestion is to create extra differential channels like
in_voltage0-involtage100_* where 100 is an arbitrary number. These
channels would be defined as the difference between the two measurements
on the same pin. The event attributes would use these channels since they
are triggered by exceeding the threshold value according to this difference
measurement.

To complicate matters, these chips can also be configured so that two
input pins can be configured as a fully differential pair. And we can
also do open wire detection on these differential pairs. In this case
we would configure input pins VIN0 and VIN1 as a differential pair, then
read the difference of those two pins using channel register 1, then
read again using channel register 2. The we see if the difference of the
two differences exceeds the threshold.

In this case, we can't have in_(voltage0-voltage1)-(voltage100-voltage101)_*
attributes. So I guess we would have to do something like
in_voltage200-voltage300 to handle this case? (voltage200 would represent
the first measurement of voltage0-voltage1 and voltage300 would represent
the 2nd measurement of the same).

On Mon, 18 Nov 2024 21:48, David Lechner <dlechner@baylibre.com> wrote:
>On 11/15/24 4:34 AM, Guillaume Ranquet wrote:
>> 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>
>> ---
>> 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.
>>
>> As presented here, the code sends a THRESH Rising event tied to the
>> in_voltageX_raw channel on which it happened.
>>
>> We think this is not correct but we can't seem to find an implementation
>> that would be elegant.
>>
>> The main idea would be to add a specific channel for openwire detection
>> but we still would need to have the openwire enablement and threshold
>> tied to the voltage channel.
>>
>> Any thought on this?
>>
>Just to spell this out in a bit more detail, my understanding is that
>the procedure works like this...
>
>To detect an open wire on a single-ended input, we measure the voltage
>on pin VIN0 using channel register 15, the we read the voltage on the
>same pin again using channel register 0. The datasheet isn't clear on
>the details, but on one or the other or both of these, the ADC chip is
>applying some kind of pull up or pull down on the input pin so that
>each measurement will be nearly the same if there is a wire attached
>with a 0-10V signal on it. Or if the wire is detached and the pins are
>left floating, the two measurements will be different (> 300 mV).
>
>So this threshold value (the 300 mV) is measured in terms of the
>difference between two voltage measurements, but of the same input pin.
>
>My suggestion is to create extra differential channels like
>in_voltage0-involtage100_* where 100 is an arbitrary number. These
>channels would be defined as the difference between the two measurements
>on the same pin. The event attributes would use these channels since they
>are triggered by exceeding the threshold value according to this difference
>measurement.
>
>To complicate matters, these chips can also be configured so that two
>input pins can be configured as a fully differential pair. And we can
>also do open wire detection on these differential pairs. In this case
>we would configure input pins VIN0 and VIN1 as a differential pair, then
>read the difference of those two pins using channel register 1, then
>read again using channel register 2. The we see if the difference of the
>two differences exceeds the threshold.
>
>In this case, we can't have in_(voltage0-voltage1)-(voltage100-voltage101)_*
>attributes. So I guess we would have to do something like
>in_voltage200-voltage300 to handle this case? (voltage200 would represent
>the first measurement of voltage0-voltage1 and voltage300 would represent
>the 2nd measurement of the same).

Hi Jonathan,

I was wondering if you had any opinions on this RFC?

Thx,
Guillaume.

On Mon, 9 Dec 2024 07:41:01 -0800
Guillaume Ranquet <granquet@baylibre.com> wrote:

> On Mon, 18 Nov 2024 21:48, David Lechner <dlechner@baylibre.com> wrote:
> >On 11/15/24 4:34 AM, Guillaume Ranquet wrote:  
> >> 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>
> >> ---
> >> 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.
> >>
> >> As presented here, the code sends a THRESH Rising event tied to the
> >> in_voltageX_raw channel on which it happened.
> >>
> >> We think this is not correct but we can't seem to find an implementation
> >> that would be elegant.
> >>
> >> The main idea would be to add a specific channel for openwire detection
> >> but we still would need to have the openwire enablement and threshold
> >> tied to the voltage channel.
See below, but a few things in this intro that will make that
make more sense...

The threshold does make things fiddlier. I wonder if the right value
is a actually always a hardware circuit characteristic and should be
encoded in firmware.  That may not help though as we still need
to trigger an explicit test of the channel if I understand correctly.


> >>
> >> Any thought on this?
> >>  
> >Just to spell this out in a bit more detail, my understanding is that
> >the procedure works like this...
> >
> >To detect an open wire on a single-ended input, we measure the voltage
> >on pin VIN0 using channel register 15, the we read the voltage on the
> >same pin again using channel register 0. The datasheet isn't clear on
> >the details, but on one or the other or both of these, the ADC chip is
> >applying some kind of pull up or pull down on the input pin so that
> >each measurement will be nearly the same if there is a wire attached
> >with a 0-10V signal on it. Or if the wire is detached and the pins are
> >left floating, the two measurements will be different (> 300 mV).

Ok. So it's a kind of input impedance measure.

> >
> >So this threshold value (the 300 mV) is measured in terms of the
> >difference between two voltage measurements, but of the same input pin.
> >
> >My suggestion is to create extra differential channels like
> >in_voltage0-involtage100_* where 100 is an arbitrary number. These
> >channels would be defined as the difference between the two measurements
> >on the same pin. The event attributes would use these channels since they
> >are triggered by exceeding the threshold value according to this difference
> >measurement.
> >
> >To complicate matters, these chips can also be configured so that two
> >input pins can be configured as a fully differential pair. And we can
> >also do open wire detection on these differential pairs. In this case
> >we would configure input pins VIN0 and VIN1 as a differential pair, then
> >read the difference of those two pins using channel register 1, then
> >read again using channel register 2. The we see if the difference of the
> >two differences exceeds the threshold.
> >
> >In this case, we can't have in_(voltage0-voltage1)-(voltage100-voltage101)_*
> >attributes. So I guess we would have to do something like
> >in_voltage200-voltage300 to handle this case? (voltage200 would represent
> >the first measurement of voltage0-voltage1 and voltage300 would represent
> >the 2nd measurement of the same).  
That's getting non intuitive fast.

We already have the channels, so maybe if we are doing something that is
channels specific we should have a trigger attribute on the relevant channel.
_check that runs any appropriate checks.  That doesn't give the reporting path
though.

> 
> Hi Jonathan,
> 
> I was wondering if you had any opinions on this RFC?

You caught me hiding from the tricky question :)  This was always the last email
in my todo list because almost all the others were easier!

Hmm. Lack of sensible handling for hardware failure of this sort of
'wire fell out' category has been a long term kernel gap in general, not
just in IIO.  I've talked to various maintainers etc about it over the years
and it was always in the category of things that were interesting but no one
had the time to take on.

I've never really come up with a satisfactory plan on what to do about it.
With my server kernel engineer hat on, I think there are masses of well established
paths for what is basically a RAS event, but are those well suited for embedded
ADC cases.  To give background on how we do it for big systems (ignoring EDAC as
that's definitely not relevant here).

1) Error detection anywhere in software spits out a tracepoint.
2) Tracepoints are scooped up by interested software. The opensource project for
   those is RASdaemon https://github.com/mchehab/rasdaemon
   +CC Mauro - my goto person for RAS events :)
   Note that any other interested application with sufficient priv can also listen
   for those tracepoints.

So what to do for embedded cases. I'd love a general agreement as a wire out is a wire
out whether it's an spi bus wire, a line to a camera or a input line to an ADC but
maybe that's a stretch too far.

So we could do:
a) Something a bit like you have here. Squirt it out as some sort of event on an
   IIO channel. I think it needs to be on the normal channel and not care about the
   complexity of how it is done under the hood. The solution of magic channels is
   way too device specific.   That may mean new attributes to trigger the check
   and new events to report a failure.   + New event types - I'm fine with a top
   level code for this if we go that way rather than using THRESH.
b) Do the triggering of checks as above (may well use a userspace script to do that
   periodically or even an in kernel thread).
   Spit the error out as a RAS trace event.  We'd need to try and define some general
   it broke event structures to avoid a massive proliferation with all the userspace
   tooling mess that brings.

Mauro, how does a trace event sound for this sort of thing to you?  Do you think we'd
get any sort of general adoption?  It's a bit early to poke the likely folk (hwmon,
SPI, I2C, other buses, maybe clk, regulator etc + all the the other subsytems that
have the sort of signals where wire out or short are common events to detect) but
if we feel it is a path forwards then I guess I'm sure we can put together a
good proposal for what will end up as a stable set of tracepoints.

IIO folk, would a trace event be too hard to deal with?  Steven Rostedt has some
nice libraries etc that wrap up all the complexity pretty well.  The tracepoint
syntax itself to declare them takes some getting used to but once you've got your
head around them is really neat.

Jonathan

> 
> Thx,
> Guillaume.