| Message ID | 20230612113059.247275-1-linux@rasmusvillemoes.dk |
|---|---|
| Headers | show |
| Series | rtc: isl12022: battery backup voltage and clock support | expand |
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:55+0200, Rasmus Villemoes wrote: > Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > bits. Translate the former to "battery low", and the latter to > "battery empty or not-present". > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > drivers/rtc/rtc-isl12022.c | 41 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c > index cb8f1d92e116..1b6659a9b33a 100644 > --- a/drivers/rtc/rtc-isl12022.c > +++ b/drivers/rtc/rtc-isl12022.c > @@ -203,7 +203,48 @@ static int isl12022_rtc_set_time(struct device *dev, struct rtc_time *tm) > return regmap_bulk_write(regmap, ISL12022_REG_SC, buf, sizeof(buf)); > } > > +static int isl12022_read_sr(struct regmap *regmap) > +{ > + int ret; > + u32 val; > + > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > + if (ret < 0) > + return ret; > + return val; > +} > + > +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > +{ > + struct regmap *regmap = dev_get_drvdata(dev); > + u32 user = 0; > + int ret; > + > + switch (cmd) { > + case RTC_VL_READ: > + ret = isl12022_read_sr(regmap); > + if (ret < 0) > + return ret; > + > + if (ret & ISL12022_SR_LBAT85) > + user |= RTC_VL_BACKUP_LOW; > + > + if (ret & ISL12022_SR_LBAT75) > + user |= RTC_VL_BACKUP_EMPTY; > + > + return put_user(user, (u32 __user *)arg); > + > + case RTC_VL_CLR: > + return regmap_clear_bits(regmap, ISL12022_REG_SR, > + ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75); I'm against using RTC_VL_CLR for this as it deletes important information (i.e. the date is probably invalid). You should let the RTC clear the bits once the battery has been changed: "The LBAT75 bit is set when the VBAT has dropped below the pre-selected trip level, and will self clear when the VBAT is above the pre-selected trip level at the next detection cycle either by manual or automatic trigger." > + > + default: > + return -ENOIOCTLCMD; > + } > +} > + > static const struct rtc_class_ops isl12022_rtc_ops = { > + .ioctl = isl12022_rtc_ioctl, > .read_time = isl12022_rtc_read_time, > .set_time = isl12022_rtc_set_time, > }; > -- > 2.37.2 >
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 Mon, Jun 12, 2023 at 01:30:54PM +0200, Rasmus Villemoes wrote: > Implement support for using the values given in the > isil,trip-level[87]5-microvolt properties to set appropriate values in > the VB[87]5TP bits in the PWR_VBAT register. ... > + for (x85 = 0; x85 < ARRAY_SIZE(trip_level85) - 1; ++x85) > + if (level85 <= trip_level85[x85]) > + break; > + > + for (x75 = 0; x75 < ARRAY_SIZE(trip_level75) - 1; ++x75) > + if (level75 <= trip_level75[x75]) > + break; Does preincrement give us anything in comparison to postincrement?
On Mon, Jun 12, 2023 at 01:30:55PM +0200, Rasmus Villemoes wrote: > Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > bits. Translate the former to "battery low", and the latter to > "battery empty or not-present". ... > +static int isl12022_read_sr(struct regmap *regmap) > +{ > + int ret; > + u32 val; > + > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > + if (ret < 0) > + return ret; > + return val; Wondering if the bit 31 is in use with this register (note, I haven't checked the register width nor datasheet). > +}
On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote: > On Mon, Jun 12, 2023 at 01:30:55PM +0200, Rasmus Villemoes wrote: > > Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > > bits. Translate the former to "battery low", and the latter to > > "battery empty or not-present". > > ... > > > +static int isl12022_read_sr(struct regmap *regmap) > > +{ > > + int ret; > > + u32 val; > > + > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > + if (ret < 0) > > + return ret; > > + return val; > > Wondering if the bit 31 is in use with this register (note, I haven't checked > the register width nor datasheet). > register width is in the driver: static const struct regmap_config regmap_config = { .reg_bits = 8, .val_bits = 8, .use_single_write = true, }; > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On 12/06/2023 16.07, Alexandre Belloni wrote: > On 12/06/2023 13:30:55+0200, Rasmus Villemoes wrote: >> +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) >> +{ >> + struct regmap *regmap = dev_get_drvdata(dev); >> + u32 user = 0; >> + int ret; >> + >> + switch (cmd) { >> + case RTC_VL_READ: >> + ret = isl12022_read_sr(regmap); >> + if (ret < 0) >> + return ret; >> + >> + if (ret & ISL12022_SR_LBAT85) >> + user |= RTC_VL_BACKUP_LOW; >> + >> + if (ret & ISL12022_SR_LBAT75) >> + user |= RTC_VL_BACKUP_EMPTY; >> + >> + return put_user(user, (u32 __user *)arg); >> + >> + case RTC_VL_CLR: >> + return regmap_clear_bits(regmap, ISL12022_REG_SR, >> + ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75); > > I'm against using RTC_VL_CLR for this as it deletes important > information (i.e. the date is probably invalid). You should let the RTC > clear the bits once the battery has been changed: > > "The LBAT75 bit is set when the > VBAT has dropped below the pre-selected trip level, and will self > clear when the VBAT is above the pre-selected trip level at the > next detection cycle either by manual or automatic trigger." Well, the same thing means that the bit would get set again within a minute after the RTC_VL_CLR, so the information isn't lost as such. I actually don't understand what RTC_VL_CLR would be for if not this (though, again, in this case at least it would only have a very short-lived effect), but I'm perfectly happy to just rip out the RTC_VL_CLR case. Rasmus
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 12/06/2023 18.10, Alexandre Belloni wrote: > On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote: >> On Mon, Jun 12, 2023 at 01:30:55PM +0200, Rasmus Villemoes wrote: >>> Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 >>> bits. Translate the former to "battery low", and the latter to >>> "battery empty or not-present". >> >> ... >> >>> +static int isl12022_read_sr(struct regmap *regmap) >>> +{ >>> + int ret; >>> + u32 val; >>> + >>> + ret = regmap_read(regmap, ISL12022_REG_SR, &val); >>> + if (ret < 0) >>> + return ret; >>> + return val; >> >> Wondering if the bit 31 is in use with this register (note, I haven't checked >> the register width nor datasheet). >> > > register width is in the driver: > > static const struct regmap_config regmap_config = { > .reg_bits = 8, > .val_bits = 8, > .use_single_write = true, > }; Yeah. But I only factored that out because I wanted to read the SR also in the isl12022_set_trip_levels() to emit the warning at boot time, but when that goes away, there's no longer any reason to not just fold this back into the ioctl() handler. 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 >
On 13/06/2023 09:53:03+0200, Rasmus Villemoes wrote: > On 12/06/2023 18.10, Alexandre Belloni wrote: > > On 12/06/2023 18:48:49+0300, Andy Shevchenko wrote: > >> On Mon, Jun 12, 2023 at 01:30:55PM +0200, Rasmus Villemoes wrote: > >>> Hook up support for reading the values of the SR_LBAT85 and SR_LBAT75 > >>> bits. Translate the former to "battery low", and the latter to > >>> "battery empty or not-present". > >> > >> ... > >> > >>> +static int isl12022_read_sr(struct regmap *regmap) > >>> +{ > >>> + int ret; > >>> + u32 val; > >>> + > >>> + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > >>> + if (ret < 0) > >>> + return ret; > >>> + return val; > >> > >> Wondering if the bit 31 is in use with this register (note, I haven't checked > >> the register width nor datasheet). > >> > > > > register width is in the driver: > > > > static const struct regmap_config regmap_config = { > > .reg_bits = 8, > > .val_bits = 8, > > .use_single_write = true, > > }; > > Yeah. > > But I only factored that out because I wanted to read the SR also in the > isl12022_set_trip_levels() to emit the warning at boot time, but when > that goes away, there's no longer any reason to not just fold this back > into the ioctl() handler. That would be to clear a not self clearable battery low (but not empty) flag or a backup voltage switch flag. > > Rasmus >
The current handling of the low-battery bits in the status register is wrong. The first six patches fix that and implement proper support for RTC_VL_READ. The last two patches allow describing the isl12022 as a clock provider, for now just as a fixed 32kHz clock. They are also tangentially related to the backup battery, in that when the isl12022 is not used as a clock source, one can save some power consumption in battery mode by setting the FOx bits to 0. Rasmus Villemoes (8): rtc: isl12022: remove wrong warning for low battery level dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file dt-bindings: rtc: isl12022: add bindings for battery alarm trip levels rtc: isl12022: add support for trip level DT bindings rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls rtc: isl12022: trigger battery level detection during probe dt-bindings: rtc: isl12022: add #clock-cells property rtc: isl12022: implement support for the #clock-cells DT property .../bindings/rtc/intersil,isl12022.yaml | 66 +++++++++ .../devicetree/bindings/rtc/trivial-rtc.yaml | 2 - drivers/rtc/rtc-isl12022.c | 140 +++++++++++++++++- 3 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml