Skip to content

Conversation

@jakubtopic
Copy link
Member

@jakubtopic jakubtopic commented May 28, 2024

This PR adds basic support for microcrystal RV3028 RTC connected to the I2C bus:

  • It supports getting/setting time and time alarm/update callbacks.
  • Backup switchover and trickle charger configuration using devicetree.
  • I also added an API extension for accessing the user RAM registers and EEPROM.
    • Accessing the user RAM and EEPROM will be handled using the retained_mem API in a separate PR
  • I didn't implement the device PM API because it wasn't clear to me how it should behave in terms of power switchover functionality.

I have tested the driver using the RTC API test suite. The Y2K test is skipped because this RTC only supports the final two digits of years.

Thanks

@zephyrbot zephyrbot added area: RTC Real Time Clock area: Devicetree Binding PR modifies or adds a Device Tree binding labels May 28, 2024
@github-actions
Copy link

Hello @jakubtopic, 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. 😊

@jakubtopic
Copy link
Member Author

Oh, I just realized my PR overlaps with #73385. I didn't see it at the time of submission. This is my first zephyr contribution, so I'd still appreciate any suggestions for the next time 🙂

@bjarki-andreasen
Copy link
Contributor

Wild timing, only 1 hour after #73385 was pushed :) It would be great if you help review #73385 since you probably have quite a bit of knowledge of that specific RTC :)

@Kampi
Copy link
Contributor

Kampi commented May 28, 2024

Hi @jakubtopic and @bjarki-trackunit,

I removed the RV-3028 stuff from my PR because the driver from @jakubtopic is more advanced than my driver. I will focus on RV-8263 because this RTC is more important to me. 👍

Copy link
Contributor

@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.

Regarding the Y2K test failing, it really shouldn't be, if the year can't be set to 1999, the RTC should return -EINVAL, and the test should be skipped, see

int ret = rtc_set_time(rtc, &rtm[Y99]);
if (ret == -EINVAL) {
TC_PRINT("Rollover not supported\n");
ztest_test_skip();
} else {
zassert_ok(ret, "RTC Set Time Failed");
}

It may be a legitimate bug :)

@jakubtopic
Copy link
Member Author

Hi @Kampi, I wouldn't say it is more advanced, but I appreciate your trust. I'll keep working on my driver then. Good luck with the other device!

@jakubtopic
Copy link
Member Author

jakubtopic commented May 31, 2024

@jakubtopic
Copy link
Member Author

@Kampi, do you want me to implement the RTC calibration?

@jakubtopic jakubtopic requested a review from decsny May 31, 2024 20:47
@jakubtopic
Copy link
Member Author

jakubtopic commented May 31, 2024

I would suggest removing this test case dependency so we can test boards that have RTC connected externally (it already looks for the rtc alias in the DT) but do not list it in the supported features.

@Kampi
Copy link
Contributor

Kampi commented Jun 1, 2024

@Kampi, do you want me to implement the RTC calibration?

The RTC doesn´t have a calibration function so you have to add it (somehow) in the driver. Not sure if this would make sense.

@jakubtopic
Copy link
Member Author

By calibration I meant setting frequency offset correction using the rtc_set_calibration function. I noticed it in your driver for RV8263, so I wasn't sure if you needed it for RV3028 as well.

@Kampi
Copy link
Contributor

Kampi commented Jun 1, 2024

By calibration I meant setting frequency offset correction using the rtc_set_calibration function. I noticed it in your driver for RV8263, so I wasn't sure if you needed it for RV3028 as well.

I added it because the RTC contains a register for a calibration value so it's done in hardware. You can do a software calibration for the RV3028 too and use this interface.

@jakubtopic
Copy link
Member Author

I added it because the RTC contains a register for a calibration value so it's done in hardware. You can do a software calibration for the RV3028 too and use this interface.

I think both ICs support frequency offset compensations. It's not a feature I need, so I was wondering if you hadn't originally planned it for RV3028 as well 🙂

@Kampi
Copy link
Contributor

Kampi commented Jun 1, 2024

I added it because the RTC contains a register for a calibration value so it's done in hardware. You can do a software calibration for the RV3028 too and use this interface.

I think both ICs support frequency offset compensations. It's not a feature I need, so I was wondering if you hadn't originally planned it for RV3028 as well 🙂

No I haven´t planed it for RV3028 because I haven´t found a calibration register or function during a quick check. 🙂

@jakubtopic
Copy link
Member Author

Fixed formatting errors

@jakubtopic
Copy link
Member Author

@bjarki-trackunit, @decsny Kindly asking for re-review :)

Copy link
Contributor

@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.

The driver looks really good, Minor nits and a request to use k_sem for locking to optimize size and speed compared to a mutex :)

Copy link
Member Author

@jakubtopic jakubtopic left a comment

Choose a reason for hiding this comment

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

@bjarki-trackunit Thanks for all the feedback. I've addressed the requested changes.

Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

binding seems fine

@jakubtopic jakubtopic requested a review from decsny June 7, 2024 20:24
Copy link
Contributor

@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.

Great! I have one suggestion but it is just a minor nit IMO, great work!

Adds a devicetree binding for the Micro Crystal RV3028 RTC connected to
the I2C bus.

Signed-off-by: Jakub Topic <[email protected]>
Adds support for the Micro Crystal RV3028 RTC connected to the I2C bus.

Signed-off-by: Jakub Topic <[email protected]>
@nashif nashif merged commit 2efc447 into zephyrproject-rtos:main Jun 11, 2024
@github-actions
Copy link

Hi @jakubtopic!
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! 🪁

@jakubtopic jakubtopic deleted the driver_rv3028 branch August 15, 2024 20:10
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants