On 13/06/2023 09:44:55+0200, Rasmus Villemoes wrote:
> 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@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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.
>
Ok, fine
> >
> >> + 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.
>
No need, I had a patch removing the message anyway.
> 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.
>
Exactly.
> Rasmus
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
