drivers: adc: ads1x1x: add support for RDY pin

[efra-mx-aqua]

Some hardware configuration require rely on the ALERT/RDY pin to know when and ADC conversion is completed.

• A pin called int-gpios can optionally be define in the DT.
• The polling thread is left as fallback, when the pin is not defined.
• The semaphore is only used if the thread is running
• It introduces a new DT macro DT_ANY_COMPAT_HAS_PROP_STATUS_OKAY
  Fixes adc: ads1115: do conversion using the RDY pin #57952

[efra-mx-aqua]

This ready for review

[XenuIsWatching]

I'm thinking a test case may need to be added for building with the TRIGGER support within https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/adc

[efra-mx-aqua]

I'm thinking a test case may need to be added for building with the TRIGGER support within https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/adc

I am not familiar with those tests, do you have any pointers? It seems all the drivers are only built with the default configuration

By default it is built with CONFIG=ADC_ADS1X1X_TRIGGER=y

[efra-mx-aqua]

@XenuIsWatching I have done the changes you requested

[XenuIsWatching]

I have a comment on #ifdef CONFIG_ADC_ADS1X1X_TRIGGER, but I'm beginning to think there is a better way to infer this from the dts without needing a kConfig at all. If the dts has the define for the alert-rdy-gpios, then it should be implied that it is to use the interrupt pin rather than threaded output.

Maybe just use the macro #ifdef DT_ANY_INST_HAS_PROP_STATUS_OKAY(alert_rdy_gpios) rather than the Kconfig. This should allow it for if at least 1 ads1x1x has the alert-rdy-gpios defined, then it will compile all the code need for the interrupt pin without needing to think about enabling or disabling the kconfig

[XenuIsWatching]

Please fix the compliance issues from the CI and it appears there's another issue with compiling.... other than that it looks good

[sslupsky]

@efra-mx Thanks for this. I was forced to write my own out of tree driver a while ago to support this feature. One thing that you might consider giving some thought to with regard to this driver is the mux. Unfortunately, this driver does not support sequencing the input mux so this driver is unusable for anyone that has more than one channel connected to the ADC's inputs.

[efra-mx]

@efra-mx Thanks for this. I was forced to write my own out of tree driver a while ago to support this feature. One thing that you might consider giving some thought to with regard to this driver is the mux. Unfortunately, this driver does not support sequencing the input mux so this driver is unusable for anyone that has more than one channel connected to the ADC's inputs.

It is not unusable, but it indeed requires a different configuration, you only need different configuration and get one channel at the time

[decsny]

@XenuIsWatching well process here has been very slow. I have rebased several times, but it has become pointless since nobody is doing the review of the other files.

This MR sill become 1-year old. How could we improve that?

I need review from @vaishnavachath @mbolivar-ampere

You can follow the escalation path if needed

[sslupsky]

It is not unusable, but it indeed requires a different configuration, you only need different configuration and get one channel at the time

I am not clear by what you mean. The driver as written only configures one input channel so this driver does not work if you ask it to configure two or more input channels. It returns an ENOTSUP error:

zephyr/drivers/adc/adc_ads1x1x.c

Lines 300 to 303 in 3d1a403

	if (channel_cfg->channel_id != 0) {
	LOG_ERR("unsupported channel id '%d'", channel_cfg->channel_id);
	return -ENOTSUP;
	}

Are you suggesting that the user reconfigure the ADC manually every time? Having to manually update the configuration for each sample means the user cannot use the adc_sequence features and must duplicate a significant amount of the sampling scaffolding that is already provided by the ADC API.

I am not sure what the Zephyr policy is regarding what elements of the ADC API are mandatory but it seems necessary to include support for adc_sequence since the adc_sequence types are always included (not conditional) in the data structure:
https://github.com/zephyrproject-rtos/zephyr/blob/1680223003355673decdf18e7fd98efd9fbd2502/drivers/adc/adc_context.h#L55C1-L74C1

[sslupsky]

This looks incorrect:
https://github.com/efra-mx/zephyr/blame/c2eed75ed0658c445f3bac66b9159a96a3b38269/drivers/adc/adc_ads1x1x.c#L481-L482

The COMP_MODE bit sets the comparator mode, it does not disable the comparator. The COMP_QUE bits disable the comparator.

Also, you should consider initializing the COMP_POL, COMP_LAT and COMP_QUE bits.

Should the config include support for specifying a comparator configuration in the dts for users that use the comparator function?

[XenuIsWatching]

This looks incorrect: https://github.com/efra-mx/zephyr/blame/c2eed75ed0658c445f3bac66b9159a96a3b38269/drivers/adc/adc_ads1x1x.c#L481-L482

The COMP_MODE bit sets the comparator mode, it does not disable the comparator. The COMP_QUE bits disable the comparator.

Also, you should consider initializing the COMP_POL, COMP_LAT and COMP_QUE bits.

Yeah... that makes sense... but that probably should be in a seperate PR

[XenuIsWatching]

I did speak to @mbolivar-ampere over Discord, he said he doesn't see anything crazy but he'm not maintaining DT right now due to dayjob time constraints. I'll ping around on "pr-help" on Discord then

[efra-mx]

Thank you! Hopefully it will be merged.

…

On Thu, Mar 21, 2024, 23:46 Ryan McClelland ***@***.***> wrote: I did speak to @mbolivar-ampere <https://github.com/mbolivar-ampere> over Discord, he said he doesn't see anything crazy but he'm not maintaining DT right now due to dayjob time constraints. I'll ping around on "pr-help" on Discord then — Reply to this email directly, view it on GitHub <#57954 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C7W44ZHCBBR6GB7V5LWTYZNPKRAVCNFSM6AAAAAAYDYB4WWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJTHE3TQOJXG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[vaishnavachath]

@efra-mx-aqua @efra-mx this comment is still open, there are devices in the list which does not expose alert/rdy GPIO, sorry for the delay in review

[sslupsky]

This looks incorrect: https://github.com/efra-mx/zephyr/blame/c2eed75ed0658c445f3bac66b9159a96a3b38269/drivers/adc/adc_ads1x1x.c#L481-L482
The COMP_MODE bit sets the comparator mode, it does not disable the comparator. The COMP_QUE bits disable the comparator.
Also, you should consider initializing the COMP_POL, COMP_LAT and COMP_QUE bits.

Yeah... that makes sense... but that probably should be in a seperate PR

I disagree. This PR is already a year old. Let's get this right and fix the alert issue properly and resolve this known issue and also add the missing adc sequence that is an essential component of the ADC API.

[efra-mx]

Would you like to take over this MR? I have no access to hardware anymore since I have changed job.

…

On Fri, Mar 22, 2024, 14:28 sslupsky ***@***.***> wrote: This looks incorrect: https://github.com/efra-mx/zephyr/blame/c2eed75ed0658c445f3bac66b9159a96a3b38269/drivers/adc/adc_ads1x1x.c#L481-L482 The COMP_MODE bit sets the comparator mode, it does not disable the comparator. The COMP_QUE bits disable the comparator. Also, you should consider initializing the COMP_POL, COMP_LAT and COMP_QUE bits. Yeah... that makes sense... but that probably should be in a seperate PR I disagree. This PR is already a year old. Let's get this right and fix the alert issue properly and resolve this known issue and also add the missing adc sequence that is an essential component of the ADC API. — Reply to this email directly, view it on GitHub <#57954 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C7W2QYVWC5D47UHQGSPDYZQWXNAVCNFSM6AAAAAAYDYB4WWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVGEYDAMRWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[efra-mx]

Damn. I will check it again, it was a while ago. Maybe the change got missed during a rebase

…

On Fri, Mar 22, 2024, 13:20 Vaishnav M A ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In drivers/adc/adc_ads1x1x.c <#57954 (comment)> : > @@ -20,6 +21,17 @@ LOG_MODULE_REGISTER(ADS1X1X, CONFIG_ADC_LOG_LEVEL); +#if DT_ANY_COMPAT_HAS_PROP_STATUS_OKAY(ti_ads1115, alert_rdy_gpios) || \ + DT_ANY_COMPAT_HAS_PROP_STATUS_OKAY(ti_ads1114, alert_rdy_gpios) || \ + DT_ANY_COMPAT_HAS_PROP_STATUS_OKAY(ti_ads1113, alert_rdy_gpios) || \ @efra-mx-aqua <https://github.com/efra-mx-aqua> @efra-mx <https://github.com/efra-mx> this comment is still open, there are devices in the list which does not expose alert/rdy GPIO, sorry for the delay in review — Reply to this email directly, view it on GitHub <#57954 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C7W3QYELHMGPJBNI7I5TYZQOY3AVCNFSM6AAAAAAYDYB4WWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJUGQ4DOMZQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[XenuIsWatching]

This looks incorrect: https://github.com/efra-mx/zephyr/blame/c2eed75ed0658c445f3bac66b9159a96a3b38269/drivers/adc/adc_ads1x1x.c#L481-L482
The COMP_MODE bit sets the comparator mode, it does not disable the comparator. The COMP_QUE bits disable the comparator.
Also, you should consider initializing the COMP_POL, COMP_LAT and COMP_QUE bits.

Yeah... that makes sense... but that probably should be in a seperate PR

I disagree. This PR is already a year old. Let's get this right and fix the alert issue properly and resolve this known issue and also add the missing adc sequence that is an essential component of the ADC API.

A PR should be doing one thing and that is independent of what this is doing. You are welcome to submit your own PR with that code integrated from your OOT driver, and I will take a look when you do so.

[efra-mx]

@vaishnavachath done i have fixed that

[sslupsky]

A PR should be doing one thing and that is independent of what this is doing. You are welcome to submit your own PR with that code integrated from your OOT driver, and I will take a look when you do so.

I understand this PR is adding the support for the ALERT/RDY pin? The COMP_POL, COMP_LAT and COMP_QUE configuration is essential to the proper operation of this feature. So, at a minimum, this PR should address this particular issue.

I also suggested the mux should be addressed. I do agree that supporting the mux is not part of the original scope of the PR. This driver should not have been merged without this capability. Nevertheless, this PR does move the yardstick an inch or two forward so that is progress.

Regarding your suggestion that I make a PR. The length of time it takes to get anything merged in Zephyr is outrageous. Frankly, I don't want to have to keep rebasing a PR for a year or two until it gets merged. I did that already and finally closed the PR's because nobody seemed to care about the bugs in some of the drivers. You are welcome to go pick it out of my repo and submit the PR.

[decsny]

A PR should be doing one thing and that is independent of what this is doing. You are welcome to submit your own PR with that code integrated from your OOT driver, and I will take a look when you do so.

I understand this PR is adding the support for the ALERT/RDY pin? The COMP_POL, COMP_LAT and COMP_QUE configuration is essential to the proper operation of this feature. So, at a minimum, this PR should address this particular issue.

I also suggested the mux should be addressed. I do agree that supporting the mux is not part of the original scope of the PR. This driver should not have been merged without this capability. Nevertheless, this PR does move the yardstick an inch or two forward so that is progress.

Regarding your suggestion that I make a PR. The length of time it takes to get anything merged in Zephyr is outrageous. Frankly, I don't want to have to keep rebasing a PR for a year or two until it gets merged. I did that already and finally closed the PR's because nobody seemed to care about the bugs in some of the drivers. You are welcome to go pick it out of my repo and submit the PR.

Don't know when you were having those PRs but lately the project is trying to address the review velocity, there are new rules about escalation, strict expectations of assignees, and there is a public merge queue site that the release team is constantly monitoring every day, I suggest you to try again

[XenuIsWatching]

I saw the change in a previous commit, but looks like the change where this was using gpio_pin_interrupt_configure_dt() and checking the return value was also lost....
The original commit was here: d84f67d

Anyways I did recover your old commit with some "git fu".
You can recover with the following commands

git fetch https://github.com/zephyrproject-rtos/zephyr.git d84f67dc114c6472306ecd111b2885fccb131426:my_lost_branch

Then reset your branch to that, and rebase to main

[XenuIsWatching]

see comment here: #57954 (comment)

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.