Received-SPF: pass (google.com: domain of linux-kernel+bounces-174553-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <0fef36f8-a7db-40cc-86bd-9449cb4ab46e@gmail.com>
Date: Thu, 9 May 2024 16:48:08 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x
To: Jonathan Cameron <jic23@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>, dumitru.ceclan@analog.com,
 Lars-Peter Clausen <lars@metafoo.de>,
 Michael Hennerich <Michael.Hennerich@analog.com>,
 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-1-34618a9cc502@analog.com>
 <CAMknhBHeKAQ45=5-dL1T1tv-mZcPN+bNo3vxWJYgWpEPE+8p3Q@mail.gmail.com>
 <25cb3514-1281-49a8-9e9b-40ead9b050dc@gmail.com>
 <CAMknhBHu8DveBgV3cor8RP2Up4Zs-+QRx7S2aoHZ_3iKiErVjg@mail.gmail.com>
 <20240406155328.447b594f@jic23-huawei>
 <64b7fd83-f226-4b1f-a801-0fe1cf20f842@gmail.com>
 <20240413114825.74e7f3fa@jic23-huawei>
 <89e93f4d-e569-46ee-802d-a1668a01b882@gmail.com>
 <20240420153310.7876cb8a@jic23-huawei>
 <681f3bfb-76d0-41a8-82b5-6d27641c24b6@gmail.com>
 <20240428181334.5792e1df@jic23-huawei>
Content-Language: en-US
From: "Ceclan, Dumitru" <mitrutzceclan@gmail.com>
In-Reply-To: <20240428181334.5792e1df@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 28/04/2024 20:13, Jonathan Cameron wrote:
> On Tue, 23 Apr 2024 11:18:47 +0300
> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
> 
>> On 20/04/2024 17:33, Jonathan Cameron wrote:
>>> On Mon, 15 Apr 2024 21:42:50 +0300
>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>   
>>>> On 13/04/2024 13:49, Jonathan Cameron wrote:  
>>>>> On Tue, 9 Apr 2024 11:08:28 +0300
>>>>> "Ceclan, Dumitru" <mitrutzceclan@gmail.com> wrote:
>>>>>     
>>>>>> On 06/04/2024 17:53, Jonathan Cameron wrote:    
>>>>>>> On Wed, 3 Apr 2024 10:40:39 -0500
>>>>>>> David Lechner <dlechner@baylibre.com> wrote:
>>>>>>>       
>>>>>>>> On Wed, Apr 3, 2024 at 2:43 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:      
>>>>>>>>>
>>>>>>>>> On 01/04/2024 22:37, 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>        
>>>>>>>>>    
..
>>
>>>>  
>>>>>>
>>>>>> I think that a distinction needs to be made here:
>>>>>> - When a device is only pseudo differential, sure, it is in a different category
>>>>>> - When a device is fully differential and you are using it as pseudo-differential
>>>>>>   you are having two inputs compared to one another
>>>>>>
>>>>>> I would need more clarification is why would supply-vincom be a requirement.
>>>>>> The voltage supplied to VINCOM will not be used in any computation within 
>>>>>> the driver. From the perspective of getting the data it doesn't matter if 
>>>>>> you are using the channel in a pseudo-differential, single ended or fully
>>>>>> differential manner.    
>>>>>
>>>>> I'd missed until now that the datasheet (I looked ad4114) says aincomm should be connected to analog
>>>>> ground so indeed nothing to turn on in this case and no offset to supply
>>>>> (the offset will be 0 so we don't present it).
>>>>>
>>>>> I'll note the datasheet describes the VINCOM as follows.
>>>>>
>>>>> Voltage-Input Copmmon. Voltage inputs are reference to VINCOM when the inputs are configured
>>>>> as single-ended.  Connect AINCOM to analog ground.
>>>>>
>>>>> The reference to single ended is pretty clear hint to me that this case
>>>>> is not a differential channel. The more complex case is the one David
>>>>> raised of the AD4116 where we have actual pseudo differential inputs.
>>>>>     
>>>>
>>>> Alright, from my perspective they all pass through the same mux but okay,
>>>> not differential. The only issue would differentiating cases in AD4116 where
>>>> the pair VIN10 - VINCOM is specified as single-ended or differential pair.
>>>>
>>>> Also, AD4116:
>>>> "0101101111 ADCIN11, ADCIN15. (pseudo differential or differential pair)
>>>>  0110001111 ADCIN12, ADCIN15. (pseudo differential or differential pair)
>>>>  0110101111 ADCIN13, ADCIN15. (pseudo differential or differential pair)
>>>>  0111001111 ADCIN14, ADCIN15. (pseudo differential or differential pair)"
>>>>
>>>> Not really sure where the "actual pseudo differential" sits.
>>>>
>>>> Would you agree with having device tree flags that specifies how is the
>>>> channel used: single-ended, pseudo-differential, differential.
>>>> For the first two, the differential flag will not be set in IIO.  
>>>
>>> Yes. I think that makes sense - though as you observe in some cases
>>> the actual device settings end up the same (the ad4116 note above).
>>>  
>>  This precisely why I suggest this approach, because a channel used as
>> single-ended, pseudo or fully differential will have the same register
>> configuration on all models. I do not see any other way to know from
>> the driver this information.
>>
>>> If a given channel supports single-ended and pseudo-differential is
>>> that really just a low reference change (I assume from an input to the
>>> the IO ground)? Or is there more going on?
>>>   
>>  I'm not sure if I understood what was said here. The reference specified
>> in the channel setup does not need to change.
> 
> So what is the effective difference?  My assumption was that single-ended
> means reference to 0V in all cases.  Pseudo differential means reference
> to an input that is common across multiple channels, but not necessarily 0V?
> 

I do not have a clear answer...

This ADI page for example, defines pseudo-differential precisely what we
assumed (non 0V on IN-):
	"On the negative input IN- you apply a DC voltage to shift your signal."
https://ez.analog.com/data_converters/precision_adcs/w/documents/2875/difference-between-pseudo-differential-and-single-ended-mode-of-an-adc

While this page defines what we call single-ended as "pseudo-differential unipolar".
https://www.analog.com/en/resources/technical-articles/sar-adc-input-types.html

These two use-cases are not clearly differentiated from one another, what we are
referring to as pseudo-differential is called single-ended in some datasheets.


I've made a summary of how each datasheet defines these cases:

AD411x (1, 2, 4, 5) only mention single-ended as IN- selecting VINCOM.
                                                         (connected to AVSS)

AD4116 mentions both single-ended and pseudo 
       (pseudo specified as IN- connected to a non 0V reference)

AD717x (2-2, 3-8) 
	Does not contain pseudo differential references
	Single-ended is exemplified as IN- connected to 0V or a non 0V reference.

AD717x (5-2, 5-8, 7-2)
	Mentions at the start that pseudo differential is supported.
	Single-ended is exemplified as both IN- connected to 0V and a non 0V reference.

AD7172-4 only mentions single-ended with the example IN- connected to AVSS.
                                 (This model does not have the internal reference)

AD7176-2 is special, as has only the pseudo differential section but
calls them single-ended:
"Pseudo Differential Inputs
 The user can also choose to measure four different single-ended
 analog inputs. In this case, each of the analog inputs is converted
 as being the difference between the single-ended input to be
 measured and a set analog input common pin."

>>
>>> If it's the reference, then can we provide that as the binding control
>>> signal?  We have other drivers that do that (though we could perhaps make
>>> it more generic) e.g. adi,ad7124 with adi,reference-select
>>>  We already have adi,reference-select in the binding and driver, I do not  
>> see how it could help the driver differentiate between (single, pseudo...)
> 
> Indeed that doesn't work.  Problem in this discussion is I've normally forgotten
> the earlier discussion when I come back to it :( 
>>
>>> I don't like that binding because it always ends up have a local enum
>>> of values, but can't really think of a better solution.
>>>  
>>
>>>>  
>>>>>>
>>>>>> Regarding VINX VINX+1, it is not mandatory to respect those, from AD4111 page27:
>>>>>> "Due to the matching resistors on the analog front end, the
>>>>>>  differential inputs must be paired together in the following
>>>>>>  pairs: VIN0 and VIN1, VIN2 and VIN3, VIN4 and VIN5, and
>>>>>>  VIN6 and VIN7. If any two voltage inputs are paired in a
>>>>>>  configuration other than what is described in this data sheet,
>>>>>>  the accuracy of the device cannot be guaranteed."    
>>>>>
>>>>> OK, but I'll assume no 'good' customer of ADI will do that as any support
>>>>> engineer would grumpily point at that statement if they ever reported
>>>>> a problem :)
>>>>>     
>>>>>>
>>>>>> Tried the device and it works as fully differential when pairing any
>>>>>> VINx with VINCOM. Still works when selecting VINCOM as the positive
>>>>>> input of the ADC.
>>>>>>
>>>>>> I really see this as overly complicated and unnecessary. These families
>>>>>> of ADCs are fully differential. If you are using it to measure a single ended
>>>>>> (Be it compared to 0V or pseudo differential where you are comparing to Vref/2
>>>>>> and obtaining results [Vref/2 , -Vref/2]) the final result will not require knowing
>>>>>> the common voltage.    
>>>>>
>>>>> For single ended VINCOM should be tied to analog 0V.  If the chip docs allowed
>>>>> you to tie it to a different voltage then the single ended mode would be offset
>>>>> wrt to that value.
>>>>>
>>>>> For the AD4116 case in pseudo differential mode, You would need an ADCIN15 supply because
>>>>> that is not connected to analog 0V.  If the device is being used in a pseudo differential
>>>>> mode that provides a fixed offset voltage.
>>>>>
>>>>> So my preference (though I could maybe be convinced it's not worth the effort)
>>>>> is to treat pseudo differential as single ended channels where 'negative' pin is
>>>>> providing a fixed voltage (or 0V if that's relevant).  Thus measurements provided
>>>>> to userspace include the information of that offset.
>>>>>     
>>>>
>>>> What do you mean by offset? I currently understand that the user will have
>>>> a way of reading the voltage of that specific supply from the driver.   
>>>
>>> How?  We could do it that way, but we don't have existing ABI for this that
>>> I can think of.
>>>   
>> Expose a voltage channel which is not reading from the device...but that is
>> too much of a hack to be accepted here
> 
> We have done things like that for a few corner cases where we were really stuck
> but it is indeed nasty and hard to comprehend. Also so far they've been 'out'
> channels I think which doesn't make sense here.
> 
>>>>
>>>> If you mean provide a different channel offset value when using it as
>>>> pseudo-differential then I would disagree  
>>>
>>> Provided to user space as _offset on the channel, userspace can either
>>> incorporate it if it wants to compute absolute (relative to some 0V somewhere) value
>>> or ignore it if it only wants the difference from the reference value.
>>>
>>> I'm open to discussion other ABI options, but this is the one we most naturally have
>>> available.  
>> _offset is already used when the bipolar coding is enabled on the channel
>> and is computed along datasheet specifications of how data should be processed,
>> this is why I disagree with this.
> 
> OK.  It would be easy enough to apply an offset to that, but it would
> complicate the driver and could seem a little odd.
> 
>>
>> This feels over-engineered, most of the times if a channel is pseudo
>> differential, the relevant measurement will be the differences between
>> those two inputs.
>>
>> If a user needs to know the voltage on the common input, he just needs to
>> also configure a single ended channel with the common input where the
>> negative AIN is connected to AVSS.
> 
> OK.  I'm somewhat convinced that this is enough of a pain to describe that
> we should just rely on them having some other way to get that offset if they
> are deliberately using it to shift the range. We can revisit if it ever
> becomes a problem.
> 
> So, I think the conclusion is just don't represent AIN-COMM (or similar)
> as an explicit voltage we can measure.
> 
Perfect. I'll keep the adi,channel-type attribute in the next version 
(that is there just to control the differential flag from the iio
channel struct) and we'll see if it's alright.

>>>>  
>>>>> We haven't handled pseudo differential channels that well in the past, but the
>>>>> recent discussions have lead to a cleaner overall solution and it would be good
>>>>> to be consistent going forwards.  We could deprecate the previous bindings in
>>>>> existing drivers, but that is a job for another day  (possibly never happens!)
>>>>>     
>>>>
>>>> I really hope that a clean solution could be obtained for this driver as well :)   
>>>
>>> I bet you wish sometimes that you had easier parts to write drivers for! :)
>>> These continue to stretch the boundaries which is good, but slow.
>>>
>>> Jonathan  
>>
>> Not easier, fewer crammed into the same driver :)
> 
> I sympathise! It's been an annoyingly busy kernel cycle in the day job. I was hoping to
> get back to you sooner so that more of this was fresh(ish) in my mind :(
> 
> My gut feeling is that this is a case for documentation / really detailed cover
> letter for the next version to make sure we have come to at least a (mostly)
> consistent conclusion.
> 
> Jonathan
>