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

[Jonathan Cameron <jic23@kernel.org>]

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
>