Received-SPF: pass (google.com: domain of linux-kernel+bounces-129315-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <77064a96-c195-4f34-a323-ac01137e448d@gmail.com>
Date: Wed, 3 Apr 2024 11:15:04 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 6/6] iio: adc: ad7173: Add support for AD411x devices
To: David Lechner <dlechner@baylibre.com>, dumitru.ceclan@analog.com
Cc: Lars-Peter Clausen <lars@metafoo.de>,
 Michael Hennerich <Michael.Hennerich@analog.com>,
 Jonathan Cameron <jic23@kernel.org>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
 <20240401-ad4111-v1-6-34618a9cc502@analog.com>
 <CAMknhBH-YmFrqNTQCB_KafCTxEqSL+36pkE0O44NqiL89hm64Q@mail.gmail.com>
Content-Language: en-US
From: "Ceclan, Dumitru" <mitrutzceclan@gmail.com>
In-Reply-To: <CAMknhBH-YmFrqNTQCB_KafCTxEqSL+36pkE0O44NqiL89hm64Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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@kernel.org> wrote:
>>
>> From: Dumitru Ceclan <dumitru.ceclan@analog.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@analog.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.