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

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

[Andreas Kemnade]

In the variant rc5t619 the mfd has a 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.
Probably the irq definitions are the same except missing rtc + charger
but due to missing information about that I do not add them.
There might be some oddity about the charger irq which should be 
researched when adding the charger subdevice.

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.

Andreas Kemnade (5):
  mfd: rn5t618: prepare for irq handling
  mfd: rn5t618: add irq support
  mfd: rn5t618: add rtc related registers
  mfd: rn5t618: add more subdevices
  rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

 drivers/mfd/Kconfig                       |   1 +
 drivers/mfd/Makefile                      |   2 +
 drivers/mfd/{rn5t618.c => rn5t618-core.c} |  49 ++-
 drivers/mfd/rn5t618-irq.c                 |  92 ++++++
 drivers/rtc/Kconfig                       |  10 +
 drivers/rtc/Makefile                      |   1 +
 drivers/rtc/rtc-rc5t619.c                 | 476 ++++++++++++++++++++++++++++++
 include/linux/mfd/rn5t618.h               |  29 ++
 8 files changed, 658 insertions(+), 2 deletions(-)
 rename drivers/mfd/{rn5t618.c => rn5t618-core.c} (79%)
 create mode 100644 drivers/mfd/rn5t618-irq.c
 create mode 100644 drivers/rtc/rtc-rc5t619.c

-- 
2.11.0

[PATCH 1/5] mfd: rn5t618: prepare for irq handling

[Andreas Kemnade]

rn5t618 currently lacks irq handling. To prepare implementation
in a rn5t618-irq.c, move main file to rn5t618-core.c

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/Makefile                      | 2 ++
 drivers/mfd/{rn5t618.c => rn5t618-core.c} | 0
 2 files changed, 2 insertions(+)
 rename drivers/mfd/{rn5t618.c => rn5t618-core.c} (100%)

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f026ada68f6a..7d7e0ec4e325 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -219,6 +219,8 @@ obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
 obj-$(CONFIG_MFD_RK808)		+= rk808.o
+
+rn5t618-objs			:= rn5t618-core.o
 obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
 obj-$(CONFIG_MFD_SEC_CORE)	+= sec-core.o sec-irq.o
 obj-$(CONFIG_MFD_SYSCON)	+= syscon.o
diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618-core.c
similarity index 100%
rename from drivers/mfd/rn5t618.c
rename to drivers/mfd/rn5t618-core.c
-- 
2.11.0

[PATCH 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>
---
 drivers/mfd/Kconfig         |  1 +
 drivers/mfd/Makefile        |  2 +-
 drivers/mfd/rn5t618-core.c  | 35 ++++++++++++++++-
 drivers/mfd/rn5t618-irq.c   | 92 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rn5t618.h | 18 +++++++++
 5 files changed, 146 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mfd/rn5t618-irq.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f129f9678940..7534f9c8478a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1063,6 +1063,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/Makefile b/drivers/mfd/Makefile
index 7d7e0ec4e325..27a67fc7cac1 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -220,7 +220,7 @@ obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
 obj-$(CONFIG_MFD_RK808)		+= rk808.o
 
-rn5t618-objs			:= rn5t618-core.o
+rn5t618-objs			:= rn5t618-core.o rn5t618-irq.o
 obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
 obj-$(CONFIG_MFD_SEC_CORE)	+= sec-core.o sec-irq.o
 obj-$(CONFIG_MFD_SYSCON)	+= syscon.o
diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c
index da5cd9c92a59..d4ed2865ed8b 100644
--- a/drivers/mfd/rn5t618-core.c
+++ b/drivers/mfd/rn5t618-core.c
@@ -8,6 +8,7 @@
 
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/rn5t618.h>
 #include <linux/module.h>
@@ -105,7 +106,8 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, priv);
 	priv->variant = (long)of_id->data;
-
+	priv->chip_irq = i2c->irq;
+	priv->dev = &i2c->dev;
 	priv->regmap = devm_regmap_init_i2c(i2c, &rn5t618_regmap_config);
 	if (IS_ERR(priv->regmap)) {
 		ret = PTR_ERR(priv->regmap);
@@ -137,6 +139,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (priv->chip_irq > 0) {
+		if (rn5t618_irq_init(priv))
+			priv->chip_irq = 0;
+	}
+
 	return 0;
 }
 
@@ -154,15 +161,41 @@ 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->chip_irq)
+		disable_irq(priv->chip_irq);
+
+	return 0;
+}
+
+static int __maybe_unused rn5t618_i2c_resume(struct device *dev)
+{
+	struct rn5t618 *priv = dev_get_drvdata(dev);
+
+	if (priv->chip_irq)
+		enable_irq(priv->chip_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/drivers/mfd/rn5t618-irq.c b/drivers/mfd/rn5t618-irq.c
new file mode 100644
index 000000000000..384fc844bad4
--- /dev/null
+++ b/drivers/mfd/rn5t618-irq.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 Andreas Kemnade
+ */
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/rn5t618.h>
+
+static const struct regmap_irq rc5t619_irqs[] = {
+	[RN5T618_IRQ_SYS] = {
+		.reg_offset = 0,
+		.mask = (0 << 1)
+	},
+	[RN5T618_IRQ_DCDC] = {
+		.reg_offset = 0,
+		.mask = (1 << 1)
+	},
+	[RN5T618_IRQ_RTC]  = {
+		.reg_offset = 0,
+		.mask = (1 << 2)
+	},
+	[RN5T618_IRQ_ADC] = {
+		.reg_offset = 0,
+		.mask = (1 << 3)
+	},
+	[RN5T618_IRQ_GPIO] = {
+		.reg_offset = 0,
+		.mask = (1 << 4)
+	},
+#if 0
+/*
+ * REVISIT when implementing charger:
+ * according to some other implementation it needs special treatment,
+ * inverted but experiments seem not to confirm that.
+ */
+	[RN5T618_IRQ_CHG] = {
+		.reg_offset = 0,
+		.mask = (1 << 6),
+	}
+#endif
+};
+
+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,
+};
+
+int rn5t618_irq_init(struct rn5t618 *rn5t618)
+{
+	const struct regmap_irq_chip *irq_chip;
+	int ret;
+
+	if (!rn5t618->chip_irq)
+		return 0;
+
+	switch (rn5t618->variant) {
+	case RC5T619:
+		irq_chip = &rc5t619_irq_chip;
+		break;
+
+		/* TODO: check irq definitions for other variants */
+
+	default:
+		irq_chip = NULL;
+		break;
+	}
+
+	if (!irq_chip) {
+		dev_err(rn5t618->dev, "no IRQ definition known for variant\n");
+		return -ENOENT;
+	}
+
+	ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap,
+				rn5t618->chip_irq,
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				0, irq_chip, &rn5t618->irq_data);
+	if (ret != 0) {
+		dev_err(rn5t618->dev, "Failed to register IRQ chip\n");
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h
index d62ef48060b5..4d680f34ad2f 100644
--- a/include/linux/mfd/rn5t618.h
+++ b/include/linux/mfd/rn5t618.h
@@ -242,9 +242,27 @@ enum {
 	RC5T619,
 };
 
+
+/* RN5T618 IRQ definitions */
+enum {
+	RN5T618_IRQ_SYS,
+	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 chip_irq;
+	struct regmap_irq_chip_data *irq_data;
 };
 
+extern int rn5t618_irq_init(struct rn5t618 *rn5t618);
 #endif /* __LINUX_MFD_RN5T618_H */
-- 
2.11.0

[PATCH 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-core.c  |  2 ++
 include/linux/mfd/rn5t618.h | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c
index d4ed2865ed8b..0e3ec9dafb40 100644
--- a/drivers/mfd/rn5t618-core.c
+++ b/drivers/mfd/rn5t618-core.c
@@ -32,6 +32,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 4d680f34ad2f..eb2ee7a776ab 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 4/5] mfd: rn5t618: add more subdevices

[Andreas Kemnade]

The rc5t619 has a rtc which are missing in the
rn5t618. Add it as subdevice to prepare for their implementation

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/rn5t618-core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/rn5t618-core.c b/drivers/mfd/rn5t618-core.c
index 0e3ec9dafb40..b037c97e6747 100644
--- a/drivers/mfd/rn5t618-core.c
+++ b/drivers/mfd/rn5t618-core.c
@@ -21,6 +21,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) {
@@ -117,7 +123,11 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	ret = devm_mfd_add_devices(&i2c->dev, -1, rn5t618_cells,
+	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);
-- 
2.11.0

[PATCH 5/5] rtc: rtc-rc5t583: 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

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/rtc/Kconfig       |  10 +
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-rc5t619.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 487 insertions(+)
 create mode 100644 drivers/rtc/rtc-rc5t619.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e72f65b61176..a4dc04c49fec 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -588,6 +588,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 6b09c21dc1b6..1d0673fd0954 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -136,6 +136,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..311788ff0723
--- /dev/null
+++ b/drivers/rtc/rtc-rc5t619.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * drivers/rtc/rtc-ricoh619.c
+ *
+ * Real time clock driver for RICOH R5T619 power management chip.
+ *
+ * Copyright (C) 2019 Andreas Kemnade
+ *
+ * Based on code
+ *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
+ *
+ * Based on code
+ *  Copyright (C) 2011 NVIDIA Corporation
+ */
+
+/* #define debug		1 */
+/* #define verbose_debug	1 */
+
+#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
+
+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;
+}
+
+static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+
+	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);
+}
+
+static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
+{
+	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;
+
+	if (reg_data & CTRL2_PON) {
+		*pon_f = 1;
+		/* 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);
+	} else {
+		*pon_f = 0;
+	}
+
+	return err;
+}
+
+/* 0-12hour, 1-24hour */
+static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+
+	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
+			CTRL1_24HR, mode ? CTRL1_24HR : 0);
+}
+
+
+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;
+
+	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
+				buff, sizeof(buff));
+	if (err < 0) {
+		dev_err(dev, "failed to read time: %d\n", err);
+		return err;
+	}
+
+	if (buff[5] & 0x80)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */
+
+	tm->tm_sec  = bcd2bin(buff[0]);
+	tm->tm_min  = bcd2bin(buff[1]);
+	tm->tm_hour = bcd2bin(buff[2]);		/* bit5 PA_H20 */
+	tm->tm_wday = bcd2bin(buff[3]);
+	tm->tm_mday = bcd2bin(buff[4]);
+	tm->tm_mon  = bcd2bin(buff[5]) - 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;
+
+	if (tm->tm_year >= 100)
+		cent_flag = 1;
+	else
+		cent_flag = 0;
+
+	tm->tm_mon = tm->tm_mon + 1;
+	buff[0] = bin2bcd(tm->tm_sec);
+	buff[1] = bin2bcd(tm->tm_min);
+	buff[2] = bin2bcd(tm->tm_hour);
+	buff[3] = bin2bcd(tm->tm_wday);
+	buff[4] = bin2bcd(tm->tm_mday);
+	buff[5] = bin2bcd(tm->tm_mon);		/* system set 0-11 */
+	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
+
+	if (cent_flag)
+		buff[5] |= 0x80;
+
+	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;
+}
+
+static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	int err;
+	unsigned int reg_data;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
+	if (err) {
+		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
+		*enabled = 0;
+	} else {
+		if (reg_data & CTRL1_ALARM_ENABLED)
+			*enabled = 1;
+		else
+			*enabled = 0;
+	}
+
+	return err;
+}
+
+/* 0-disable, 1-enable */
+static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
+{
+	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
+	int err;
+
+	err = regmap_update_bits(rtc->rn5t618->regmap,
+			RN5T618_RTC_CTRL1,
+			CTRL1_ALARM_ENABLED,
+			enabled ? CTRL1_ALARM_ENABLED : 0);
+	if (err < 0)
+		return err;
+
+	return 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 enabled_flag;
+
+	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 & 0x80)
+		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;
+
+	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
+				&enabled_flag);
+	if (err)
+		return err;
+
+	if (enabled_flag & CTRL1_ALARM_ENABLED)
+		enabled_flag = 1;
+	else
+		enabled_flag = 0;
+
+
+	buff[3] = buff[3] & 0x3f;
+
+	alrm->time.tm_sec  = bcd2bin(buff[0]);
+	alrm->time.tm_min  = bcd2bin(buff[1]);
+	alrm->time.tm_hour = 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 = enabled_flag;
+	dev_dbg(dev, "read alarm: %d/%d/%d %d:%d:%d\n",
+		(alrm->time.tm_mon), alrm->time.tm_mday, alrm->time.tm_year,
+		 alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec);
+
+	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;
+
+	err = 0;
+	rc5t619_rtc_alarm_enable(dev, 0);
+	if (rtc->irq == -1)
+		return -EIO;
+
+	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);
+	buff[2] = 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] |= 0x80;	/* set DAL_EXT */
+
+	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
+				buff, sizeof(buff));
+	if (err) {
+		dev_err(dev, "unable to set alarm: %d\n", err);
+		return -EBUSY;
+	}
+
+	rc5t619_rtc_alarm_enable(dev, alrm->enabled);
+
+	return 0;
+}
+
+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;
+	uint8_t pon_flag, alarm_flag;
+	int err;
+
+	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
+	if (IS_ERR(rtc)) {
+		err = PTR_ERR(rtc);
+		dev_err(&pdev->dev, "no enough memory for rc5t619_rtc using\n");
+		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) {
+		dev_err(dev, "no irq specified, wakeup is disabled\n");
+		rtc->irq = -1;
+	}
+
+	/* get interrupt flag */
+	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
+	if (err)
+		return err;
+
+	/* get PON flag */
+	err = rc5t619_rtc_pon_get_clr(&pdev->dev, &pon_flag);
+	if (err) {
+		dev_err(&pdev->dev, "get PON flag error: %d\n", err);
+		return err;
+	}
+
+	/* using 24h-mode */
+	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
+
+	/* disable rtc periodic function */
+	err = rc5t619_rtc_periodic_disable(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "disable rtc periodic int: %d\n", err);
+		return err;
+	}
+
+	/* clearing RTC Adjust register */
+	err = rc5t619_rtc_clk_adjust(&pdev->dev, 0);
+	if (err) {
+		dev_err(&pdev->dev, "unable to program RTC_ADJUST: %d\n", err);
+		return err;
+	}
+
+	/* disable interrupt */
+	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
+	if (err) {
+		dev_err(&pdev->dev, "disable alarm interrupt: %d\n", err);
+		return err;
+	}
+
+	if (pon_flag) {
+		alarm_flag = 0;
+		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
+		if (err) {
+			dev_err(&pdev->dev,
+				"pon=1 clear alarm flag error: %d\n", err);
+			return err;
+		}
+	}
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
+				       &rc5t619_rtc_ops, THIS_MODULE);
+
+	if (IS_ERR(rtc->rtc)) {
+		err = PTR_ERR(rtc->rtc);
+		dev_err(dev, "RTC device register: err %d\n", err);
+		return err;
+	}
+
+	/* 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 */
+			enable_irq_wake(rtc->irq);
+			/* enable alarm_d */
+			err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
+			if (err) {
+				dev_err(&pdev->dev, "failed rtc setup\n");
+				return -EBUSY;
+			}
+		}
+	} else {
+		/* system don't want to using alarm interrupt, so close it */
+		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
+		if (err) {
+			dev_err(&pdev->dev, "disable rtc alarm error\n");
+			return err;
+		}
+
+		dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
+	}
+
+	return 0;
+}
+
+static int rc5t619_rtc_remove(struct platform_device *pdev)
+{
+	rc5t619_rtc_alarm_enable(&pdev->dev, 0);
+
+	return 0;
+}
+
+static struct platform_driver rc5t619_rtc_driver = {
+	.driver	= {
+		.name	= "rc5t619-rtc",
+	},
+	.probe	= rc5t619_rtc_probe,
+	.remove	= rc5t619_rtc_remove,
+};
+
+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 2/5] mfd: rn5t618: add irq support

[Alexandre Belloni]

Hi,

On 21/10/2019 07:41:01+0200, Andreas Kemnade wrote:
> +#if 0
> +/*
> + * REVISIT when implementing charger:
> + * according to some other implementation it needs special treatment,
> + * inverted but experiments seem not to confirm that.
> + */
> +	[RN5T618_IRQ_CHG] = {
> +		.reg_offset = 0,
> +		.mask = (1 << 6),
> +	}
> +#endif

Please avoid submitting dead code.

> +};
> +
> +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,
> +};
> +
> +int rn5t618_irq_init(struct rn5t618 *rn5t618)
> +{
> +	const struct regmap_irq_chip *irq_chip;
> +	int ret;
> +
> +	if (!rn5t618->chip_irq)
> +		return 0;
> +
> +	switch (rn5t618->variant) {
> +	case RC5T619:
> +		irq_chip = &rc5t619_irq_chip;
> +		break;
> +
> +		/* TODO: check irq definitions for other variants */
> +
> +	default:
> +		irq_chip = NULL;
> +		break;
> +	}
> +
> +	if (!irq_chip) {
> +		dev_err(rn5t618->dev, "no IRQ definition known for variant\n");
> +		return -ENOENT;
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(rn5t618->dev, rn5t618->regmap,
> +				rn5t618->chip_irq,
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				0, irq_chip, &rn5t618->irq_data);
> +	if (ret != 0) {
> +		dev_err(rn5t618->dev, "Failed to register IRQ chip\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/linux/mfd/rn5t618.h b/include/linux/mfd/rn5t618.h
> index d62ef48060b5..4d680f34ad2f 100644
> --- a/include/linux/mfd/rn5t618.h
> +++ b/include/linux/mfd/rn5t618.h
> @@ -242,9 +242,27 @@ enum {
>  	RC5T619,
>  };
>  
> +
> +/* RN5T618 IRQ definitions */
> +enum {
> +	RN5T618_IRQ_SYS,
> +	RN5T618_IRQ_DCDC,
> +	RN5T618_IRQ_RTC,
> +	RN5T618_IRQ_ADC,
> +	RN5T618_IRQ_GPIO,
> +	RN5T618_IRQ_CHG,
> +	RN5T618_NR_IRQS,
> +};
> +
> +

Spurious blank line.

>  struct rn5t618 {
>  	struct regmap *regmap;
> +	struct device *dev;
>  	long variant;
> +
> +	int chip_irq;
> +	struct regmap_irq_chip_data *irq_data;
>  };
>  
> +extern int rn5t618_irq_init(struct rn5t618 *rn5t618);
>  #endif /* __LINUX_MFD_RN5T618_H */
> -- 
> 2.11.0
> 

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Alexandre Belloni]

Hi,

The subject line is weird, how is it related to rc5t583?

On 21/10/2019 07:41:04+0200, Andreas Kemnade wrote:
>  config RTC_DRV_S35390A
>  	tristate "Seiko Instruments S-35390A"
>  	select BITREVERSE
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 6b09c21dc1b6..1d0673fd0954 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -136,6 +136,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..311788ff0723
> --- /dev/null
> +++ b/drivers/rtc/rtc-rc5t619.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * drivers/rtc/rtc-ricoh619.c
> + *
> + * Real time clock driver for RICOH R5T619 power management chip.
> + *
> + * Copyright (C) 2019 Andreas Kemnade
> + *
> + * Based on code
> + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> + *
> + * Based on code
> + *  Copyright (C) 2011 NVIDIA Corporation

Based on is not useful.

> + */
> +
> +/* #define debug		1 */
> +/* #define verbose_debug	1 */
> +

No dead code please.

> +#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
> +
> +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;
> +}
> +
> +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +
> +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);

Is it useful to have a function for a single regmap_write?

Also what is that adjusting? If this is adding/removing clock cycles,
you need to use .set_offset and .read_offset.

> +}
> +
> +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> +{
> +	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;
> +
> +	if (reg_data & CTRL2_PON) {
> +		*pon_f = 1;
> +		/* clear VDET PON */
> +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
> +		reg_data |= 0x20;	/* 0010-0000 */

Is it possible to have more defines for those magic values?

> +		err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
> +					reg_data);
> +	} else {
> +		*pon_f = 0;
> +	}
> +
> +	return err;
> +}
> +
> +/* 0-12hour, 1-24hour */
> +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> +			CTRL1_24HR, mode ? CTRL1_24HR : 0);

Is it useful to have a function for a single regmap_update_bits?

> +}
> +
> +
> +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;
> +
> +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
> +				buff, sizeof(buff));
> +	if (err < 0) {
> +		dev_err(dev, "failed to read time: %d\n", err);

Please reconsider adding so many strings in the driver, they add almost
no value but will increase the kernel memory footprint.

> +		return err;
> +	}
> +
> +	if (buff[5] & 0x80)
A define for the century bit would be good.

> +		cent_flag = 1;
> +	else
> +		cent_flag = 0;
> +
> +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */

This assignment is unnecessary, you can mask the value when using it.

Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
in the commit log:

https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c

> +
> +	tm->tm_sec  = bcd2bin(buff[0]);
> +	tm->tm_min  = bcd2bin(buff[1]);
> +	tm->tm_hour = bcd2bin(buff[2]);		/* bit5 PA_H20 */
> +	tm->tm_wday = bcd2bin(buff[3]);
> +	tm->tm_mday = bcd2bin(buff[4]);
> +	tm->tm_mon  = bcd2bin(buff[5]) - 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;
> +
> +	if (tm->tm_year >= 100)
> +		cent_flag = 1;
> +	else
> +		cent_flag = 0;
> +
> +	tm->tm_mon = tm->tm_mon + 1;

This assignment is not necessary.

> +	buff[0] = bin2bcd(tm->tm_sec);
> +	buff[1] = bin2bcd(tm->tm_min);
> +	buff[2] = bin2bcd(tm->tm_hour);
> +	buff[3] = bin2bcd(tm->tm_wday);
> +	buff[4] = bin2bcd(tm->tm_mday);
> +	buff[5] = bin2bcd(tm->tm_mon);		/* system set 0-11 */
> +	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
> +
> +	if (cent_flag)
> +		buff[5] |= 0x80;
> +
> +	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;
> +}
> +
> +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +	int err;
> +	unsigned int reg_data;
> +
> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
> +	if (err) {
> +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
> +		*enabled = 0;

Is it necessary to set enabled here?

> +	} else {
> +		if (reg_data & CTRL1_ALARM_ENABLED)
> +			*enabled = 1;
> +		else
> +			*enabled = 0;
> +	}
> +
> +	return err;
> +}
> +
> +/* 0-disable, 1-enable */
> +static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +	int err;

err is not necessary.

> +
> +	err = regmap_update_bits(rtc->rn5t618->regmap,
> +			RN5T618_RTC_CTRL1,
> +			CTRL1_ALARM_ENABLED,
> +			enabled ? CTRL1_ALARM_ENABLED : 0);
> +	if (err < 0)
> +		return err;
> +
> +	return 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 enabled_flag;
> +
> +	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 & 0x80)
> +		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;
> +
> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> +				&enabled_flag);
> +	if (err)
> +		return err;
> +
> +	if (enabled_flag & CTRL1_ALARM_ENABLED)
> +		enabled_flag = 1;

Why don't you set alrm->enabled directly here?

alrm->enabled = !!(enabled_flag & CTRL1_ALARM_ENABLED);

> +	else
> +		enabled_flag = 0;
> +
> +
> +	buff[3] = buff[3] & 0x3f;
> +
> +	alrm->time.tm_sec  = bcd2bin(buff[0]);
> +	alrm->time.tm_min  = bcd2bin(buff[1]);
> +	alrm->time.tm_hour = 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 = enabled_flag;
> +	dev_dbg(dev, "read alarm: %d/%d/%d %d:%d:%d\n",

Use %ptR

> +		(alrm->time.tm_mon), alrm->time.tm_mday, alrm->time.tm_year,
> +		 alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec);
> +
> +	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;
> +
> +	err = 0;
> +	rc5t619_rtc_alarm_enable(dev, 0);

This may fail

> +	if (rtc->irq == -1)
> +		return -EIO;

This has to be -EINVAL to get UIE emulation working.

> +
> +	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);
> +	buff[2] = 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] |= 0x80;	/* set DAL_EXT */

This bit needs a define.

> +
> +	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
> +				buff, sizeof(buff));
> +	if (err) {
> +		dev_err(dev, "unable to set alarm: %d\n", err);
> +		return -EBUSY;

Why EBUSY?

> +	}
> +
> +	rc5t619_rtc_alarm_enable(dev, alrm->enabled);

This may fail.

> +
> +	return 0;
> +}
> +
> +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;
> +	uint8_t pon_flag, alarm_flag;
> +	int err;
> +
> +	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> +	if (IS_ERR(rtc)) {
> +		err = PTR_ERR(rtc);
> +		dev_err(&pdev->dev, "no enough memory for rc5t619_rtc using\n");
> +		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) {
> +		dev_err(dev, "no irq specified, wakeup is disabled\n");
> +		rtc->irq = -1;
> +	}
> +
> +	/* get interrupt flag */
> +	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> +	if (err)
> +		return err;
> +
> +	/* get PON flag */
> +	err = rc5t619_rtc_pon_get_clr(&pdev->dev, &pon_flag);
> +	if (err) {
> +		dev_err(&pdev->dev, "get PON flag error: %d\n", err);
> +		return err;
> +	}
> +
> +	/* using 24h-mode */
> +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> +

Doesn't that corrupt the time if the RTC was previously set in 12h-mode?


> +	/* disable rtc periodic function */
> +	err = rc5t619_rtc_periodic_disable(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "disable rtc periodic int: %d\n", err);
> +		return err;
> +	}
> +
> +	/* clearing RTC Adjust register */
> +	err = rc5t619_rtc_clk_adjust(&pdev->dev, 0);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to program RTC_ADJUST: %d\n", err);
> +		return err;
> +	}
> +
> +	/* disable interrupt */
> +	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> +	if (err) {
> +		dev_err(&pdev->dev, "disable alarm interrupt: %d\n", err);
> +		return err;
> +	}
> +
> +	if (pon_flag) {
> +		alarm_flag = 0;
> +		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"pon=1 clear alarm flag error: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);

Do you want to do that even without an irq?

> +
> +	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +				       &rc5t619_rtc_ops, THIS_MODULE);
> +

Please use devm_rtc_allocate_device and rtc_register_device

> +	if (IS_ERR(rtc->rtc)) {
> +		err = PTR_ERR(rtc->rtc);
> +		dev_err(dev, "RTC device register: err %d\n", err);
> +		return err;
> +	}
> +
> +	/* 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 */
> +			enable_irq_wake(rtc->irq);
> +			/* enable alarm_d */
> +			err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
> +			if (err) {
> +				dev_err(&pdev->dev, "failed rtc setup\n");
> +				return -EBUSY;
> +			}
> +		}
> +	} else {
> +		/* system don't want to using alarm interrupt, so close it */
> +		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> +		if (err) {
> +			dev_err(&pdev->dev, "disable rtc alarm error\n");
> +			return err;
> +		}
> +
> +		dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rc5t619_rtc_remove(struct platform_device *pdev)
> +{
> +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);

If the PMIC can be used to start the platform, you probably don't want
to disable the alarm here. Even if it doesn't, is it actually useful to
disable the alarm?

> +
> +	return 0;
> +}
> +
> +static struct platform_driver rc5t619_rtc_driver = {
> +	.driver	= {
> +		.name	= "rc5t619-rtc",
> +	},
> +	.probe	= rc5t619_rtc_probe,
> +	.remove	= rc5t619_rtc_remove,
> +};
> +
> +module_platform_driver(rc5t619_rtc_driver);
> +MODULE_ALIAS("platform:rc5t619-rtc");
> +MODULE_DESCRIPTION("RICOH RC5T619 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.11.0
> 

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

Re: [Letux-kernel] [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[H. Nikolaus Schaller]

Just a meta-comment...

> Am 21.10.2019 um 12:15 schrieb Alexandre Belloni <alexandre.belloni@bootlin.com>:
> 
> Hi,
> 
> The subject line is weird, how is it related to rc5t583?
> 
> On 21/10/2019 07:41:04+0200, Andreas Kemnade wrote:
>> config RTC_DRV_S35390A
>> 	tristate "Seiko Instruments S-35390A"
>> 	select BITREVERSE
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index 6b09c21dc1b6..1d0673fd0954 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -136,6 +136,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..311788ff0723
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-rc5t619.c
>> @@ -0,0 +1,476 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * drivers/rtc/rtc-ricoh619.c
>> + *
>> + * Real time clock driver for RICOH R5T619 power management chip.
>> + *
>> + * Copyright (C) 2019 Andreas Kemnade
>> + *
>> + * Based on code
>> + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
>> + *
>> + * Based on code
>> + *  Copyright (C) 2011 NVIDIA Corporation
> 
> Based on is not useful.

Yes, it is difficult to track what the real contribution was
and what is left over.

On the other hand it is IMHO fair to attribute those who have
spent time to save ours.

What would be a better way for attribution?

> 
>> + */
>> +
>> +/* #define debug		1 */
>> +/* #define verbose_debug	1 */
>> +
> 
> No dead code please.
> 
>> +#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
>> +
>> +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;
>> +}
>> +
>> +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
>> +{
>> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +
>> +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);
> 
> Is it useful to have a function for a single regmap_write?

I'd say yes. It is wrapping all regmap accesses in getter/setter functions
whose name describes what it is setting. And it may do type conversion.
IMHO this makes code better readable and maintainable.
And a good compiler may even decide to inline this.

> 
> Also what is that adjusting? If this is adding/removing clock cycles,
> you need to use .set_offset and .read_offset.
> 
>> +}
>> +
>> +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
>> +{
>> +	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;
>> +
>> +	if (reg_data & CTRL2_PON) {
>> +		*pon_f = 1;
>> +		/* clear VDET PON */
>> +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
>> +		reg_data |= 0x20;	/* 0010-0000 */
> 
> Is it possible to have more defines for those magic values?
> 
>> +		err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
>> +					reg_data);
>> +	} else {
>> +		*pon_f = 0;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +/* 0-12hour, 1-24hour */
>> +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
>> +{
>> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +
>> +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
>> +			CTRL1_24HR, mode ? CTRL1_24HR : 0);
> 
> Is it useful to have a function for a single regmap_update_bits?

Same as above. I can immediately understand

	r = rc5t619_rtc_24hour_mode_set(dev, MODE_SOMETHING);

somewhere else in code but deciphering 

	r = regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
				CTRL1_24HR, mode ? CTRL1_24HR : 0);

spread over several places is probably difficult.

> 
>> +}
>> +
>> +
>> +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;
>> +
>> +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
>> +				buff, sizeof(buff));
>> +	if (err < 0) {
>> +		dev_err(dev, "failed to read time: %d\n", err);
> 
> Please reconsider adding so many strings in the driver, they add almost
> no value but will increase the kernel memory footprint.

You mean removing error messages is better than taking some footprint?

> 
>> +		return err;
>> +	}
>> +
>> +	if (buff[5] & 0x80)
> A define for the century bit would be good.
> 
>> +		cent_flag = 1;
>> +	else
>> +		cent_flag = 0;
>> +
>> +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */
> 
> This assignment is unnecessary, you can mask the value when using it.
> 
> Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> in the commit log:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> 
>> +
>> +	tm->tm_sec  = bcd2bin(buff[0]);
>> +	tm->tm_min  = bcd2bin(buff[1]);
>> +	tm->tm_hour = bcd2bin(buff[2]);		/* bit5 PA_H20 */
>> +	tm->tm_wday = bcd2bin(buff[3]);
>> +	tm->tm_mday = bcd2bin(buff[4]);
>> +	tm->tm_mon  = bcd2bin(buff[5]) - 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;
>> +
>> +	if (tm->tm_year >= 100)
>> +		cent_flag = 1;
>> +	else
>> +		cent_flag = 0;
>> +
>> +	tm->tm_mon = tm->tm_mon + 1;
> 
> This assignment is not necessary.
> 
>> +	buff[0] = bin2bcd(tm->tm_sec);
>> +	buff[1] = bin2bcd(tm->tm_min);
>> +	buff[2] = bin2bcd(tm->tm_hour);
>> +	buff[3] = bin2bcd(tm->tm_wday);
>> +	buff[4] = bin2bcd(tm->tm_mday);
>> +	buff[5] = bin2bcd(tm->tm_mon);		/* system set 0-11 */
>> +	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
>> +
>> +	if (cent_flag)
>> +		buff[5] |= 0x80;
>> +
>> +	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;
>> +}
>> +
>> +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
>> +{
>> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +	int err;
>> +	unsigned int reg_data;
>> +
>> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
>> +	if (err) {
>> +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
>> +		*enabled = 0;
> 
> Is it necessary to set enabled here?

Well, in case of error it is probably more safe to assume it is *not* enabled
that keeping the random value passed by the caller of this function.

> 
>> +	} else {
>> +		if (reg_data & CTRL1_ALARM_ENABLED)
>> +			*enabled = 1;
>> +		else
>> +			*enabled = 0;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +/* 0-disable, 1-enable */
>> +static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
>> +{
>> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +	int err;
> 
> err is not necessary.
> 
>> +
>> +	err = regmap_update_bits(rtc->rn5t618->regmap,
>> +			RN5T618_RTC_CTRL1,
>> +			CTRL1_ALARM_ENABLED,
>> +			enabled ? CTRL1_ALARM_ENABLED : 0);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	return 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 enabled_flag;
>> +
>> +	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 & 0x80)
>> +		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;
>> +
>> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
>> +				&enabled_flag);
>> +	if (err)
>> +		return err;
>> +
>> +	if (enabled_flag & CTRL1_ALARM_ENABLED)
>> +		enabled_flag = 1;
> 
> Why don't you set alrm->enabled directly here?
> 
> alrm->enabled = !!(enabled_flag & CTRL1_ALARM_ENABLED);
> 
>> +	else
>> +		enabled_flag = 0;
>> +
>> +
>> +	buff[3] = buff[3] & 0x3f;
>> +
>> +	alrm->time.tm_sec  = bcd2bin(buff[0]);
>> +	alrm->time.tm_min  = bcd2bin(buff[1]);
>> +	alrm->time.tm_hour = 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 = enabled_flag;
>> +	dev_dbg(dev, "read alarm: %d/%d/%d %d:%d:%d\n",
> 
> Use %ptR
> 
>> +		(alrm->time.tm_mon), alrm->time.tm_mday, alrm->time.tm_year,
>> +		 alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec);
>> +
>> +	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;
>> +
>> +	err = 0;
>> +	rc5t619_rtc_alarm_enable(dev, 0);
> 
> This may fail
> 
>> +	if (rtc->irq == -1)
>> +		return -EIO;
> 
> This has to be -EINVAL to get UIE emulation working.
> 
>> +
>> +	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);
>> +	buff[2] = 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] |= 0x80;	/* set DAL_EXT */
> 
> This bit needs a define.
> 
>> +
>> +	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
>> +				buff, sizeof(buff));
>> +	if (err) {
>> +		dev_err(dev, "unable to set alarm: %d\n", err);
>> +		return -EBUSY;
> 
> Why EBUSY?
> 
>> +	}
>> +
>> +	rc5t619_rtc_alarm_enable(dev, alrm->enabled);
> 
> This may fail.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	uint8_t pon_flag, alarm_flag;
>> +	int err;
>> +
>> +	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
>> +	if (IS_ERR(rtc)) {
>> +		err = PTR_ERR(rtc);
>> +		dev_err(&pdev->dev, "no enough memory for rc5t619_rtc using\n");
>> +		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) {
>> +		dev_err(dev, "no irq specified, wakeup is disabled\n");
>> +		rtc->irq = -1;
>> +	}
>> +
>> +	/* get interrupt flag */
>> +	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
>> +	if (err)
>> +		return err;
>> +
>> +	/* get PON flag */
>> +	err = rc5t619_rtc_pon_get_clr(&pdev->dev, &pon_flag);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "get PON flag error: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* using 24h-mode */
>> +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
>> +
> 
> Doesn't that corrupt the time if the RTC was previously set in 12h-mode?
> 
> 
>> +	/* disable rtc periodic function */
>> +	err = rc5t619_rtc_periodic_disable(&pdev->dev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "disable rtc periodic int: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* clearing RTC Adjust register */
>> +	err = rc5t619_rtc_clk_adjust(&pdev->dev, 0);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "unable to program RTC_ADJUST: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* disable interrupt */
>> +	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "disable alarm interrupt: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	if (pon_flag) {
>> +		alarm_flag = 0;
>> +		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
>> +		if (err) {
>> +			dev_err(&pdev->dev,
>> +				"pon=1 clear alarm flag error: %d\n", err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	device_init_wakeup(&pdev->dev, 1);
> 
> Do you want to do that even without an irq?
> 
>> +
>> +	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>> +				       &rc5t619_rtc_ops, THIS_MODULE);
>> +
> 
> Please use devm_rtc_allocate_device and rtc_register_device
> 
>> +	if (IS_ERR(rtc->rtc)) {
>> +		err = PTR_ERR(rtc->rtc);
>> +		dev_err(dev, "RTC device register: err %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* 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 */
>> +			enable_irq_wake(rtc->irq);
>> +			/* enable alarm_d */
>> +			err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
>> +			if (err) {
>> +				dev_err(&pdev->dev, "failed rtc setup\n");
>> +				return -EBUSY;
>> +			}
>> +		}
>> +	} else {
>> +		/* system don't want to using alarm interrupt, so close it */
>> +		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
>> +		if (err) {
>> +			dev_err(&pdev->dev, "disable rtc alarm error\n");
>> +			return err;
>> +		}
>> +
>> +		dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rc5t619_rtc_remove(struct platform_device *pdev)
>> +{
>> +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> 
> If the PMIC can be used to start the platform, you probably don't want
> to disable the alarm here. Even if it doesn't, is it actually useful to
> disable the alarm?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver rc5t619_rtc_driver = {
>> +	.driver	= {
>> +		.name	= "rc5t619-rtc",
>> +	},
>> +	.probe	= rc5t619_rtc_probe,
>> +	.remove	= rc5t619_rtc_remove,
>> +};
>> +
>> +module_platform_driver(rc5t619_rtc_driver);
>> +MODULE_ALIAS("platform:rc5t619-rtc");
>> +MODULE_DESCRIPTION("RICOH RC5T619 RTC driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.11.0
>> 

BR,
Nikolaus

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Stefan Agner]

On 2019-10-21 07:41, Andreas Kemnade wrote:
> Add an RTC driver for the RTC device on Ricoh MFD rc5t619,
> which is implemented as a variant of rn5t618
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/rtc/Kconfig       |  10 +
>  drivers/rtc/Makefile      |   1 +
>  drivers/rtc/rtc-rc5t619.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++

Parts of this driver look very similar to drivers/rtc/rtc-rc5t583.c. Can
it maybe shared?

--
Stefan

>  3 files changed, 487 insertions(+)
>  create mode 100644 drivers/rtc/rtc-rc5t619.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e72f65b61176..a4dc04c49fec 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -588,6 +588,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 6b09c21dc1b6..1d0673fd0954 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -136,6 +136,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..311788ff0723
> --- /dev/null
> +++ b/drivers/rtc/rtc-rc5t619.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * drivers/rtc/rtc-ricoh619.c
> + *
> + * Real time clock driver for RICOH R5T619 power management chip.
> + *
> + * Copyright (C) 2019 Andreas Kemnade
> + *
> + * Based on code
> + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> + *
> + * Based on code
> + *  Copyright (C) 2011 NVIDIA Corporation
> + */
> +
> +/* #define debug		1 */
> +/* #define verbose_debug	1 */
> +
> +#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
> +
> +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;
> +}
> +
> +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +
> +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);
> +}
> +
> +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> +{
> +	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;
> +
> +	if (reg_data & CTRL2_PON) {
> +		*pon_f = 1;
> +		/* 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);
> +	} else {
> +		*pon_f = 0;
> +	}
> +
> +	return err;
> +}
> +
> +/* 0-12hour, 1-24hour */
> +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> +			CTRL1_24HR, mode ? CTRL1_24HR : 0);
> +}
> +
> +
> +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;
> +
> +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
> +				buff, sizeof(buff));
> +	if (err < 0) {
> +		dev_err(dev, "failed to read time: %d\n", err);
> +		return err;
> +	}
> +
> +	if (buff[5] & 0x80)
> +		cent_flag = 1;
> +	else
> +		cent_flag = 0;
> +
> +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */
> +
> +	tm->tm_sec  = bcd2bin(buff[0]);
> +	tm->tm_min  = bcd2bin(buff[1]);
> +	tm->tm_hour = bcd2bin(buff[2]);		/* bit5 PA_H20 */
> +	tm->tm_wday = bcd2bin(buff[3]);
> +	tm->tm_mday = bcd2bin(buff[4]);
> +	tm->tm_mon  = bcd2bin(buff[5]) - 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;
> +
> +	if (tm->tm_year >= 100)
> +		cent_flag = 1;
> +	else
> +		cent_flag = 0;
> +
> +	tm->tm_mon = tm->tm_mon + 1;
> +	buff[0] = bin2bcd(tm->tm_sec);
> +	buff[1] = bin2bcd(tm->tm_min);
> +	buff[2] = bin2bcd(tm->tm_hour);
> +	buff[3] = bin2bcd(tm->tm_wday);
> +	buff[4] = bin2bcd(tm->tm_mday);
> +	buff[5] = bin2bcd(tm->tm_mon);		/* system set 0-11 */
> +	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
> +
> +	if (cent_flag)
> +		buff[5] |= 0x80;
> +
> +	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;
> +}
> +
> +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +	int err;
> +	unsigned int reg_data;
> +
> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
> +	if (err) {
> +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
> +		*enabled = 0;
> +	} else {
> +		if (reg_data & CTRL1_ALARM_ENABLED)
> +			*enabled = 1;
> +		else
> +			*enabled = 0;
> +	}
> +
> +	return err;
> +}
> +
> +/* 0-disable, 1-enable */
> +static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = regmap_update_bits(rtc->rn5t618->regmap,
> +			RN5T618_RTC_CTRL1,
> +			CTRL1_ALARM_ENABLED,
> +			enabled ? CTRL1_ALARM_ENABLED : 0);
> +	if (err < 0)
> +		return err;
> +
> +	return 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 enabled_flag;
> +
> +	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 & 0x80)
> +		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;
> +
> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> +				&enabled_flag);
> +	if (err)
> +		return err;
> +
> +	if (enabled_flag & CTRL1_ALARM_ENABLED)
> +		enabled_flag = 1;
> +	else
> +		enabled_flag = 0;
> +
> +
> +	buff[3] = buff[3] & 0x3f;
> +
> +	alrm->time.tm_sec  = bcd2bin(buff[0]);
> +	alrm->time.tm_min  = bcd2bin(buff[1]);
> +	alrm->time.tm_hour = 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 = enabled_flag;
> +	dev_dbg(dev, "read alarm: %d/%d/%d %d:%d:%d\n",
> +		(alrm->time.tm_mon), alrm->time.tm_mday, alrm->time.tm_year,
> +		 alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec);
> +
> +	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;
> +
> +	err = 0;
> +	rc5t619_rtc_alarm_enable(dev, 0);
> +	if (rtc->irq == -1)
> +		return -EIO;
> +
> +	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);
> +	buff[2] = 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] |= 0x80;	/* set DAL_EXT */
> +
> +	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
> +				buff, sizeof(buff));
> +	if (err) {
> +		dev_err(dev, "unable to set alarm: %d\n", err);
> +		return -EBUSY;
> +	}
> +
> +	rc5t619_rtc_alarm_enable(dev, alrm->enabled);
> +
> +	return 0;
> +}
> +
> +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;
> +	uint8_t pon_flag, alarm_flag;
> +	int err;
> +
> +	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> +	if (IS_ERR(rtc)) {
> +		err = PTR_ERR(rtc);
> +		dev_err(&pdev->dev, "no enough memory for rc5t619_rtc using\n");
> +		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) {
> +		dev_err(dev, "no irq specified, wakeup is disabled\n");
> +		rtc->irq = -1;
> +	}
> +
> +	/* get interrupt flag */
> +	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> +	if (err)
> +		return err;
> +
> +	/* get PON flag */
> +	err = rc5t619_rtc_pon_get_clr(&pdev->dev, &pon_flag);
> +	if (err) {
> +		dev_err(&pdev->dev, "get PON flag error: %d\n", err);
> +		return err;
> +	}
> +
> +	/* using 24h-mode */
> +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> +
> +	/* disable rtc periodic function */
> +	err = rc5t619_rtc_periodic_disable(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "disable rtc periodic int: %d\n", err);
> +		return err;
> +	}
> +
> +	/* clearing RTC Adjust register */
> +	err = rc5t619_rtc_clk_adjust(&pdev->dev, 0);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to program RTC_ADJUST: %d\n", err);
> +		return err;
> +	}
> +
> +	/* disable interrupt */
> +	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> +	if (err) {
> +		dev_err(&pdev->dev, "disable alarm interrupt: %d\n", err);
> +		return err;
> +	}
> +
> +	if (pon_flag) {
> +		alarm_flag = 0;
> +		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"pon=1 clear alarm flag error: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +				       &rc5t619_rtc_ops, THIS_MODULE);
> +
> +	if (IS_ERR(rtc->rtc)) {
> +		err = PTR_ERR(rtc->rtc);
> +		dev_err(dev, "RTC device register: err %d\n", err);
> +		return err;
> +	}
> +
> +	/* 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 */
> +			enable_irq_wake(rtc->irq);
> +			/* enable alarm_d */
> +			err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
> +			if (err) {
> +				dev_err(&pdev->dev, "failed rtc setup\n");
> +				return -EBUSY;
> +			}
> +		}
> +	} else {
> +		/* system don't want to using alarm interrupt, so close it */
> +		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> +		if (err) {
> +			dev_err(&pdev->dev, "disable rtc alarm error\n");
> +			return err;
> +		}
> +
> +		dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rc5t619_rtc_remove(struct platform_device *pdev)
> +{
> +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rc5t619_rtc_driver = {
> +	.driver	= {
> +		.name	= "rc5t619-rtc",
> +	},
> +	.probe	= rc5t619_rtc_probe,
> +	.remove	= rc5t619_rtc_remove,
> +};
> +
> +module_platform_driver(rc5t619_rtc_driver);
> +MODULE_ALIAS("platform:rc5t619-rtc");
> +MODULE_DESCRIPTION("RICOH RC5T619 RTC driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Alexandre Belloni]

On 21/10/2019 15:19:09+0200, Stefan Agner wrote:
> On 2019-10-21 07:41, Andreas Kemnade wrote:
> > Add an RTC driver for the RTC device on Ricoh MFD rc5t619,
> > which is implemented as a variant of rn5t618
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/rtc/Kconfig       |  10 +
> >  drivers/rtc/Makefile      |   1 +
> >  drivers/rtc/rtc-rc5t619.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> Parts of this driver look very similar to drivers/rtc/rtc-rc5t583.c. Can
> it maybe shared?
> 

If this could be done it would be better. I can't find any public
datasheet though...

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

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

[Andreas Kemnade]

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

On Mon, 21 Oct 2019 11:07:10 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Hi,
> 
> On 21/10/2019 07:41:01+0200, Andreas Kemnade wrote:
> > +#if 0
> > +/*
> > + * REVISIT when implementing charger:
> > + * according to some other implementation it needs special treatment,
> > + * inverted but experiments seem not to confirm that.
> > + */
> > +	[RN5T618_IRQ_CHG] = {
> > +		.reg_offset = 0,
> > +		.mask = (1 << 6),
> > +	}
> > +#endif  
> 
> Please avoid submitting dead code.
> 
ok, I can do some more testing here and add that thing properly.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Alexandre Belloni]

On 21/10/2019 12:31:30+0200, H. Nikolaus Schaller wrote:
> >> @@ -0,0 +1,476 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * drivers/rtc/rtc-ricoh619.c
> >> + *
> >> + * Real time clock driver for RICOH R5T619 power management chip.
> >> + *
> >> + * Copyright (C) 2019 Andreas Kemnade
> >> + *
> >> + * Based on code
> >> + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> >> + *
> >> + * Based on code
> >> + *  Copyright (C) 2011 NVIDIA Corporation
> > 
> > Based on is not useful.
> 
> Yes, it is difficult to track what the real contribution was
> and what is left over.
> 
> On the other hand it is IMHO fair to attribute those who have
> spent time to save ours.
> 
> What would be a better way for attribution?
> 

I don't think this require attribution

> >> +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +
> >> +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);
> > 
> > Is it useful to have a function for a single regmap_write?
> 
> I'd say yes. It is wrapping all regmap accesses in getter/setter functions
> whose name describes what it is setting. And it may do type conversion.
> IMHO this makes code better readable and maintainable.
> And a good compiler may even decide to inline this.
> 

But this takes up a lot of space in the file which makes it harder to
read while adding no information, how is rc5t619_rtc_clk_adjust more
informative than regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST,
clk)?

in both cases, I can easily deduce that the RTC adjust register is
changed.


> >> +/* 0-12hour, 1-24hour */
> >> +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +
> >> +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> >> +			CTRL1_24HR, mode ? CTRL1_24HR : 0);
> > 
> > Is it useful to have a function for a single regmap_update_bits?
> 
> Same as above. I can immediately understand
> 
> 	r = rc5t619_rtc_24hour_mode_set(dev, MODE_SOMETHING);
> 
> somewhere else in code but deciphering 
> 
> 	r = regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> 				CTRL1_24HR, mode ? CTRL1_24HR : 0);
> 
> spread over several places is probably difficult.
> 

I can immediately understand updating CTRL1_24HR in RN5T618_RTC_CTRL1
And it is used exactly once...

If this is all about naming and understanding, then why that driver
still had so many magic values?


> > 
> >> +}
> >> +
> >> +
> >> +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;
> >> +
> >> +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
> >> +				buff, sizeof(buff));
> >> +	if (err < 0) {
> >> +		dev_err(dev, "failed to read time: %d\n", err);
> > 
> > Please reconsider adding so many strings in the driver, they add almost
> > no value but will increase the kernel memory footprint.
> 
> You mean removing error messages is better than taking some footprint?
> 

Definitively yes, what is the value of the error message on the device?
What should the end user do about it? Is there even an end user reading
that message?

> >> +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +	int err;
> >> +	unsigned int reg_data;
> >> +
> >> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
> >> +	if (err) {
> >> +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
> >> +		*enabled = 0;
> > 
> > Is it necessary to set enabled here?
> 
> Well, in case of error it is probably more safe to assume it is *not* enabled
> that keeping the random value passed by the caller of this function.
> 

I would certainly hope that the caller is smart enough to not use a
value when the function calling it returns an error. This has to be
removed, it is useless.


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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

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

On Mon, 21 Oct 2019 15:50:28 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> On 21/10/2019 15:19:09+0200, Stefan Agner wrote:
> > On 2019-10-21 07:41, Andreas Kemnade wrote:  
> > > Add an RTC driver for the RTC device on Ricoh MFD rc5t619,
> > > which is implemented as a variant of rn5t618
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > >  drivers/rtc/Kconfig       |  10 +
> > >  drivers/rtc/Makefile      |   1 +
> > >  drivers/rtc/rtc-rc5t619.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++  
> > 
> > Parts of this driver look very similar to drivers/rtc/rtc-rc5t583.c. Can
> > it maybe shared?
> >   
> 
> If this could be done it would be better. I can't find any public
> datasheet though...
> 
at least they have different alarm configurations, The rc5t619 can specify
alarm in seconds, the rc5t583 not but has other alarm configurations which
are not present in the rn5t619 (judging by the lack of unused registers where
thoes information could be filled in). 

Register addresses do not match.
Some details seem to be the same like century flag.
Interestingly the rc5t583 driver does not care about 12h/24h mode.
So there is a bug there. 

Regards,
Andreas

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

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

Hi,

On Mon, 21 Oct 2019 12:15:28 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Hi,
> 
> The subject line is weird, how is it related to rc5t583?
> 
well, yes, rc5t583 driver source was opened next to the window where I wrote
the commit message...

> On 21/10/2019 07:41:04+0200, Andreas Kemnade wrote:
> >  config RTC_DRV_S35390A
> >  	tristate "Seiko Instruments S-35390A"
> >  	select BITREVERSE
> > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > index 6b09c21dc1b6..1d0673fd0954 100644
> > --- a/drivers/rtc/Makefile
> > +++ b/drivers/rtc/Makefile
> > @@ -136,6 +136,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..311788ff0723
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-rc5t619.c
> > @@ -0,0 +1,476 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * drivers/rtc/rtc-ricoh619.c
> > + *
> > + * Real time clock driver for RICOH R5T619 power management chip.
> > + *
> > + * Copyright (C) 2019 Andreas Kemnade
> > + *
> > + * Based on code
> > + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> > + *
> > + * Based on code
> > + *  Copyright (C) 2011 NVIDIA Corporation  
> 
> Based on is not useful.
> 
Well, ok, I guess 90 % of the lines are rewritten by me.
So I could remove all that Based on copyright notices?

> > + */
> > +
> > +/* #define debug		1 */
> > +/* #define verbose_debug	1 */
> > +  
> 
> No dead code please.
> 
ok.

> > +#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
> > +
> > +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;
> > +}
> > +
> > +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
> > +{
> > +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);  
> 
> Is it useful to have a function for a single regmap_write?

well, I could live without.
> 
> Also what is that adjusting? If this is adding/removing clock cycles,
> you need to use .set_offset and .read_offset.
> 
It the moment I am just clearing it at init. Since I do not know the
exact meaning of the value, I am not offering it through set_offset/read_offset.

> > +}
> > +
> > +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> > +{
> > +	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;
> > +
> > +	if (reg_data & CTRL2_PON) {
> > +		*pon_f = 1;
> > +		/* clear VDET PON */
> > +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
> > +		reg_data |= 0x20;	/* 0010-0000 */  
> 
> Is it possible to have more defines for those magic values?
> 
Well, I only have some GPLed source code as documentation, no good documentation.
So I only know that one bit must be VDET.
rtc-rc5t583 seems not to be helpful to find these magic numbers.
0x40 might be VDET. At least that is conform with rtc-rs5c348.c.
rc5t583 seems not to know a VDET.
But there seems to be no clear duplicate anywhere of those two
ctrl registers. It seems that the rc5t619 is a cross of everything, having
6 byte bcd time in common with everything. 
So we have to keep a bit of magic here.

> > +		err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
> > +					reg_data);
> > +	} else {
> > +		*pon_f = 0;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +/* 0-12hour, 1-24hour */
> > +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
> > +{
> > +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> > +			CTRL1_24HR, mode ? CTRL1_24HR : 0);  
> 
> Is it useful to have a function for a single regmap_update_bits?
> 
Again I could live without that.

> > +}
> > +
> > +
> > +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;
> > +
> > +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
> > +				buff, sizeof(buff));
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to read time: %d\n", err);  
> 
> Please reconsider adding so many strings in the driver, they add almost
> no value but will increase the kernel memory footprint.
>
Well, removing all those things which are just i2c io errors makes sense.
 
> > +		return err;
> > +	}
> > +
> > +	if (buff[5] & 0x80)  
> A define for the century bit would be good.
> 
yes, that is a clearly understood bit.

> > +		cent_flag = 1;
> > +	else
> > +		cent_flag = 0;
> > +
> > +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */  
> 
> This assignment is unnecessary, you can mask the value when using it.
> 
ok.

> Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> in the commit log:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> 
# ./rtc-range 

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_RD_TIME returned 22 (line 138)

Testing 2106-02-07 06:28:15.
OK

Testing 2262-04-11 23:47:16.
KO  Read back 2102-04-11 23:47:16.

I think it is 1900 to 2099.

> > +
> > +	tm->tm_sec  = bcd2bin(buff[0]);
> > +	tm->tm_min  = bcd2bin(buff[1]);
> > +	tm->tm_hour = bcd2bin(buff[2]);		/* bit5 PA_H20 */
> > +	tm->tm_wday = bcd2bin(buff[3]);
> > +	tm->tm_mday = bcd2bin(buff[4]);
> > +	tm->tm_mon  = bcd2bin(buff[5]) - 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;
> > +
> > +	if (tm->tm_year >= 100)
> > +		cent_flag = 1;
> > +	else
> > +		cent_flag = 0;
> > +
> > +	tm->tm_mon = tm->tm_mon + 1;  
> 
> This assignment is not necessary.
> 
ok.

> > +	buff[0] = bin2bcd(tm->tm_sec);
> > +	buff[1] = bin2bcd(tm->tm_min);
> > +	buff[2] = bin2bcd(tm->tm_hour);
> > +	buff[3] = bin2bcd(tm->tm_wday);
> > +	buff[4] = bin2bcd(tm->tm_mday);
> > +	buff[5] = bin2bcd(tm->tm_mon);		/* system set 0-11 */
> > +	buff[6] = bin2bcd(tm->tm_year - cent_flag * 100);
> > +
> > +	if (cent_flag)
> > +		buff[5] |= 0x80;
> > +
> > +	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;
> > +}
> > +
> > +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
> > +{
> > +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> > +	int err;
> > +	unsigned int reg_data;
> > +
> > +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
> > +	if (err) {
> > +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
> > +		*enabled = 0;  
> 
> Is it necessary to set enabled here?
> 
probably not.

> > +	} else {
> > +		if (reg_data & CTRL1_ALARM_ENABLED)
> > +			*enabled = 1;
> > +		else
> > +			*enabled = 0;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +/* 0-disable, 1-enable */
> > +static int rc5t619_rtc_alarm_enable(struct device *dev, unsigned int enabled)
> > +{
> > +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> > +	int err;  
> 
> err is not necessary.
> 
ok.

> > +
> > +	err = regmap_update_bits(rtc->rn5t618->regmap,
> > +			RN5T618_RTC_CTRL1,
> > +			CTRL1_ALARM_ENABLED,
> > +			enabled ? CTRL1_ALARM_ENABLED : 0);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return 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 enabled_flag;
> > +
> > +	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 & 0x80)
> > +		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;
> > +
> > +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> > +				&enabled_flag);
> > +	if (err)
> > +		return err;
> > +
> > +	if (enabled_flag & CTRL1_ALARM_ENABLED)
> > +		enabled_flag = 1;  
> 
> Why don't you set alrm->enabled directly here?
> 
> alrm->enabled = !!(enabled_flag & CTRL1_ALARM_ENABLED);
> 
yes, makes sense.

> > +	else
> > +		enabled_flag = 0;
> > +
> > +
> > +	buff[3] = buff[3] & 0x3f;
> > +
> > +	alrm->time.tm_sec  = bcd2bin(buff[0]);
> > +	alrm->time.tm_min  = bcd2bin(buff[1]);
> > +	alrm->time.tm_hour = 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 = enabled_flag;
> > +	dev_dbg(dev, "read alarm: %d/%d/%d %d:%d:%d\n",  
> 
> Use %ptR
> 
oh, that is nice.

> > +		(alrm->time.tm_mon), alrm->time.tm_mday, alrm->time.tm_year,
> > +		 alrm->time.tm_hour, alrm->time.tm_min, alrm->time.tm_sec);
> > +
> > +	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;
> > +
> > +	err = 0;
> > +	rc5t619_rtc_alarm_enable(dev, 0);  
> 
> This may fail
> 
will add checks

> > +	if (rtc->irq == -1)
> > +		return -EIO;  
> 
> This has to be -EINVAL to get UIE emulation working.
>
so then ordinary hwclock still works without irq?
 
> > +
> > +	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);
> > +	buff[2] = 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] |= 0x80;	/* set DAL_EXT */  
> 
> This bit needs a define.
> 
ok.

> > +
> > +	err = regmap_bulk_write(rtc->rn5t618->regmap, RN5T618_RTC_ALARM_Y_SEC,
> > +				buff, sizeof(buff));
> > +	if (err) {
> > +		dev_err(dev, "unable to set alarm: %d\n", err);
> > +		return -EBUSY;  
> 
> Why EBUSY?
> 
will just return err without message.
> > +	}
> > +
> > +	rc5t619_rtc_alarm_enable(dev, alrm->enabled);  
> 
> This may fail.
> 
will add a check.

> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +	uint8_t pon_flag, alarm_flag;
> > +	int err;
> > +
> > +	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> > +	if (IS_ERR(rtc)) {
> > +		err = PTR_ERR(rtc);
> > +		dev_err(&pdev->dev, "no enough memory for rc5t619_rtc using\n");
> > +		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) {
> > +		dev_err(dev, "no irq specified, wakeup is disabled\n");
> > +		rtc->irq = -1;
> > +	}
> > +
> > +	/* get interrupt flag */
> > +	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> > +	if (err)
> > +		return err;
> > +
> > +	/* get PON flag */
> > +	err = rc5t619_rtc_pon_get_clr(&pdev->dev, &pon_flag);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "get PON flag error: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	/* using 24h-mode */
> > +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> > +  
> 
> Doesn't that corrupt the time if the RTC was previously set in 12h-mode?
> 
> 
Probably it can. but probably that is only important in a multi-boot scenario but
who knows... If that is important enough I can try to implement 12h mode.

Is there any testing tool for 12h vs. 24h? Or should I expand
rtc-range.c? BTW: apparently rtc-rc5t583.c seems not to properly care
about 24h mode too, so I took a bad example. Well, not tested that...

> > +	/* disable rtc periodic function */
> > +	err = rc5t619_rtc_periodic_disable(&pdev->dev);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "disable rtc periodic int: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	/* clearing RTC Adjust register */
> > +	err = rc5t619_rtc_clk_adjust(&pdev->dev, 0);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "unable to program RTC_ADJUST: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	/* disable interrupt */
> > +	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "disable alarm interrupt: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (pon_flag) {
> > +		alarm_flag = 0;
> > +		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> > +		if (err) {
> > +			dev_err(&pdev->dev,
> > +				"pon=1 clear alarm flag error: %d\n", err);
> > +			return err;
> > +		}
> > +	}
> > +
> > +	device_init_wakeup(&pdev->dev, 1);  
> 
> Do you want to do that even without an irq?
> 
probably not.

> > +
> > +	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> > +				       &rc5t619_rtc_ops, THIS_MODULE);
> > +  
> 
> Please use devm_rtc_allocate_device and rtc_register_device
> 
ok.

> > +	if (IS_ERR(rtc->rtc)) {
> > +		err = PTR_ERR(rtc->rtc);
> > +		dev_err(dev, "RTC device register: err %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	/* 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 */
> > +			enable_irq_wake(rtc->irq);
> > +			/* enable alarm_d */
> > +			err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
> > +			if (err) {
> > +				dev_err(&pdev->dev, "failed rtc setup\n");
> > +				return -EBUSY;
> > +			}
> > +		}
> > +	} else {
> > +		/* system don't want to using alarm interrupt, so close it */
> > +		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "disable rtc alarm error\n");
> > +			return err;
> > +		}
> > +
> > +		dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rc5t619_rtc_remove(struct platform_device *pdev)
> > +{
> > +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);  
> 
> If the PMIC can be used to start the platform, you probably don't want
> to disable the alarm here. Even if it doesn't, is it actually useful to
> disable the alarm?
> 

well, I think this is not executed if you do
rtcwake -m off -s something
At least my device powers on after that.

Somehow I expect the driver to clean up there. e.g. rc5t583 does that, too.
But no strong opinion here.

Regards,
Andreas

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Alexandre Belloni]

On 21/10/2019 23:13:59+0200, Andreas Kemnade wrote:
> > > + * Based on code
> > > + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> > > + *
> > > + * Based on code
> > > + *  Copyright (C) 2011 NVIDIA Corporation  
> > 
> > Based on is not useful.
> > 
> Well, ok, I guess 90 % of the lines are rewritten by me.
> So I could remove all that Based on copyright notices?
> 

Yes

> > Also what is that adjusting? If this is adding/removing clock cycles,
> > you need to use .set_offset and .read_offset.
> > 
> It the moment I am just clearing it at init. Since I do not know the
> exact meaning of the value, I am not offering it through set_offset/read_offset.
> 

It is actually kind of an issue to clear it because it may have been set
to a useful value. Anyway, I understand you don't know much about the
register...

> > > +}
> > > +
> > > +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> > > +{
> > > +	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;
> > > +
> > > +	if (reg_data & CTRL2_PON) {
> > > +		*pon_f = 1;
> > > +		/* clear VDET PON */
> > > +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
> > > +		reg_data |= 0x20;	/* 0010-0000 */  
> > 
> > Is it possible to have more defines for those magic values?
> > 
> Well, I only have some GPLed source code as documentation, no good documentation.
> So I only know that one bit must be VDET.
> rtc-rc5t583 seems not to be helpful to find these magic numbers.
> 0x40 might be VDET. At least that is conform with rtc-rs5c348.c.
> rc5t583 seems not to know a VDET.
> But there seems to be no clear duplicate anywhere of those two
> ctrl registers. It seems that the rc5t619 is a cross of everything, having
> 6 byte bcd time in common with everything. 
> So we have to keep a bit of magic here.
> 

It would be very useful to be able to detect voltage drop. Also PON is a
useful information that is lost when probing the driver. You
definitively know the time is incorrect after power on and this should
be reset only after setting the time at least once (the same for voltage
drop).

> > > +		cent_flag = 1;
> > > +	else
> > > +		cent_flag = 0;
> > > +
> > > +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */  
> > 
> > This assignment is unnecessary, you can mask the value when using it.
> > 
> ok.
> 
> > Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> > in the commit log:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> > 
> # ./rtc-range 
> 
> 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_RD_TIME returned 22 (line 138)
> 
> Testing 2106-02-07 06:28:15.
> OK
> 
> Testing 2262-04-11 23:47:16.
> KO  Read back 2102-04-11 23:47:16.
> 
> I think it is 1900 to 2099.
> 

It is, I'm very curious to know why it doesn't roll over properly from
2099-12-31 23:59:59 to 1900-01-01 00:00:00.

You can set range_min and range_max accordingly.

> > > +	if (rtc->irq == -1)
> > > +		return -EIO;  
> > 
> > This has to be -EINVAL to get UIE emulation working.
> >
> so then ordinary hwclock still works without irq?
>  

Yes

> > > +	/* using 24h-mode */
> > > +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> > > +  
> > 
> > Doesn't that corrupt the time if the RTC was previously set in 12h-mode?
> > 
> > 
> Probably it can. but probably that is only important in a multi-boot scenario but
> who knows... If that is important enough I can try to implement 12h mode.
> 
> Is there any testing tool for 12h vs. 24h? Or should I expand
> rtc-range.c? BTW: apparently rtc-rc5t583.c seems not to properly care
> about 24h mode too, so I took a bad example. Well, not tested that...
> 

There is no specific test because reading any time in the afternoon
would fail if there is a mismatch. The correct way to handle this would
be to support both 12h and 24h mode in read_time and always set 24h mode
in set_time instead of setting it at probe time.

People usually forget that the RTC is still running while Linux is not.
So for an RTC driver, it is a bad idea to do initialization at every
probe, unlike many other peripheral.

> > > +static int rc5t619_rtc_remove(struct platform_device *pdev)
> > > +{
> > > +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);  
> > 
> > If the PMIC can be used to start the platform, you probably don't want
> > to disable the alarm here. Even if it doesn't, is it actually useful to
> > disable the alarm?
> > 
> 
> well, I think this is not executed if you do
> rtcwake -m off -s something
> At least my device powers on after that.
> 

This is waking from suspend, I was thinking powering up the platform.

> Somehow I expect the driver to clean up there. e.g. rc5t583 does that, too.
> But no strong opinion here.
> 

Again, the RTC is still running after a shutdown (which will run the
.remove callback). Disabling the alarm means that it will not fire while
linux is not running I'm not sure what the upside is. But this prevents
the RTC from starting the system if someone uses the alarm pin to do
that.

Also, having an alarm set means that userspace did it intentionally. The
driver removing the alarm is not something that is expected.

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

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

Hi,

On Thu, 24 Oct 2019 01:17:17 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> On 21/10/2019 23:13:59+0200, Andreas Kemnade wrote:
> > > > + * Based on code
> > > > + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> > > > + *
> > > > + * Based on code
> > > > + *  Copyright (C) 2011 NVIDIA Corporation    
> > > 
> > > Based on is not useful.
> > >   
> > Well, ok, I guess 90 % of the lines are rewritten by me.
> > So I could remove all that Based on copyright notices?
> >   
> 
> Yes
> 
> > > Also what is that adjusting? If this is adding/removing clock cycles,
> > > you need to use .set_offset and .read_offset.
> > >   
> > It the moment I am just clearing it at init. Since I do not know the
> > exact meaning of the value, I am not offering it through set_offset/read_offset.
> >   
> 
> It is actually kind of an issue to clear it because it may have been set
> to a useful value. Anyway, I understand you don't know much about the
> register...
> 
Or to some nonsense. The reason I am clearing it is that the original driver
is doing that and is also exporting it via sysfs. But I could reduce it to 
clearing it at PON condition, then chances are low that there is something
important in there.

> > > > +}
> > > > +
> > > > +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> > > > +{
> > > > +	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;
> > > > +
> > > > +	if (reg_data & CTRL2_PON) {
> > > > +		*pon_f = 1;
> > > > +		/* clear VDET PON */
> > > > +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
> > > > +		reg_data |= 0x20;	/* 0010-0000 */    
> > > 
> > > Is it possible to have more defines for those magic values?
> > >   
> > Well, I only have some GPLed source code as documentation, no good documentation.
> > So I only know that one bit must be VDET.
> > rtc-rc5t583 seems not to be helpful to find these magic numbers.
> > 0x40 might be VDET. At least that is conform with rtc-rs5c348.c.
> > rc5t583 seems not to know a VDET.
> > But there seems to be no clear duplicate anywhere of those two
> > ctrl registers. It seems that the rc5t619 is a cross of everything, having
> > 6 byte bcd time in common with everything. 
> > So we have to keep a bit of magic here.
> >   
> 
> It would be very useful to be able to detect voltage drop. Also PON is a
> useful information that is lost when probing the driver. You
> definitively know the time is incorrect after power on and this should
> be reset only after setting the time at least once (the same for voltage
> drop).
> 
So returing -EIO in read_time and clearing things only after set_time?

Do you see tha VDET issue as a blocker for accepting this driver?
I agree it might be useful. But IMHO it is too much guesswork and hard to test.
But contrary to the other bits used with a clear name it would be pure guesswork
and hard to test.  

> > > > +		cent_flag = 1;
> > > > +	else
> > > > +		cent_flag = 0;
> > > > +
> > > > +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */    
> > > 
> > > This assignment is unnecessary, you can mask the value when using it.
> > >   
> > ok.
> >   
> > > Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> > > in the commit log:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> > >   
> > # ./rtc-range 
> > 
> > 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_RD_TIME returned 22 (line 138)
> > 
> > Testing 2106-02-07 06:28:15.
> > OK
> > 
> > Testing 2262-04-11 23:47:16.
> > KO  Read back 2102-04-11 23:47:16.
> > 
> > I think it is 1900 to 2099.
> >   
> 
> It is, I'm very curious to know why it doesn't roll over properly from
> 2099-12-31 23:59:59 to 1900-01-01 00:00:00.
> 
> You can set range_min and range_max accordingly.
> 
yes, will do.

> > > > +	if (rtc->irq == -1)
> > > > +		return -EIO;    
> > > 
> > > This has to be -EINVAL to get UIE emulation working.
> > >  
> > so then ordinary hwclock still works without irq?
> >    
> 
> Yes
> 
> > > > +	/* using 24h-mode */
> > > > +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> > > > +    
> > > 
> > > Doesn't that corrupt the time if the RTC was previously set in 12h-mode?
> > > 
> > >   
> > Probably it can. but probably that is only important in a multi-boot scenario but
> > who knows... If that is important enough I can try to implement 12h mode.
> > 
> > Is there any testing tool for 12h vs. 24h? Or should I expand
> > rtc-range.c? BTW: apparently rtc-rc5t583.c seems not to properly care
> > about 24h mode too, so I took a bad example. Well, not tested that...
> >   
> 
> There is no specific test because reading any time in the afternoon
> would fail if there is a mismatch. The correct way to handle this would
> be to support both 12h and 24h mode in read_time and always set 24h mode
> in set_time instead of setting it at probe time.
> 
I would expect undefined behavior, not necessarily a fail. Interesting
things will happen the next time the hour register is touched.
If you assume that the scenario with other users in 12h mode is important
enough to consider, then you are right, supporting both 12h and 24h in read_time
is better. About always set 24h: If there is some other user only prepared for
12h mode, there will be trouble. Also we might need to reconfigure an alarm
to the selected mode.

I guess setting 24h mode at PON and checking at every time and alarm read/write
what mode we are in. Then we do not need to reprogram alarm.

What driver is doing 24h/12h in a good way, just as a good example?

> People usually forget that the RTC is still running while Linux is not.
> So for an RTC driver, it is a bad idea to do initialization at every
> probe, unlike many other peripheral.
> 
> > > > +static int rc5t619_rtc_remove(struct platform_device *pdev)
> > > > +{
> > > > +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);    
> > > 
> > > If the PMIC can be used to start the platform, you probably don't want
> > > to disable the alarm here. Even if it doesn't, is it actually useful to
> > > disable the alarm?
> > >   
> > 
> > well, I think this is not executed if you do
> > rtcwake -m off -s something
> > At least my device powers on after that.
> >   
> 
> This is waking from suspend, I was thinking powering up the platform.
> 
well, -m mem is waking from suspend
-m off is halt

man rtcwake says:

              off    ACPI state S5 (Poweroff).  This is done by calling
              '/sbin/shutdown'.  Not officially supported by ACPI, but
              it usually works.


testing
rtcwake -s 60 -m no
halt

it powers on successfully.

but
rtcwake -s 60 -m no
cd /sys/bus/platform/drivers/rc5t619-rtc
echo rc5t619-rtc >unbind
halt

does *not* power on. So only here .remove is called.

But that is just for understanding what we are doing here.
It is more important to have a common behavior across drivers.

> > Somehow I expect the driver to clean up there. e.g. rc5t583 does that, too.
> > But no strong opinion here.
> >   
> 
So it seems that I have found a driver doing something uncommon here an
an example.

> Again, the RTC is still running after a shutdown (which will run the
> .remove callback). Disabling the alarm means that it will not fire while
> linux is not running I'm not sure what the upside is. But this prevents
> the RTC from starting the system if someone uses the alarm pin to do
> that.
> 
As said disabling alarm at this place means not disabling alarm at shutdown
but think it is simply not necessary.

> Also, having an alarm set means that userspace did it intentionally. The
> driver removing the alarm is not something that is expected.
> 
Well, at least it is userspace job to disable it at shutdown if the device 
hould not wake up. And it is UI job to make clear to user what kind of alarm
is enabled.

Regards,
Andreas

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Alexandre Belloni]

On 24/10/2019 22:25:24+0200, Andreas Kemnade wrote:
> > It is actually kind of an issue to clear it because it may have been set
> > to a useful value. Anyway, I understand you don't know much about the
> > register...
> > 
> Or to some nonsense. The reason I am clearing it is that the original driver
> is doing that and is also exporting it via sysfs. But I could reduce it to 
> clearing it at PON condition, then chances are low that there is something
> important in there.
> 

That would be preferable to do it only at PON.

> > It would be very useful to be able to detect voltage drop. Also PON is a
> > useful information that is lost when probing the driver. You
> > definitively know the time is incorrect after power on and this should
> > be reset only after setting the time at least once (the same for voltage
> > drop).
> > 
> So returing -EIO in read_time and clearing things only after set_time?
> 

That would be EINVAL but yes, this would be the best.

> Do you see tha VDET issue as a blocker for accepting this driver?
> I agree it might be useful. But IMHO it is too much guesswork and hard to test.
> But contrary to the other bits used with a clear name it would be pure guesswork
> and hard to test.  
> 

I understand this would be too much guessing so you can leave it out.

> > There is no specific test because reading any time in the afternoon
> > would fail if there is a mismatch. The correct way to handle this would
> > be to support both 12h and 24h mode in read_time and always set 24h mode
> > in set_time instead of setting it at probe time.
> > 
> I would expect undefined behavior, not necessarily a fail. Interesting
> things will happen the next time the hour register is touched.
> If you assume that the scenario with other users in 12h mode is important
> enough to consider, then you are right, supporting both 12h and 24h in read_time
> is better. About always set 24h: If there is some other user only prepared for
> 12h mode, there will be trouble. Also we might need to reconfigure an alarm
> to the selected mode.
> 
> I guess setting 24h mode at PON and checking at every time and alarm read/write
> what mode we are in. Then we do not need to reprogram alarm.
> 
> What driver is doing 24h/12h in a good way, just as a good example?
> 

Very few drivers are caring and most are assuming 24h mode. The best
ones are handling 12h in read. Very few are are keeping 12h mode when
setting the time and I don't think it is worth it.

My point was that it isn't really useful to set the 24h mode in probe
because if it is in 12h mode, then the time will be invalid until the
next set_time.

> > > > If the PMIC can be used to start the platform, you probably don't want
> > > > to disable the alarm here. Even if it doesn't, is it actually useful to
> > > > disable the alarm?
> > > >   
> > > 
> > > well, I think this is not executed if you do
> > > rtcwake -m off -s something
> > > At least my device powers on after that.
> > >   
> > 
> > This is waking from suspend, I was thinking powering up the platform.
> > 
> well, -m mem is waking from suspend
> -m off is halt
> 
> man rtcwake says:
> 
>               off    ACPI state S5 (Poweroff).  This is done by calling
>               '/sbin/shutdown'.  Not officially supported by ACPI, but
>               it usually works.
> 
> 
> testing
> rtcwake -s 60 -m no
> halt
> 
> it powers on successfully.
> 
> but
> rtcwake -s 60 -m no
> cd /sys/bus/platform/drivers/rc5t619-rtc
> echo rc5t619-rtc >unbind
> halt
> 
> does *not* power on. So only here .remove is called.
> 
> But that is just for understanding what we are doing here.
> It is more important to have a common behavior across drivers.
> 

You are right and I'm wrong

> > > Somehow I expect the driver to clean up there. e.g. rc5t583 does that, too.
> > > But no strong opinion here.
> > >   
> > 
> So it seems that I have found a driver doing something uncommon here an
> an example.
> 
> > Again, the RTC is still running after a shutdown (which will run the
> > .remove callback). Disabling the alarm means that it will not fire while
> > linux is not running I'm not sure what the upside is. But this prevents
> > the RTC from starting the system if someone uses the alarm pin to do
> > that.
> > 
> As said disabling alarm at this place means not disabling alarm at shutdown
> but think it is simply not necessary.
> 

That would still be my opinion.

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

Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

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

On Thu, 24 Oct 2019 01:17:17 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

[...]
> > > Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> > > in the commit log:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> > >   
> > # ./rtc-range 
> > 
> > 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_RD_TIME returned 22 (line 138)
> > 
> > Testing 2106-02-07 06:28:15.
> > OK
> > 
> > Testing 2262-04-11 23:47:16.
> > KO  Read back 2102-04-11 23:47:16.
> > 
> > I think it is 1900 to 2099.
> >   
> 
> It is, I'm very curious to know why it doesn't roll over properly from
> 2099-12-31 23:59:59 to 1900-01-01 00:00:00.
> 
It rolls over correcly, but rtc_valid_tm() bails out if
year < 70.

I am preparing a v2 of this series.

Regards,
Andreas

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