Message ID | 20230612113059.247275-6-linux@rasmusvillemoes.dk |
---|---|
State | Superseded |
Headers | show |
Series | rtc: isl12022: battery backup voltage and clock support | expand |
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 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 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: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 >
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); + + 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, };
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(+)