Re: [PATCH v2 2/2] rtc: mt6397: Fix mt6357 RTC year offset handling for hwclock commands

[Alexandre Mergnat <amergnat@baylibre.com>]

On 02/04/2025 15:03, AngeloGioacchino Del Regno wrote:
> Il 02/04/25 12:51, Alexandre Mergnat ha scritto:
>> The mt6357 RTC was failing when using the `hwclock -r --verbose` command,
>> despite reading correctly through sysfs. There is high chance for other
>> platform to be impacted by the year offset handling issue.
>>
>> The hardware RTC registers store years relative to 1968, but the driver
>> wasn't consistently applying the offset when converting between
>> hardware and Linux time representation.
>>
>> This inconsistency caused alarm rollover failures during device
>> registration, with the error "alarm rollover not handled -22" in the
>> logs, causing hwclock commands to fail.
>>
>> The ioctl interface used by the hwclock command requires proper time
>> range validation that wasn't happening with the inconsistent year
>> offsets.
>>
>> Fixes the issue by applying the year offset in all operations:
>>     - Adding (RTC_MIN_YEAR - RTC_BASE_YEAR) when reading from hardware
>>     - Subtracting the same offset when writing to hardware
>>     - Using the same logic for both regular time and alarm operations
>>
>> With these changes, the hwclock command works correctly and time
>> values are consistently handled across all interfaces.
>>
>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>> ---
>>   drivers/rtc/rtc-mt6397.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
>> index 692c00ff544b2..ba52e225dc8fa 100644
>> --- a/drivers/rtc/rtc-mt6397.c
>> +++ b/drivers/rtc/rtc-mt6397.c
>> @@ -77,7 +77,8 @@ static int __mtk_rtc_read_time(struct mt6397_rtc *rtc,
>>       tm->tm_mday = data[RTC_OFFSET_DOM];
>>       tm->tm_wday = data[RTC_OFFSET_DOW];
>>       tm->tm_mon = data[RTC_OFFSET_MTH] & RTC_TC_MTH_MASK;
>> -    tm->tm_year = data[RTC_OFFSET_YEAR];
>> +    /* The RTC registers store years since 1968 (hardware's base year) */
>> +    tm->tm_year = data[RTC_OFFSET_YEAR] + (RTC_MIN_YEAR - RTC_BASE_YEAR);
> 
> This patch received a NACK because of RTC_MIN_YEAR_OFFSET.
> 
> What you're doing here is avoiding to use the "RTC_MIN_YEAR_OFFSET" definition name
> but otherwise doing the very same thing that was NACKed before.

You're right sorry. In my mind, the rtc framework was working well, then I try-hard to
fix the issue in this driver... but I was wrong. :(
The RTC framework have issues so fixes should be in the framework directly. My next suggestion:
https://lore.kernel.org/r/20250109-enable-rtc-v3-0-f003e8144419@baylibre.com


-- 
Regards,
Alexandre