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

[Esteban Blanc]

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