[RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Karel Balej]

RTC lives on the base register page of the chip. Add definitions of the
registers needed for a basic set/read time functionality.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/mfd/88pm886.c       | 1 +
 include/linux/mfd/88pm886.h | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
index dbe9efc027d2..891fdce5d8c1 100644
--- a/drivers/mfd/88pm886.c
+++ b/drivers/mfd/88pm886.c
@@ -37,6 +37,7 @@ static struct resource pm886_onkey_resources[] = {
 static struct mfd_cell pm886_devs[] = {
 	MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
 	MFD_CELL_NAME("88pm886-regulator"),
+	MFD_CELL_NAME("88pm886-rtc"),
 };
 
 static int pm886_power_off_handler(struct sys_off_data *sys_off_data)
diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
index 133aa302e492..85eca44f39ab 100644
--- a/include/linux/mfd/88pm886.h
+++ b/include/linux/mfd/88pm886.h
@@ -31,6 +31,15 @@
 #define PM886_INT_WC			BIT(1)
 #define PM886_INT_MASK_MODE		BIT(2)
 
+#define PM886_REG_RTC_CNT1		0xd1
+#define PM886_REG_RTC_CNT2		0xd2
+#define PM886_REG_RTC_CNT3		0xd3
+#define PM886_REG_RTC_CNT4		0xd4
+#define PM886_REG_RTC_SPARE1		0xea
+#define PM886_REG_RTC_SPARE2		0xeb
+#define PM886_REG_RTC_SPARE3		0xec
+#define PM886_REG_RTC_SPARE4		0xed
+#define PM886_REG_RTC_SPARE5		0xee
 #define PM886_REG_RTC_SPARE6		0xef
 
 #define PM886_REG_BUCK_EN		0x08
-- 
2.46.0

[RFC PATCH 2/2] rtc: add driver for Marvell 88PM886 PMIC RTC

[Karel Balej]

Only basic set/read time functionality is supported at the moment.
Tested with the samsung,coreprimevelte smartphone which contains this
PMIC and whose vendor kernel tree has also served as the sole reference
for this.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 MAINTAINERS               |  1 +
 drivers/rtc/Kconfig       | 10 ++++
 drivers/rtc/Makefile      |  1 +
 drivers/rtc/rtc-88pm886.c | 97 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+)
 create mode 100644 drivers/rtc/rtc-88pm886.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cc40a9d9b8cd..fe50a43f4e0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13518,6 +13518,7 @@ F:	Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
 F:	drivers/input/misc/88pm886-onkey.c
 F:	drivers/mfd/88pm886.c
 F:	drivers/regulators/88pm886-regulator.c
+F:	drivers/rtc/rtc-88pm886.c
 F:	include/linux/mfd/88pm886.h
 
 MARVELL ARMADA 3700 PHY DRIVERS
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2a95b05982ad..6c9d51c585a2 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -182,6 +182,16 @@ config RTC_DRV_88PM80X
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-88pm80x.
 
+config RTC_DRV_88PM886
+	tristate "Marvell 88PM886 RTC driver"
+	depends on MFD_88PM886_PMIC
+	help
+	  If you say yes here you will get support for the RTC function in the
+	  Marvell 88PM886 chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-88pm886.
+
 config RTC_DRV_ABB5ZES3
 	select REGMAP_I2C
 	tristate "Abracon AB-RTCMC-32.768kHz-B5ZE-S3"
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 3004e372f25f..0cd2c4943b7b 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_RTC_LIB_KUNIT_TEST)	+= lib_test.o
 
 obj-$(CONFIG_RTC_DRV_88PM80X)	+= rtc-88pm80x.o
 obj-$(CONFIG_RTC_DRV_88PM860X)	+= rtc-88pm860x.o
+obj-$(CONFIG_RTC_DRV_88PM886)	+= rtc-88pm886.o
 obj-$(CONFIG_RTC_DRV_AB8500)	+= rtc-ab8500.o
 obj-$(CONFIG_RTC_DRV_ABB5ZES3)	+= rtc-ab-b5ze-s3.o
 obj-$(CONFIG_RTC_DRV_ABEOZ9)	+= rtc-ab-eoz9.o
diff --git a/drivers/rtc/rtc-88pm886.c b/drivers/rtc/rtc-88pm886.c
new file mode 100644
index 000000000000..57e9b0a66eed
--- /dev/null
+++ b/drivers/rtc/rtc-88pm886.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/limits.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#include <linux/mfd/88pm886.h>
+
+/*
+ * Time is calculated as the sum of a 32-bit read-only advancing counter and a
+ * writeable constant offset stored in the chip's spare registers.
+ */
+
+static int pm886_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 time;
+	u32 buf;
+	int ret;
+
+	ret = regmap_bulk_read(regmap, PM886_REG_RTC_SPARE1, &buf, 4);
+	if (ret)
+		return ret;
+	time = buf;
+
+	ret = regmap_bulk_read(regmap, PM886_REG_RTC_CNT1, &buf, 4);
+	if (ret)
+		return ret;
+	time += buf;
+
+	rtc_time64_to_tm(time, tm);
+
+	return 0;
+}
+
+static int pm886_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 buf;
+	int ret;
+
+	ret = regmap_bulk_read(regmap, PM886_REG_RTC_CNT1, &buf, 4);
+	if (ret)
+		return ret;
+
+	buf = rtc_tm_to_time64(tm) - buf;
+
+	return regmap_bulk_write(regmap, PM886_REG_RTC_SPARE1, &buf, 4);
+}
+
+static const struct rtc_class_ops pm886_rtc_ops = {
+	.read_time = pm886_rtc_read_time,
+	.set_time = pm886_rtc_set_time,
+};
+
+static int pm886_rtc_probe(struct platform_device *pdev)
+{
+	struct pm886_chip *chip = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct rtc_device *rtc;
+	int ret;
+
+	platform_set_drvdata(pdev, chip->regmap);
+
+	rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(rtc))
+		return dev_err_probe(dev, PTR_ERR(rtc),
+				"Failed to allocate RTC device\n");
+
+	rtc->ops = &pm886_rtc_ops;
+	rtc->range_max = U32_MAX;
+
+	ret = devm_rtc_register_device(rtc);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register RTC device\n");
+
+	return 0;
+}
+
+static const struct platform_device_id pm886_rtc_id_table[] = {
+	{ "88pm886-rtc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, pm886_rtc_id_table);
+
+static struct platform_driver pm886_rtc_driver = {
+	.driver = {
+		.name = "88pm886-rtc",
+	},
+	.probe = pm886_rtc_probe,
+	.id_table = pm886_rtc_id_table,
+};
+module_platform_driver(pm886_rtc_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM886 RTC driver");
+MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Lee Jones]

On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> RTC lives on the base register page of the chip. Add definitions of the
> registers needed for a basic set/read time functionality.
> 
> 

Applied, thanks!

[1/2] mfd: 88pm886: add the RTC cell and relevant definitions
      commit: 0a936c2c45884b9a3800379f3cab4d0a685d63f5

--
Lee Jones [李琼斯]

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Karel Balej]

Lee Jones, 2024-10-09T11:06:43+01:00:
> On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > RTC lives on the base register page of the chip. Add definitions of the
> > registers needed for a basic set/read time functionality.
> > 
> > 
>
> Applied, thanks!

Thank you, however I'm a little perplexed.

It was my understanding that RFC patches should not be applied without
further agreement, is that not the case? Obviously this patch was very
simple and I used RFC mainly because of the RTC driver itself, but I'm
curious to know for future submissions.

Also, I expected the entire series to go at once through the rtc tree
with your ack as while it is not a strict dependency in terms of
breakage, the first patch seems rather pointless without the follow-up
which could theoretically take a long time to get applied and even some
requested changes could require changes to this patch. Could you please
explain what the policy is on this?

Thank you, kind regards,
K. B.

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Lee Jones]

On Wed, 09 Oct 2024, Karel Balej wrote:

> Lee Jones, 2024-10-09T11:06:43+01:00:
> > On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > > RTC lives on the base register page of the chip. Add definitions of the
> > > registers needed for a basic set/read time functionality.
> > > 
> > > 
> >
> > Applied, thanks!
> 
> Thank you, however I'm a little perplexed.
> 
> It was my understanding that RFC patches should not be applied without
> further agreement, is that not the case? Obviously this patch was very
> simple and I used RFC mainly because of the RTC driver itself, but I'm
> curious to know for future submissions.

I missed the fact that this was an RFC.  I can unapply it if you like?

> Also, I expected the entire series to go at once through the rtc tree
> with your ack as while it is not a strict dependency in terms of
> breakage, the first patch seems rather pointless without the follow-up
> which could theoretically take a long time to get applied and even some
> requested changes could require changes to this patch. Could you please
> explain what the policy is on this?

The policy is flexible.  However, the generally accepted rule is that if
there are build-time dependencies between patches, then one maintainer
(usually me since MFD is usually at the centre of these cross-subsystem
patch-sets) takes them and sends out a pull-request for an immutable
branch for the other maintainers to pull from.

However in this case, there are no build-time dependencies so the
patches are able to and therefore should go in via their respective
repos.

-- 
Lee Jones [李琼斯]

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Lee Jones]

On Thu, 10 Oct 2024, Lee Jones wrote:

> On Wed, 09 Oct 2024, Karel Balej wrote:
> 
> > Lee Jones, 2024-10-09T11:06:43+01:00:
> > > On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > > > RTC lives on the base register page of the chip. Add definitions of the
> > > > registers needed for a basic set/read time functionality.
> > > > 
> > > > 
> > >
> > > Applied, thanks!
> > 
> > Thank you, however I'm a little perplexed.
> > 
> > It was my understanding that RFC patches should not be applied without
> > further agreement, is that not the case? Obviously this patch was very
> > simple and I used RFC mainly because of the RTC driver itself, but I'm
> > curious to know for future submissions.
> 
> I missed the fact that this was an RFC.  I can unapply it if you like?
> 
> > Also, I expected the entire series to go at once through the rtc tree
> > with your ack as while it is not a strict dependency in terms of
> > breakage, the first patch seems rather pointless without the follow-up
> > which could theoretically take a long time to get applied and even some
> > requested changes could require changes to this patch. Could you please
> > explain what the policy is on this?
> 
> The policy is flexible.  However, the generally accepted rule is that if
> there are build-time dependencies between patches, then one maintainer
> (usually me since MFD is usually at the centre of these cross-subsystem
> patch-sets) takes them and sends out a pull-request for an immutable
> branch for the other maintainers to pull from.
> 
> However in this case, there are no build-time dependencies so the
> patches are able to and therefore should go in via their respective
> repos.

Actually, it looks like there are build-time deps between them.

Please break out the inclusion of the additional defines and place them
into the RTC patch.  I will then Ack that one.  The patch making changes
to driver/mfd will still go in via the MFD repo.

-- 
Lee Jones [李琼斯]

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Karel Balej]

Lee Jones, 2024-10-10T09:35:19+01:00:
> On Thu, 10 Oct 2024, Lee Jones wrote:
>
> > On Wed, 09 Oct 2024, Karel Balej wrote:
> > 
> > > Lee Jones, 2024-10-09T11:06:43+01:00:
> > > > On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > > > > RTC lives on the base register page of the chip. Add definitions of the
> > > > > registers needed for a basic set/read time functionality.
> > > > > 
> > > > > 
> > > >
> > > > Applied, thanks!
> > > 
> > > Thank you, however I'm a little perplexed.
> > > 
> > > It was my understanding that RFC patches should not be applied without
> > > further agreement, is that not the case? Obviously this patch was very
> > > simple and I used RFC mainly because of the RTC driver itself, but I'm
> > > curious to know for future submissions.
> > 
> > I missed the fact that this was an RFC.  I can unapply it if you like?
> > 
> > > Also, I expected the entire series to go at once through the rtc tree
> > > with your ack as while it is not a strict dependency in terms of
> > > breakage, the first patch seems rather pointless without the follow-up
> > > which could theoretically take a long time to get applied and even some
> > > requested changes could require changes to this patch. Could you please
> > > explain what the policy is on this?
> > 
> > The policy is flexible.  However, the generally accepted rule is that if
> > there are build-time dependencies between patches, then one maintainer
> > (usually me since MFD is usually at the centre of these cross-subsystem
> > patch-sets) takes them and sends out a pull-request for an immutable
> > branch for the other maintainers to pull from.
> > 
> > However in this case, there are no build-time dependencies so the
> > patches are able to and therefore should go in via their respective
> > repos.
>
> Actually, it looks like there are build-time deps between them.

Indeed, I didn't realize that yesterday. What I had in mind before was
in fact the other part of the patch: I was wondering about the policy of
applying a patch adding a MFD cell for which there is no driver
available. That's what I meant by "not a strict dependency in terms of
breakage".

> Please break out the inclusion of the additional defines and place them
> into the RTC patch.  I will then Ack that one.  The patch making changes
> to driver/mfd will still go in via the MFD repo.

So the above in other words: it sounds like you would apply this updated
patch independently of the RTC driver because otherwise you could just
ack the current version, is that correct? If so, I cannot see why this
would be preferable given what I wrote before about the RTC driver being
possibly delayed or eventually given up on (not that I would expect that
to be the case here :-). Could you please still comment on this then?

Thank you,
K. B.

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Lee Jones]

On Thu, 10 Oct 2024, Karel Balej wrote:

> Lee Jones, 2024-10-10T09:35:19+01:00:
> > On Thu, 10 Oct 2024, Lee Jones wrote:
> >
> > > On Wed, 09 Oct 2024, Karel Balej wrote:
> > > 
> > > > Lee Jones, 2024-10-09T11:06:43+01:00:
> > > > > On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > > > > > RTC lives on the base register page of the chip. Add definitions of the
> > > > > > registers needed for a basic set/read time functionality.
> > > > > > 
> > > > > > 
> > > > >
> > > > > Applied, thanks!
> > > > 
> > > > Thank you, however I'm a little perplexed.
> > > > 
> > > > It was my understanding that RFC patches should not be applied without
> > > > further agreement, is that not the case? Obviously this patch was very
> > > > simple and I used RFC mainly because of the RTC driver itself, but I'm
> > > > curious to know for future submissions.
> > > 
> > > I missed the fact that this was an RFC.  I can unapply it if you like?
> > > 
> > > > Also, I expected the entire series to go at once through the rtc tree
> > > > with your ack as while it is not a strict dependency in terms of
> > > > breakage, the first patch seems rather pointless without the follow-up
> > > > which could theoretically take a long time to get applied and even some
> > > > requested changes could require changes to this patch. Could you please
> > > > explain what the policy is on this?
> > > 
> > > The policy is flexible.  However, the generally accepted rule is that if
> > > there are build-time dependencies between patches, then one maintainer
> > > (usually me since MFD is usually at the centre of these cross-subsystem
> > > patch-sets) takes them and sends out a pull-request for an immutable
> > > branch for the other maintainers to pull from.
> > > 
> > > However in this case, there are no build-time dependencies so the
> > > patches are able to and therefore should go in via their respective
> > > repos.
> >
> > Actually, it looks like there are build-time deps between them.
> 
> Indeed, I didn't realize that yesterday. What I had in mind before was
> in fact the other part of the patch: I was wondering about the policy of
> applying a patch adding a MFD cell for which there is no driver
> available. That's what I meant by "not a strict dependency in terms of
> breakage".

I've become less strict about that over the years.  The chances of the
accompanying driver not going in over the next release or so is usually
very small.

> > Please break out the inclusion of the additional defines and place them
> > into the RTC patch.  I will then Ack that one.  The patch making changes
> > to driver/mfd will still go in via the MFD repo.
> 
> So the above in other words: it sounds like you would apply this updated
> patch independently of the RTC driver because otherwise you could just
> ack the current version, is that correct? If so, I cannot see why this
> would be preferable given what I wrote before about the RTC driver being
> possibly delayed or eventually given up on (not that I would expect that
> to be the case here :-). Could you please still comment on this then?

As above.  I trust you. :)

-- 
Lee Jones [李琼斯]

Re: (subset) [RFC PATCH 1/2] mfd: 88pm886: add the RTC cell and relevant definitions

[Lee Jones]

On Wed, 09 Oct 2024, Lee Jones wrote:

> On Fri, 20 Sep 2024 18:12:34 +0200, Karel Balej wrote:
> > RTC lives on the base register page of the chip. Add definitions of the
> > registers needed for a basic set/read time functionality.
> > 
> > 
> 
> Applied, thanks!
> 
> [1/2] mfd: 88pm886: add the RTC cell and relevant definitions
>       commit: 0a936c2c45884b9a3800379f3cab4d0a685d63f5

Unapplied, thanks.

-- 
Lee Jones [李琼斯]