Skip to content

driver: dac: add esp32 support #51271

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 13, 2022
Merged

driver: dac: add esp32 support #51271

merged 4 commits into from
Dec 13, 2022

Conversation

marekmatej
Copy link
Collaborator

@marekmatej marekmatej commented Oct 13, 2022 •
edited
Loading

Add initial support for the DAC peripheral on ESP32 and ESP32-S2. The basic driver is tested with the samples/driver/dac.

Add board specific sample application to demonstrate the on-chip cosine wave generator in samples/boards/esp32/dac_cw.

@zephyrbot zephyrbot added area: DAC Digital-to-Analog Converter area: Xtensa Xtensa Architecture platform: ESP32 Espressif ESP32 area: Devicetree Binding PR modifies or adds a Device Tree binding labels Oct 13, 2022
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 13, 2022 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_espressif zephyrproject-rtos/hal_espressif@d53c07e zephyrproject-rtos/hal_espressif@1416c47 (zephyr) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

sylvioalves
sylvioalves reviewed Oct 13, 2022
View reviewed changes
sylvioalves
sylvioalves reviewed Oct 13, 2022
View reviewed changes
martinjaeger
martinjaeger requested changes Oct 13, 2022
View reviewed changes
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the addition of this driver.

I think the approach with the vendor-specific additional config parameter to allow configuration of the sine wave generator is not a very clean solution. Instead we should add a new API for wave form generation which can be re-used for other DAC drivers as well.

I suggest to start only with the basic DAC driver for ESP32 in this PR and afterwards we can discuss how an additional API for sine wave, sawtooth wave or triangle wave may look like.

uLipe
uLipe requested changes Oct 13, 2022
View reviewed changes
Copy link
Member

@uLipe uLipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, and I don't feel comfortable about the cw as pointed by @martinjaeger .

otherwise LGTM.

@marekmatej marekmatej force-pushed the feature/esp32_dac branch 2 times, most recently from 997d1a6 to dec4c8d Compare October 14, 2022 11:52
galak
galak previously requested changes Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dma dts prop doesnt seem to be used in driver. remove it from binding.

uLipe
uLipe requested changes Nov 1, 2022
View reviewed changes
Copy link
Member

@uLipe uLipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns and nipicks please have a look.

Happy to resolve them if get bettter understanding.

@stephanosio stephanosio added area: West West utility and removed west labels Nov 2, 2022
@martinjaeger
Copy link
Member

Thanks for the updates. Looks good from my side.

Please have a look at the failing build in CI: https://github.com/zephyrproject-rtos/zephyr/actions/runs/3396238497/jobs/5647093859#step:13:698

Looks like some ESP-IDF internal files need to be added to CMake.

@marekmatej
Copy link
Collaborator Author

Thanks for the updates. Looks good from my side.

Please have a look at the failing build in CI: https://github.com/zephyrproject-rtos/zephyr/actions/runs/3396238497/jobs/5647093859#step:13:698

Looks like some ESP-IDF internal files need to be added to CMake.

This build issue is due to to unmerged PR which my PR in hal_espressif depends on.

uLipe
uLipe previously approved these changes Nov 7, 2022
View reviewed changes
@martinjaeger martinjaeger mentioned this pull request Nov 22, 2022
Open
@martinjaeger
Copy link
Member

@marekmatej can you please check the CI failures and fix them?

@marekmatej marekmatej marked this pull request as draft December 5, 2022 12:44
@marekmatej marekmatej marked this pull request as ready for review December 5, 2022 21:25
martinjaeger
martinjaeger previously approved these changes Dec 6, 2022
View reviewed changes
sylvioalves
sylvioalves previously approved these changes Dec 6, 2022
View reviewed changes
@MaureenHelm
Copy link
Member

@galak please revisit

Marek Matej added 4 commits December 7, 2022 10:36
drivers: dac: esp32: Add support for DAC controller
edfde39 
Initial DAC driver for the ESP32/ESP32-S2 SOCs

Signed-off-by: Marek Matej <[email protected]>
samples: board: esp32: DAC sample code
1826f73 
Sample code demonstrate the DAC on ESP32 and ESP32-S2

Signed-off-by: Marek Matej <[email protected]>
tests: dac: esp32: API test for CI
e538157 
Add base DAC test used during CI.

Signed-off-by: Marek Matej <[email protected]>
manifest: update hal espressif
e1bdd55 
Add latest peripheral support to hal.

Signed-off-by: Marek Matej <[email protected]>
@marekmatej marekmatej dismissed stale reviews from sylvioalves and martinjaeger via e1bdd55 December 7, 2022 09:39
@marekmatej
Copy link
Collaborator Author

@galak can you please take a look once again?

@martinjaeger martinjaeger dismissed galak’s stale review December 13, 2022 11:37

The previous comment has been addressed.

@fabiobaltieri fabiobaltieri merged commit e4dcccc into zephyrproject-rtos:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvioalves sylvioalves sylvioalves approved these changes

@martinjaeger martinjaeger martinjaeger approved these changes

@nashif nashif Awaiting requested review from nashif

@glaubermaroto glaubermaroto Awaiting requested review from glaubermaroto

@dcpleung dcpleung Awaiting requested review from dcpleung

@andyross andyross Awaiting requested review from andyross

@LucasTambor LucasTambor Awaiting requested review from LucasTambor

@carlescufi carlescufi Awaiting requested review from carlescufi

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@galak galak Awaiting requested review from galak

@uLipe uLipe Awaiting requested review from uLipe

Assignees

@sylvioalves sylvioalves

Labels
area: DAC Digital-to-Analog Converter area: Devicetree Binding PR modifies or adds a Device Tree binding area: West West utility area: Xtensa Xtensa Architecture manifest manifest-hal_espressif platform: ESP32 Espressif ESP32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@marekmatej @zephyrbot @martinjaeger @MaureenHelm @uLipe @sylvioalves @galak @fabiobaltieri @stephanosio