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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <77064a96-c195-4f34-a323-ac01137e448d@gmail.com>
Date: Wed, 3 Apr 2024 11:15:04 +0300
From: "Ceclan, Dumitru" <mitrutzceclan@...il.com>
To: David Lechner <dlechner@...libre.com>, dumitru.ceclan@...log.com
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices

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@...nel.org> wrote:
>>
>> From: Dumitru Ceclan <dumitru.ceclan@...log.com>
>>
>> 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@...log.com>
>> ---
> 
> ...
> 
>> @@ -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.