Re: [PATCH 2/4] rtc: zynqmp: rework read_offset

[Tomas Melin <tomas.melin@vaisala.com>]

Hi,

On 09/12/2025 19:28, T, Harini wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Tomas Melin <tomas.melin@vaisala.com>
>> Sent: Monday, December 1, 2025 6:20 PM
>> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
>> <michal.simek@amd.com>
>> Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Tomas Melin <tomas.melin@vaisala.com>
>> Subject: [PATCH 2/4] rtc: zynqmp: rework read_offset
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> read_offset() was using static frequency for determining the tick offset. It was
>> also using remainder from do_div() operation as tick_mult value which
>> caused the offset to be incorrect.
>>
>> At the same time, rework function to improve readability.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>  drivers/rtc/rtc-zynqmp.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
>> 856bc1678e7d31144f320ae9f75fc58c742a2a64..7af5f6f99538f961a53ff56bfc6
>> 56c907611b900 100644
>> --- a/drivers/rtc/rtc-zynqmp.c
>> +++ b/drivers/rtc/rtc-zynqmp.c
>> @@ -178,21 +178,28 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
>> *xrtcdev)  static int xlnx_rtc_read_offset(struct device *dev, long *offset)  {
>>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> -       unsigned long long rtc_ppb = RTC_PPB;
>> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
>> -       unsigned int calibval;
>> +       unsigned int calibval, fract_data, fract_part;
> Prefer one variable assignment per line for readability.
This is after all quite common practice, and in a function like this
where several variables are needed, I would argue that this is more
readable than the alternative. Is there some convention I'm not aware of?

>> +       int max_tick, tick_mult;
> It would be better to explain why tick_mult is changed to int in the commit message.
This is part of the refactoring, mixing signed and unsigned variables in
operations is more risky than having same type.

>> +       int freq = xrtcdev->freq;
> Please follow reverse xmas tree variable ordering.
Ok fixing this and other occurances.

>>         long offset_val;
>>
>> +       /* ticks to reach RTC_PPB */
> The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
Perhaps the comment was not well formulated. I suggest changing to
/* Tick to offset multiplier */
 as that it what it is primarily used for. Would that be okay for You?

Thanks,
Tomas

>> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, freq);
>> +
>>         calibval = readl(xrtcdev->reg_base + RTC_CALIB_RD);
>>         /* Offset with seconds ticks */
>> -       offset_val = calibval & RTC_TICK_MASK;
>> -       offset_val = offset_val - RTC_CALIB_DEF;
>> -       offset_val = offset_val * tick_mult;
>> +       max_tick = calibval & RTC_TICK_MASK;
>> +       offset_val = max_tick - freq;
>> +       /* Convert to ppb */
>> +       offset_val *= tick_mult;
>>
>>         /* Offset with fractional ticks */
>> -       if (calibval & RTC_FR_EN)
>> -               offset_val += ((calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT)
>> -                       * (tick_mult / RTC_FR_MAX_TICKS);
>> +       if (calibval & RTC_FR_EN) {
>> +               fract_data = (calibval & RTC_FR_MASK) >> RTC_FR_DATSHIFT;
>> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
>> +               offset_val += (fract_part * fract_data);
>> +       }
>> +
>>         *offset = offset_val;
>>
>>         return 0;
>>
>> --
>> 2.47.3
>>
> Regards,
> Harini T
>