Re: [PATCH 6/8] rtc: isl12022: trigger battery level detection during probe

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Alessandro Zummo <a.zummo@towertech.it>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/8] rtc: isl12022: trigger battery level detection during probe
Date: Mon, 12 Jun 2023 16:15:18 +0200	[thread overview]
Message-ID: <20230612141518c2119835@mail.local> (raw)
In-Reply-To: <20230612113059.247275-7-linux@rasmusvillemoes.dk>

On 12/06/2023 13:30:56+0200, Rasmus Villemoes wrote:
> Since the meaning of the SR_LBAT85 and SR_LBAT75 bits are different in
> battery backup mode, they may very well be set after power on, and
> stay set for up to a minute (i.e. until the battery detection in VDD
> mode happens when the seconds counter hits 59). This would mean that
> userspace doing a ioctl(RTC_VL_READ) early on could get a false
> positive.
> 
> The battery level detection can also be triggered by explicitly
> writing a 1 to the TSE bit in the BETA register. Do that once during
> boot (well, probe), and emit a single warning to the kernel log if the
> battery is already low.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/rtc/rtc-isl12022.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
> index 1b6659a9b33a..690dbb446d1a 100644
> --- a/drivers/rtc/rtc-isl12022.c
> +++ b/drivers/rtc/rtc-isl12022.c
> @@ -280,8 +280,25 @@ static void isl12022_set_trip_levels(struct device *dev)
>  	mask = ISL12022_REG_VB85_MASK | ISL12022_REG_VB75_MASK;
>  
>  	ret = regmap_update_bits(regmap, ISL12022_REG_PWR_VBAT, mask, val);
> -	if (ret)
> +	if (ret) {
>  		dev_warn(dev, "unable to set battery alarm levels: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = regmap_write_bits(regmap, ISL12022_REG_BETA,
> +				ISL12022_BETA_TSE, ISL12022_BETA_TSE);
> +	if (ret) {
> +		dev_warn(dev, "unable to trigger battery level detection: %d\n", ret);

This is too verbose, there is no action for the user upon getting this
message.

Setting TSE also enables temperature compensation, which may be an
undesirable side effect. Shouldn't this be reverted if necessary?

> +		return;
> +	}
> +
> +	ret = isl12022_read_sr(regmap);
> +	if (ret < 0) {
> +		dev_warn(dev, "unable to read status register: %d\n", ret);
> +	} else if (ret & (ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75)) {
> +		dev_warn(dev, "battery voltage is below %u%%\n",
> +			 (ret & ISL12022_SR_LBAT75) ? 75 : 85);

This message is useless, I'd drop the whole block.

> +	}
>  }
>  
>  static int isl12022_probe(struct i2c_client *client)
> -- 
> 2.37.2
> 

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