Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

["Mateusz Jończyk" <mat.jonczyk@o2.pl>]

W dniu 22.06.2025 o 05:29, Alex Elder pisze:
> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
> only setting and reading the time are supported.
>
> The PMIC is implemented as a multi-function device.  This RTC is
> probed based on this driver being named in a MFD cell in the simple
> MFD I2C driver.
>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v3: - Added this driver to the series, in response to Lee Jones saying
>        more than one MFD sub-device was required to be acceptable
[snip]
> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
> +	if (ret)
> +		return ret;

Hello,

Are you sure that you read the time parts consistently here? I mean:
is there a risk that the clock is updating below you - so that for
example during the transition

12:59:59 -> 13:00:00

you are going to read 12:00:00 or 12:59:00?

If so, you could for example read the time in a loop until you get
two same readouts.

> +
> +	t->tm_sec = time[tu_second] & GENMASK(5, 0);
> +	t->tm_min = time[tu_minute] & GENMASK(5, 0);
> +	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
> +	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
> +	t->tm_mon = time[tu_month] & GENMASK(3, 0);
> +	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;

Is it necessary to use the bitmasks here? Are the higher bits undefined
in hardware? If so, is it necessary to mask them while writing the time
in p1_rtc_set_time()?

> +	/* tm_wday, tm_yday, and tm_isdst aren't used */
> +
> +	return 0;
> +}
> +
> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	time[tu_second] = t->tm_sec;
> +	time[tu_minute] = t->tm_min;
> +	time[tu_hour] = t->tm_hour;
> +	time[tu_day] = t->tm_mday - 1;
> +	time[tu_month] = t->tm_mon;
> +	time[tu_year] = t->tm_year - 100;
> +
> +	/* Disable the RTC to update; re-enable again when done */
> +	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
> +
> +	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);
Perhaps you shouldn't ignore any errors here - failing to restart
the clock should make p1_rtc_set_time() return an error code.
> +
> +	return ret;
> +}
> +

Greetings,

Mateusz