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

[Public]

Hi,

-----Original Message-----
From: Tomas Melin <redacted>
Sent: Wednesday, December 10, 2025 5:35 PM
To: T, Harini <redacted>; Alexandre Belloni
[off-list ref]; Simek, Michal [off-list ref]
Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org
Subject: Re: [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.


Hi,

On 09/12/2025 19:28, T, Harini wrote:

quoted

[Public]

Hi,

quoted

-----Original Message-----
From: Tomas Melin <redacted>
Sent: Monday, December 1, 2025 6:20 PM
To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Simek, Michal
[off-list ref]
Cc: linux-rtc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux- kernel@vger.kernel.org; Tomas Melin [off-list ref]
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 <redacted>
---
 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..7af5f6f99538f961a53ff56
bfc6

quoted

quoted

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?

There is no such mandatory convention. It's up to the RTC maintainer.

quoted

quoted

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

I agree. But tick_mult is int in read_offset and unsigned in set_offset.
It would be better if both uses int to maintain consistency during the math operations.

quoted

quoted

+       int freq = xrtcdev->freq;

Please follow reverse xmas tree variable ordering.

Ok fixing this and other occurances.

quoted

quoted

        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?

Yeah

Thanks,
Tomas

quoted

quoted

+       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

Regards,
Harini T