-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Conversation
Hello @gregistech, and thank you very much for your first pull request to the Zephyr project! |
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 :) |
Thanks for clearing up copyright too :) |
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.
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 ;)
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!
Understandable, squashing should be easy. |
866872c
to
87c327f
Compare
Will be on a vacation for a week, not sure if I get to write code in that period. 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 |
Gist looks good, enjoy your time off :) |
335f76a
to
e1d158f
Compare
@bjarki-andreasen how should I handle that:
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) |
Return -EINVAL if the user tries to set both weekday and monthday at once, the mask arg is used for this purpose.
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! |
Actually, both interrupts can't be active.
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.) |
drivers/rtc/rtc_ds3231.c
Outdated
|
||
/* TODO: make sure we're in 24h mode */ | ||
/* TODO: implement configurable settings */ | ||
/* TODO: implement get_temp */ |
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.
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.
axp192_pmic: axp192@34 { |
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.
Was about to ask this question now! Thank you for pointing me in the right direction.
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. |
Just commited code for the sensor's API, not sure how to reopen the PR. |
@gregvari just re-opened it for you, although it doesn't look like you've actually pushed any change to the branch (yet?) |
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
8f46aae
to
0f6bbcb
Compare
e6b77b4
to
c6b515a
Compare
c6b515a
to
9a8a542
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.
Looks really good, I think we need to add examples to the dts bindings
9a8a542
to
238a223
Compare
238a223
to
0d69cf8
Compare
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>
0d69cf8
to
b0752f0
Compare
Hi @gergovari! 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! 🪁 |
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.