Re: [PATCH 0/4] iio: adc: new ti-ads112c14 driver

[David Lechner <dlechner@baylibre.com>]

On 6/16/26 12:26 PM, Kurt Borja wrote:
> On Tue Jun 16, 2026 at 10:21 AM -05, David Lechner wrote:
>> On 6/15/26 7:18 PM, Kurt Borja wrote:
>>> On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:

...

>>>> All of these chips have in common that they are designed for use with
>>>> RTDs and thermocouples and so they look very similar to each other in
>>>> terms of wiring and feature set, even if the register maps are
>>>> different. They are in the gray area where we could either keep them
>>>> separate because they are just different enough, or we could do like
>>>> we've done before with ad_sigma_delta and have a bit of an abstraction
>>>> layer for the register differences and otherwise try to share as much
>>>> code as possible. Normally, I would lean towards keeping them separate,
>>>> but in this case, I'm considering trying to share code because the
>>>> devicetree bindings for the inputs is complex and is going to be mostly
>>>> the same across all of these chips.
>>>
>>> The channel configuration is indeed very similar for the three chips.
>>> All three have IDAC, BOC and VREF configurations.
>>
>> Hmm... I forgot to include the burnout current in the DT bindings. Following
>> the channel = "conditions for measurement" pattern that I have set out here
>> I guess that would mean that we would need to have the same inputs twice
>> when using the burnout. One "channel" would be the one used to do a "precision"
>> measurement and the other would be the one to do open/short circuit detection.
>>
>>
>>     i2c {
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>>
>>         adc@40 {
>>             compatible = "ti,ads112c14";
>>             reg = <0x40>;
>>
>>             avdd-supply = <&avdd>;
>>             dvdd-supply = <&dvdd>;
>>
>>             refp-supply = <&avdd>;
>>
>>             #address-cells = <1>;
>>             #size-cells = <0>;
>>
>>             channel@0 {
>>                 reg = <0>;
>>                 diff-channels = <1>, <2>;
>>                 excitation-channels = <0>, <3>;
>>                 excitation-current-microamp = <500>;
>>                 current-chopping;
>>                 ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
>>                 label = "rtd-precision";
>>             };
>>
>>             channel@1 {
>>                 reg = <0>;
>>                 diff-channels = <1>, <2>;
>>                 excitation-channels = <0>, <3>;
>>                 excitation-current-microamp = <500>;
>>                 burnout-current-nanoamp = <1000>;
>>                 ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
>>                 label = "rtd-diagnostic";
>>             };
> 
> This would mean we wouldn't be able to use iio_chan_spec .channel and
> .channel2 to describe inputs because of duplicate sysfs attributes, no?
> 

Yes, that is a bit unfortunate. At least there the labels to tell them
apart. I guess we would just need to use consecutive channel and channel2
when dynamically allocating the channels to avoid conflict. 

>>>> This makes things more flexible, but does make the driver a bit more
>>>> complex. For example, knowing when the current output needs to be
>>>> enabled or disabled. For now, I have chosen a lazy-enable where they
>>>> are not turned on until the first measurement is taken that requires
>>>> them, but then they stay on until another measurement is taken that
>>>> doesn't require them. This can lead to some oddness with the diagnostic
>>>> channels that may be measuring something that indirectly requires the
>>>> current output (i.e. the external reference voltage when it is connected
>>>> to a resistor rather than a power supply). This means you need to take
>>>> a measurement that requires the current output to be enabled before the
>>>> diagnostic channels will give accurate readings.
>>>
>>> This is the same approach I took around the BOC, it feels kinda hacky
>>> but it makes sense. Just an idea I thought about just now: What if we
>>> have an additional write-only "_enable" sysfs attribute for these
>>> channels?
>>
>> I would not want to make a write-only attribute, we always want to be
>> able to read back what the current state is.
> 
> Yeah, I don't know why I said WO. Reading would be fine too.
> 
>>
>> Do you mean an _enable for just the BOC? I think I would do it like I
>> suggested above instead.
> 
> No, no just the BOC. The BOC, IDAC and rest of side effects. Thinking
> about it some more, it would be a bit redundant but clearer if proper
> documentation is provided.
> 
I would be interested to see what Jonathan has to say about this too.
Generally, his advice has been to avoid attributes that power things
on and off if we can help it.