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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 16 Jan 2025 16:01:47 +0100
Guillaume Ranquet <granquet@baylibre.com> 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>
Hi Guillaume.

In general this series looks fine to me.
A few things inline + maybe drop the RFC for v4.
There are some reviewers who will not take a close look until after that.
Not sure that applies to any of our regulars in IIO but it is appropriate
to drop it anyway at this point I think!

Jonathan

> ---
>  drivers/iio/adc/ad7173.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 161 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 11ff148cb5a315d32485acf04b8d6f7d0fb6e5fa..d1cba93722673d2f7fd9239074d36e5274527557 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -35,6 +35,7 @@

> +/*
> + * 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);

Given you need to do a v4 for some issues below, please also rewrap to sub 80 chars
where it doesn't hurt readability.


> +	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_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;
> +}

...

> @@ -1112,12 +1201,58 @@ 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;

Fall through looks unlikely to be intended and if it were would
need marking.

		return 0; here and drop the return below.
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +	}
> +
> +	return 0;
Unreachable so drop this.

> +}