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á