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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 23/06/2025 15:03:25-0500, Alex Elder wrote:
> On 6/23/25 2:14 PM, Alexandre Belloni wrote:
> > Hello,
> > 
> > On 21/06/2025 22:29:36-0500, Alex Elder wrote:
> > > 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
> > > 
> > >   drivers/rtc/Kconfig  |  10 ++++
> > >   drivers/rtc/Makefile |   1 +
> > >   drivers/rtc/rtc-p1.c | 137 +++++++++++++++++++++++++++++++++++++++++++
> > 
> > We need something more descriptive than p1 here
> 
> Are you referring to the chip itself, or do you want a longer
> file name?   Do you prefer "rtc-spacemit-p1.c" or something?

Yes, this would be better, I feel like p1 is too short and is going to conflict
later on.

> 
> > >   3 files changed, 148 insertions(+)
> > >   create mode 100644 drivers/rtc/rtc-p1.c
> > > 
> > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > > index 9aec922613cec..27cff02ba4e66 100644
> > > --- a/drivers/rtc/Kconfig
> > > +++ b/drivers/rtc/Kconfig
> > > @@ -406,6 +406,16 @@ config RTC_DRV_MAX77686
> > >   	  This driver can also be built as a module. If so, the module
> > >   	  will be called rtc-max77686.
> > > +config RTC_DRV_P1
> > 
> > Ditto
> > 
> > > +	tristate "SpacemiT P1 RTC"
> > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > +	select MFD_SPACEMIT_P1
> > > +	default ARCH_SPACEMIT
> > > +	help
> > > +	  Enable support for the RTC function in the SpacemiT P1 PMIC.
> > > +	  This driver can also be built as a module, which will be called
> > > +	  "spacemit-p1-rtc".
> > > +
> > >   config RTC_DRV_NCT3018Y
> > >   	tristate "Nuvoton NCT3018Y"
> > >   	depends on OF
> > > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > > index 4619aa2ac4697..f8588426e2ba4 100644
> > > --- a/drivers/rtc/Makefile
> > > +++ b/drivers/rtc/Makefile
> > > @@ -171,6 +171,7 @@ obj-$(CONFIG_RTC_DRV_SD2405AL)	+= rtc-sd2405al.o
> > >   obj-$(CONFIG_RTC_DRV_SD3078)	+= rtc-sd3078.o
> > >   obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
> > >   obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
> > > +obj-$(CONFIG_RTC_DRV_P1)	+= rtc-p1.o
> > >   obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
> > >   obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
> > >   obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
> > > diff --git a/drivers/rtc/rtc-p1.c b/drivers/rtc/rtc-p1.c
> > > new file mode 100644
> > > index 0000000000000..e0d2c0c822142
> > > --- /dev/null
> > > +++ b/drivers/rtc/rtc-p1.c
> > > @@ -0,0 +1,137 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for the RTC found in the SpacemiT P1 PMIC
> > > + *
> > > + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> > > + */
> > > +
> > > +#include <linux/bits.h>
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/rtc.h>
> > > +
> > > +#define MOD_NAME	"spacemit-p1-rtc"
> > > +
> > > +/* Offset to byte containing the given time unit */
> > > +enum time_unit {
> > > +	tu_second = 0,		/* 0-59 */
> > > +	tu_minute,		/* 0-59 */
> > > +	tu_hour,		/* 0-59 */
> > > +	tu_day,			/* 0-30 (struct tm uses 1-31) */
> > > +	tu_month,		/* 0-11 */
> > > +	tu_year,		/* Years since 2000 (struct tm uses 1900) */
> > > +	tu_count,		/* Last; not a time unit */
> > > +};
> > 
> > I'm not sure this enum actually brings anything
> 
> It's just defining the sequence of register values
> symbolically.  Do you prefer #defines?
> 
> It doesn't matter much to me, I just want to know what
> you'd prefer.

Most of the drivers simply use the index number directly as the tm_* members are
descriptive enough.

> 
> > 
> > > +
> > > +/* Consecutive bytes contain seconds, minutes, etc. */
> > > +#define RTC_COUNT_BASE		0x0d
> > > +
> > > +#define RTC_CTRL		0x1d
> > > +#define RTC_EN		BIT(2)
> > > +
> > > +struct p1_rtc {
> > > +	struct regmap *regmap;
> > > +	struct rtc_device *rtc;
> > > +};
> > > +
> > > +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;
> > > +
> > > +	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;
> > > +	/* 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);
> > 
> > Don't you care whether the RTC has been reenabled?
> 
> Yes, Mateusz pointed this out.  I'll fix this.
> 
> I hope that disabling and re-enabling isn't require,
> which makes the error possibilities a lot simpler.
> 
> Otherwise it's not clear how best to recover from
> an error re-enabling the RTC (but yes, if no error
> occurs in the bulk write, an error when re-enabling
> will be returned).
> 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct rtc_class_ops p1_rtc_class_ops = {
> > > +	.read_time = p1_rtc_read_time,
> > > +	.set_time = p1_rtc_set_time,
> > > +};
> > > +
> > > +static int p1_rtc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct rtc_device *rtc;
> > > +	struct p1_rtc *p1;
> > > +	int ret;
> > > +
> > > +	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
> > > +	if (!p1)
> > > +		return -ENOMEM;
> > > +	dev_set_drvdata(dev, p1);
> > > +
> > > +	p1->regmap = dev_get_regmap(dev->parent, NULL);
> > > +	if (!p1->regmap)
> > > +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
> > > +
> > > +	rtc = devm_rtc_allocate_device(dev);
> > > +	if (IS_ERR(rtc))
> > > +		return dev_err_probe(dev, PTR_ERR(rtc),
> > > +				     "error allocating device\n");
> > > +	p1->rtc = rtc;
> > > +
> > > +	rtc->ops = &p1_rtc_class_ops;
> > > +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > > +	rtc->range_max = RTC_TIMESTAMP_END_2063;
> > > +
> > > +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> > > +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> > > +
> > > +	ret = devm_rtc_register_device(rtc);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "error registering RTC\n");
> > 
> > This message is unnecessary, there are no silent error path in
> > devm_rtc_register_device
> 
> Awesome.  I'll just return what devm_rtc_regsister_device()
> returns.
> 
> Thanks a lot.
> 
> 					-Alex
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver p1_rtc_driver = {
> > > +	.probe = p1_rtc_probe,
> > > +	.driver = {
> > > +		.name = MOD_NAME,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(p1_rtc_driver);
> > > +
> > > +MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:" MOD_NAME);
> > 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com