Expand support for ads1x4s0x series ADCs #83569

TerryGeng · 2025-01-05T18:39:27Z

The existing support is only for ADS114S08 (16-bit, 12-channel). It has one sister ADS114S06 (16-bit, 6-channel).

Meanwhile, there are the 24-bit variations ADS124S06 and ADS124S08, which are pin-to-pin compatible with the ADS114S0X series. Their documents are almost verbatim identical to the ADS114S0X series with a slightly different read format.

This PR expands support to the remaining three chips, ADS114S06, ADS124S06, and ADS124S08.

Since this is a refactor involving many name changes, I separate the file rename into a commit to make the changes to the actual code more visible. It will be squashed before this PR is ready for merging.

As for the other two commits:

The first one follows the example set in drivers/adc/adc_lmp90xxx.c, introducing two variables in the config structure to save the resolution and number of channels for different variations. It also includes branching statements in the read functions to do the conversion from the raw data to integers correctly. It has a memory footprint of

Memory region         Used Size  Region Size  %age Used
      BOOT_FLASH:         256 B        256 B    100.00%
           FLASH:       74264 B    2096896 B      3.54%
             RAM:        8744 B       264 KB      3.23%
        IDT_LIST:          0 GB        32 KB      0.00%

The second commit presents a different way of implementing the same. I define the read functions in a separate template file and populate the right resolution and channel number using macros. I would think this is more elegant since most boards will have only one type of ADS1X4S0X and it's a waste of resources to save the resolution and channel numbers into the stack, and use extra branching statement to perform the check every time the read is invoked. It has a slightly smaller memory footprint compared to the previous one.

Memory region         Used Size  Region Size  %age Used
      BOOT_FLASH:         256 B        256 B    100.00%
           FLASH:       74232 B    2096896 B      3.54%
             RAM:        8744 B       264 KB      3.23%
        IDT_LIST:          0 GB        32 KB      0.00%

~~I would leave the decision of which one to use to the reviewer. These three (or two) commits will be squashed into one before merging.~~
Our reviewer @benediktibk has pointed out a better way out. I implemented that instead.

Testing

Tested with an ADS124S06 chip with arpi_pico.

The 16-bit read was tested with setting compatible = "ti,ads114s06"; in the overlay. Since the 16-bit read will ignore the last byte of the response from the chip, it should still behave but with only reduced precision.

My overlay file:

&spi0 {
    status = "ok";
    cs-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
    pinctrl-0 = <&spi0_default>;
    pinctrl-names = "default";

	adc_ads124s06: ads124s06@0 {
		compatible = "ti,ads124s06";

        #io-channel-cells = <1>;
        #address-cells = <1>;
        #size-cells = <0>;
		reg = <0>;
		spi-max-frequency = <1000000>;
        drdy-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;

        ads124s06_gpio: ads124s06_gpio {
            compatible = "ti,ads1x4s0x-gpio";
            gpio-controller;
            ngpios = <4>;
            #gpio-cells = <2>;
        };

        channel@0 {
            reg = <0>;
            zephyr,gain = "ADC_GAIN_1";
            zephyr,reference = "ADC_REF_INTERNAL";
            zephyr,acquisition-time = <ADC_ACQ_TIME_DEFAULT>;
            zephyr,resolution = <24>;
            zephyr,input-positive = <0>;
            zephyr,input-negative = <1>;
        };

        channel@1 {
            reg = <1>;
            zephyr,gain = "ADC_GAIN_1";
            zephyr,reference = "ADC_REF_INTERNAL";
            zephyr,acquisition-time = <ADC_ACQ_TIME_DEFAULT>;
            zephyr,resolution = <24>;
            zephyr,input-positive = <2>;
            zephyr,input-negative = <3>;
        };

        channel@2 {
            reg = <2>;
            zephyr,gain = "ADC_GAIN_1";
            zephyr,reference = "ADC_REF_INTERNAL";
            zephyr,acquisition-time = <ADC_ACQ_TIME_DEFAULT>;
            zephyr,resolution = <24>;
            zephyr,input-positive = <4>;
            zephyr,input-negative = <5>;
        };
	};
}

Note: the 24-bit read will not work with the examples in samples/drivers/adc/ since they assume 16-bit input. One has to change the buffer to int32_t to see the correct behavior.

The correct conversion (assume the read buffer is buf):

int32_t val = (int32_t)buf;
float my_fval_mv = 2500.0 * val / (1 << 23);

or

 int64_t my_val_mv = (int64_t)val * 2500 >> 23;

benediktibk · 2025-01-15T19:43:03Z

I've skipped through the list of precision ADC from TI (there are astonishingly plenty of them) and it looks like the pattern ads1x4s0x does not match something this driver doesn't (or shouldn't) support.

benediktibk

The rename commit should already reflect the changes as well for instance in the CMakeLists.txt. Basically, the usual approach is not to squash the commits but keep them as they are, but ensure that every commit by itself can be used respectively built.

benediktibk · 2025-01-15T19:46:32Z

dts/bindings/adc/ti,ads114s08.yaml

@@ -1,58 +1,8 @@
 # Copyright (c) 2023 SILA Embedded Solutions GmbH
 # SPDX-License-Identifier: Apache-2.0

-description: Texas Instrument 12 channels 16 bit SPI ADC
+description: Texas Instrument 12 channels 24 bit SPI ADC


Shouldn't this be the 16 bit version?

benediktibk · 2025-01-15T19:55:19Z

drivers/adc/adc_ads1x4s0x.c

 			LOG_ERR("%s: positive channel input %i is invalid", dev->name,
 				channel_cfg->input_positive);
 			return -EINVAL;
 		}

-		if (channel_cfg->input_negative >= ADS114S0X_INPUT_SELECTION_AINCOM) {
+		if (channel_cfg->input_negative >= config->channels \
+				&& channel_cfg->input_negative != ADS1X4S0X_INPUT_SELECTION_AINCOM)  {


Should this be an or instead of an and?

Actually && is the correct choice. ADS124S06 and S08 both have the AINCOM to be 12, but S06 has only 0~5 usable channels whereas S08 has 0~11 all usable. So this is to check that for S06 devices the channels are within the range of {0~5, 12}.

Actually, according to the TI datasheet, for S06 devices, the channel 6 and 7 are mapped to REFP1 and REFN1 so they are also usable, but I don't plan to write them into the code because it is quite confusing.

benediktibk · 2025-01-15T19:55:33Z

drivers/adc/adc_ads1x4s0x.c

 		pin_selections[0] = channel_cfg->input_positive;
 		pin_selections[1] = channel_cfg->input_negative;
 	} else {
 		LOG_DBG("%s: configuring channel for single ended measurement from input %i",
 			dev->name, channel_cfg->input_positive);
-		if (channel_cfg->input_positive >= ADS114S0X_INPUT_SELECTION_AINCOM) {
+		if (channel_cfg->input_positive >= config->channels \
+				&& channel_cfg->input_positive != ADS1X4S0X_INPUT_SELECTION_AINCOM)  {


Should this be an or instead of an and?

benediktibk · 2025-01-15T19:57:29Z

drivers/adc/adc_ads1x4s0x.c

-#define ADS114S0X_POWER_ON_RESET_TIME_IN_US                 2200
-#define ADS114S0X_VBIAS_PIN_MAX                             7
-#define ADS114S0X_VBIAS_PIN_MIN                             0
+#define ADS1X4S0X_CLK_FREQ_IN_KHZ                           4096


I'm definitely sure that it would be easier to review this if the renaming (especially all the defines, ... etc.) would be in a separate commit, because the actual changes are easily missed in this way.

My apology... It was too late before I actually realized this.

It is rather difficult for me to figure out the actual changes within all the renaming. But as I'm neither a collaborator nor a maintainer I don't want to enforce something which is not required.

@anangl @henrikbrixandersen Do you think the renaming should be separated? And should each commit by itself successfully build, therefore removing the necessity of squashing?

So here is the problem: If I rename a file but also change its content, there is a good chance that git cannot pick it up as a rename but instead treat it as a deletion and addition. This is especially troublesome since the name changes inside the file are more than 50% of the file itself.

What I can do is to consolidate all name changes inside a single commit, so the the code will function exactly the same. But the price is that commit will be rather unreadable (as you will see the deletion of a file and the addition of a new file but they are essentially the same). If you would trust me that the only thing I did in that commit is renaming, I will be happy to do so 😄

Nope... I tried that.

Proposal: I do a commit that renames all variables first, then do a second commit that renames the files (and references to those files), just for clarity. These two rename commits will be squashed after this PR is approved, so it will be a large rename commit and should be able to build.

Fine for me, but I also do not think the squash is necessary.

Done. Now all these rename commits should be able to compile.

To rebase or not rebase, it's a problem and I shall leave to others to decide ;)

benediktibk · 2025-01-15T20:04:27Z

tests/drivers/build_all/gpio/adc_ads1145s0x_gpio.overlay

@@ -39,8 +39,8 @@
 				drdy-gpios = <&test_gpio 0 0>;
 				start-sync-gpios = <&test_gpio 0 0>;

-				test_spi_ads114s08_gpio: ads114s0x_gpio {
-					compatible = "ti,ads114s0x-gpio";
+				test_spi_ads114s08_gpio: ads1x4s0x_gpio {


Could you please add one instance of each binding to the build_all tests?

benediktibk · 2025-01-16T07:44:57Z

drivers/adc/adc_ads1x4s0x.c

+/*
+ * ADS114S06: 16 bit, 6 channels
+ */
+#undef DT_DRV_COMPAT


I had a pretty similar issue with some DACs in the past, which were almost similar to each other expect number of channels and resolution. For this we ended up at an approach with a little bit less macro-foo, maybe you can take this as an inspiration? It would probably also help to tackle this kind of problems similarly throughout the project to get some consistency.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dac/dac_ad56xx.c

This is also a nice idea!
I do somewhat dislike my current way because it requires me to move some functions to another header file.

The way in the code you referred to is not to completely redefine those functions by marcos but just redefine a static array holding the channel info.

My concern is: since the channel info is stored in the config structure, which is only passed to the functions in runtime, I think it is likely that the if statement that branches over different chip types inside those functions will not be optimized out by the compiler.

In my case, the resolution-dependent functions include channel_setup, read_sample, adc_perform_read, and validate_sequence.

What's your take on this? Does it actually matter in most cases?

The other way I can think of is to just define those functions for different resolutions and put the pointers to them in the config structure.

It is in the end a tradeoff between readability and runtime speed. But considering the loss is only one additional if I would go for better readability.
The size cost can be mitigate via putting functions which are only necessary in one chip variant in an #ifdef which depends on DT_HAS_..._ENABLED.

Yep, I implemented your proposal (using #ifdef to save size).

TerryGeng · 2025-01-26T20:06:25Z

@benediktibk Sorry about the force push... I scraped my macro commit. The only changes are around the lines you commented on. Also, I added two read_sample functions for 16 and 24 bit devices inside two #ifdef branches, plus the supporting code in perform_read function.

I think I'm ready for you to review again.

…evices This commit shall be squashed into the next one before merging. Signed-off-by: Terry Geng <terry@terriex.com>

This commit works together with the previous one. Signed-off-by: Terry Geng <terry@terriex.com>

Refactor code for ADS114S0X (16 bit) to accommodate ADS124S0X (24 bit). Signed-off-by: Terry Geng <terry@terriex.com>

zephyrbot added area: ADC Analog-to-Digital Converter (ADC) area: GPIO platform: TI SimpleLink Texas Instruments SimpleLink MCU labels Jan 5, 2025

zephyrbot requested review from anangl, henrikbrixandersen, mnkp, vaishnavachath and vanti January 5, 2025 18:40

zephyrbot assigned anangl Jan 5, 2025

TerryGeng force-pushed the ads1x4s0x branch 2 times, most recently from 72406ed to 533aea5 Compare January 5, 2025 19:19

benediktibk reviewed Jan 15, 2025

View reviewed changes

benediktibk suggested changes Jan 15, 2025

View reviewed changes

benediktibk reviewed Jan 15, 2025

View reviewed changes

benediktibk reviewed Jan 16, 2025

View reviewed changes

TerryGeng force-pushed the ads1x4s0x branch from 533aea5 to f59841a Compare January 26, 2025 19:58

TerryGeng force-pushed the ads1x4s0x branch 2 times, most recently from cf5b735 to 83292de Compare January 27, 2025 01:59

TerryGeng added 2 commits January 27, 2025 10:28

drivers: adc: ads114s0x: Rename variables, preparing for adding new d…

9d2c8b1

…evices This commit shall be squashed into the next one before merging. Signed-off-by: Terry Geng <terry@terriex.com>

drivers: adc: ads1x4s0x: Rename files, preparing for adding new devices

36c68ba

This commit works together with the previous one. Signed-off-by: Terry Geng <terry@terriex.com>

TerryGeng force-pushed the ads1x4s0x branch 2 times, most recently from 3abb537 to a74d4f2 Compare January 27, 2025 15:36

drivers: adc: ads1x4s0x: Add ADS124S0X support into existing drivers

7c90029

Refactor code for ADS114S0X (16 bit) to accommodate ADS124S0X (24 bit). Signed-off-by: Terry Geng <terry@terriex.com>

TerryGeng force-pushed the ads1x4s0x branch from a74d4f2 to 7c90029 Compare January 27, 2025 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand support for ads1x4s0x series ADCs #83569

Expand support for ads1x4s0x series ADCs #83569

TerryGeng commented Jan 5, 2025 •

edited

Loading

benediktibk commented Jan 15, 2025

benediktibk left a comment

benediktibk Jan 15, 2025

TerryGeng Jan 26, 2025

benediktibk Jan 15, 2025

TerryGeng Jan 26, 2025 •

edited

Loading

TerryGeng Jan 26, 2025 •

edited

Loading

benediktibk Jan 15, 2025

benediktibk Jan 15, 2025

TerryGeng Jan 15, 2025

benediktibk Jan 27, 2025

benediktibk Jan 27, 2025

TerryGeng Jan 27, 2025 •

edited

Loading

benediktibk Jan 27, 2025

TerryGeng Jan 27, 2025

benediktibk Jan 27, 2025

TerryGeng Jan 27, 2025 •

edited

Loading

benediktibk Jan 15, 2025

TerryGeng Jan 26, 2025

benediktibk Jan 16, 2025

TerryGeng Jan 16, 2025

benediktibk Jan 16, 2025

TerryGeng Jan 26, 2025

TerryGeng commented Jan 26, 2025

Expand support for ads1x4s0x series ADCs #83569

Are you sure you want to change the base?

Expand support for ads1x4s0x series ADCs #83569

Conversation

TerryGeng commented Jan 5, 2025 • edited Loading

Testing

benediktibk commented Jan 15, 2025

benediktibk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

TerryGeng Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryGeng commented Jan 26, 2025

TerryGeng commented Jan 5, 2025 •

edited

Loading

TerryGeng Jan 26, 2025 •

edited

Loading

TerryGeng Jan 26, 2025 •

edited

Loading

TerryGeng Jan 27, 2025 •

edited

Loading

TerryGeng Jan 27, 2025 •

edited

Loading