Add a driver for Epson RX8111 RTC

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 <[email protected]>

---

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 <[email protected]>

drivers/rtc/rtc-rx8111.c new file +40 -0

+
+#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. */

drivers/rtc/rtc-rx8111.c new file +61 -0

+
+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),

drivers/rtc/rtc-rx8111.c new file +10 -0

+
+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];

drivers/rtc/rtc-rx8111.c new file +38 -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. */

drivers/rtc/rtc-rx8111.c new file +21 -0

+	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)]);

drivers/rtc/rtc-rx8111.c new file +53 -0

+	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).
+		 */

drivers/rtc/rtc-rx8111.c new file +11 -0

+	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) {

drivers/rtc/rtc-rx8111.c new file +12 -0

+	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)

drivers/rtc/rtc-rx8111.c new file +28 -0

+
+	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;

drivers/rtc/rtc-rx8111.c new file +24 -0

+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Epson RX8111 RTC driver");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.30.2

Hello,

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

drivers/rtc/rtc-rx8111.c line 15

Quoted 8 lines

drivers/rtc/rtc-rx8111.c new file +8 -0

+#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

drivers/rtc/rtc-rx8111.c line 65

Quoted 7 lines

drivers/rtc/rtc-rx8111.c new file +7 -0

+#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.

Quoted 68 lines

drivers/rtc/rtc-rx8111.c new file +67 -0

+
+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),

drivers/rtc/rtc-rx8111.c line 136

Quoted 4 lines

drivers/rtc/rtc-rx8111.c new file +4 -0

+	[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.

Quoted 7 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

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

drivers/rtc/rtc-rx8111.c line 156

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

+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.

drivers/rtc/rtc-rx8111.c line 159

Quoted 4 lines

drivers/rtc/rtc-rx8111.c new file +3 -0

+	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.

drivers/rtc/rtc-rx8111.c line 164

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +5 -0

+		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.

Quoted 43 lines

drivers/rtc/rtc-rx8111.c new file +42 -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;
+

drivers/rtc/rtc-rx8111.c line 212

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

+	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.

Quoted 27 lines

drivers/rtc/rtc-rx8111.c new file +26 -0

+	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;

drivers/rtc/rtc-rx8111.c line 243

Quoted 5 lines

drivers/rtc/rtc-rx8111.c new file +5 -0

+
+	/*
+	 * 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.

Quoted 27 lines

drivers/rtc/rtc-rx8111.c new file +26 -0

+	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.
+	 */

drivers/rtc/rtc-rx8111.c

Quoted 8 lines

drivers/rtc/rtc-rx8111.c new file +8 -0

+	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

Quoted 23 lines

drivers/rtc/rtc-rx8111.c new file +23 -0

+
+	/* 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. */

drivers/rtc/rtc-rx8111.c line 306

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

+	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.

drivers/rtc/rtc-rx8111.c line 314

Quoted 9 lines

drivers/rtc/rtc-rx8111.c new file +8 -0

+	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;

drivers/rtc/rtc-rx8111.c line 325

Quoted 11 lines

drivers/rtc/rtc-rx8111.c new file +11 -0

+	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);

drivers/rtc/rtc-rx8111.c line 327

Quoted 2 lines

drivers/rtc/rtc-rx8111.c new file +2 -0

+	case RTC_VL_CLR:
+		return rx8111_clear_vl_flag(data);

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

Quoted 17 lines

drivers/rtc/rtc-rx8111.c new file +16 -0

+	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;

drivers/rtc/rtc-rx8111.c line 349

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

+	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.

Quoted 34 lines

drivers/rtc/rtc-rx8111.c new file +33 -0

+
+	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);
+

drivers/rtc/rtc-rx8111.c line 387

Quoted 5 lines

drivers/rtc/rtc-rx8111.c new file +5 -0

+	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.

Quoted 10 lines

drivers/rtc/rtc-rx8111.c line 65

Quoted 8 lines

drivers/rtc/rtc-rx8111.c new file +7 -0

+#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.

Quoted 4 lines

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 :)

Quoted 7 lines

drivers/rtc/rtc-rx8111.c line 159

Quoted 4 lines

drivers/rtc/rtc-rx8111.c new file +3 -0

+	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.

Quoted 12 lines

Quoted 12 lines

drivers/rtc/rtc-rx8111.c new file +12 -0

+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;
+

Quoted 8 lines

drivers/rtc/rtc-rx8111.c line 212

Quoted 6 lines

drivers/rtc/rtc-rx8111.c new file +6 -0

+	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.

Quoted 1 line

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?

Quoted 9 lines

drivers/rtc/rtc-rx8111.c line 306

Quoted 7 lines

drivers/rtc/rtc-rx8111.c new file +7 -0

+	/* 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)?

Quoted 4 lines

drivers/rtc/rtc-rx8111.c line 327

Quoted 2 lines

drivers/rtc/rtc-rx8111.c new file +2 -0

+	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...)