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

[Kurt Borja]

On Sun Jun 21, 2026 at 2:14 PM -05, Jonathan Cameron wrote:
> On Tue, 16 Jun 2026 13:16:46 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
>> 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>;
> Maybe use an example with more stuff changing? Do we want same excitation
> for burn out? I've no idea.
>
>> >>                 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. 
>
> From a very initial look, maybe do something similar to the folk have
> been looking at for the more complex DDS devices where we have lots
> of channels that are on the same 'wires'.  Basically add a numbering
> scheme to keep them reasonably separate - channel numbers are cheap.
> Maybe first channel is 10->1f, second 20-2f etc.  They are differential
> so it will get ugly.  Perhaps have a play around and see if there is
> a reasonable channel naming scheme for this 'same inputs, different thing
> being measured case'

May I also suggest having some sort of IIO_VOLTAGE_DIAGNOSTIC channel
type? Would that be worth the trouble?

We could also maybe just drop burn-out current completely from
dt-bindings and add IIO_CHAN_INFO_BURNOUT_CURRENT. Given that this
feature is only used ocasionally for diagnostic purposes (I assume...).

>
> I'm not yet sure I'm convinced that a separate channel model makes sense.
> Even less in the DT given these are different settings for one channel.
> That doesn't mean we don't split them up in the driver if channels
> are the best implementation / ABI to userspace.
>
> Basically nothing actually says diff-channels numbers match the
> userspace ABI, so break that link.
>
>> 
>> >>>> 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.
>> 
> This is again a bit similar to the DDS case, but there we have more than
> one on at a time (as controlling different parts of the signal - freq
> / phase / amplitude).  Here we at least avoid that complexity.
>
> I didn't really like that solution but we didn't come up with any other
> way to support the complex stuff going on.  Hence I'm not necessarily
> suggesting to go that way here.
>
> I think this may be a case of laying out different options in an ABI
> doc then reviewing that to make sure we spot corner cases etc.
>
> Thanks,
>
> Jonathan
>
> p.s. Sometimes it feels like the world just keeps getting more complex!

-- 
Thanks,
 ~ Kurt