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

[andy.shevchenko@gmail.com]

Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:
> TPS6594 PMIC is a MFD. This patch adds support for
> the RTC found inside TPS6594 family of PMIC.
> 
> Alarm is also supported.

...

> +/*
> + * Min and max values supported with 'offset' interface (swapped sign)
> + * After conversion, the values does not exceed the range [-32767, 33767] which COMP_REG must
> + * conform to

Please, format it better and do not forget to use proper punctuation (commas,
periods, etc.).

> + */
> +#define MIN_OFFSET (-277774)
> +#define MAX_OFFSET (277774)

...

> +/* Multiplier for ppb conversions */
> +#define PPB_MULT (1000000000LL)

We have something in units.h. Can you use generic macro?

...

> +	ret = regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
> +				 TPS6594_BIT_IT_ALARM, val);
> +
> +	return ret;

	return regmap_update_bits(...);

...

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

...

> +	/* Check if RTC is running. */

Please, keep a single style for the one-line comments (with or without period
at the end).

> +	alm->enabled = int_val & TPS6594_BIT_IT_ALARM ? 1 : 0;

Ternary is reduntand.

> +	return ret;

Why not return 0 explicitly? Or do you return positive value?

...

> +	comp_data[0] = (u16)value & 0xFF;
> +	comp_data[1] = ((u16)value >> 8) & 0xFF;

Use proper bitwise type, i.e. __le16.

...

> +	value = (u16)comp_data[0] | ((u16)comp_data[1] << 8);

Ditto.

...

> +	/* Convert from RTC calibration register format to ppb format */
> +	tmp = calibration * (s64)PPB_MULT;

Is casting really needed?

> +	if (tmp < 0)
> +		tmp -= TICKS_PER_HOUR / 2LL;
> +	else
> +		tmp += TICKS_PER_HOUR / 2LL;

Is it guaranteed to have no overflow here?

> +	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?!

> +}
> +
> +static int tps6594_rtc_set_offset(struct device *dev, long offset)
> +{
> +	int calibration;
> +	s64 tmp;

Similar questions here as per above routine.

> +	/* Make sure offset value is within supported range */
> +	if (offset < MIN_OFFSET || offset > MAX_OFFSET)
> +		return -ERANGE;
> +
> +	/* Convert from ppb format to RTC calibration register format */
> +	tmp = offset * (s64)TICKS_PER_HOUR;
> +	if (tmp < 0)
> +		tmp -= PPB_MULT / 2LL;
> +	else
> +		tmp += PPB_MULT / 2LL;
> +	tmp = div_s64(tmp, PPB_MULT);
> +
> +	/* Offset value operates in negative way, so swap sign */
> +	calibration = (int)-tmp;
> +
> +	return tps6594_rtc_set_calibration(dev, calibration);
> +}

...

> +static int tps6594_rtc_probe(struct platform_device *pdev)
> +{
> +	struct tps6594 *tps;
> +	struct rtc_device *rtc;
> +	int irq;
> +	int ret;

> +	tps = dev_get_drvdata(pdev->dev.parent);

Can be united with definition of tps above.

...

> +	/* RTC not running */
> +	if (ret == 0) {
> +		/* Start rtc */

RTC for the sake of consistency.

But I think one of the comment is redundant.

...

> +		mdelay(100);

Such long delays have to be explicitly elaborated (in the comment on top).

> +	}

...

> +	irq = platform_get_irq_byname(pdev, TPS6594_IRQ_NAME_ALARM);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq\n");
> +		return irq;

	return dev_err_probe(...);

> +	}

...

> +		dev_err(&pdev->dev, "Failed to request_threaded_irq\n");
> +		return ret;

Ditto.

...

> +		dev_err(&pdev->dev, "Failed to init rtc as wakeup source\n");
> +		return ret;

Ditto.

...

> +

Blank line is not needed.

> +module_platform_driver(tps6594_rtc_driver);

-- 
With Best Regards,
Andy Shevchenko