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á
