[v2,2/2] rtc: Add driver for Epson RX8111

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

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

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
>

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!

[...]

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.