On Mon, Aug 4, 2025 at 11:47 PM Guenter Roeck [off-list ref] wrote:

On 8/4/25 21:22, sw617.shin@samsung.com wrote:

quoted

On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko [off-list ref] wrote:

quoted

How about something like this instead?

8<--------------------------------------------------------------------->8
static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
     const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
                 S3C2410_WTCON_MAXDIV; /* 32768 */
     const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
     u64 t_max = n_max / freq;

     if (t_max > UINT_MAX)
         t_max = UINT_MAX;

     return (unsigned int)t_max;
}
8<--------------------------------------------------------------------->8

This implementation's result:
   - is never greater than real timeout, as it loses the decimal part after
integer division in t_max
   - much closer to the real timeout value, as it benefits from very big
n_max in the numerator (this is the main trick here)
   - prepared for using 32-bit max counter value in your next patch, as it
uses u64 type for calculations

For example, at the clock frequency of 33 kHz:
   - real timeout is: 65074.269 sec
   - old function returns: 65535 sec
   - your function returns: 32767 sec
   - the suggested function returns: 65074 sec

Thank you for your feedback.
I'll make the code changes as follows in the next patch set:

static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
  {
         const unsigned long freq = s3c2410wdt_get_freq(wdt);
+       const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
+                       S3C2410_WTCON_MAXDIV;
+       const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;

Not sure if splitting this expression adds any value. Why not just the following ?

const u64 n_max = (u64)(S3C2410_WTCON_PRESCALE_MAX + 1) *
                    S3C2410_WTCON_MAXDIV * S3C2410_WTCNT_MAXCNT;

Or just use a define ?

Oh, that code snippet above is just a suggestion, please treat it as
pseudo-code for this purpose, -- I just wanted to illustrate the idea,
and should've been more clear! Yes, definitely should be revised and
re-tested. And yeah, I agree, one single const or define would be
enough, no need to make it too verbose. Also, the naming may be not
the best, e.g. cnt_max might be better than n_max for example. But
I'll leave it to Sangwook Shin to decide on the implementation, just
wanted to communicate the idea.

quoted

+       u64 t_max = n_max / freq;

Make sure this compiles on 32-bit builds.

Can you please elaborate what might be the possible problem -- just
curious? I admit I never though about 32-bit case when writing that
code, but don't see any immediate issues with that too.

quoted

-       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
-                                      / S3C2410_WTCON_MAXDIV);
+       if (t_max > UINT_MAX)
+               t_max = UINT_MAX;
+
+       return (unsigned int)t_max;

I am quite sure that this typecast is unnecessary.

Guenter