Re: [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices

[Nuno Sá]

On Fri, 2024-06-07 at 12:41 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:20, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
> > > 
> > > Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> > > 
> > > The AD411X family encompasses a series of low power, low noise, 24-bit,
> > > sigma-delta analog-to-digital converters that offer a versatile range of
> > > specifications.
> > > 
> > > This family of ADCs integrates an analog front end suitable for processing
> > > both fully differential and single-ended, bipolar voltage inputs
> > > addressing a wide array of industrial and instrumentation requirements.
> > > 
> > > - All ADCs have inputs with a precision voltage divider with a division
> > >   ratio of 10.
> > > - AD4116 has 5 low level inputs without a voltage divider.
> > > - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> > >   shunt resistor.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 317
> > > ++++++++++++++++++++++++++++++++++++++++++----
> > > -
> > >  1 file changed, 285 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 58da5717fd36..cfcd12447e24 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > 
> > ...
> > 
> > >  static const struct ad7173_device_info ad7172_2_device_info = {
> > >  	.name = "ad7172-2",
> > >  	.id = AD7172_2_ID,
> > > -	.num_inputs = 5,
> > > +	.num_voltage_in = 5,
> > >  	.num_channels = 4,
> > >  	.num_configs = 4,
> > >  	.num_gpios = 2,
> > > +	.higher_gpio_bits = false,
> > 
> > No need to explicitly set to 'false'. Ditto for the other places...
> > 
> > ...
> > 
> > > 
> > >  static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > >  					      unsigned int ain0, unsigned
> > > int
> > > ain1)
> > >  {
> > > @@ -946,15 +1145,30 @@ static int
> > > ad7173_validate_voltage_ain_inputs(struct
> > > ad7173_state *st,
> > >  	    st->info->has_pow_supply_monitoring)
> > >  		return 0;
> > >  
> > > -	special_input0 = AD7173_IS_REF_INPUT(ain0);
> > > -	special_input1 = AD7173_IS_REF_INPUT(ain1);
> > > +	special_input0 = AD7173_IS_REF_INPUT(ain0) ||
> > > +			 (ain0 == AD4111_VINCOM_INPUT && st->info-
> > > > has_vincom_input);
> > > +	special_input1 = AD7173_IS_REF_INPUT(ain1) ||
> > > +			 (ain1 == AD4111_VINCOM_INPUT && st->info-
> > > > has_vincom_input);
> > > +
> > 
> > Wondering... can ain1 (or ain0) be AD4111_VINCOM_INPUT and !st->info-
> > > has_vincom_input? Would that actually be acceptable? It would assume it's
> > > not
> > so we should check that right? Or am I missing something?
> > 
> > - Nuno Sá
> > 
> 
> It will fail when we check for the number of voltage inputs:
> (ain0 >= st->info->num_voltage_in && !special_input0) 
> as special_input will not be true if has_vincom_input is false
> 
> Indeed this check is a bit hidden, should it be more explicit?
> 

Hmm I see... Up to you. I guess I was not paying enough attention to 
st->info->num_voltage_in and to the fact that VINCOM and REFs are not counted by
it.

OTOH, yes, an explicit check could make sense because the log you output:

"Input pin number out of range for pair..."

It's really not mentioning the real problem (or it's a very hidden message IOW).
having something like

if (ain0 == AD4111_VINCOM_INPUT && !st->info-has_vincom_input)
	return dev_err_probe(dev, -EINVAL, "VINCOM not supported for %s\n",
			part_name);

would be something way easier to understand :)

- Nuno Sá