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

[Waqar Hameed <waqar.hameed@axis.com>]

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!

[...]