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

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

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

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