Skip to content
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

drivers: rtc: support for Nuvoton numaker m46x #71929

Merged
merged 1 commit into from Apr 30, 2024

Conversation

cyliangtw
Copy link
Contributor

@cyliangtw cyliangtw commented Apr 25, 2024

This PR is to add Nuvoton numaker RTC driver including RTC alarm feature for M46x series.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 25, 2024

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

Name Old Revision New Revision Diff
hal_nuvoton zephyrproject-rtos/hal_nuvoton@34efb92 zephyrproject-rtos/hal_nuvoton@ab342e6 (master) zephyrproject-rtos/hal_nuvoton@34efb92e..ab342e69

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

@zephyrbot zephyrbot added manifest manifest-hal_nuvoton DNM This PR should not be merged (Do Not Merge) labels Apr 25, 2024
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
@decsny decsny removed their request for review April 25, 2024 13:24
Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit left a comment

Choose a reason for hiding this comment

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

Have you run the RTC API test suite? Looking at the code I think there may be some inconsistencies with the API, running the test suite is good at identifying them if they exist :)

drivers/rtc/rtc_numaker.c Show resolved Hide resolved
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
drivers/rtc/rtc_numaker.c Outdated Show resolved Hide resolved
Comment on lines +78 to +81
config HAS_NUMAKER_RTC
bool "NuMaker RTC"
help
Enable Nuvoton RTC HAL module driver
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may be interested in asking DTS whether you have your RTC enabled, for example the way we do that for Nordic:

config NRFX_RTC
bool
config NRFX_RTC0
bool "RTC0 driver instance"
depends on $(dt_nodelabel_has_compat,rtc0,$(DT_COMPAT_NORDIC_NRF_RTC))
select NRFX_RTC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks of your suggestion.
My opinion is: hal is to support zephyr driver. So, Kconfig.numaker of zephyr rtc driver will asking DTS and select rtc's hal config.

Comment on lines +9 to +10
select HAS_NUMAKER_RTC
depends on DT_HAS_NUVOTON_NUMAKER_RTC_ENABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you take my comment https://github.com/zephyrproject-rtos/zephyr/pull/71929/files#r1579753122 into account, then you can have the select replaced with depends on and the depends on DT_HAS_NUVOTON_NUMAKER_RTC_ENABLED can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks of your opinion, DT_HAS_xx is asking DTS, and my thinking is like as:
Kconfig.nrfx:

config SPI_NRFX_SPI
	def_bool y
	depends on DT_HAS_NORDIC_NRF_SPI_ENABLED
	select NRFX_SPI0 if HAS_HW_NRF_SPI0

west.yml Outdated Show resolved Hide resolved
Add Nuvoton numaker RTC driver including RTC alarm feature.

Signed-off-by: cyliang tw <cyliang@nuvoton.com>
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Apr 30, 2024
Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit left a comment

Choose a reason for hiding this comment

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

Looks good :)

@nashif nashif merged commit 183edd2 into zephyrproject-rtos:main Apr 30, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: RTC Real Time Clock manifest manifest-hal_nuvoton platform: Nuvoton NPCX Nuvoton NPCX platform: Nuvoton Numicro Numaker Nuvoton Technology Corporation, Numicro Numaker platform: Nuvoton Nuvoton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants