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

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
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: Tue, 13 Jun 2023 09:44:55 +0200	[thread overview]
Message-ID: <f3dc01bc-cdd1-ab0c-5891-083f6d255a4c@rasmusvillemoes.dk> (raw)
In-Reply-To: <20230612141518c2119835@mail.local>

On 12/06/2023 16.15, Alexandre Belloni wrote:
> 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.

OK.

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

Well, I can't imagine the board designer not wanting/expecting
temperature compensation to be enabled since they've spent the $$ on
using a part with that capability. Also, we anyway set TSE if
CONFIG_HWMON so that the TEMP registers get updated once per minute.

If you insist I'll do the proper logic to set it back to 0 if it wasn't
set beforehand, but I prefer to just keep it as-is.

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

I only added this as "compensation" for ripping out the warning in 1/8,
since I assumed somebody actually wanted at least one warning in the
kernel log if the battery is low.

I/we are not going to scrape dmesg but will use the ioctl() to monitor
the battery, so I'm perfectly happy to just remove this. That will also
make the question of how long to wait after setting TSE moot, since
certainly userspace won't be able to issue the ioctl() soon enough to
see stale values in the LBAT bits.

Rasmus