linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add a driver for Epson RX8111 RTC
@ 2023-08-22 10:25 Waqar Hameed
  2023-08-22 10:25 ` [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111 Waqar Hameed
  2023-08-22 10:25 ` [PATCH v2 2/2] rtc: Add driver for " Waqar Hameed
  0 siblings, 2 replies; 8+ messages in thread
From: Waqar Hameed @ 2023-08-22 10:25 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Krzysztof Kozlowski
  Cc: kernel, linux-kernel, Alexandre Belloni, linux-rtc, devicetree

In this patch series we add a basic driver for Epson RX8111 RTC with support for
only reading/writing the time and the `ioctl`s `RTC_VL_READ` and `RTC_VL_CLR`.

Datasheet: https://support.epson.biz/td/api/doc_check.php?dl=app_RX8111CE&lang=en

Changes in v2:
* Move dt-bindings to `trivial-rtc.yaml`.

Link to v1: https://lore.kernel.org/lkml/cover.1691158774.git.waqar.hameed@axis.com/

I decided to send v2 now since it has been a while, and I didn't get any
comments for `rtc-rx8111.c` in v1.

Waqar Hameed (2):
  dt-bindings: rtc: Add Epson RX8111
  rtc: Add driver for Epson RX8111

 .../devicetree/bindings/rtc/trivial-rtc.yaml  |   1 +
 drivers/rtc/Kconfig                           |  10 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-rx8111.c                      | 411 ++++++++++++++++++
 4 files changed, 423 insertions(+)
 create mode 100644 drivers/rtc/rtc-rx8111.c


base-commit: ec351c8f2e6211054e51e661589cddd1fe856425
-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111
  2023-08-22 10:25 [PATCH v2 0/2] Add a driver for Epson RX8111 RTC Waqar Hameed
@ 2023-08-22 10:25 ` Waqar Hameed
  2023-08-22 11:48   ` Krzysztof Kozlowski
  2023-08-22 10:25 ` [PATCH v2 2/2] rtc: Add driver for " Waqar Hameed
  1 sibling, 1 reply; 8+ messages in thread
From: Waqar Hameed @ 2023-08-22 10:25 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: kernel, Alexandre Belloni, linux-rtc, devicetree, linux-kernel

Epson RX8111 is an RTC with timestamp functionality. Add devicetree
bindings requiring the compatible string and I2C slave address (reg)
through `trivial-rtc.yaml`.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index 9af77f21bb7f..b0c07f734f14 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -38,6 +38,7 @@ properties:
       - epson,rx8025
       - epson,rx8035
       # I2C-BUS INTERFACE REAL TIME CLOCK MODULE with Battery Backed RAM
+      - epson,rx8111
       - epson,rx8571
       # I2C-BUS INTERFACE REAL TIME CLOCK MODULE
       - epson,rx8581
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] rtc: Add driver for Epson RX8111
  2023-08-22 10:25 [PATCH v2 0/2] Add a driver for Epson RX8111 RTC Waqar Hameed
  2023-08-22 10:25 ` [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111 Waqar Hameed
@ 2023-08-22 10:25 ` Waqar Hameed
  2023-09-26  9:20   ` Waqar Hameed
  2023-11-03 22:53   ` Alexandre Belloni
  1 sibling, 2 replies; 8+ messages in thread
From: Waqar Hameed @ 2023-08-22 10:25 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni; +Cc: kernel, linux-kernel, linux-rtc

Epson RX8111 is an RTC with alarm, timer and timestamp functionality.
Add a basic driver with support for only reading/writing time (for now).

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 drivers/rtc/Kconfig      |  10 +
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-rx8111.c | 411 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/rtc/rtc-rx8111.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05f4b2d66290..65e7ea3c2427 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -639,6 +639,16 @@ config RTC_DRV_RX8010
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-rx8010.
 
+config RTC_DRV_RX8111
+	tristate "Epson RX8111"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  If you say yes here you will get support for the Epson RX8111 RTC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called rtc-rx8111.
+
 config RTC_DRV_RX8581
 	tristate "Epson RX-8571/RX-8581"
 	select REGMAP_I2C
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index fd209883ee2e..c245f39bffc6 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_RTC_DRV_RX4581)	+= rtc-rx4581.o
 obj-$(CONFIG_RTC_DRV_RX6110)	+= rtc-rx6110.o
 obj-$(CONFIG_RTC_DRV_RX8010)	+= rtc-rx8010.o
 obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
+obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
 obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
 obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
 obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
diff --git a/drivers/rtc/rtc-rx8111.c b/drivers/rtc/rtc-rx8111.c
new file mode 100644
index 000000000000..15ff776f739e
--- /dev/null
+++ b/drivers/rtc/rtc-rx8111.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Epson RX8111 RTC.
+ *
+ * Copyright (C) 2023 Axis Communications AB
+ */
+
+#include <linux/bcd.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/rtc.h>
+
+#define RX8111_DRV_NAME "rtc-rx8111"
+
+#define RX8111_REG_SEC			0x10	/* Second counter. */
+#define RX8111_REG_MIN			0x11	/* Minute counter */
+#define RX8111_REG_HOUR			0x12	/* Hour counter. */
+#define RX8111_REG_WEEK			0x13	/* Week day counter. */
+#define RX8111_REG_DAY			0x14	/* Month day counter. */
+#define RX8111_REG_MONTH		0x15	/* Month counter. */
+#define RX8111_REG_YEAR			0x16	/* Year counter. */
+
+#define RX8111_REG_ALARM_MIN		0x17	/* Alarm minute. */
+#define RX8111_REG_ALARM_HOUR		0x18	/* Alarm hour. */
+#define RX8111_REG_ALARM_WEEK_DAY	0x19	/* Alarm week or month day. */
+
+#define RX8111_REG_TIMER_COUNTER0	0x1a	/* Timer counter LSB. */
+#define RX8111_REG_TIMER_COUNTER1	0x1b	/* Timer counter. */
+#define RX8111_REG_TIMER_COUNTER2	0x1c	/* Timer counter MSB. */
+
+#define RX8111_REG_EXT			0x1d	/* Extension register. */
+#define RX8111_REG_FLAG			0x1e	/* Flag register. */
+#define RX8111_REG_CTRL			0x1f	/* Control register. */
+
+#define RX8111_REG_TS_1_1000_SEC	0x20	/* Timestamp 256 or 512 Hz . */
+#define RX8111_REG_TS_1_100_SEC		0x21	/* Timestamp 1 - 128 Hz. */
+#define RX8111_REG_TS_SEC		0x22	/* Timestamp second. */
+#define RX8111_REG_TS_MIN		0x23	/* Timestamp minute. */
+#define RX8111_REG_TS_HOUR		0x24	/* Timestamp hour. */
+#define RX8111_REG_TS_WEEK		0x25	/* Timestamp week day. */
+#define RX8111_REG_TS_DAY		0x26	/* Timestamp month day. */
+#define RX8111_REG_TS_MONTH		0x27	/* Timestamp month. */
+#define RX8111_REG_TS_YEAR		0x28	/* Timestamp year. */
+#define RX8111_REG_TS_STATUS		0x29	/* Timestamp status. */
+
+#define RX8111_REG_EVIN_SETTING		0x2b	/* Timestamp trigger setting. */
+#define RX8111_REG_ALARM_SEC		0x2c	/* Alarm second. */
+#define RX8111_REG_TIMER_CTRL		0x2d	/* Timer control. */
+#define RX8111_REG_TS_CTRL0		0x2e	/* Timestamp control 0. */
+#define RX8111_REG_CMD_TRIGGER		0x2f	/* Timestamp trigger. */
+#define RX8111_REG_PWR_SWITCH_CTRL	0x32	/* Power switch control. */
+#define RX8111_REG_STATUS_MON		0x33	/* Status monitor. */
+#define RX8111_REG_TS_CTRL1		0x34	/* Timestamp control 1. */
+#define RX8111_REG_TS_CTRL2		0x35	/* Timestamp control 2. */
+#define RX8111_REG_TS_CTRL3		0x36	/* Timestamp control 3. */
+
+#define RX8111_TIME_BUF_SZ (RX8111_REG_YEAR - RX8111_REG_SEC + 1)
+#define RX8111_TIME_BUF_IDX(reg)                                               \
+	({                                                                     \
+		BUILD_BUG_ON_MSG(reg < RX8111_REG_SEC || reg > RX8111_REG_YEAR,\
+				 "Invalid reg value");                         \
+		(reg - RX8111_REG_SEC);                                        \
+	})
+
+enum rx8111_regfield {
+	/* RX8111_REG_EXT. */
+	RX8111_REGF_TSEL0,
+	RX8111_REGF_TSEL1,
+	RX8111_REGF_ETS,
+	RX8111_REGF_WADA,
+	RX8111_REGF_TE,
+	RX8111_REGF_USEL,
+	RX8111_REGF_FSEL0,
+	RX8111_REGF_FSEL1,
+
+	/* RX8111_REG_FLAG. */
+	RX8111_REGF_XST,
+	RX8111_REGF_VLF,
+	RX8111_REGF_EVF,
+	RX8111_REGF_AF,
+	RX8111_REGF_TF,
+	RX8111_REGF_UF,
+	RX8111_REGF_POR,
+
+	/* RX8111_REG_CTRL. */
+	RX8111_REGF_STOP,
+	RX8111_REGF_EIE,
+	RX8111_REGF_AIE,
+	RX8111_REGF_TIE,
+	RX8111_REGF_UIE,
+
+	/* RX8111_REG_PWR_SWITCH_CTRL. */
+	RX8111_REGF_SMPT0,
+	RX8111_REGF_SMPT1,
+	RX8111_REGF_SWSEL0,
+	RX8111_REGF_SWSEL1,
+	RX8111_REGF_INIEN,
+	RX8111_REGF_CHGEN,
+
+	/* Sentinel value. */
+	RX8111_REGF_MAX
+};
+
+static const struct reg_field rx8111_regfields[] = {
+	[RX8111_REGF_TSEL0] = REG_FIELD(RX8111_REG_EXT, 0, 0),
+	[RX8111_REGF_TSEL1] = REG_FIELD(RX8111_REG_EXT, 1, 1),
+	[RX8111_REGF_ETS]   = REG_FIELD(RX8111_REG_EXT, 2, 2),
+	[RX8111_REGF_WADA]  = REG_FIELD(RX8111_REG_EXT, 3, 3),
+	[RX8111_REGF_TE]    = REG_FIELD(RX8111_REG_EXT, 4, 4),
+	[RX8111_REGF_USEL]  = REG_FIELD(RX8111_REG_EXT, 5, 5),
+	[RX8111_REGF_FSEL0] = REG_FIELD(RX8111_REG_EXT, 6, 6),
+	[RX8111_REGF_FSEL1] = REG_FIELD(RX8111_REG_EXT, 7, 7),
+
+	[RX8111_REGF_XST] = REG_FIELD(RX8111_REG_FLAG, 0, 0),
+	[RX8111_REGF_VLF] = REG_FIELD(RX8111_REG_FLAG, 1, 1),
+	[RX8111_REGF_EVF] = REG_FIELD(RX8111_REG_FLAG, 2, 2),
+	[RX8111_REGF_AF]  = REG_FIELD(RX8111_REG_FLAG, 3, 3),
+	[RX8111_REGF_TF]  = REG_FIELD(RX8111_REG_FLAG, 4, 4),
+	[RX8111_REGF_UF]  = REG_FIELD(RX8111_REG_FLAG, 5, 5),
+	[RX8111_REGF_POR] = REG_FIELD(RX8111_REG_FLAG, 7, 7),
+
+	[RX8111_REGF_STOP] = REG_FIELD(RX8111_REG_CTRL, 0, 0),
+	[RX8111_REGF_EIE]  = REG_FIELD(RX8111_REG_CTRL, 2, 2),
+	[RX8111_REGF_AIE]  = REG_FIELD(RX8111_REG_CTRL, 3, 3),
+	[RX8111_REGF_TIE]  = REG_FIELD(RX8111_REG_CTRL, 4, 4),
+	[RX8111_REGF_UIE]  = REG_FIELD(RX8111_REG_CTRL, 5, 5),
+
+	[RX8111_REGF_SMPT0]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 0, 0),
+	[RX8111_REGF_SMPT1]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 1, 1),
+	[RX8111_REGF_SWSEL0] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 2, 2),
+	[RX8111_REGF_SWSEL1] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 3, 3),
+	[RX8111_REGF_INIEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 6, 6),
+	[RX8111_REGF_CHGEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 7, 7),
+};
+
+static const struct regmap_config rx8111_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RX8111_REG_TS_CTRL3,
+};
+
+struct rx8111_data {
+	struct regmap *regmap;
+	struct regmap_field *regfields[RX8111_REGF_MAX];
+	struct device *dev;
+	struct rtc_device *rtc;
+};
+
+static int rx8111_setup(struct rx8111_data *data)
+{
+	int ret;
+
+	/* Disable extended functionality (timer, events, timestamps etc.). */
+	ret = regmap_write(data->regmap, RX8111_REG_EXT, 0);
+	if (ret) {
+		dev_err(data->dev,
+			"Could not disable extended functionality (%d)\n", ret);
+		return ret;
+	}
+
+	/* Disable all interrupts. */
+	ret = regmap_write(data->regmap, RX8111_REG_CTRL, 0);
+	if (ret) {
+		dev_err(data->dev, "Could not disable interrupts (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rx8111_read_vl_flag(struct rx8111_data *data, unsigned int *vlval)
+{
+	int ret;
+
+	ret = regmap_field_read(data->regfields[RX8111_REGF_VLF], vlval);
+	if (ret)
+		dev_err(data->dev, "Could not read VL flag (%d)", ret);
+
+	return ret;
+}
+
+static int rx8111_clear_vl_flag(struct rx8111_data *data)
+{
+	int ret;
+
+	ret = regmap_field_write(data->regfields[RX8111_REGF_VLF], 0);
+	if (ret)
+		dev_err(data->dev, "Could not write VL flag (%d)", ret);
+
+	return ret;
+}
+
+static int rx8111_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rx8111_data *data = dev_get_drvdata(dev);
+	u8 buf[RX8111_TIME_BUF_SZ];
+	unsigned int regval;
+	int ret;
+
+	/* Check status. */
+	ret = rx8111_read_vl_flag(data, &regval);
+	if (ret)
+		return ret;
+
+	if (regval) {
+		dev_warn(data->dev,
+			 "Low voltage detected, time is not reliable\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_field_read(data->regfields[RX8111_REGF_STOP], &regval);
+	if (ret) {
+		dev_err(data->dev, "Could not read clock status (%d)\n", ret);
+		return ret;
+	}
+
+	if (regval) {
+		dev_warn(data->dev, "Clock stopped, time is not reliable\n");
+		return -EINVAL;
+	}
+
+	/* Read time. */
+	ret = regmap_bulk_read(data->regmap, RX8111_REG_SEC, buf,
+			       ARRAY_SIZE(buf));
+	if (ret) {
+		dev_err(data->dev, "Could not bulk read time (%d)\n", ret);
+		return ret;
+	}
+
+	tm->tm_sec = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)]);
+	tm->tm_min = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)]);
+	tm->tm_hour = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)]);
+	tm->tm_mday = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)]);
+
+	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
+	tm->tm_mon = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)]) - 1;
+
+	/*
+	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
+	 * year 1900.
+	 */
+	tm->tm_year = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)]) + 100;
+
+	/* A single bit specifies the week day [0, 6]. Note that ffs(1) = 1. */
+	tm->tm_wday = ffs(buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)]) - 1;
+
+	return 0;
+}
+
+static int rx8111_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rx8111_data *data = dev_get_drvdata(dev);
+	u8 buf[RX8111_TIME_BUF_SZ];
+	int ret;
+
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)] = bin2bcd(tm->tm_sec);
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)] = bin2bcd(tm->tm_min);
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)] = bin2bcd(tm->tm_hour);
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)] = bin2bcd(tm->tm_mday);
+
+	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)] = bin2bcd(tm->tm_mon + 1);
+
+	/*
+	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
+	 * year 1900.
+	 */
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)] = bin2bcd(tm->tm_year - 100);
+
+	/* A single bit specifies the week day [0, 6].*/
+	buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)] = BIT(tm->tm_wday);
+
+	ret = rx8111_clear_vl_flag(data);
+	if (ret)
+		return ret;
+
+	/* Stop the clock. */
+	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 1);
+	if (ret) {
+		dev_err(data->dev, "Could not stop the clock (%d)\n", ret);
+		return ret;
+	}
+
+	/* Set the time. */
+	ret = regmap_bulk_write(data->regmap, RX8111_REG_SEC, buf,
+				ARRAY_SIZE(buf));
+	if (ret) {
+		dev_err(data->dev, "Could not bulk write time (%d)\n", ret);
+
+		/*
+		 * We don't bother with trying to start the clock again. We
+		 * check for this in rx8111_read_time() (and thus force user to
+		 * call rx8111_set_time() to try again).
+		 */
+		return ret;
+	}
+
+	/* Start the clock. */
+	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 0);
+	if (ret) {
+		dev_err(data->dev, "Could not start the clock (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rx8111_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct rx8111_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	unsigned long vlval;
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		ret = rx8111_read_vl_flag(data, &regval);
+		if (ret)
+			return ret;
+
+		vlval = regval ? RTC_VL_DATA_INVALID : 0;
+
+		return put_user(vlval, (unsigned long __user *)arg);
+	case RTC_VL_CLR:
+		return rx8111_clear_vl_flag(data);
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
+static const struct rtc_class_ops rx8111_rtc_ops = {
+	.read_time = rx8111_read_time,
+	.set_time = rx8111_set_time,
+	.ioctl = rx8111_ioctl,
+};
+
+static int rx8111_probe(struct i2c_client *client)
+{
+	struct rx8111_data *data;
+	struct rtc_device *rtc;
+	size_t i;
+	int ret;
+
+	data = devm_kmalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return dev_err_probe(&client->dev, -ENOMEM,
+				     "Could not allocate device data\n");
+
+	data->dev = &client->dev;
+	dev_set_drvdata(data->dev, data);
+
+	data->regmap = devm_regmap_init_i2c(client, &rx8111_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
+				     "Could not initialize regmap\n");
+
+	for (i = 0; i < RX8111_REGF_MAX; ++i) {
+		data->regfields[i] = devm_regmap_field_alloc(
+			data->dev, data->regmap, rx8111_regfields[i]);
+		if (IS_ERR(data->regfields[i]))
+			return dev_err_probe(
+				data->dev, PTR_ERR(data->regfields[i]),
+				"Could not allocate register field %zu\n", i);
+	}
+
+	ret = rx8111_setup(data);
+	if (ret)
+		return ret;
+
+	rtc = devm_rtc_allocate_device(data->dev);
+	if (IS_ERR(rtc))
+		return dev_err_probe(data->dev, PTR_ERR(rtc),
+				     "Could not allocate rtc device\n");
+
+	rtc->ops = &rx8111_rtc_ops;
+	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+	clear_bit(RTC_FEATURE_ALARM, rtc->features);
+
+	ret = devm_rtc_register_device(rtc);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not register rtc device (%d)\n",
+				     ret);
+
+	return 0;
+}
+
+static const struct of_device_id rx8111_of_match[] = {
+	{
+		.compatible = "epson,rx8111",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, rx8111_of_match);
+
+static struct i2c_driver rx8111_driver = {
+	.driver = {
+		.name = RX8111_DRV_NAME,
+		.of_match_table = rx8111_of_match,
+	},
+	.probe = rx8111_probe,
+};
+module_i2c_driver(rx8111_driver);
+
+MODULE_AUTHOR("Waqar Hameed <waqar.hameed@axis.com>");
+MODULE_DESCRIPTION("Epson RX8111 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111
  2023-08-22 10:25 ` [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111 Waqar Hameed
@ 2023-08-22 11:48   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-08-22 11:48 UTC (permalink / raw)
  To: Waqar Hameed, Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: kernel, linux-rtc, devicetree, linux-kernel

On 22/08/2023 12:25, Waqar Hameed wrote:
> Epson RX8111 is an RTC with timestamp functionality. Add devicetree
> bindings requiring the compatible string and I2C slave address (reg)
> through `trivial-rtc.yaml`.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rtc: Add driver for Epson RX8111
  2023-08-22 10:25 ` [PATCH v2 2/2] rtc: Add driver for " Waqar Hameed
@ 2023-09-26  9:20   ` Waqar Hameed
  2023-11-03 22:53   ` Alexandre Belloni
  1 sibling, 0 replies; 8+ messages in thread
From: Waqar Hameed @ 2023-09-26  9:20 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, kernel, linux-kernel, linux-rtc

On Tue, Aug 22, 2023 at 12:25 +0200 Waqar Hameed <waqar.hameed@axis.com> wrote:

> Epson RX8111 is an RTC with alarm, timer and timestamp functionality.
> Add a basic driver with support for only reading/writing time (for now).
>
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>

Friendly ping incoming!

Please tell if there is anything more needed to be done here. (Or that
the patch will simply not be accepted at all...)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rtc: Add driver for Epson RX8111
  2023-08-22 10:25 ` [PATCH v2 2/2] rtc: Add driver for " Waqar Hameed
  2023-09-26  9:20   ` Waqar Hameed
@ 2023-11-03 22:53   ` Alexandre Belloni
  2023-11-06 14:36     ` Waqar Hameed
  1 sibling, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2023-11-03 22:53 UTC (permalink / raw)
  To: Waqar Hameed; +Cc: Alessandro Zummo, kernel, linux-kernel, linux-rtc

Hello,

I'm sorry for the very late review...

On 22/08/2023 12:25:31+0200, Waqar Hameed wrote:
> +#include <linux/bcd.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/rtc.h>
> +
> +#define RX8111_DRV_NAME "rtc-rx8111"

This define is not necessary

> +
> +#define RX8111_REG_SEC			0x10	/* Second counter. */
> +#define RX8111_REG_MIN			0x11	/* Minute counter */
> +#define RX8111_REG_HOUR			0x12	/* Hour counter. */
> +#define RX8111_REG_WEEK			0x13	/* Week day counter. */
> +#define RX8111_REG_DAY			0x14	/* Month day counter. */
> +#define RX8111_REG_MONTH		0x15	/* Month counter. */
> +#define RX8111_REG_YEAR			0x16	/* Year counter. */
> +
> +#define RX8111_REG_ALARM_MIN		0x17	/* Alarm minute. */
> +#define RX8111_REG_ALARM_HOUR		0x18	/* Alarm hour. */
> +#define RX8111_REG_ALARM_WEEK_DAY	0x19	/* Alarm week or month day. */
> +
> +#define RX8111_REG_TIMER_COUNTER0	0x1a	/* Timer counter LSB. */
> +#define RX8111_REG_TIMER_COUNTER1	0x1b	/* Timer counter. */
> +#define RX8111_REG_TIMER_COUNTER2	0x1c	/* Timer counter MSB. */
> +
> +#define RX8111_REG_EXT			0x1d	/* Extension register. */
> +#define RX8111_REG_FLAG			0x1e	/* Flag register. */
> +#define RX8111_REG_CTRL			0x1f	/* Control register. */
> +
> +#define RX8111_REG_TS_1_1000_SEC	0x20	/* Timestamp 256 or 512 Hz . */
> +#define RX8111_REG_TS_1_100_SEC		0x21	/* Timestamp 1 - 128 Hz. */
> +#define RX8111_REG_TS_SEC		0x22	/* Timestamp second. */
> +#define RX8111_REG_TS_MIN		0x23	/* Timestamp minute. */
> +#define RX8111_REG_TS_HOUR		0x24	/* Timestamp hour. */
> +#define RX8111_REG_TS_WEEK		0x25	/* Timestamp week day. */
> +#define RX8111_REG_TS_DAY		0x26	/* Timestamp month day. */
> +#define RX8111_REG_TS_MONTH		0x27	/* Timestamp month. */
> +#define RX8111_REG_TS_YEAR		0x28	/* Timestamp year. */
> +#define RX8111_REG_TS_STATUS		0x29	/* Timestamp status. */
> +
> +#define RX8111_REG_EVIN_SETTING		0x2b	/* Timestamp trigger setting. */
> +#define RX8111_REG_ALARM_SEC		0x2c	/* Alarm second. */
> +#define RX8111_REG_TIMER_CTRL		0x2d	/* Timer control. */
> +#define RX8111_REG_TS_CTRL0		0x2e	/* Timestamp control 0. */
> +#define RX8111_REG_CMD_TRIGGER		0x2f	/* Timestamp trigger. */
> +#define RX8111_REG_PWR_SWITCH_CTRL	0x32	/* Power switch control. */
> +#define RX8111_REG_STATUS_MON		0x33	/* Status monitor. */
> +#define RX8111_REG_TS_CTRL1		0x34	/* Timestamp control 1. */
> +#define RX8111_REG_TS_CTRL2		0x35	/* Timestamp control 2. */
> +#define RX8111_REG_TS_CTRL3		0x36	/* Timestamp control 3. */
> +
> +#define RX8111_TIME_BUF_SZ (RX8111_REG_YEAR - RX8111_REG_SEC + 1)
> +#define RX8111_TIME_BUF_IDX(reg)                                               \
> +	({                                                                     \
> +		BUILD_BUG_ON_MSG(reg < RX8111_REG_SEC || reg > RX8111_REG_YEAR,\
> +				 "Invalid reg value");                         \
> +		(reg - RX8111_REG_SEC);                                        \
> +	})

I don't feel like this is improving the legibility of the code. Also,
the BUILD_BUG_ON_MSG is never going to happen and doesn't protect
against a frequent issue.

> +
> +enum rx8111_regfield {
> +	/* RX8111_REG_EXT. */
> +	RX8111_REGF_TSEL0,
> +	RX8111_REGF_TSEL1,
> +	RX8111_REGF_ETS,
> +	RX8111_REGF_WADA,
> +	RX8111_REGF_TE,
> +	RX8111_REGF_USEL,
> +	RX8111_REGF_FSEL0,
> +	RX8111_REGF_FSEL1,
> +
> +	/* RX8111_REG_FLAG. */
> +	RX8111_REGF_XST,
> +	RX8111_REGF_VLF,
> +	RX8111_REGF_EVF,
> +	RX8111_REGF_AF,
> +	RX8111_REGF_TF,
> +	RX8111_REGF_UF,
> +	RX8111_REGF_POR,
> +
> +	/* RX8111_REG_CTRL. */
> +	RX8111_REGF_STOP,
> +	RX8111_REGF_EIE,
> +	RX8111_REGF_AIE,
> +	RX8111_REGF_TIE,
> +	RX8111_REGF_UIE,
> +
> +	/* RX8111_REG_PWR_SWITCH_CTRL. */
> +	RX8111_REGF_SMPT0,
> +	RX8111_REGF_SMPT1,
> +	RX8111_REGF_SWSEL0,
> +	RX8111_REGF_SWSEL1,
> +	RX8111_REGF_INIEN,
> +	RX8111_REGF_CHGEN,
> +
> +	/* Sentinel value. */
> +	RX8111_REGF_MAX
> +};
> +
> +static const struct reg_field rx8111_regfields[] = {
> +	[RX8111_REGF_TSEL0] = REG_FIELD(RX8111_REG_EXT, 0, 0),
> +	[RX8111_REGF_TSEL1] = REG_FIELD(RX8111_REG_EXT, 1, 1),
> +	[RX8111_REGF_ETS]   = REG_FIELD(RX8111_REG_EXT, 2, 2),
> +	[RX8111_REGF_WADA]  = REG_FIELD(RX8111_REG_EXT, 3, 3),
> +	[RX8111_REGF_TE]    = REG_FIELD(RX8111_REG_EXT, 4, 4),
> +	[RX8111_REGF_USEL]  = REG_FIELD(RX8111_REG_EXT, 5, 5),
> +	[RX8111_REGF_FSEL0] = REG_FIELD(RX8111_REG_EXT, 6, 6),
> +	[RX8111_REGF_FSEL1] = REG_FIELD(RX8111_REG_EXT, 7, 7),
> +
> +	[RX8111_REGF_XST] = REG_FIELD(RX8111_REG_FLAG, 0, 0),
> +	[RX8111_REGF_VLF] = REG_FIELD(RX8111_REG_FLAG, 1, 1),
> +	[RX8111_REGF_EVF] = REG_FIELD(RX8111_REG_FLAG, 2, 2),
> +	[RX8111_REGF_AF]  = REG_FIELD(RX8111_REG_FLAG, 3, 3),
> +	[RX8111_REGF_TF]  = REG_FIELD(RX8111_REG_FLAG, 4, 4),
> +	[RX8111_REGF_UF]  = REG_FIELD(RX8111_REG_FLAG, 5, 5),
> +	[RX8111_REGF_POR] = REG_FIELD(RX8111_REG_FLAG, 7, 7),
> +
> +	[RX8111_REGF_STOP] = REG_FIELD(RX8111_REG_CTRL, 0, 0),
> +	[RX8111_REGF_EIE]  = REG_FIELD(RX8111_REG_CTRL, 2, 2),
> +	[RX8111_REGF_AIE]  = REG_FIELD(RX8111_REG_CTRL, 3, 3),
> +	[RX8111_REGF_TIE]  = REG_FIELD(RX8111_REG_CTRL, 4, 4),
> +	[RX8111_REGF_UIE]  = REG_FIELD(RX8111_REG_CTRL, 5, 5),
> +
> +	[RX8111_REGF_SMPT0]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 0, 0),
> +	[RX8111_REGF_SMPT1]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 1, 1),
> +	[RX8111_REGF_SWSEL0] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 2, 2),
> +	[RX8111_REGF_SWSEL1] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 3, 3),
> +	[RX8111_REGF_INIEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 6, 6),
> +	[RX8111_REGF_CHGEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 7, 7),
> +};

I'm not quite sure using reg_field is actually an improvement. I don't
have anything against it either, unless it adds bus reads/writes when
reading or setting the time.

> +
> +static const struct regmap_config rx8111_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RX8111_REG_TS_CTRL3,
> +};
> +
> +struct rx8111_data {
> +	struct regmap *regmap;
> +	struct regmap_field *regfields[RX8111_REGF_MAX];
> +	struct device *dev;
> +	struct rtc_device *rtc;
> +};
> +
> +static int rx8111_setup(struct rx8111_data *data)
> +{
> +	int ret;
> +
> +	/* Disable extended functionality (timer, events, timestamps etc.). */
> +	ret = regmap_write(data->regmap, RX8111_REG_EXT, 0);

This will lead to issues later on, you should leave the default values.

> +	if (ret) {
> +		dev_err(data->dev,
> +			"Could not disable extended functionality (%d)\n", ret);

You should cut down on the number of message, this would be a bus error
and the user has no actual action after seeing the message.

> +		return ret;
> +	}
> +
> +	/* Disable all interrupts. */
> +	ret = regmap_write(data->regmap, RX8111_REG_CTRL, 0);

This will also lead to issues later on when adding alarm support.

> +	if (ret) {
> +		dev_err(data->dev, "Could not disable interrupts (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rx8111_read_vl_flag(struct rx8111_data *data, unsigned int *vlval)
> +{
> +	int ret;
> +
> +	ret = regmap_field_read(data->regfields[RX8111_REGF_VLF], vlval);
> +	if (ret)
> +		dev_err(data->dev, "Could not read VL flag (%d)", ret);
> +
> +	return ret;
> +}
> +
> +static int rx8111_clear_vl_flag(struct rx8111_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_field_write(data->regfields[RX8111_REGF_VLF], 0);
> +	if (ret)
> +		dev_err(data->dev, "Could not write VL flag (%d)", ret);
> +
> +	return ret;
> +}
> +
> +static int rx8111_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rx8111_data *data = dev_get_drvdata(dev);
> +	u8 buf[RX8111_TIME_BUF_SZ];
> +	unsigned int regval;
> +	int ret;
> +
> +	/* Check status. */
> +	ret = rx8111_read_vl_flag(data, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (regval) {
> +		dev_warn(data->dev,
> +			 "Low voltage detected, time is not reliable\n");
> +		return -EINVAL;
> +	}
> +

Should you check XST too? And with this, using reg_field is worse.

> +	ret = regmap_field_read(data->regfields[RX8111_REGF_STOP], &regval);
> +	if (ret) {
> +		dev_err(data->dev, "Could not read clock status (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	if (regval) {
> +		dev_warn(data->dev, "Clock stopped, time is not reliable\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read time. */
> +	ret = regmap_bulk_read(data->regmap, RX8111_REG_SEC, buf,
> +			       ARRAY_SIZE(buf));
> +	if (ret) {
> +		dev_err(data->dev, "Could not bulk read time (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	tm->tm_sec = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)]);
> +	tm->tm_min = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)]);
> +	tm->tm_hour = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)]);
> +	tm->tm_mday = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)]);
> +
> +	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
> +	tm->tm_mon = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)]) - 1;
> +
> +	/*
> +	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
> +	 * year 1900.
> +	 */

Theses comments are not super useful because most of the RTC drivers are
behaving this way and it is quite obvious why this is the case.

> +	tm->tm_year = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)]) + 100;
> +
> +	/* A single bit specifies the week day [0, 6]. Note that ffs(1) = 1. */
> +	tm->tm_wday = ffs(buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)]) - 1;
> +
> +	return 0;
> +}
> +
> +static int rx8111_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rx8111_data *data = dev_get_drvdata(dev);
> +	u8 buf[RX8111_TIME_BUF_SZ];
> +	int ret;
> +
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)] = bin2bcd(tm->tm_sec);
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)] = bin2bcd(tm->tm_min);
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)] = bin2bcd(tm->tm_hour);
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)] = bin2bcd(tm->tm_mday);
> +
> +	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)] = bin2bcd(tm->tm_mon + 1);
> +
> +	/*
> +	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
> +	 * year 1900.
> +	 */
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)] = bin2bcd(tm->tm_year - 100);
> +
> +	/* A single bit specifies the week day [0, 6].*/
> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)] = BIT(tm->tm_wday);
> +
> +	ret = rx8111_clear_vl_flag(data);
> +	if (ret)
> +		return ret;A
> +
> +	/* Stop the clock. */
> +	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 1);
> +	if (ret) {
> +		dev_err(data->dev, "Could not stop the clock (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/* Set the time. */
> +	ret = regmap_bulk_write(data->regmap, RX8111_REG_SEC, buf,
> +				ARRAY_SIZE(buf));
> +	if (ret) {
> +		dev_err(data->dev, "Could not bulk write time (%d)\n", ret);
> +
> +		/*
> +		 * We don't bother with trying to start the clock again. We
> +		 * check for this in rx8111_read_time() (and thus force user to
> +		 * call rx8111_set_time() to try again).
> +		 */
> +		return ret;
> +	}
> +
> +	/* Start the clock. */
> +	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 0);
> +	if (ret) {
> +		dev_err(data->dev, "Could not start the clock (%d)\n", ret);
> +		return ret;
> +	}
> +

You definitively need to handle XST here too.

> +	return 0;
> +}
> +
> +static int rx8111_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +	struct rx8111_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	unsigned long vlval;
> +	int ret;
> +
> +	switch (cmd) {
> +	case RTC_VL_READ:
> +		ret = rx8111_read_vl_flag(data, &regval);
> +		if (ret)
> +			return ret;
> +
> +		vlval = regval ? RTC_VL_DATA_INVALID : 0;
> +
> +		return put_user(vlval, (unsigned long __user *)arg);
> +	case RTC_VL_CLR:
> +		return rx8111_clear_vl_flag(data);

Do not allow userspace to clear VLF without setting the time.

> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +}
> +
> +static const struct rtc_class_ops rx8111_rtc_ops = {
> +	.read_time = rx8111_read_time,
> +	.set_time = rx8111_set_time,
> +	.ioctl = rx8111_ioctl,
> +};
> +
> +static int rx8111_probe(struct i2c_client *client)
> +{
> +	struct rx8111_data *data;
> +	struct rtc_device *rtc;
> +	size_t i;
> +	int ret;
> +
> +	data = devm_kmalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				     "Could not allocate device data\n");

Please, less strings in probe or at least, use dev_dbg.

> +
> +	data->dev = &client->dev;
> +	dev_set_drvdata(data->dev, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &rx8111_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
> +				     "Could not initialize regmap\n");
> +
> +	for (i = 0; i < RX8111_REGF_MAX; ++i) {
> +		data->regfields[i] = devm_regmap_field_alloc(
> +			data->dev, data->regmap, rx8111_regfields[i]);
> +		if (IS_ERR(data->regfields[i]))
> +			return dev_err_probe(
> +				data->dev, PTR_ERR(data->regfields[i]),
> +				"Could not allocate register field %zu\n", i);
> +	}
> +
> +	ret = rx8111_setup(data);
> +	if (ret)
> +		return ret;
> +
> +	rtc = devm_rtc_allocate_device(data->dev);
> +	if (IS_ERR(rtc))
> +		return dev_err_probe(data->dev, PTR_ERR(rtc),
> +				     "Could not allocate rtc device\n");
> +
> +	rtc->ops = &rx8111_rtc_ops;
> +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> +
> +	ret = devm_rtc_register_device(rtc);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Could not register rtc device (%d)\n",
> +				     ret);

devm_rtc_register_device already has messages in all the error path,
simply return here.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rx8111_of_match[] = {
> +	{
> +		.compatible = "epson,rx8111",
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rx8111_of_match);
> +
> +static struct i2c_driver rx8111_driver = {
> +	.driver = {
> +		.name = RX8111_DRV_NAME,
> +		.of_match_table = rx8111_of_match,
> +	},
> +	.probe = rx8111_probe,
> +};
> +module_i2c_driver(rx8111_driver);
> +
> +MODULE_AUTHOR("Waqar Hameed <waqar.hameed@axis.com>");
> +MODULE_DESCRIPTION("Epson RX8111 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rtc: Add driver for Epson RX8111
  2023-11-03 22:53   ` Alexandre Belloni
@ 2023-11-06 14:36     ` Waqar Hameed
  2023-11-16 23:34       ` Alexandre Belloni
  0 siblings, 1 reply; 8+ messages in thread
From: Waqar Hameed @ 2023-11-06 14:36 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Alessandro Zummo, kernel, linux-kernel, linux-rtc

On Fri, Nov 03, 2023 at 23:53 +0100 Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Hello,
>
> I'm sorry for the very late review...

No worries! Thank you very much for reviewing!

>
> On 22/08/2023 12:25:31+0200, Waqar Hameed wrote:
>> +#include <linux/bcd.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/rtc.h>
>> +
>> +#define RX8111_DRV_NAME "rtc-rx8111"
>
> This define is not necessary

Alright, will remove and enter it directly in `.name = ...`.

[...]

>> +#define RX8111_TIME_BUF_SZ (RX8111_REG_YEAR - RX8111_REG_SEC + 1)
>> +#define RX8111_TIME_BUF_IDX(reg)                                               \
>> +	({                                                                     \
>> +		BUILD_BUG_ON_MSG(reg < RX8111_REG_SEC || reg > RX8111_REG_YEAR,\
>> +				 "Invalid reg value");                         \
>> +		(reg - RX8111_REG_SEC);                                        \
>> +	})
>
> I don't feel like this is improving the legibility of the code. 

Sure, I just wanted to minimize `reg - RX8111_REG_SEC` expressions
everywhere. I think it's a matter of taste here. I'll remove the
macro `RX8111_TIME_BUF_IDX()` altogether.


> Also, the BUILD_BUG_ON_MSG is never going to happen and doesn't
> protect against a frequent issue.

Yeah, it's probably not that frequent. Just wanted to help the next
person here :)

>
>> +
>> +enum rx8111_regfield {
>> +	/* RX8111_REG_EXT. */
>> +	RX8111_REGF_TSEL0,
>> +	RX8111_REGF_TSEL1,
>> +	RX8111_REGF_ETS,
>> +	RX8111_REGF_WADA,
>> +	RX8111_REGF_TE,
>> +	RX8111_REGF_USEL,
>> +	RX8111_REGF_FSEL0,
>> +	RX8111_REGF_FSEL1,
>> +
>> +	/* RX8111_REG_FLAG. */
>> +	RX8111_REGF_XST,
>> +	RX8111_REGF_VLF,
>> +	RX8111_REGF_EVF,
>> +	RX8111_REGF_AF,
>> +	RX8111_REGF_TF,
>> +	RX8111_REGF_UF,
>> +	RX8111_REGF_POR,
>> +
>> +	/* RX8111_REG_CTRL. */
>> +	RX8111_REGF_STOP,
>> +	RX8111_REGF_EIE,
>> +	RX8111_REGF_AIE,
>> +	RX8111_REGF_TIE,
>> +	RX8111_REGF_UIE,
>> +
>> +	/* RX8111_REG_PWR_SWITCH_CTRL. */
>> +	RX8111_REGF_SMPT0,
>> +	RX8111_REGF_SMPT1,
>> +	RX8111_REGF_SWSEL0,
>> +	RX8111_REGF_SWSEL1,
>> +	RX8111_REGF_INIEN,
>> +	RX8111_REGF_CHGEN,
>> +
>> +	/* Sentinel value. */
>> +	RX8111_REGF_MAX
>> +};
>> +
>> +static const struct reg_field rx8111_regfields[] = {
>> +	[RX8111_REGF_TSEL0] = REG_FIELD(RX8111_REG_EXT, 0, 0),
>> +	[RX8111_REGF_TSEL1] = REG_FIELD(RX8111_REG_EXT, 1, 1),
>> +	[RX8111_REGF_ETS]   = REG_FIELD(RX8111_REG_EXT, 2, 2),
>> +	[RX8111_REGF_WADA]  = REG_FIELD(RX8111_REG_EXT, 3, 3),
>> +	[RX8111_REGF_TE]    = REG_FIELD(RX8111_REG_EXT, 4, 4),
>> +	[RX8111_REGF_USEL]  = REG_FIELD(RX8111_REG_EXT, 5, 5),
>> +	[RX8111_REGF_FSEL0] = REG_FIELD(RX8111_REG_EXT, 6, 6),
>> +	[RX8111_REGF_FSEL1] = REG_FIELD(RX8111_REG_EXT, 7, 7),
>> +
>> +	[RX8111_REGF_XST] = REG_FIELD(RX8111_REG_FLAG, 0, 0),
>> +	[RX8111_REGF_VLF] = REG_FIELD(RX8111_REG_FLAG, 1, 1),
>> +	[RX8111_REGF_EVF] = REG_FIELD(RX8111_REG_FLAG, 2, 2),
>> +	[RX8111_REGF_AF]  = REG_FIELD(RX8111_REG_FLAG, 3, 3),
>> +	[RX8111_REGF_TF]  = REG_FIELD(RX8111_REG_FLAG, 4, 4),
>> +	[RX8111_REGF_UF]  = REG_FIELD(RX8111_REG_FLAG, 5, 5),
>> +	[RX8111_REGF_POR] = REG_FIELD(RX8111_REG_FLAG, 7, 7),
>> +
>> +	[RX8111_REGF_STOP] = REG_FIELD(RX8111_REG_CTRL, 0, 0),
>> +	[RX8111_REGF_EIE]  = REG_FIELD(RX8111_REG_CTRL, 2, 2),
>> +	[RX8111_REGF_AIE]  = REG_FIELD(RX8111_REG_CTRL, 3, 3),
>> +	[RX8111_REGF_TIE]  = REG_FIELD(RX8111_REG_CTRL, 4, 4),
>> +	[RX8111_REGF_UIE]  = REG_FIELD(RX8111_REG_CTRL, 5, 5),
>> +
>> +	[RX8111_REGF_SMPT0]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 0, 0),
>> +	[RX8111_REGF_SMPT1]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 1, 1),
>> +	[RX8111_REGF_SWSEL0] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 2, 2),
>> +	[RX8111_REGF_SWSEL1] = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 3, 3),
>> +	[RX8111_REGF_INIEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 6, 6),
>> +	[RX8111_REGF_CHGEN]  = REG_FIELD(RX8111_REG_PWR_SWITCH_CTRL, 7, 7),
>> +};
>
> I'm not quite sure using reg_field is actually an improvement. I don't
> have anything against it either, unless it adds bus reads/writes when
> reading or setting the time.

We shouldn't of course use `reg_field` to do extra unnecessary bus
calls. Also see the comment below when "`reg_field` is worse".

>
>> +
>> +static const struct regmap_config rx8111_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = RX8111_REG_TS_CTRL3,
>> +};

[...]

>> +static int rx8111_setup(struct rx8111_data *data)
>> +{
>> +	int ret;
>> +
>> +	/* Disable extended functionality (timer, events, timestamps etc.). */
>> +	ret = regmap_write(data->regmap, RX8111_REG_EXT, 0);
>
> This will lead to issues later on, you should leave the default values.

Alright, I understand. Will remove.

>
>> +	if (ret) {
>> +		dev_err(data->dev,
>> +			"Could not disable extended functionality (%d)\n", ret);
>
> You should cut down on the number of message, this would be a bus error
> and the user has no actual action after seeing the message.

True, will convert it to `dev_dbg()` then.

>
>> +		return ret;
>> +	}
>> +
>> +	/* Disable all interrupts. */
>> +	ret = regmap_write(data->regmap, RX8111_REG_CTRL, 0);
>
> This will also lead to issues later on when adding alarm support.

I see, will remove.

>
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not disable interrupts (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rx8111_read_vl_flag(struct rx8111_data *data, unsigned int *vlval)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_field_read(data->regfields[RX8111_REGF_VLF], vlval);
>> +	if (ret)
>> +		dev_err(data->dev, "Could not read VL flag (%d)", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rx8111_clear_vl_flag(struct rx8111_data *data)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_field_write(data->regfields[RX8111_REGF_VLF], 0);
>> +	if (ret)
>> +		dev_err(data->dev, "Could not write VL flag (%d)", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rx8111_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct rx8111_data *data = dev_get_drvdata(dev);
>> +	u8 buf[RX8111_TIME_BUF_SZ];
>> +	unsigned int regval;
>> +	int ret;
>> +
>> +	/* Check status. */
>> +	ret = rx8111_read_vl_flag(data, &regval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (regval) {
>> +		dev_warn(data->dev,
>> +			 "Low voltage detected, time is not reliable\n");
>> +		return -EINVAL;
>> +	}
>> +
>
> Should you check XST too? 

According to the datasheet
(https://support.epson.biz/td/api/doc_check.php?dl=app_RX8111CE&lang=en),
when the VLF bit is set, "Either power on reset _or_ X'tal oscillation
stop is detected". It should therefore be sufficient to only check the
VLF bit?

However, I do understand that it's maybe more "robust" to also check XST
(and to be able to distinguish and report it). We could add that.

> And with this, using reg_field is worse.

Reading two fields in the same register with `reg_field` sure is worse.
If we now also want to check XST, a better (usual) way is to do a
`regmap_read()` and then `FIELD_GET()`. What do you think?

>
>> +	ret = regmap_field_read(data->regfields[RX8111_REGF_STOP], &regval);
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not read clock status (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (regval) {
>> +		dev_warn(data->dev, "Clock stopped, time is not reliable\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read time. */
>> +	ret = regmap_bulk_read(data->regmap, RX8111_REG_SEC, buf,
>> +			       ARRAY_SIZE(buf));
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not bulk read time (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	tm->tm_sec = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)]);
>> +	tm->tm_min = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)]);
>> +	tm->tm_hour = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)]);
>> +	tm->tm_mday = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)]);
>> +
>> +	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
>> +	tm->tm_mon = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)]) - 1;
>> +
>> +	/*
>> +	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
>> +	 * year 1900.
>> +	 */
>
> Theses comments are not super useful because most of the RTC drivers are
> behaving this way and it is quite obvious why this is the case.

Sure, I'll remove them.

>
>> +	tm->tm_year = bcd2bin(buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)]) + 100;
>> +
>> +	/* A single bit specifies the week day [0, 6]. Note that ffs(1) = 1. */
>> +	tm->tm_wday = ffs(buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)]) - 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rx8111_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct rx8111_data *data = dev_get_drvdata(dev);
>> +	u8 buf[RX8111_TIME_BUF_SZ];
>> +	int ret;
>> +
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_SEC)] = bin2bcd(tm->tm_sec);
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MIN)] = bin2bcd(tm->tm_min);
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_HOUR)] = bin2bcd(tm->tm_hour);
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_DAY)] = bin2bcd(tm->tm_mday);
>> +
>> +	/* Our month range is [1, 12] and tm_mon has [0, 11]. */
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_MONTH)] = bin2bcd(tm->tm_mon + 1);
>> +
>> +	/*
>> +	 * We begin at year 2000 (c.f. rtc->range_min) and tm_year starts at
>> +	 * year 1900.
>> +	 */
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_YEAR)] = bin2bcd(tm->tm_year - 100);
>> +
>> +	/* A single bit specifies the week day [0, 6].*/
>> +	buf[RX8111_TIME_BUF_IDX(RX8111_REG_WEEK)] = BIT(tm->tm_wday);
>> +
>> +	ret = rx8111_clear_vl_flag(data);
>> +	if (ret)
>> +		return ret;A

(What happened here? Hopefully not present in original patch.)

>> +
>> +	/* Stop the clock. */
>> +	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 1);
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not stop the clock (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Set the time. */
>> +	ret = regmap_bulk_write(data->regmap, RX8111_REG_SEC, buf,
>> +				ARRAY_SIZE(buf));
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not bulk write time (%d)\n", ret);
>> +
>> +		/*
>> +		 * We don't bother with trying to start the clock again. We
>> +		 * check for this in rx8111_read_time() (and thus force user to
>> +		 * call rx8111_set_time() to try again).
>> +		 */
>> +		return ret;
>> +	}
>> +
>> +	/* Start the clock. */
>> +	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 0);
>> +	if (ret) {
>> +		dev_err(data->dev, "Could not start the clock (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>
> You definitively need to handle XST here too.

Do you mean to also clear XST the same way we clear VLF (before stopping
the clock)?

>
>> +	return 0;
>> +}
>> +
>> +static int rx8111_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct rx8111_data *data = dev_get_drvdata(dev);
>> +	unsigned int regval;
>> +	unsigned long vlval;

Just caught this; it should actually be an `unsigned int`...

>> +	int ret;
>> +
>> +	switch (cmd) {
>> +	case RTC_VL_READ:
>> +		ret = rx8111_read_vl_flag(data, &regval);
>> +		if (ret)
>> +			return ret;
>> +
>> +		vlval = regval ? RTC_VL_DATA_INVALID : 0;
>> +
>> +		return put_user(vlval, (unsigned long __user *)arg);

... and then change this accordingly.

>> +	case RTC_VL_CLR:
>> +		return rx8111_clear_vl_flag(data);
>
> Do not allow userspace to clear VLF without setting the time.

Hm, makes sense. Let's remove it here (since we already clear it in
`rx8111_set_time()`).

(I think I got "fooled" when doing a quick search and seeing some
drivers allowing this. They sure are in the minority though...)

>
>> +	default:
>> +		return -ENOIOCTLCMD;
>> +	}
>> +}
>> +
>> +static const struct rtc_class_ops rx8111_rtc_ops = {
>> +	.read_time = rx8111_read_time,
>> +	.set_time = rx8111_set_time,
>> +	.ioctl = rx8111_ioctl,
>> +};
>> +
>> +static int rx8111_probe(struct i2c_client *client)
>> +{
>> +	struct rx8111_data *data;
>> +	struct rtc_device *rtc;
>> +	size_t i;
>> +	int ret;
>> +
>> +	data = devm_kmalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return dev_err_probe(&client->dev, -ENOMEM,
>> +				     "Could not allocate device data\n");
>
> Please, less strings in probe or at least, use dev_dbg.

Alright, I'll convert them to `dev_dbg()`.

>
>> +
>> +	data->dev = &client->dev;
>> +	dev_set_drvdata(data->dev, data);
>> +
>> +	data->regmap = devm_regmap_init_i2c(client, &rx8111_regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(data->dev, PTR_ERR(data->regmap),
>> +				     "Could not initialize regmap\n");
>> +
>> +	for (i = 0; i < RX8111_REGF_MAX; ++i) {
>> +		data->regfields[i] = devm_regmap_field_alloc(
>> +			data->dev, data->regmap, rx8111_regfields[i]);
>> +		if (IS_ERR(data->regfields[i]))
>> +			return dev_err_probe(
>> +				data->dev, PTR_ERR(data->regfields[i]),
>> +				"Could not allocate register field %zu\n", i);
>> +	}
>> +
>> +	ret = rx8111_setup(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	rtc = devm_rtc_allocate_device(data->dev);
>> +	if (IS_ERR(rtc))
>> +		return dev_err_probe(data->dev, PTR_ERR(rtc),
>> +				     "Could not allocate rtc device\n");
>> +
>> +	rtc->ops = &rx8111_rtc_ops;
>> +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> +	rtc->range_max = RTC_TIMESTAMP_END_2099;
>> +
>> +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
>> +
>> +	ret = devm_rtc_register_device(rtc);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Could not register rtc device (%d)\n",
>> +				     ret);
>
> devm_rtc_register_device already has messages in all the error path,
> simply return here.

Ah, that's nice! Will do then!

[...]


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rtc: Add driver for Epson RX8111
  2023-11-06 14:36     ` Waqar Hameed
@ 2023-11-16 23:34       ` Alexandre Belloni
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2023-11-16 23:34 UTC (permalink / raw)
  To: Waqar Hameed; +Cc: Alessandro Zummo, kernel, linux-kernel, linux-rtc

On 06/11/2023 15:36:55+0100, Waqar Hameed wrote:
> 
> >> +#define RX8111_TIME_BUF_SZ (RX8111_REG_YEAR - RX8111_REG_SEC + 1)
> >> +#define RX8111_TIME_BUF_IDX(reg)                                               \
> >> +	({                                                                     \
> >> +		BUILD_BUG_ON_MSG(reg < RX8111_REG_SEC || reg > RX8111_REG_YEAR,\
> >> +				 "Invalid reg value");                         \
> >> +		(reg - RX8111_REG_SEC);                                        \
> >> +	})
> >
> > I don't feel like this is improving the legibility of the code. 
> 
> Sure, I just wanted to minimize `reg - RX8111_REG_SEC` expressions
> everywhere. I think it's a matter of taste here. I'll remove the
> macro `RX8111_TIME_BUF_IDX()` altogether.

Simply use offsets, using macro doesn't bring much as the rtc_tm member
name already carry the information.

> 
> 
> > Also, the BUILD_BUG_ON_MSG is never going to happen and doesn't
> > protect against a frequent issue.
> 
> Yeah, it's probably not that frequent. Just wanted to help the next
> person here :)


Well, regmap will do the checking at runtime which is probably enough.
> >
> >> +	if (ret) {
> >> +		dev_err(data->dev,
> >> +			"Could not disable extended functionality (%d)\n", ret);
> >
> > You should cut down on the number of message, this would be a bus error
> > and the user has no actual action after seeing the message.
> 
> True, will convert it to `dev_dbg()` then.

Just to be clear, this applies to most of the error messages.

> >> +static int rx8111_read_time(struct device *dev, struct rtc_time *tm)
> >> +{
> >> +	struct rx8111_data *data = dev_get_drvdata(dev);
> >> +	u8 buf[RX8111_TIME_BUF_SZ];
> >> +	unsigned int regval;
> >> +	int ret;
> >> +
> >> +	/* Check status. */
> >> +	ret = rx8111_read_vl_flag(data, &regval);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (regval) {
> >> +		dev_warn(data->dev,
> >> +			 "Low voltage detected, time is not reliable\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >
> > Should you check XST too? 
> 
> According to the datasheet
> (https://support.epson.biz/td/api/doc_check.php?dl=app_RX8111CE&lang=en),
> when the VLF bit is set, "Either power on reset _or_ X'tal oscillation
> stop is detected". It should therefore be sufficient to only check the
> VLF bit?
> 

Not sure it is, maybe the oscillator has an issue that is not voltage
related? Or maybe it has been stopped explicitly and never restarted
(like when setting the time and then you get a bus error).

> However, I do understand that it's maybe more "robust" to also check XST
> (and to be able to distinguish and report it). We could add that.
> 
> > And with this, using reg_field is worse.
> 
> Reading two fields in the same register with `reg_field` sure is worse.
> If we now also want to check XST, a better (usual) way is to do a
> `regmap_read()` and then `FIELD_GET()`. What do you think?

Yes, REG_FIELD is what I prefer.

> >> +	/* Start the clock. */
> >> +	ret = regmap_field_write(data->regfields[RX8111_REGF_STOP], 0);
> >> +	if (ret) {
> >> +		dev_err(data->dev, "Could not start the clock (%d)\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >
> > You definitively need to handle XST here too.
> 
> Do you mean to also clear XST the same way we clear VLF (before stopping
> the clock)?

Yes.

> >> +	case RTC_VL_CLR:
> >> +		return rx8111_clear_vl_flag(data);
> >
> > Do not allow userspace to clear VLF without setting the time.
> 
> Hm, makes sense. Let's remove it here (since we already clear it in
> `rx8111_set_time()`).
> 
> (I think I got "fooled" when doing a quick search and seeing some
> drivers allowing this. They sure are in the minority though...)

I think I removed most of those, the remaining one should be clearing a
bit that indicated low voltage and reduced functionality but not loss of
time/date which is right.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-11-16 23:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-22 10:25 [PATCH v2 0/2] Add a driver for Epson RX8111 RTC Waqar Hameed
2023-08-22 10:25 ` [PATCH v2 1/2] dt-bindings: rtc: Add Epson RX8111 Waqar Hameed
2023-08-22 11:48   ` Krzysztof Kozlowski
2023-08-22 10:25 ` [PATCH v2 2/2] rtc: Add driver for " Waqar Hameed
2023-09-26  9:20   ` Waqar Hameed
2023-11-03 22:53   ` Alexandre Belloni
2023-11-06 14:36     ` Waqar Hameed
2023-11-16 23:34       ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).