On 10/16/2024 12:42 PM, Uwe Kleine-König wrote:
Hello Uwe,
Thanks for your review!
Hello,
On Tue, Oct 15, 2024 at 01:51:31PM +0300, Ciprian Costea wrote:
+static void rtc_disable(struct rtc_priv *priv)
A very generic name for a driver specific function. I'm a big fan of
driver specific prefixes and I wonder why this isn't called
s34g_rtc_disable().
Also a comment about what is actually disabled here would be nice (or
maybe a better name). I hope this doesn't stop the RTC ticking??
Indeed, 's32g_rtc_disable' would be a better name. I will update in V3
and add a comment before the function definition.
It does stop the RTC running counter because as per RTC module
documentation from S32G2/S32G3 SoC Reference Manual, the clock source
and divisors selection should be performed with RTC counter disabled in
order to not cause any synchronization issues.
So following this limitation, when the clock source is switched (as it
is the case for S32G2 and S32G3 and Suspend to RAM) the RTC counter
should be disabled.
+static struct platform_driver rtc_driver = {
+ .driver = {
+ .name = "s32g-rtc",
+ .pm = &rtc_pm_ops,
+ .of_match_table = rtc_dt_ids,
+ },
+ .probe = rtc_probe,
+ .remove_new = rtc_remove,
+};
After commit 0edb555a65d1 ("platform: Make platform_driver::remove()
return void") .remove() is (again) the right callback to implement for
platform drivers. Please just drop "_new".
Thanks for pointing this out. I was not aware of this change. I will
update in V3.
Best regards
Uwe
Best Regards,
Ciprian
