Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, May 17, 2023 at 7:47 PM Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
> On Fri May 12, 2023 at 7:22 PM CEST,  wrote:
> > Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:

...

> > > +/* Multiplier for ppb conversions */
> > > +#define PPB_MULT (1000000000LL)
> >
> > We have something in units.h. Can you use generic macro?
>
> I found GIGA, NANO and NANOHZ_PER_HZ that have the same value in
> units.h. However I'm not sure any of them have the correct meaning in
> this situation.

MULT[IPLIER] has no units AFAIU, so SI macro can be used, no? NANO or
GIGA depends on what the actual sign of the exponent of the multiplier
is. Write it on paper and check the exponent in the equation(s) and
hence decide which one to use.

...

> > > +   if (tmp < 0)
> > > +           tmp -= TICKS_PER_HOUR / 2LL;
> > > +   else
> > > +           tmp += TICKS_PER_HOUR / 2LL;
> >
> > Is it guaranteed to have no overflow here?
>
> We know from `tps6594_rtc_set_offset` that the loaded value can't be
> more than 277774 (register default value is 0), So `tmp` can't exceed
> 277774000000000 which is lower than 2^63-1. No overflow here.
>
> TICK_PER_HOUR / 2LL = 117964800, so at the end of this computation,
> `tmp` can have a maximum value of 277774117964800 which is still
> inferior to 2^63-1.

Please add a respective comment.
-- 
With Best Regards,
Andy Shevchenko