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

[Ceclan, Dumitru]

On 02/04/2024 00:53, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <devnull+dumitru.ceclan.analog.com@xxxxxxxxxx> 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>
>> ---
> 
> ...
> 
>> @@ -951,7 +1117,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>>         struct device *dev = indio_dev->dev.parent;
>>         struct iio_chan_spec *chan_arr, *chan;
>>         unsigned int ain[2], chan_index = 0;
>> -       int ref_sel, ret, num_channels;
>> +       int ref_sel, ret, reg, num_channels;
>>
>>         num_channels = device_get_child_node_count(dev);
>>
> 
> Another thing that is missing in this function both for the chips
> being added here and the existing chips are channels for _all_
> possible inputs. The driver is adding a fixed input channel for the
> temperature sensor, as it should. But all of the chips also have a
> similar input channel configuration that measures the reference
> voltage. Currently, there doesn't seem to be a way to make use of this
> feature. I would expect a hard-coded voltage input channel that is
> always added for this reference voltage similar to the temperature
> channel.
> 

AD7173-8:
 Channel input configs:
AINPOS0: REF+ 10101: 21
AINNEG0: REF- 10110: 22

For the user to define the REF measurement channel:
 diff-channels = <21 22>;
So it is possible from the binding side. It would just need support from
the driver as currently any value above the stated number of inputs is
rejected. Maybe document this in a comment like you suggested below.

I really do not agree with using up channels without letting the user
decide. I can accept to dedicate one for the temp where applicable but
more than that and it feels like we are restricting the usage too much.


> The ad717x chips (except AD7173-8 and AD7176-2) also have a common
> mode voltage input ("((AVDD1 − AVSS)/5)") that could work the same.
> 

Again, would be resolved if I added support from the driver.

> In the case of the ad717x chips though, it looks like these channels
> are not "fixed" like they are in ad411x. It looks like these inputs
> can be mixed and matched with AINx inputs and/or each other as
> differential pairs. So if that is actually the case, I would expect
> the DT bindings for ad717x to look like adi,ad4130.yaml where these
> additional input sources are listed in the diff-channels property
> instead of having hard-coded channels in the driver like I have
> suggested above.
> 

Yep, agree.

> But, as always, fixes for ad717x should be in a separate patch series.
> Still, I think adding a hard-coded channel for the reference voltage
> input for ad411x chips in this patch makes sense.

As stated above, not comfortable with using up channels with hard-coded
values.