Linux IIO development
 help / color / mirror / Atom feed
From: "Kurt Borja" <kuurtb@gmail.com>
To: "David Lechner" <dlechner@baylibre.com>,
	"Kurt Borja" <kuurtb@gmail.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Andy Shevchenko" <andy@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>
Cc: "Nguyen Minh Tien" <zizuzacker@gmail.com>,
	<linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/4] iio: adc: new ti-ads112c14 driver
Date: Tue, 16 Jun 2026 12:26:58 -0500	[thread overview]
Message-ID: <DJANEYYA4QTA.1JBN2L78PNXDD@gmail.com> (raw)
In-Reply-To: <d3270250-ae18-4c0f-a0fe-e0fdabfce046@baylibre.com>

On Tue Jun 16, 2026 at 10:21 AM -05, David Lechner wrote:
> On 6/15/26 7:18 PM, Kurt Borja wrote:
>> Hi David,
>> 
>> On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:
>>> This adds support for TI ADS112C14 and ADS122C14 ADC chips.
>>>
>>> The closest thing we've seen to this in the kernel already is ads124s08.
>>> However, that has a completely different register map and the DT
>>> bindings are incomplete and the driver is extremely basic. So I've just
>>> started from scratch here.
>>>
>>> We've also had a similar submission recently for ADS1220 [1]. That chip
>>> is in a similar situation to ads124s08 in that it has a different
>>> register map (but the submitted DT bindings are better than the ones for
>>> ads124s08, even if still a bit incomplete). And literally as I was
>>> writing the previous sentence, another series [2]  was sent for yet
>>> another similar family of chips (ADS1262). That one is even more complex
>>> in the feature set than the ones I am working on. I was going to polish
>>> up the driver a bit more before submitting it, but now it seems more
>>> urgent to coordinate with the other two series to align on how we would
>>> like to handle all of these.
>>>
>>> [1]: https://lore.kernel.org/linux-iio/20260610151342.44274-1-zizuzacker@gmail.com/
>>> [2]: https://lore.kernel.org/linux-iio/20260612-ads126x-v1-0-894c788d03ed@gmail.com/
>>>
>>> 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?

>         };
>     };
>
>> 
>>>
>>> If we decide to go the route of sharing code, we could still merge this
>>> series as-is and then do the refactoring to add the abstraction layer in
>>> a follow-up series that also adds support for the first of the other
>>> chips.
>> 
>> Do you have a proposal of how such an abstraction would look like? I do
>> like the idea of abstracting the firmware parsing, scales and shared
>> calculations.
>
> As mentioned, it would look a bit like the ad_sigma_delta ADC driver.
> Basically, each chip (family with same registers) would still have it's
> own driver that include the chip-specific const info structs. In this
> case, these would also include function pointers for all of the functions
> that need to access registers. And there will be lots of supports_... bool
> flags for the shared code to use.
>
>> 
>>>
>>> This series includes just basic support for reading single measurements
>>> from the ADC and gain selection via the scale attribute. I plan to
>>> follow this up with additional series to add support for buffered reads,
>>> filtering/oversampling configuration, event support, gpio controller
>>> support and perhaps a few other things that are slipping my mind right
>>> now.
>>>
>>> The most interesting part about this (that I alluded to above) is the
>>> way channels are handled. These are multipling ADCs with differential
>>> and single-ended inputs. But what sets them apart from other similar
>>> chips is that since they are designed for use with RTDs, there can also
>>> be a current output required to excite the RTD and this current output
>>> might be different for different channels. So the way I conceptualized
>>> the channels is that the devicetree specifies the conditions needed
>>> to take a particular measurement rather than being purely a physical
>>> channel.
>>>
>>> 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.

>
>> 
>> The approach I took for the IDAC was to have a single configuration that
>> it's enabled for all channels. This makes some sense in my device when
>> thinking about optimal software sequencial reads, because of the
>> register layout, but I also see the value in having per-channel IDAC
>> configuration. I think I will take your approach, so we have the same
>> channel configuration around this.
>
> There are only two IDACs but on chips with 8 AIN pins we could have two
> RTDs wired up and if both require 2 IDACs, then we can't have a fixed
> setting for the IDACs. This is why I made it per-channel.

I'll make it per-channel too.

>
>> 
>> Have you thought about how to implement the BOC? In the ADS1262 the
>> feature can be found "Sensor Bias". What I did was add per channel DT
>> properties for this too.
>
> See above.
>
>> 
>> Another question. When you implement power management in the future,
>> will you enable autosuspend? IDAC currents will be lost if autosuspend
>> is enabled. Is this acceptable? In my case I did enable autosuspend, but
>> I have some doubts about this.
>
> I don't like to implement power management unless I have an application
> that actually requires it. Otherwise, I consider it premature optimization.
> It is difficult to know if we would be implementing it in a way that is
> actually useful for a real-world use case. These chips seem like they
> would mostly be used in an industrial setting, not in battery powered
> applications, so I'm not sure anyone is going to worry about saving a few
> milliamps.

Makes sense. I can probably drop the autosuspend in that case.

>
>> 
>>>
>>> I have also pushed a branch to [3] that contains the start of some
>>> documentation for this driver that can give some more insight into how
>>> the implementation works. It still needs some work and also documents
>>> some things that haven't been implemented yet, so I haven't included it
>>> in this series yet.
>>>
>>> [3]: https://github.com/dlech/linux/blob/b4/iio-adc-ti-ads122c14/Documentation/iio/ads112c14.rst
>>>
>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>> ---
>>> David Lechner (TI) (4):
>>>       dt-bindings: iio: adc: add ti,ads122c14
>>>       iio: adc: add ti-ads112c14 driver
>>>       iio: adc: ti-ads112c14: implement gain on internal short SYS_MON channel
>>>       iio: adc: ti-ads112c14: add measurement channel support
>>>
>>>  .../devicetree/bindings/iio/adc/ti,ads112c14.yaml  |  224 +++++
>>>  MAINTAINERS                                        |    8 +
>>>  drivers/iio/adc/Kconfig                            |   12 +
>>>  drivers/iio/adc/Makefile                           |    1 +
>>>  drivers/iio/adc/ti-ads112c14.c                     | 1053 ++++++++++++++++++++
>>>  include/dt-bindings/iio/adc/ti,ads112c14.h         |   11 +
>>>  6 files changed, 1309 insertions(+)
>>> ---
>>> base-commit: ec039126b7fac4e3af35ebccaa7c6f9b6875ba81
>>> change-id: 20260514-iio-adc-ti-ads122c14-d0b92479334e
>>>
>>> Best regards,
>>> --  
>>> David Lechner (TI) <dlechner@baylibre.com>
>> 

-- 
Thanks,
 ~ Kurt

  reply	other threads:[~2026-06-16 17:27 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-15 21:59 [PATCH 0/4] iio: adc: new ti-ads112c14 driver David Lechner (TI)
2026-06-15 21:59 ` [PATCH 1/4] dt-bindings: iio: adc: add ti,ads122c14 David Lechner (TI)
2026-06-16  0:26   ` Kurt Borja
2026-06-16 15:22     ` David Lechner
2026-06-16 17:31       ` Kurt Borja
2026-06-16 16:07   ` Conor Dooley
2026-06-16 19:54     ` David Lechner
2026-06-16 20:50       ` Conor Dooley
2026-06-16 21:04         ` David Lechner
2026-06-15 22:00 ` [PATCH 2/4] iio: adc: add ti-ads112c14 driver David Lechner (TI)
2026-06-16  7:32   ` Andy Shevchenko
2026-06-16 15:38     ` David Lechner
2026-06-15 22:00 ` [PATCH 3/4] iio: adc: ti-ads112c14: implement gain on internal short SYS_MON channel David Lechner (TI)
2026-06-16  7:58   ` Andy Shevchenko
2026-06-16 10:03     ` Nuno Sá
2026-06-15 22:00 ` [PATCH 4/4] iio: adc: ti-ads112c14: add measurement channel support David Lechner (TI)
2026-06-16  8:36   ` Andy Shevchenko
2026-06-16 15:55     ` David Lechner
2026-06-16 15:30   ` David Lechner
2026-06-16  0:18 ` [PATCH 0/4] iio: adc: new ti-ads112c14 driver Kurt Borja
2026-06-16 15:21   ` David Lechner
2026-06-16 17:26     ` Kurt Borja [this message]
2026-06-16 18:16       ` David Lechner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DJANEYYA4QTA.1JBN2L78PNXDD@gmail.com \
    --to=kuurtb@gmail.com \
    --cc=andy@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlechner@baylibre.com \
    --cc=jic23@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nuno.sa@analog.com \
    --cc=robh@kernel.org \
    --cc=zizuzacker@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox