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

[<Oliver.Rohe@wago.com>]

Hi Alexandre,

thanks a lot!!

On 05.02.19 22:55, Alexandre Belloni wrote:
> Hi,
> 
> On 11/01/2019 13:15:39+0000, Oliver.Rohe@wago.com 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@wago.com>
>> ---
>>  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.

VDET means in case the chip is powered by a battery, that is about to die sometime soon, it gives you heads up.
> 
>> +		if (ctrl2 & R2x2x_CTRL2_PON)
>> +			dev_warn(&client->dev, "rtc battery voltage drop to zero detected.\n");
> 
> You should return -EINVAL directly here

The PON implies the XSTP bit, so I only return from there, just to print all the warning messages, but I can return directly from here if you want.
> 
>> +		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.

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

OK
> 
>