Re: [PATCH] rtc: rs5c372: Fix reading from rtc when the oscillator got interrupted.

[Alexandre Belloni]

Hi,

On 11/01/2019 13:15:39+0000, Oliver.Rohe@xxxxxxxx wrote:
> When the oscillator of the rtc gets interrupted,
> e.g. due to an empty battery, reading from the rtc will now return an error
> and the oscillator bit will be cleared, once the rtc is successfully reset.
> 
> Signed-off-by: Oliver Rohe <oliver.rohe@xxxxxxxx>
> ---
>  drivers/rtc/rtc-rs5c372.c | 60 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
> index ff35dce..91d1475 100644
> --- a/drivers/rtc/rtc-rs5c372.c
> +++ b/drivers/rtc/rtc-rs5c372.c
> @@ -52,8 +52,10 @@
>  #	define RS5C_CTRL1_CT4		(4 << 0)	/* 1 Hz level irq */
>  #define RS5C_REG_CTRL2		15
>  #	define RS5C372_CTRL2_24		(1 << 5)
> -#	define R2x2x_CTRL2_XSTP		(1 << 5)	/* only if R2x2x */
> -#	define RS5C_CTRL2_XSTP		(1 << 4)	/* only if !R2x2x */
> +#	define RS5C_CTRL2_XSTP          (1 << 4)        /* only if !R2x2x */

This should probably use tabs instead of spaces

> +#	define R2x2x_CTRL2_VDET		(1 << 6)	/* only if  R2x2x */
> +#	define R2x2x_CTRL2_XSTP		(1 << 5)	/* only if  R2x2x */
> +#	define R2x2x_CTRL2_PON		(1 << 4)	/* only if  R2x2x */
>  #	define RS5C_CTRL2_CTFG		(1 << 2)
>  #	define RS5C_CTRL2_AAFG		(1 << 1)	/* or WAFG */
>  #	define RS5C_CTRL2_BAFG		(1 << 0)	/* or DAFG */
> @@ -212,10 +214,31 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct rs5c372	*rs5c = i2c_get_clientdata(client);
>  	int		status = rs5c_get_regs(rs5c);
> +	unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
>  
>  	if (status < 0)
>  		return status;
>  
> +	switch (rs5c->type) {
> +	case rtc_r2025sd:
> +	case rtc_r2221tl:
> +		if (ctrl2 & R2x2x_CTRL2_VDET)
> +			dev_warn(&client->dev, "rtc battery voltage drop below threshold detected.\n");

VDET doesn't mean anything specific regarding timekeeping so I wouldn't warn here.

> +		if (ctrl2 & R2x2x_CTRL2_PON)
> +			dev_warn(&client->dev, "rtc battery voltage drop to zero detected.\n");

You should return -EINVAL directly here

> +		if ((rs5c->type == rtc_r2025sd && !(ctrl2 & R2x2x_CTRL2_XSTP)) ||
> +		    (rs5c->type == rtc_r2221tl &&  (ctrl2 & R2x2x_CTRL2_XSTP))) {
> +			dev_warn(&client->dev, "rtc oscillator interruption detected. Please reset the rtc clock.\n");
> +			return -EIO;

-EINVAL is the correct error code here.

> +		}
> +		break;
> +	default:
> +		if (ctrl2 & RS5C_CTRL2_XSTP) {
> +			dev_warn(&client->dev, "rtc oscillator interruption detected. Please reset the rtc clock.\n");
> +			return -EIO;

and here.


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