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.
> > + /*
> > + * Set GET_TIME to 0. This way, next time we set GET_TIME to 1 we are sure to store an
> > + * up-to-date timestamp
> > + */
>
> Please, check all your multi-line comments for proper punctuation.
Ok. I will do my best.
> > + /* Check if RTC is running. */
>
> Please, keep a single style for the one-line comments (with or without period
> at the end).
Sure.
> > + return ret;
>
> Why not return 0 explicitly? Or do you return positive value?
I should return 0 here, you are right.
> > + comp_data[0] = (u16)value & 0xFF;
> > + comp_data[1] = ((u16)value >> 8) & 0xFF;
>
> Use proper bitwise type, i.e. __le16.
I was not aware of theses kind of types. Thanks!
> > + /* Convert from RTC calibration register format to ppb format */
> > + tmp = calibration * (s64)PPB_MULT;
>
> Is casting really needed?
No it's not. Thanks!
> > + 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.
> > + tmp = div_s64(tmp, TICKS_PER_HOUR);
> > +
> > + /*
> > + * Offset value operates in negative way, so swap sign.
> > + * See 8.3.10.5, (32768 - COMP_REG)
> > + */
> > + *offset = (long)-tmp;
> > +
> > + return ret;
>
> ret?!
That's a mistake. Thanks!
> > + /* RTC not running */
> > + if (ret == 0) {
> > + /* Start rtc */
>
> RTC for the sake of consistency.
>
> But I think one of the comment is redundant.
I will remove the second one
> > + mdelay(100);
>
> Such long delays have to be explicitly elaborated (in the comment on top).
This should just not be there.
Best regards,
--
Esteban Blanc
BayLibre
