[PATCH v4 0/5] Add rtc support for rn5t618 mfd

[PATCH v4 0/5] Add rtc support for rn5t618 mfd

[Andreas Kemnade]

In the variant RC5T619 the mfd has an RTC. This patchset adds
support for it. To do so it adds the missing register defines in 
rn5t618.h and general irq handling for that.
It seems that the irq definitions are the same except missing RTC
but due to missing ability to test that I do not add them here.

The rtc driver itself is based on 
https://github.com/kobolabs/Kobo-Reader/blob/master/hw/imx6sll-clara/kernel.tar.bz2
but heavily reworked.

It was tested on the Kobo Clara HD.

Changes in v4:
- use macros for IRQ definitions
- merge rn5t618-core.c and rn5t618-irq.c

Changes in v3:
- alignment cleanup
- output cleanup, remove useless toggling of alarm flag in rtc probe
- updated bindings description, so patch 1/5 becomes 2/6 and so on

Changes in v2:
- no dead code in irq code
- various improvements and cleanups in rtc driver itself

Andreas Kemnade (5):
  dt-bindings: mfd: rn5t618: Document optional property interrupts
  mfd: rn5t618: add IRQ support
  mfd: rn5t618: add RTC related registers
  mfd: rn5t618: add more subdevices
  rtc: rc5t619: add ricoh rc5t619 RTC driver

 Documentation/devicetree/bindings/mfd/rn5t618.txt |   4 +
 drivers/mfd/Kconfig                               |   1 +
 drivers/mfd/rn5t618.c                             | 106 +++++-
 drivers/rtc/Kconfig                               |  10 +
 drivers/rtc/Makefile                              |   1 +
 drivers/rtc/rtc-rc5t619.c                         | 444 ++++++++++++++++++++++
 include/linux/mfd/rn5t618.h                       |  26 ++
 7 files changed, 590 insertions(+), 2 deletions(-)
 create mode 100644 drivers/rtc/rtc-rc5t619.c

-- 
2.11.0

[PATCH v4 1/5] dt-bindings: mfd: rn5t618: Document optional property interrupts

[Andreas Kemnade]

These chips use interrupts for various things like RTC alarm.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v4: add refernce to interrupt binding
documentation
 Documentation/devicetree/bindings/mfd/rn5t618.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/rn5t618.txt b/Documentation/devicetree/bindings/mfd/rn5t618.txt
index b74e5e94d1cb..16778ea00dbc 100644
--- a/Documentation/devicetree/bindings/mfd/rn5t618.txt
+++ b/Documentation/devicetree/bindings/mfd/rn5t618.txt
@@ -15,6 +15,8 @@ Required properties:
  - reg: the I2C slave address of the device
 
 Optional properties:
+ - interrupts: interrupt mapping for IRQ
+   See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
  - system-power-controller:
    See Documentation/devicetree/bindings/power/power-controller.txt
 
@@ -32,6 +34,8 @@ Example:
 	pmic@32 {
 		compatible = "ricoh,rn5t618";
 		reg = <0x32>;
+		interrupt-parent = <&gpio5>;
+		interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
 		system-power-controller;
 
 		regulators {
-- 
2.11.0

[PATCH v4 2/5] mfd: rn5t618: add IRQ support

[Andreas Kemnade]

This adds support for IRQ handling in the RC5T619 which is required
for properly implementing subdevices like RTC.
For now only definitions for the variant RC5T619 are included.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v4:
merge rn5t618-irq.c into rn5t618.c
use macros for IRQ table

Changes in v3:
alignment cleanup

Changes in v2:
- no dead code, did some more testing and thinking for that
- remove extra empty lines
 drivers/mfd/Kconfig         |  1 +
 drivers/mfd/rn5t618.c       | 88 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rn5t618.h | 15 ++++++++
 3 files changed, 104 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ae24d3ea68ea..522e068d0082 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1057,6 +1057,7 @@ config MFD_RN5T618
 	depends on OF
 	select MFD_CORE
 	select REGMAP_I2C
+	select REGMAP_IRQ
 	help
 	  Say yes here to add support for the Ricoh RN5T567,
 	  RN5T618, RC5T619 PMIC.
diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index da5cd9c92a59..76d997c0cfe4 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -8,6 +8,8 @@
 
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/rn5t618.h>
 #include <linux/module.h>
@@ -45,9 +47,63 @@ static const struct regmap_config rn5t618_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_irq rc5t619_irqs[] = {
+	REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_RTC, 0, BIT(2)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_ADC, 0, BIT(3)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_GPIO, 0, BIT(4)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_CHG, 0, BIT(6)),
+};
+
+static const struct regmap_irq_chip rc5t619_irq_chip = {
+	.name = "rc5t619",
+	.irqs = rc5t619_irqs,
+	.num_irqs = ARRAY_SIZE(rc5t619_irqs),
+	.num_regs = 1,
+	.status_base = RN5T618_INTMON,
+	.mask_base = RN5T618_INTEN,
+	.mask_invert = true,
+};
+
 static struct rn5t618 *rn5t618_pm_power_off;
 static struct notifier_block rn5t618_restart_handler;
 
+int rn5t618_irq_init(struct rn5t618 *rn5t618)
+{
+	const struct regmap_irq_chip *irq_chip;
+	int ret;
+
+	if (!rn5t618->irq)
+		return 0;
+
+	switch (rn5t618->variant) {
+	case RC5T619:
+		irq_chip = &rc5t619_irq_chip;
+		break;
+	default:
+		irq_chip = NULL;
+		break;
+	}
+
+	if (!irq_chip) {
+		dev_err(rn5t618->dev, "Currently no IRQ support for variant %d\n",
+			(int)rn5t618->variant);
+		return -ENOENT;
+	}
+
+	ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap,
+				       rn5t618->irq,
+				       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				       0, irq_chip, &rn5t618->irq_data);
+	if (ret) {
+		dev_err(rn5t618->dev, "Failed to register IRQ chip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static void rn5t618_trigger_poweroff_sequence(bool repower)
 {
 	/* disable automatic repower-on */
@@ -105,6 +161,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, priv);
 	priv->variant = (long)of_id->data;
+	priv->irq = i2c->irq;
+	priv->dev = &i2c->dev;
 
 	priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config);
 	if (IS_ERR(priv->regmap)) {
@@ -137,6 +195,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (priv->irq > 0) {
+		if (rn5t618_irq_init(priv))
+			priv->irq = 0;
+	}
+
 	return 0;
 }
 
@@ -154,15 +217,40 @@ static int rn5t618_i2c_remove(struct i2c_client *i2c)
 	return 0;
 }
 
+static int __maybe_unused rn5t618_i2c_suspend(struct device *dev)
+{
+	struct rn5t618 *priv = dev_get_drvdata(dev);
+
+	if (priv->irq)
+		disable_irq(priv->irq);
+
+	return 0;
+}
+
+static int __maybe_unused rn5t618_i2c_resume(struct device *dev)
+{
+	struct rn5t618 *priv = dev_get_drvdata(dev);
+
+	if (priv->irq)
+		enable_irq(priv->irq);
+
+	return 0;
+}
+
 static const struct i2c_device_id rn5t618_i2c_id[] = {
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rn5t618_i2c_id);
 
+static SIMPLE_DEV_PM_OPS(rn5t618_i2c_dev_pm_ops,
+			rn5t618_i2c_suspend,
+			rn5t618_i2c_resume);
+
 static struct i2c_driver rn5t618_i2c_driver = {
 	.driver = {
 		.name = "rn5t618",
 		.of_match_table = of_match_ptr(rn5t618_of_match),
+		.pm = &rn5t618_i2c_dev_pm_ops,
 	},
 	.probe = rn5t618_i2c_probe,
 	.remove = rn5t618_i2c_remove,
diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h
index d62ef48060b5..739571656f2b 100644
--- a/include/linux/mfd/rn5t618.h
+++ b/include/linux/mfd/rn5t618.h
@@ -242,9 +242,24 @@ enum {
 	RC5T619,
 };
 
+/* RN5T618 IRQ definitions */
+enum {
+	RN5T618_IRQ_SYS = 0,
+	RN5T618_IRQ_DCDC,
+	RN5T618_IRQ_RTC,
+	RN5T618_IRQ_ADC,
+	RN5T618_IRQ_GPIO,
+	RN5T618_IRQ_CHG,
+	RN5T618_NR_IRQS,
+};
+
 struct rn5t618 {
 	struct regmap *regmap;
+	struct device *dev;
 	long variant;
+
+	int irq;
+	struct regmap_irq_chip_data *irq_data;
 };
 
 #endif /* __LINUX_MFD_RN5T618_H */
-- 
2.11.0

[PATCH v4 3/5] mfd: rn5t618: add RTC related registers

[Andreas Kemnade]

Defines for some RTC related registers were missing, also
they were not included in the volatile register list

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---

 drivers/mfd/rn5t618.c       |  2 ++
 include/linux/mfd/rn5t618.h | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index 76d997c0cfe4..d78eb29b94a4 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -33,6 +33,8 @@ static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
 	case RN5T618_IR_GPF:
 	case RN5T618_MON_IOIN:
 	case RN5T618_INTMON:
+	case RN5T618_RTC_CTRL1 ... RN5T618_RTC_CTRL2:
+	case RN5T618_RTC_SECONDS ... RN5T618_RTC_YEAR:
 		return true;
 	default:
 		return false;
diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h
index 739571656f2b..fba0df13d9a8 100644
--- a/include/linux/mfd/rn5t618.h
+++ b/include/linux/mfd/rn5t618.h
@@ -139,6 +139,17 @@
 #define RN5T618_INTPOL			0x9c
 #define RN5T618_INTEN			0x9d
 #define RN5T618_INTMON			0x9e
+
+#define RN5T618_RTC_SECONDS     0xA0
+#define RN5T618_RTC_MDAY        0xA4
+#define RN5T618_RTC_MONTH       0xA5
+#define RN5T618_RTC_YEAR        0xA6
+#define RN5T618_RTC_ADJUST      0xA7
+#define RN5T618_RTC_ALARM_Y_SEC 0xA8
+#define RN5T618_RTC_DAL_MONTH   0xAC
+#define RN5T618_RTC_CTRL1       0xAE
+#define RN5T618_RTC_CTRL2       0xAF
+
 #define RN5T618_PREVINDAC		0xb0
 #define RN5T618_BATDAC			0xb1
 #define RN5T618_CHGCTL1			0xb3
-- 
2.11.0

[PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Andreas Kemnade]

The RC5T619 has a RTC which is missing in the
RN5T618. Add it as subdevice to prepare for their implementation

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v3:
- alignment cleanup
 drivers/mfd/rn5t618.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index d78eb29b94a4..18d56a732b20 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
 	{ .name = "rn5t618-wdt" },
 };
 
+static const struct mfd_cell rc5t619_cells[] = {
+	{ .name = "rn5t618-regulator" },
+	{ .name = "rc5t619-rtc" },
+	{ .name = "rn5t618-wdt" },
+};
+
 static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
-				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
+	if (priv->variant == RC5T619)
+		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,
+					   ARRAY_SIZE(rc5t619_cells),
+					   NULL, 0, NULL);
+	else
+		ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
+					   ARRAY_SIZE(rn5t618_cells),
+					   NULL, 0, NULL);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret);
 		return ret;
-- 
2.11.0

[PATCH v4 5/5] rtc: rc5t619: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

Add an RTC driver for the RTC device on Ricoh MFD RC5T619,
which is implemented as a variant of RN5T618.

rtc-range output:
Testing 2000-02-28 23:59:59.
OK

Testing 2038-01-19 03:14:07.
OK

Testing 2069-12-31 23:59:59.
OK

Testing 2099-12-31 23:59:59.
KO RTC_RD_TIME returned 22 (line 138)

Testing 2100-02-28 23:59:59.
KO RTC_SET_TIME returned 34 (line 122)

Testing 2106-02-07 06:28:15.
KO RTC_SET_TIME returned 34 (line 122)

Testing 2262-04-11 23:47:16.
KO RTC_SET_TIME returned 34 (line 122)

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
Changes in v3:
- further output cleanup
- remove useless toggling of alarm flag in rtc probe
- alignment cleanup

Changes in v2:
- correct subject line
- reset pon flag not at probe but later
- initialize things only on pon
- 12h handling
- ranges
- style cleanup
- less magic values

 drivers/rtc/Kconfig       |  10 ++
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-rc5t619.c | 444 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 455 insertions(+)
 create mode 100644 drivers/rtc/rtc-rc5t619.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 1adf9f815652..b8e5bfa8efc6 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -600,6 +600,16 @@ config RTC_DRV_RC5T583
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-rc5t583.
 
+config RTC_DRV_RC5T619
+	tristate "RICOH RC5T619 RTC driver"
+	depends on MFD_RN5T618
+	help
+	  If you say yes here you get support for the RTC on the
+	  RICOH RC5T619 chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-rc5t619.
+
 config RTC_DRV_S35390A
 	tristate "Seiko Instruments S-35390A"
 	select BITREVERSE
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 4ac8f19fb631..7612912cdf00 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -137,6 +137,7 @@ obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R7301)	+= rtc-r7301.o
 obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
 obj-$(CONFIG_RTC_DRV_RC5T583)	+= rtc-rc5t583.o
+obj-$(CONFIG_RTC_DRV_RC5T619)	+= rtc-rc5t619.o
 obj-$(CONFIG_RTC_DRV_RK808)	+= rtc-rk808.o
 obj-$(CONFIG_RTC_DRV_RP5C01)	+= rtc-rp5c01.o
 obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
diff --git a/drivers/rtc/rtc-rc5t619.c b/drivers/rtc/rtc-rc5t619.c
new file mode 100644
index 000000000000..24e386ecbc7e
--- /dev/null
+++ b/drivers/rtc/rtc-rc5t619.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * drivers/rtc/rtc-rc5t619.c
+ *
+ * Real time clock driver for RICOH RC5T619 power management chip.
+ *
+ * Copyright (C) 2019 Andreas Kemnade
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mfd/rn5t618.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/bcd.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+#include <linux/irqdomain.h>
+
+struct rc5t619_rtc {
+	int			irq;
+	struct rtc_device	*rtc;
+	struct rn5t618 *rn5t618;
+};
+
+#define CTRL1_ALARM_ENABLED 0x40
+#define CTRL1_24HR 0x20
+#define CTRL1_PERIODIC_MASK 0xf
+
+#define CTRL2_PON 0x10
+#define CTRL2_ALARM_STATUS 0x80
+#define CTRL2_CTFG 0x4
+#define CTRL2_CTC 0x1
+
+#define MONTH_CENTFLAG 0x80
+#define HOUR_PMFLAG 0x20
+#define MDAY_DAL_EXT 0x80
+
+static uint8_t rtc5t619_12hour_bcd2bin(uint8_t hour)
+{
+	if (hour & HOUR_PMFLAG) {
+		hour = bcd2bin(hour & ~HOUR_PMFLAG);
+		return hour == 12 ? 12 : 12 + hour;
+	}
+
+	hour = bcd2bin(hour);
+	return hour == 12 ? 0 : hour;
+}
+
+static uint8_t rtc5t619_12hour_bin2bcd(uint8_t hour)
+{
+	if (!hour)
+		return 0x12;
+
+	if (hour < 12)
+		return bin2bcd(hour);
+
+	if (hour == 12)
+		return 0x12 | HOUR_PMFLAG;
+
+	return bin2bcd(hour - 12) | HOUR_PMFLAG;
+}
+
+static int rc5t619_rtc_periodic_disable(struct device *dev)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	int err;
+
+	/* disable function */
+	err = regmap_update_bits(rtc->rn5t618->regmap,
+				 RN5T618_RTC_CTRL1, CTRL1_PERIODIC_MASK, 0);
+	if (err < 0)
+		return err;
+
+	/* clear alarm flag and CTFG */
+	err = regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
+				 CTRL2_ALARM_STATUS | CTRL2_CTFG | CTRL2_CTC,
+				 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+/* things to be done once after power on */
+static int rc5t619_rtc_pon_setup(struct device *dev)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	int err;
+	unsigned int reg_data;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &reg_data);
+	if (err < 0)
+		return err;
+
+	/* clear VDET PON */
+	reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
+	reg_data |= 0x20;	/* 0010-0000 */
+	err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, reg_data);
+	if (err < 0)
+		return err;
+
+	/* clearing RTC Adjust register */
+	err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, 0);
+	if (err)
+		return err;
+
+	return regmap_update_bits(rtc->rn5t618->regmap,
+					RN5T618_RTC_CTRL1,
+					CTRL1_24HR, CTRL1_24HR);
+}
+
+static int rc5t619_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	u8 buff[7];
+	int err;
+	int cent_flag;
+	unsigned int ctrl1;
+	unsigned int ctrl2;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
+	if (err < 0)
+		return err;
+
+	if (ctrl2 & CTRL2_PON)
+		return -EINVAL;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &ctrl1);
+	if (err < 0)
+		return err;
+
+	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
+			       buff, sizeof(buff));
+	if (err < 0)
+		return err;
+
+	if (buff[5] & MONTH_CENTFLAG)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	tm->tm_sec  = bcd2bin(buff[0]);
+	tm->tm_min  = bcd2bin(buff[1]);
+
+	if (ctrl1 & CTRL1_24HR)
+		tm->tm_hour = bcd2bin(buff[2]);
+	else
+		tm->tm_hour = rtc5t619_12hour_bcd2bin(buff[2]);
+
+	tm->tm_wday = bcd2bin(buff[3]);
+	tm->tm_mday = bcd2bin(buff[4]);
+	tm->tm_mon  = bcd2bin(buff[5] & 0x1f) - 1; /* back to system 0-11 */
+	tm->tm_year = bcd2bin(buff[6]) + 100 * cent_flag;
+
+	return 0;
+}
+
+static int rc5t619_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	u8 buff[7];
+	int err;
+	int cent_flag;
+	unsigned int ctrl1;
+	unsigned int ctrl2;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
+	if (err < 0)
+		return err;
+
+	if (ctrl2 & CTRL2_PON)
+		rc5t619_rtc_pon_setup(dev);
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &ctrl1);
+	if (err < 0)
+		return err;
+
+	if (tm->tm_year >= 100)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	buff[0] = bin2bcd(tm->tm_sec);
+	buff[1] = bin2bcd(tm->tm_min);
+
+	if (ctrl1 & CTRL1_24HR)
+		buff[2] = bin2bcd(tm->tm_hour);
+	else
+		buff[2] = rtc5t619_12hour_bin2bcd(tm->tm_hour);
+
+	buff[3] = bin2bcd(tm->tm_wday);
+	buff[4] = bin2bcd(tm->tm_mday);
+	buff[5] = bin2bcd(tm->tm_mon + 1);	/* system set 0-11 */
+	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
+
+	if (cent_flag)
+		buff[5] |= MONTH_CENTFLAG;
+
+	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
+				buff, sizeof(buff));
+	if (err < 0) {
+		dev_err(dev, "failed to program new time: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+/* 0-disable, 1-enable */
+static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+
+	return regmap_update_bits(rtc->rn5t618->regmap,
+			RN5T618_RTC_CTRL1,
+			CTRL1_ALARM_ENABLED,
+			enabled ? CTRL1_ALARM_ENABLED : 0);
+}
+
+static int rc5t619_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	u8 buff[6];
+	unsigned int buff_cent;
+	int err;
+	int cent_flag;
+	unsigned int ctrl1;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &ctrl1);
+	if (err)
+		return err;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_MONTH, &buff_cent);
+	if (err < 0) {
+		dev_err(dev, "failed to read time: %d\n", err);
+		return err;
+	}
+
+	if (buff_cent & MONTH_CENTFLAG)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
+			       buff, sizeof(buff));
+	if (err)
+		return err;
+
+	buff[3] = buff[3] & 0x3f;
+
+	alrm->time.tm_sec  = bcd2bin(buff[0]);
+	alrm->time.tm_min  = bcd2bin(buff[1]);
+
+	if (ctrl1 & CTRL1_24HR)
+		alrm->time.tm_hour = bcd2bin(buff[2]);
+	else
+		alrm->time.tm_hour = rtc5t619_12hour_bcd2bin(buff[2]);
+
+	alrm->time.tm_mday = bcd2bin(buff[3]);
+	alrm->time.tm_mon  = bcd2bin(buff[4]) - 1;
+	alrm->time.tm_year = bcd2bin(buff[5]) + 100 * cent_flag;
+	alrm->enabled = !!(ctrl1 & CTRL1_ALARM_ENABLED);
+	dev_dbg(dev, "read alarm: %ptR\n", &alrm->time);
+
+	return 0;
+}
+
+static int rc5t619_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	u8 buff[6];
+	int err;
+	int cent_flag;
+	unsigned int ctrl1;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &ctrl1);
+	if (err)
+		return err;
+
+	err = rc5t619_rtc_alarm_enable(dev, 0);
+	if (err < 0)
+		return err;
+
+	if (rtc->irq == -1)
+		return -EINVAL;
+
+	if (alrm->enabled == 0)
+		return 0;
+
+	if (alrm->time.tm_year >= 100)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	alrm->time.tm_mon += 1;
+	buff[0] = bin2bcd(alrm->time.tm_sec);
+	buff[1] = bin2bcd(alrm->time.tm_min);
+
+	if (ctrl1 & CTRL1_24HR)
+		buff[2] = bin2bcd(alrm->time.tm_hour);
+	else
+		buff[2] = rtc5t619_12hour_bin2bcd(alrm->time.tm_hour);
+
+	buff[3] = bin2bcd(alrm->time.tm_mday);
+	buff[4] = bin2bcd(alrm->time.tm_mon);
+	buff[5] = bin2bcd(alrm->time.tm_year - 100 * cent_flag);
+	buff[3] |= MDAY_DAL_EXT;
+
+	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
+				buff, sizeof(buff));
+	if (err < 0)
+		return err;
+
+	return rc5t619_rtc_alarm_enable(dev, alrm->enabled);
+}
+
+static const struct rtc_class_ops rc5t619_rtc_ops = {
+	.read_time	= rc5t619_rtc_read_time,
+	.set_time	= rc5t619_rtc_set_time,
+	.set_alarm	= rc5t619_rtc_set_alarm,
+	.read_alarm	= rc5t619_rtc_read_alarm,
+	.alarm_irq_enable = rc5t619_rtc_alarm_enable,
+};
+
+static int rc5t619_rtc_alarm_flag_clr(struct device *dev)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+
+	/* clear alarm-D status bits.*/
+	return regmap_update_bits(rtc->rn5t618->regmap,
+				RN5T618_RTC_CTRL2,
+				CTRL2_ALARM_STATUS | CTRL2_CTC, 0);
+}
+
+static irqreturn_t rc5t619_rtc_irq(int irq, void *data)
+{
+	struct device *dev = data;
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+
+	rc5t619_rtc_alarm_flag_clr(dev);
+
+	rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
+	return IRQ_HANDLED;
+}
+
+static int rc5t619_rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
+	struct rc5t619_rtc *rtc;
+	unsigned int ctrl2;
+	int err;
+
+	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
+	if (IS_ERR(rtc)) {
+		err = PTR_ERR(rtc);
+		return -ENOMEM;
+	}
+
+	rtc->rn5t618 = rn5t618;
+
+	dev_set_drvdata(dev, rtc);
+	rtc->irq = -1;
+
+	if (rn5t618->irq_data)
+		rtc->irq = regmap_irq_get_virq(rn5t618->irq_data,
+					       RN5T618_IRQ_RTC);
+
+	if (rtc->irq  < 0)
+		rtc->irq = -1;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
+	if (err < 0)
+		return err;
+
+	/* disable rtc periodic function */
+	err = rc5t619_rtc_periodic_disable(&pdev->dev);
+	if (err)
+		return err;
+
+	if (ctrl2 & CTRL2_PON) {
+		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
+		if (err)
+			return err;
+	}
+
+	rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(rtc->rtc)) {
+		err = PTR_ERR(rtc->rtc);
+		dev_err(dev, "RTC device register: err %d\n", err);
+		return err;
+	}
+
+	rtc->rtc->ops = &rc5t619_rtc_ops;
+	rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_1900;
+	rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+	/* set interrupt and enable it */
+	if (rtc->irq != -1) {
+		err = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+						rc5t619_rtc_irq,
+						IRQF_ONESHOT,
+						"rtc-rc5t619",
+						&pdev->dev);
+		if (err < 0) {
+			dev_err(&pdev->dev, "request IRQ:%d fail\n", rtc->irq);
+			rtc->irq = -1;
+
+			err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
+			if (err)
+				return err;
+
+		} else {
+			/* enable wake */
+			device_init_wakeup(&pdev->dev, 1);
+			enable_irq_wake(rtc->irq);
+		}
+	} else {
+		/* system don't want to using alarm interrupt, so close it */
+		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
+		if (err)
+			return err;
+
+		dev_warn(&pdev->dev, "rc5t619 interrupt is disabled\n");
+	}
+
+	return rtc_register_device(rtc->rtc);
+}
+
+static struct platform_driver rc5t619_rtc_driver = {
+	.driver	= {
+		.name	= "rc5t619-rtc",
+	},
+	.probe	= rc5t619_rtc_probe,
+};
+
+module_platform_driver(rc5t619_rtc_driver);
+MODULE_ALIAS("platform:rc5t619-rtc");
+MODULE_DESCRIPTION("RICOH RC5T619 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH v4 2/5] mfd: rn5t618: add IRQ support

[Lee Jones]

On Wed, 11 Dec 2019, Andreas Kemnade wrote:

> This adds support for IRQ handling in the RC5T619 which is required
> for properly implementing subdevices like RTC.
> For now only definitions for the variant RC5T619 are included.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v4:
> merge rn5t618-irq.c into rn5t618.c
> use macros for IRQ table
> 
> Changes in v3:
> alignment cleanup
> 
> Changes in v2:
> - no dead code, did some more testing and thinking for that
> - remove extra empty lines
>  drivers/mfd/Kconfig         |  1 +
>  drivers/mfd/rn5t618.c       | 88 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rn5t618.h | 15 ++++++++
>  3 files changed, 104 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae24d3ea68ea..522e068d0082 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1057,6 +1057,7 @@ config MFD_RN5T618
>  	depends on OF
>  	select MFD_CORE
>  	select REGMAP_I2C
> +	select REGMAP_IRQ
>  	help
>  	  Say yes here to add support for the Ricoh RN5T567,
>  	  RN5T618, RC5T619 PMIC.
> diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> index da5cd9c92a59..76d997c0cfe4 100644
> --- a/drivers/mfd/rn5t618.c
> +++ b/drivers/mfd/rn5t618.c
> @@ -8,6 +8,8 @@
>  
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/rn5t618.h>
>  #include <linux/module.h>
> @@ -45,9 +47,63 @@ static const struct regmap_config rn5t618_regmap_config = {
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> +static const struct regmap_irq rc5t619_irqs[] = {
> +	REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
> +	REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
> +	REGMAP_IRQ_REG(RN5T618_IRQ_RTC, 0, BIT(2)),
> +	REGMAP_IRQ_REG(RN5T618_IRQ_ADC, 0, BIT(3)),
> +	REGMAP_IRQ_REG(RN5T618_IRQ_GPIO, 0, BIT(4)),
> +	REGMAP_IRQ_REG(RN5T618_IRQ_CHG, 0, BIT(6)),
> +};
> +
> +static const struct regmap_irq_chip rc5t619_irq_chip = {
> +	.name = "rc5t619",
> +	.irqs = rc5t619_irqs,
> +	.num_irqs = ARRAY_SIZE(rc5t619_irqs),
> +	.num_regs = 1,
> +	.status_base = RN5T618_INTMON,
> +	.mask_base = RN5T618_INTEN,
> +	.mask_invert = true,
> +};
> +
>  static struct rn5t618 *rn5t618_pm_power_off;
>  static struct notifier_block rn5t618_restart_handler;
>  
> +int rn5t618_irq_init(struct rn5t618 *rn5t618)

Static?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/5] dt-bindings: mfd: rn5t618: Document optional property interrupts

[Lee Jones]

On Wed, 11 Dec 2019, Andreas Kemnade wrote:

> These chips use interrupts for various things like RTC alarm.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v4: add refernce to interrupt binding
> documentation
>  Documentation/devicetree/bindings/mfd/rn5t618.txt | 4 ++++
>  1 file changed, 4 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 3/5] mfd: rn5t618: add RTC related registers

[Lee Jones]

On Wed, 11 Dec 2019, Andreas Kemnade wrote:

> Defines for some RTC related registers were missing, also
> they were not included in the volatile register list
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> 
>  drivers/mfd/rn5t618.c       |  2 ++
>  include/linux/mfd/rn5t618.h | 11 +++++++++++
>  2 files changed, 13 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Lee Jones]

On Wed, 11 Dec 2019, Andreas Kemnade wrote:

> The RC5T619 has a RTC which is missing in the
> RN5T618. Add it as subdevice to prepare for their implementation
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v3:
> - alignment cleanup
>  drivers/mfd/rn5t618.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> index d78eb29b94a4..18d56a732b20 100644
> --- a/drivers/mfd/rn5t618.c
> +++ b/drivers/mfd/rn5t618.c
> @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
>  	{ .name = "rn5t618-wdt" },
>  };
>  
> +static const struct mfd_cell rc5t619_cells[] = {
> +	{ .name = "rn5t618-regulator" },
> +	{ .name = "rc5t619-rtc" },
> +	{ .name = "rn5t618-wdt" },
> +};
> +
>  static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
>  
> -	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> -				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
> +	if (priv->variant == RC5T619)
> +		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,

Ref: The "-1", please use this as an opportunity to use the defines.

> +					   ARRAY_SIZE(rc5t619_cells),
> +					   NULL, 0, NULL);
> +	else

Are you sure it's not possible for 'variant' to be an unsupported
device?

> +		ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> +					   ARRAY_SIZE(rn5t618_cells),
> +					   NULL, 0, NULL);
>  	if (ret) {
>  		dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret);
>  		return ret;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

On Mon, 16 Dec 2019 15:31:06 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Wed, 11 Dec 2019, Andreas Kemnade wrote:
> 
> > The RC5T619 has a RTC which is missing in the
> > RN5T618. Add it as subdevice to prepare for their implementation
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v3:
> > - alignment cleanup
> >  drivers/mfd/rn5t618.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> > index d78eb29b94a4..18d56a732b20 100644
> > --- a/drivers/mfd/rn5t618.c
> > +++ b/drivers/mfd/rn5t618.c
> > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
> >  	{ .name = "rn5t618-wdt" },
> >  };
> >  
> > +static const struct mfd_cell rc5t619_cells[] = {
> > +	{ .name = "rn5t618-regulator" },
> > +	{ .name = "rc5t619-rtc" },
> > +	{ .name = "rn5t618-wdt" },
> > +};
> > +
> >  static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
> >  {
> >  	switch (reg) {
> > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
> >  		return ret;
> >  	}
> >  
> > -	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> > -				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
> > +	if (priv->variant == RC5T619)
> > +		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,  
> 
> Ref: The "-1", please use this as an opportunity to use the defines.
> 
> > +					   ARRAY_SIZE(rc5t619_cells),
> > +					   NULL, 0, NULL);
> > +	else  
> 
> Are you sure it's not possible for 'variant' to be an unsupported
> device?
> 
Well, does it change the behavior for devices other than the rc5t619?
I do not think so. If the mfd driver is bound to unsupported devices,
rn5t618_of_match is wrong.

Or do you want separate cell arrays for each of the three variant now to
ease future extensions?

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/5] mfd: rn5t618: add IRQ support

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

On Mon, 16 Dec 2019 15:27:15 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Wed, 11 Dec 2019, Andreas Kemnade wrote:
> 
> > This adds support for IRQ handling in the RC5T619 which is required
> > for properly implementing subdevices like RTC.
> > For now only definitions for the variant RC5T619 are included.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v4:
> > merge rn5t618-irq.c into rn5t618.c
> > use macros for IRQ table
> > 
> > Changes in v3:
> > alignment cleanup
> > 
> > Changes in v2:
> > - no dead code, did some more testing and thinking for that
> > - remove extra empty lines
> >  drivers/mfd/Kconfig         |  1 +
> >  drivers/mfd/rn5t618.c       | 88 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/rn5t618.h | 15 ++++++++
> >  3 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ae24d3ea68ea..522e068d0082 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1057,6 +1057,7 @@ config MFD_RN5T618
> >  	depends on OF
> >  	select MFD_CORE
> >  	select REGMAP_I2C
> > +	select REGMAP_IRQ
> >  	help
> >  	  Say yes here to add support for the Ricoh RN5T567,
> >  	  RN5T618, RC5T619 PMIC.
> > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> > index da5cd9c92a59..76d997c0cfe4 100644
> > --- a/drivers/mfd/rn5t618.c
> > +++ b/drivers/mfd/rn5t618.c
> > @@ -8,6 +8,8 @@
> >  
> >  #include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/rn5t618.h>
> >  #include <linux/module.h>
> > @@ -45,9 +47,63 @@ static const struct regmap_config rn5t618_regmap_config = {
> >  	.cache_type	= REGCACHE_RBTREE,
> >  };
> >  
> > +static const struct regmap_irq rc5t619_irqs[] = {
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_RTC, 0, BIT(2)),
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_ADC, 0, BIT(3)),
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_GPIO, 0, BIT(4)),
> > +	REGMAP_IRQ_REG(RN5T618_IRQ_CHG, 0, BIT(6)),
> > +};
> > +
> > +static const struct regmap_irq_chip rc5t619_irq_chip = {
> > +	.name = "rc5t619",
> > +	.irqs = rc5t619_irqs,
> > +	.num_irqs = ARRAY_SIZE(rc5t619_irqs),
> > +	.num_regs = 1,
> > +	.status_base = RN5T618_INTMON,
> > +	.mask_base = RN5T618_INTEN,
> > +	.mask_invert = true,
> > +};
> > +
> >  static struct rn5t618 *rn5t618_pm_power_off;
> >  static struct notifier_block rn5t618_restart_handler;
> >  
> > +int rn5t618_irq_init(struct rn5t618 *rn5t618)  
> 
> Static?
> 
yes, it should be static since IRQ and core do live
in the same file now.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Lee Jones]

On Mon, 16 Dec 2019, Andreas Kemnade wrote:

> On Mon, 16 Dec 2019 15:31:06 +0000
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Wed, 11 Dec 2019, Andreas Kemnade wrote:
> > 
> > > The RC5T619 has a RTC which is missing in the
> > > RN5T618. Add it as subdevice to prepare for their implementation
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > > Changes in v3:
> > > - alignment cleanup
> > >  drivers/mfd/rn5t618.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> > > index d78eb29b94a4..18d56a732b20 100644
> > > --- a/drivers/mfd/rn5t618.c
> > > +++ b/drivers/mfd/rn5t618.c
> > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
> > >  	{ .name = "rn5t618-wdt" },
> > >  };
> > >  
> > > +static const struct mfd_cell rc5t619_cells[] = {
> > > +	{ .name = "rn5t618-regulator" },
> > > +	{ .name = "rc5t619-rtc" },
> > > +	{ .name = "rn5t618-wdt" },
> > > +};
> > > +
> > >  static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
> > >  {
> > >  	switch (reg) {
> > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
> > >  		return ret;
> > >  	}
> > >  
> > > -	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> > > -				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
> > > +	if (priv->variant == RC5T619)
> > > +		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,  
> > 
> > Ref: The "-1", please use this as an opportunity to use the defines.
> > 
> > > +					   ARRAY_SIZE(rc5t619_cells),
> > > +					   NULL, 0, NULL);
> > > +	else  
> > 
> > Are you sure it's not possible for 'variant' to be an unsupported
> > device?
> > 
> Well, does it change the behavior for devices other than the rc5t619?
> I do not think so. If the mfd driver is bound to unsupported devices,
> rn5t618_of_match is wrong.

Right, and can you catch that?

> Or do you want separate cell arrays for each of the three variant now to
> ease future extensions?

No need for that, thank you.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]

On Tue, 17 Dec 2019 08:01:46 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Mon, 16 Dec 2019, Andreas Kemnade wrote:
> 
> > On Mon, 16 Dec 2019 15:31:06 +0000
> > Lee Jones <lee.jones@linaro.org> wrote:
> >   
> > > On Wed, 11 Dec 2019, Andreas Kemnade wrote:
> > >   
> > > > The RC5T619 has a RTC which is missing in the
> > > > RN5T618. Add it as subdevice to prepare for their implementation
> > > > 
> > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > > ---
> > > > Changes in v3:
> > > > - alignment cleanup
> > > >  drivers/mfd/rn5t618.c | 16 ++++++++++++++--
> > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> > > > index d78eb29b94a4..18d56a732b20 100644
> > > > --- a/drivers/mfd/rn5t618.c
> > > > +++ b/drivers/mfd/rn5t618.c
> > > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
> > > >  	{ .name = "rn5t618-wdt" },
> > > >  };
> > > >  
> > > > +static const struct mfd_cell rc5t619_cells[] = {
> > > > +	{ .name = "rn5t618-regulator" },
> > > > +	{ .name = "rc5t619-rtc" },
> > > > +	{ .name = "rn5t618-wdt" },
> > > > +};
> > > > +
> > > >  static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
> > > >  {
> > > >  	switch (reg) {
> > > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> > > > -				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
> > > > +	if (priv->variant == RC5T619)
> > > > +		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,    
> > > 
> > > Ref: The "-1", please use this as an opportunity to use the defines.
> > >   
BTW: of course I can clean that up.

> > > > +					   ARRAY_SIZE(rc5t619_cells),
> > > > +					   NULL, 0, NULL);
> > > > +	else    
> > > 
> > > Are you sure it's not possible for 'variant' to be an unsupported
> > > device?
> > >   
> > Well, does it change the behavior for devices other than the rc5t619?
> > I do not think so. If the mfd driver is bound to unsupported devices,
> > rn5t618_of_match is wrong.  
> 
> Right, and can you catch that?
> 
Well, maybe we don't get each other. RC5T619 has an RTC.
If I understand the code right, priv->variant is set to RC5T619.
if there is compatible = "ricoh,rc5t619" in the device tree.
So in that and only in that case I have an extended subdevice list,
which includes the RTC subdevice instead of only regulator and wdt.
For everything else I do not touch it.

So now you have doubts wether the existing regulator and watchdog subdevice
drivers (which I do not touch) works with the other two variants (RN5T618 
and RN5T567) and want me to check that again as part of my work to get
RTC support for RC5T619 in? 

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 4/5] mfd: rn5t618: add more subdevices

[Andreas Kemnade]

On Tue, 17 Dec 2019 12:25:26 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Tue, 17 Dec 2019 08:01:46 +0000
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Mon, 16 Dec 2019, Andreas Kemnade wrote:
> >   
> > > On Mon, 16 Dec 2019 15:31:06 +0000
> > > Lee Jones <lee.jones@linaro.org> wrote:
> > >     
> > > > On Wed, 11 Dec 2019, Andreas Kemnade wrote:
> > > >     
> > > > > The RC5T619 has a RTC which is missing in the
> > > > > RN5T618. Add it as subdevice to prepare for their implementation
> > > > > 
> > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > > > ---
> > > > > Changes in v3:
> > > > > - alignment cleanup
> > > > >  drivers/mfd/rn5t618.c | 16 ++++++++++++++--
> > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
> > > > > index d78eb29b94a4..18d56a732b20 100644
> > > > > --- a/drivers/mfd/rn5t618.c
> > > > > +++ b/drivers/mfd/rn5t618.c
> > > > > @@ -22,6 +22,12 @@ static const struct mfd_cell rn5t618_cells[] = {
> > > > >  	{ .name = "rn5t618-wdt" },
> > > > >  };
> > > > >  
> > > > > +static const struct mfd_cell rc5t619_cells[] = {
> > > > > +	{ .name = "rn5t618-regulator" },
> > > > > +	{ .name = "rc5t619-rtc" },
> > > > > +	{ .name = "rn5t618-wdt" },
> > > > > +};
> > > > > +
> > > > >  static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
> > > > >  {
> > > > >  	switch (reg) {
> > > > > @@ -173,8 +179,14 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > -	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
> > > > > -				   ARRAY_SIZE(rn5t618_cells), NULL, 0, NULL);
> > > > > +	if (priv->variant == RC5T619)
> > > > > +		ret = devm_mfd_add_devices(&i2c->dev, -1, rc5t619_cells,      
> > > > 
> > > > Ref: The "-1", please use this as an opportunity to use the defines.
> > > >     
> BTW: of course I can clean that up.
> 
> > > > > +					   ARRAY_SIZE(rc5t619_cells),
> > > > > +					   NULL, 0, NULL);
> > > > > +	else      
> > > > 
> > > > Are you sure it's not possible for 'variant' to be an unsupported
> > > > device?
> > > >     
> > > Well, does it change the behavior for devices other than the rc5t619?
> > > I do not think so. If the mfd driver is bound to unsupported devices,
> > > rn5t618_of_match is wrong.    
> > 
> > Right, and can you catch that?
> >   
> Well, maybe we don't get each other. RC5T619 has an RTC.
> If I understand the code right, priv->variant is set to RC5T619.
> if there is compatible = "ricoh,rc5t619" in the device tree.
> So in that and only in that case I have an extended subdevice list,
> which includes the RTC subdevice instead of only regulator and wdt.
> For everything else I do not touch it.
> 
Hmm, if the driver might be probed without device tree node,
there is:
   of_id = of_match_device(rn5t618_of_match, &i2c->dev);
        if (!of_id) {
                dev_err(&i2c->dev, "Failed to find matching DT ID\n");
                return -EINVAL;
        }

so nothing will be registered at all. So the only reasons I see the
driver might be bound to unsupported devices could be:
- a bug in of_match_device() 
- nonsense in rn5t618_of_match (I think that are well-thought entries)

Regards,
Andreas

Re: [PATCH v4 1/5] dt-bindings: mfd: rn5t618: Document optional property interrupts

[Rob Herring]

On Wed, 11 Dec 2019 22:54:05 +0100, Andreas Kemnade wrote:
> These chips use interrupts for various things like RTC alarm.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v4: add refernce to interrupt binding
> documentation
>  Documentation/devicetree/bindings/mfd/rn5t618.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>