-
Notifications
You must be signed in to change notification settings - Fork 8.7k
drivers: adc: Add Infineon PSOC4 SAR ADC support #103453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
drivers: adc: Add Infineon PSOC4 SAR ADC support #103453
Conversation
d8c406d to
90047a3
Compare
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| } | ||
|
|
||
| /* Check for 12-bit resolution */ | ||
| if (sequence->resolution == 12) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A define here will not make this more readable.
#define RESOLUTIION_12B 12 is not more readable than sequence->resolution == 12 here
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| data->channel_configs[ch].differential = ch_cfg->differential; | ||
| data->channel_configs[ch].avgEn = (sequence->oversampling > 0); | ||
| data->channel_configs[ch].resolution = | ||
| (sequence->resolution == 12) ? CY_SAR_MAX_RES : CY_SAR_SUB_RES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto as above, the resolution will not be more readable from a define here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sorts of broad "replace X with Y" comments are not helpful, #define'ing numbers is not always more readable and numbers with easily understood context (e.g. resolution = 12) won't be anymore readable by having a #define for them.
If you have suggestions on numbers that would benefit then by all means suggest it. But please do not dictate terms on how to change code without reasoning behind it.
90047a3 to
4afadd0
Compare
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| /* Configure ADC sample times */ | ||
| static void psoc4_configure_sample_times(struct psoc4_adc_data *data) | ||
| { | ||
| data->pdl_sar_cfg.sampleTime0 = data->sample_times[0] ? data->sample_times[0] : 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
a2ed841 to
0d56b99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am withdrawing my previous requested changes as I've reconsidered my position based on the technical discussion. Please disregard my previous review and proceed without my input.
Regarding the magic numbers, I still believe that using macros or defines would significantly improve code readability and ease future maintenance, but I’ll leave the final decision to the other maintainers.
Also, I tried to resolve my previous comments but was unable to, likely due to permission constraints. I kindly request someone with the necessary privileges to resolve/close them. Thank you.
10baefb to
9bd0820
Compare
cy8ckit_041s_max Test ResultsResults gathered with this commit adc_dtExpecting ~0.62V from readings adc_sequenceExpecting ~0.62V from readings adc_apiadc_error_cases |
cy8cproto_041tp Test ResultsResults gathered with this commit adc_dtExpecting ~0.62V from readings adc_sequenceExpecting ~0.62V from readings adc_apiadc_error_cases |
teburd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see a well done PR, its great to see the tests results here as well showing it works. A few comments, one of which I know will require a bit of finagling to work out. The other trivial.
I think we can safely say you have incorporated #101600 into this and given co-authorship in the commits, so its safe for @Deepika-aerlync to close that PR whenever comfortable and we can move forward with this one PR for the sar-adc support.
Thank you @Deepika-aerlync and others for the hard work!
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| .vref_mv = DT_INST_PROP(n, vref_mv), \ | ||
| .clk_dst = (en_clk_dst_t)DT_INST_PROP(n, clk_dst), \ | ||
| }; \ | ||
| static const struct adc_driver_api adc_psoc4_driver_api_##n = { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use DEVICE_API
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/device.h#L1343
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| description: | | ||
| Infineon PSOC4 SAR ADC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than tying this IP block to a part family (PSOC4) we should tie it to a version of the IP like the PDL drivers itself do.
We can do this with a compatible like (infineon,sar-adc-v2), an example here would be ST's I2C IP versioning
https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/i2c/st%2Cstm32-i2c-v1.yaml
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/i2c/Kconfig.stm32#L14
OR by adding (in case the IP is similar enough to others) adding version property
https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/dma/nxp%2Cmcux-edma.yaml#L73
Which then gets use in Kconfig and/or code to vary things based on the version.
No strong preference on my part. The versioned compatible I think matches well with the PDL which also differentiates headers with -vX.h
https://github.com/zephyrproject-rtos/hal_infineon/tree/master/mtb-pdl-cat2/devices/include/ip
|
| #include <zephyr/devicetree.h> | ||
| #include <zephyr/logging/log.h> | ||
| #include <zephyr/sys/util.h> | ||
| #include <zephyr/drivers/clock_control/clock_control_ifx_cat1.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to to add:
#include <infineon_kconfig.h>
This is consistent with other drivers, and will help ensure some PDL macros are defined correctly, such as CY_IP_M0S8PASS4A_SAR_VERSION.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| LOG_ERR("Invalid SARMUX pin number: %d", ch_cfg->vplus); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has several error return paths, but the function is returning void so there is no indication that the function actually failed. Would it make sense to add an error code output here so the caller knows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| } | ||
|
|
||
| LOG_ERR("Invalid pin number %u for routing determination", pin); | ||
| return PSOC4_ADC_ROUTE_SARMUX; /* Fallback */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to return some kind of error code here to indicate an unsupported/invalid routing path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
drivers/adc/adc_infineon_psoc4_sar.c
Outdated
| } else if (data->channel_cfg[ch].vplus_routing == | ||
| data->channel_cfg[ch].vminus_routing) { | ||
| routing_mismatch = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be routing_mismatch = false? It seems like you would want the routing to be the same for positive and negative channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified logic, changed.
| return -EINVAL; | ||
| } | ||
|
|
||
| if (data->channel_cfg[ch].vminus_routing == PSOC4_ADC_ROUTE_SARMUX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any case covering vminus_routing == PSOC4_ADC_ROUTE_SARBUS. Is this not supported in differential mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
| zephyr,input-positive = <2>; /* SARMUX pin 2 = P2.2 (V+) */ | ||
| zephyr,input-negative = <3>; /* SARMUX pin 3 = P2.3 (V-) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout the overlays in this PR, input-positive and input-negative are defined differently between the 4100tp and 4100smax boards. I think either is fine, but it'd be nice if we were consistent with which approach we use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
9bd0820 to
3a9a68e
Compare
Add driver for the PSOC4 SAR ADC peripheral with device tree binding. The driver supports: - Single-ended and differential channel configuration - Configurable resolution (8-12 bits) - Multiple voltage reference sources (internal, VDDA, VDDA/2, external) - Hardware compensation for SAR v2/v3 single-ended channel limitation - Interrupt-driven conversion completion - Integration with peripheral clock control The driver includes a compile-time per-instance API structure to correctly report reference voltage to the ADC framework for accurate raw-to-millivolts conversion. Tested on CY8CKIT-041S-MAX and CY8CPROTO-041TP boards. Signed-off-by: Braeden Lane <[email protected]> Co-authored-by: Deepika R <[email protected]> Co-authored-by: Sayooj K Karun <[email protected]>
Extend the Infineon peripheral clock control driver to support ADC peripherals. Adds IFX_RSC_ADC resource type handling and PSOC4-specific clock destination initialization (set to 0 as PSOC4 peripherals manage their own clock routing). Signed-off-by: Braeden Lane <[email protected]> Co-authored-by: Deepika R <[email protected]> Co-authored-by: Sayooj K Karun <[email protected]>
Enable the SAR ADC peripheral on CY8CKIT-041S-MAX and CY8CPROTO-041TP boards: - Add ADC node to psoc4100smax.dtsi and psoc4100tp.dtsi - Configure clk-dst for peripheral clock routing - 041S-MAX: PCLK_PASS0_CLOCK_SAR (0x12) - 041TP: PCLK_PASS0_CLOCK_SAR (0xc) - Reference peri_clk_div4 for ADC clock source - Enable peri_clk_div4 in board common devicetree includes Signed-off-by: Braeden Lane <[email protected]> Co-authored-by: Deepika R <[email protected]> Co-authored-by: Sayooj K Karun <[email protected]>
Add device tree overlays for ADC samples and tests on CY8CKIT-041S-MAX and CY8CPROTO-041TP boards. Configures ADC channels with pinctrl for analog inputs on available header pins. Includes single-ended and differential channel configurations demonstrating various ADC features. Added overlays for: - samples/drivers/adc/adc_dt - samples/drivers/adc/adc_sequence - tests/drivers/adc/adc_api - tests/drivers/adc/adc_error_cases Signed-off-by: Braeden Lane <[email protected]> Co-authored-by: Deepika R <[email protected]> Co-authored-by: Sayooj K Karun <[email protected]>
3a9a68e to
485347d
Compare
|



This enables ADC driver support for PSOC4-based boards including cy8ckit_041s_max and cy8cproto_041tp.
This supercedes #101600