Hi Alexandre,
thanks a lot!!
On 05.02.19 22:55, Alexandre Belloni wrote:
> Hi,
>
> On 11/01/2019 13:15:39+0000, oliver.r...@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.r...@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
>
>
