Re: [PATCH 1/2] rtc: hym8563: Read the valid flag directly instead of caching it

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

Hi,

On Thu 12 Dec 19, 11:36, Paul Kocialkowski wrote:
> The RTC has a valid bit in the seconds register that indicates whether
> power was lost since the pevious time set. This bit is currently read
> once at probe time, cached and updated with set_time.
> 
> Howeever, caching the bit may prevent detecting power loss at runtime
> (which can happen if the RTC's supply is distinct from the the platform's).
> 
> Writing the seconds register when setting time will clear the bit,
> so there should be no downside in reading the bit directly instead of
> caching it.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/rtc/rtc-hym8563.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
> index 443f6d05ce29..584a7815f246 100644
> --- a/drivers/rtc/rtc-hym8563.c
> +++ b/drivers/rtc/rtc-hym8563.c
> @@ -78,7 +78,6 @@
>  struct hym8563 {
>  	struct i2c_client	*client;
>  	struct rtc_device	*rtc;
> -	bool			valid;
>  #ifdef CONFIG_COMMON_CLK
>  	struct clk_hw		clkout_hw;
>  #endif
> @@ -95,15 +94,16 @@ static int hym8563_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	u8 buf[7];
>  	int ret;
>  
> -	if (!hym8563->valid) {
> -		dev_warn(&client->dev, "no valid clock/calendar values available\n");
> -		return -EPERM;
> -	}
> -
>  	ret = i2c_smbus_read_i2c_block_data(client, HYM8563_SEC, 7, buf);
>  	if (ret < 0)
>  		return ret;
>  
> +	if (ret & HYM8563_SEC_VL) {

Ouch, I should be checking buf[0] here. Sorry about that, will send v2.

Cheers,

Paul

> +		dev_warn(&client->dev,
> +			 "no valid clock/calendar values available\n");
> +		return -EPERM;
> +	}
> +
>  	tm->tm_sec = bcd2bin(buf[0] & HYM8563_SEC_MASK);
>  	tm->tm_min = bcd2bin(buf[1] & HYM8563_MIN_MASK);
>  	tm->tm_hour = bcd2bin(buf[2] & HYM8563_HOUR_MASK);
> @@ -157,8 +157,6 @@ static int hym8563_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	if (ret < 0)
>  		return ret;
>  
> -	hym8563->valid = true;
> -
>  	return 0;
>  }
>  
> @@ -556,9 +554,8 @@ static int hym8563_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	hym8563->valid = !(ret & HYM8563_SEC_VL);
>  	dev_dbg(&client->dev, "rtc information is %s\n",
> -		hym8563->valid ? "valid" : "invalid");
> +		(ret & HYM8563_SEC_VL) ? "invalid" : "valid");
>  
>  	hym8563->rtc = devm_rtc_device_register(&client->dev, client->name,
>  						&hym8563_rtc_ops, THIS_MODULE);
> -- 
> 2.24.0
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]