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: Add Maxim DS1337 RTC driver #85015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lefucjusz
Copy link
Contributor

This PR adds support for Maxim Integrated
DS1337 RTC chip.

Supported functionalities:

  • Alarm interrupt (both alarms trigger INTA pin)
  • Time setting/reading
  • Both alarms setting/reading
  • SQW frequency configuration

Tested on nRF52833-DK using rtc_api test set.

Comment on lines 510 to 513
timeptr->tm_mday = bcd2bin(regs[2] & DS1337_ALARM_DATE_MASK);
*mask |= RTC_ALARM_TIME_MASK_MONTHDAY;
} else {
timeptr->tm_wday = bcd2bin(regs[2] & DS1337_ALARM_DAY_MASK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
timeptr->tm_mday = bcd2bin(regs[2] & DS1337_ALARM_DATE_MASK);
*mask |= RTC_ALARM_TIME_MASK_MONTHDAY;
} else {
timeptr->tm_wday = bcd2bin(regs[2] & DS1337_ALARM_DAY_MASK);
timeptr->tm_mday = bcd2bin(regs[3] & DS1337_ALARM_DATE_MASK);
*mask |= RTC_ALARM_TIME_MASK_MONTHDAY;
} else {
timeptr->tm_wday = bcd2bin(regs[3] & DS1337_ALARM_DAY_MASK);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 432 to 435
} else if (mask & RTC_ALARM_TIME_MASK_WEEKDAY) {
regs[3] = bin2bcd(timeptr->tm_wday) & DS1337_ALARM_DAY_MASK;
regs[3] |= DS1337_DY_DT_MASK;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to apply DS1337_DAY_OFFSET?

Suggested change
} else if (mask & RTC_ALARM_TIME_MASK_WEEKDAY) {
regs[3] = bin2bcd(timeptr->tm_wday) & DS1337_ALARM_DAY_MASK;
regs[3] |= DS1337_DY_DT_MASK;
} else {
} else if (mask & RTC_ALARM_TIME_MASK_WEEKDAY) {
regs[3] = bin2bcd(timeptr->tm_wday - DS1337_DAY_OFFSET) & DS1337_ALARM_DAY_MASK;
regs[3] |= DS1337_DY_DT_MASK;
} else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Fixed

Comment on lines 670 to 676
if (reg_val & DS1337_A1F_MASK) {
LOG_WRN("Alarm 0 might have been missed!");
}
if (reg_val & DS1337_A2F_MASK) {
LOG_WRN("Alarm 1 might have been missed!");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (reg_val & DS1337_A1F_MASK) {
LOG_WRN("Alarm 0 might have been missed!");
}
if (reg_val & DS1337_A2F_MASK) {
LOG_WRN("Alarm 1 might have been missed!");
}
if (reg_val & DS1337_A1F_MASK) {
LOG_WRN("Alarm 1 might have been missed!");
}
if (reg_val & DS1337_A2F_MASK) {
LOG_WRN("Alarm 2 might have been missed!");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure whether I should refer to the alarms by RTC numbering or by IDs passed to alarm API functions, corrected as per your suggestion

This PR adds support for Maxim Integrated
DS1337 RTC chip.

Supported functionalities:
* Alarm interrupt (both alarms trigger INTA pin)
* Time setting/reading
* Both alarms setting/reading
* SQW frequency configuration

Tested on nRF52833-DK using rtc_api test set.

Signed-off-by: Marcin Lyda <elektromarcin@gmail.com>
@kartben
Copy link
Collaborator

kartben commented Feb 3, 2025

Btw @Lefucjusz since you mentioned you'd actually tested this, it would be good to see if there is anything that could be improved to have better coverage, or if there are tests that need to be fixed?

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

Successfully merging this pull request may close these issues.

4 participants