Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x

["Ceclan, Dumitru" <mitrutzceclan@gmail.com>]

On 06/04/2024 17:26, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 16:08:56 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 03/04/2024 18:22, David Lechner wrote:
>>> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:  
>>>> On 02/04/2024 00:16, David Lechner wrote:  
>>>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> 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>
>>>>>>>  
>>>> ...
>>>>  
>>>>>>>      properties:
>>>>>>>        reg:
>>>>>>> +        description:
>>>>>>> +          Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>>>>>>          minimum: 0
>>>>>>> -        maximum: 15
>>>>>>> +        maximum: 19  
>>>>>> This looks wrong. Isn't reg describing the number of logical channels
>>>>>> (# of channel config registers)?
>>>>>>
>>>>>> After reviewing the driver, I see that > 16 is used as a way of
>>>>>> flagging current inputs, but still seems like the wrong way to do it.
>>>>>> See suggestion below.
>>>>>>  
>>>>>>>        diff-channels:
>>>>>>> +        description:
>>>>>>> +          For using current channels specify only the positive channel.
>>>>>>> +            (IIN2+, IIN2−) -> diff-channels = <2 0>  
>>>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>>>>>> VIN0. I think it would make more sense to assign unique channel
>>>>>> numbers individually to the negative and positive current inputs.
>>>>>> Also, I think it makes sense to use the same numbers that the
>>>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>>>>>> positive).
>>>>>>
>>>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>  
>>>>> Thinking about this a bit more...
>>>>>
>>>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>>>> with multiple valid wiring configurations like the voltage inputs, do
>>>>> we even need to describe them in the devicetree?
>>>>>
>>>>> In the driver, the current channels would just be hard-coded like the
>>>>> temperature channel since there isn't any application-specific
>>>>> variation.  
>>>>  Sure, but we still need to offer the user a way to configure which
>>>> current inputs he wants and if they should use bipolar or unipolar coding.  
>>> From the datasheet, it looks like only positive current input is
>>> allowed so I'm not sure bipolar applies here. But, yes, if there is
>>> some other variation in wiring or electrical signal that needs to be
>>> describe here, then it makes sense to allow a channel configuration
>>> node for it.  
>>
>> AD4111 datasheet pg.29:
>> When the ADC is configured for bipolar operation, the output
>> code is offset binary with a negative full-scale voltage resulting
>> in a code of 000 … 000, a zero differential input voltage resulting in
>> a code of 100 … 000, and a positive full-scale input voltage
>> resulting in a code of 111 … 111. The output code for any
>> analog input voltage can be represented as
>> Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
>> The output code for any input current is represented as
>> Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
>>
>> I would say bipolar applies here, not a great idea because of the limitation on
>>  the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
>>  is available.
> Just to check I am correct in thinking you 'might' use bipolar if you want
> to be able to measure small negative currents, but the range is much larger
> in the positive direction?
> 
> J

Yes, exactly