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