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

  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(-)
  

On 12/06/2023 13.30, Rasmus Villemoes wrote:

> 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);
> +		return;
> +	}
> +
> +	ret = isl12022_read_sr(regmap);

So testing this a bit more thoroughly reveals that the LBAT85/LBAT75
bits do not get updated immediately after the TSE bit is set; one needs
to wait a little before the battery voltage detection is done and the SR
bits updated. Unfortunately, the data sheet doesn't say anything about
how long that might be or if there's some busy bit one could look at;
all it says is actually exactly what I've done above:

  The battery level monitor is not functional in battery backup
  mode. In order to read the monitor bits after powering up VDD,
  instigate a battery level measurement by setting the TSE bit to
  "1" (BETA register), and then read the bits.

IOW, please don't apply this patch until I figure out how to do this
properly.

Rasmus
  

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
>
  

On 12/06/2023 14:30:18+0200, Rasmus Villemoes wrote:
> On 12/06/2023 13.30, Rasmus Villemoes wrote:
> 
> > 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);
> > +		return;
> > +	}
> > +
> > +	ret = isl12022_read_sr(regmap);
> 
> So testing this a bit more thoroughly reveals that the LBAT85/LBAT75
> bits do not get updated immediately after the TSE bit is set; one needs
> to wait a little before the battery voltage detection is done and the SR
> bits updated. Unfortunately, the data sheet doesn't say anything about
> how long that might be or if there's some busy bit one could look at;
> all it says is actually exactly what I've done above:
> 
>   The battery level monitor is not functional in battery backup
>   mode. In order to read the monitor bits after powering up VDD,
>   instigate a battery level measurement by setting the TSE bit to
>   "1" (BETA register), and then read the bits.
> 
> IOW, please don't apply this patch until I figure out how to do this
> properly.
> 

The datasheet states 22ms for the temperature conversion so I would
expect it takes about that time.
  

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
  

On 12/06/2023 16.17, Alexandre Belloni wrote:
> On 12/06/2023 14:30:18+0200, Rasmus Villemoes wrote:

>> So testing this a bit more thoroughly reveals that the LBAT85/LBAT75
>> bits do not get updated immediately after the TSE bit is set; one needs
>> to wait a little before the battery voltage detection is done and the SR
>> bits updated. Unfortunately, the data sheet doesn't say anything about
>> how long that might be or if there's some busy bit one could look at;
>> all it says is actually exactly what I've done above:
>>
>>   The battery level monitor is not functional in battery backup
>>   mode. In order to read the monitor bits after powering up VDD,
>>   instigate a battery level measurement by setting the TSE bit to
>>   "1" (BETA register), and then read the bits.
>>
>> IOW, please don't apply this patch until I figure out how to do this
>> properly.
>>
> 
> The datasheet states 22ms for the temperature conversion so I would
> expect it takes about that time.

It's most likely much shorter than that - if I just busy-read SR until
the LBAT bits are clear, that takes no more than 2ms, and the final read
of SR still has the BUSY bit set, indicating a temp conversion being
(still) in progress. But I'd prefer to have Renesas provide the proper
value rather than using some seems-to-work-on-my-desk. But but, it's
probably moot, see other reply.

Rasmus
  

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

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
>