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

["Esteban Blanc" <eblanc@xxxxxxxxxxxx>]

On Wed May 17, 2023 at 6:52 PM CEST, Andy Shevchenko wrote:
> 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.

Thanks. I've checked and it should be NANO.

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

I've reformatted this and put it in a SAFETY comment.

Thanks for your help,

-- 
Esteban Blanc
BayLibre