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: maxim,ds3231: convert to RTC API #76644

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

ghost
Copy link

@ghost ghost commented Aug 2, 2024

This PR's goal is to convert the deprecated Counter-based API of the current driver to the newer RTC one.

I've not written actual driver code yet (that's why I made this a draft), as I wanted to see how Zephyr actually puts all the files together. (This is my first driver ever.)

Also, I already have a question: how are copyright notices handles? As you can see in the files I modified I put my name too as Modified by ... but I'm not sure what's the right move here.

Any mistakes I've made/improvement you see so far would be appreciated.

Copy link

github-actions bot commented Aug 2, 2024

Hello @gregistech, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@bjarki-andreasen
Copy link
Collaborator

Hi there :)

first of, I would not remove or alter the existing driver which uses the old API, both drivers can exist at the same time (we can remove the old one later)

Regarding the copyright, if you substantially copy another file, add your copyright under the existing copyright in the new file :)

@ghost
Copy link
Author

ghost commented Aug 2, 2024

Hi there :)

first of, I would not remove or alter the existing driver which uses the old API, both drivers can exist at the same time (we can remove the old one later)

Regarding the copyright, if you substantially copy another file, add your copyright under the existing copyright in the new file :)

Like how the spi/mipi-dbi one works for displays in the new release? That sounds great, although I'm not sure yet how that's implemented. Probably the yaml files will point me in the right direction. I think I just need to leave the .../counter/ files in as dts bindings match other than including rtc-device.yaml.

Thanks for clearing up copyright too :)

Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

There is a test suite you can build which tests the RTC found at zephyr/tests/drivers/rtc/rtc_api which requires an overlay which uses the alias rtc. See https://docs.zephyrproject.org/latest/hardware/peripherals/rtc.html#rtc-device-driver-test-suite, it can also test alarms and callbacks if those are implemented :)

Please squash all these commits and force push your branch so only one commit is present. Removing and re adding code in separate commits is not permitted, nor pretty :)

Using the RTC test, the sample should be unnecessary, especially when you can just build hello_world, and enable CONFIG_RTC, CONFIG_SHELL, and CONFIG_RTC_SHELL and play with it using the shell ;)

drivers/rtc/Kconfig.ds3231 Outdated Show resolved Hide resolved
drivers/rtc/Kconfig.ds3231 Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Aug 4, 2024

There is a test suite you can build which tests the RTC found at zephyr/tests/drivers/rtc/rtc_api which requires an overlay which uses the alias rtc. See https://docs.zephyrproject.org/latest/hardware/peripherals/rtc.html#rtc-device-driver-test-suite, it can also test alarms and callbacks if those are implemented :)
Using the RTC test, the sample should be unnecessary, especially when you can just build hello_world, and enable CONFIG_RTC, CONFIG_SHELL, and CONFIG_RTC_SHELL and play with it using the shell ;)

I've been working on those too yesterday, plus some other features the DS3231 supports in addition to DS1307. I'll be sure to look into using a shell and tests!

Please squash all these commits and force push your branch so only one commit is present. Removing and re adding code in separate commits is not permitted, nor pretty :)

Understandable, squashing should be easy.

@ghost ghost force-pushed the convert_ds3231_to_rtc branch 2 times, most recently from 866872c to 87c327f Compare August 4, 2024 06:31
@ghost
Copy link
Author

ghost commented Aug 4, 2024

Will be on a vacation for a week, not sure if I get to write code in that period.
(Feel free to still write to me.)

For now I'd post the driver code as a gist for anyone to see the direction I'm going in.

It's probably not working atm.

https://gist.github.com/gregistech/9a6eecea8382c694f8eab61f8407aad2

@bjarki-andreasen
Copy link
Collaborator

Gist looks good, enjoy your time off :)

@ghost ghost changed the title drivers: rtc: convert maxim,ds3231 to RTC (from Counter) drivers: rtc: maxim,ds3231: convert to RTC API Aug 4, 2024
@ghost ghost force-pushed the convert_ds3231_to_rtc branch 2 times, most recently from 335f76a to e1d158f Compare August 4, 2024 18:18
@ghost
Copy link
Author

ghost commented Aug 11, 2024

@bjarki-andreasen how should I handle that:

  • alarm_set_time weekday and monthday can't be set at once
  • and we either get an update callback every sec or we listen to the alarm interrupt?

These exclusive cases aren't really documented to my knowledge, how can I signal this to the user of the interface? (Obv. this is driver-specific)

@bjarki-andreasen
Copy link
Collaborator

  • alarm_set_time weekday and monthday can't be set at once

Return -EINVAL if the user tries to set both weekday and monthday at once, the mask arg is used for this purpose.

  • and we either get an update callback every sec or we listen to the alarm interrupt?

Once the interrupt occurs, the status reg of the rtc should be read. The status will show what caused the interrupt, so it should be possible to support both.

@ghost
Copy link
Author

ghost commented Aug 11, 2024

  • alarm_set_time weekday and monthday can't be set at once

Return -EINVAL if the user tries to set both weekday and monthday at once, the mask arg is used for this purpose.

  • and we either get an update callback every sec or we listen to the alarm interrupt?

Once the interrupt occurs, the status reg of the rtc should be read. The status will show what caused the interrupt, so it should be possible to support both.

Okay, the first is what I implemented already.

And yes on second thought I see how it will both work.

Thanks!

@ghost
Copy link
Author

ghost commented Aug 11, 2024

Actually, both interrupts can't be active.

When set to logic 1 with INTCN = 0 and VCC < VPF, this
bit enables the square wave. When BBSQW is logic 0,
the INT/SQW pin goes high impedance when VCC < VPF.
This bit is disabled (logic 0) when power is first applied.

BUT, I think this is what you had in mind: we can check every sec if an alarm flag got turned on.

(And in the long run, setup the module based on config settings.)

@jakubtopic jakubtopic requested review from jakubtopic and removed request for jakubtopic August 17, 2024 13:22

/* TODO: make sure we're in 24h mode */
/* TODO: implement configurable settings */
/* TODO: implement get_temp */
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to look at devices that are implementing MFD (multifunction device). Essentially you would have a different sensor driver for what concerns the temperature, and the mfd compatible would allow you to lump the RTC and sensor under the same DT node.
See ex.

for an MFD that acts as both a regulator and GPIO controller

Copy link
Author

Choose a reason for hiding this comment

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

Was about to ask this question now! Thank you for pointing me in the right direction.

Copy link

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.

@github-actions github-actions bot added the Stale label Oct 25, 2024
@github-actions github-actions bot closed this Nov 9, 2024
@ghost
Copy link
Author

ghost commented Dec 22, 2024

Just commited code for the sensor's API, not sure how to reopen the PR.

@kartben kartben reopened this Dec 22, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 22, 2024

@gregvari just re-opened it for you, although it doesn't look like you've actually pushed any change to the branch (yet?)

@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 22, 2024

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

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

drivers/rtc/rtc_ds3231.c Outdated Show resolved Hide resolved
drivers/rtc/rtc_ds3231.c Outdated Show resolved Hide resolved
@ghost ghost force-pushed the convert_ds3231_to_rtc branch from c6b515a to 9a8a542 Compare January 2, 2025 14:25
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Looks really good, I think we need to add examples to the dts bindings

drivers/mfd/mfd_ds3231.c Outdated Show resolved Hide resolved
dts/bindings/rtc/maxim,ds3231-rtc.yaml Show resolved Hide resolved
@ghost ghost force-pushed the convert_ds3231_to_rtc branch from 9a8a542 to 238a223 Compare January 6, 2025 13:00
@ghost ghost force-pushed the convert_ds3231_to_rtc branch from 238a223 to 0d69cf8 Compare January 7, 2025 11:49
This is a squash of all the groundwork needed to
get a functioning driver for the DS3231 with the RTC API.

Signed-off-by: Gergo Vari <work@gergovari.com>
@ghost ghost force-pushed the convert_ds3231_to_rtc branch from 0d69cf8 to b0752f0 Compare January 7, 2025 17:08
@kartben kartben merged commit 2759ada into zephyrproject-rtos:main Jan 7, 2025
24 checks passed
Copy link

github-actions bot commented Jan 7, 2025

Hi @gergovari!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: MFD area: RTC Real Time Clock area: Sensors Sensors platform: ADI Analog Devices, Inc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants