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

[Alexandre Belloni]

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