| Message ID | 20230613130011.305589-1-linux@rasmusvillemoes.dk |
|---|---|
| Headers | show |
| Series | rtc: isl12022: battery backup voltage and clock support | expand |
On Tue, Jun 13, 2023 at 03:00:07PM +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". A couple of nit-picks below. ... > +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > +{ > + struct regmap *regmap = dev_get_drvdata(dev); > + u32 user = 0, val; This assignment can be done in the actual case below. > + int ret; > + > + switch (cmd) { > + case RTC_VL_READ: > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > + if (ret < 0) I always feel uneasy with ' < 0' — Does positive error makes sense? Is it even possible? OTOH if the entire driver uses this idiom... oh well, let's make it consistent. > + return ret; user = 0; The rationale to avoid potential mistakes in the future in case this function will be expanded and user will be re-used. > + if (val & ISL12022_SR_LBAT85) > + user |= RTC_VL_BACKUP_LOW; > + > + if (val & ISL12022_SR_LBAT75) > + user |= RTC_VL_BACKUP_EMPTY; > + > + return put_user(user, (u32 __user *)arg); > + > + default: > + return -ENOIOCTLCMD; > + } > +}
On Tue, Jun 13, 2023 at 03:00:10PM +0200, Rasmus Villemoes wrote: > If device tree implies that the chip's IRQ/F_OUT pin is used as a > clock, expose that in the driver. For now, pretend it is a > fixed-rate (32kHz) clock; if other use cases appear the driver can be > updated to provide its own clk_ops etc. > > When the clock output is not used on a given board, one can prolong > the battery life by ensuring that the FOx bits are 0. For the hardware > I'm currently working on, the RTC draws 1.2uA with the FOx bits at > their default 0001 value, dropping to 0.88uA when those bits are > cleared. ... > +#define ISL12022_INT_FO_MASK GENMASK(3, 0) > +#define ISL12022_INT_FO_OFF 0x0 > +#define ISL12022_INT_FO_32K 0x1 A nit-pick. Are they decimal or bit fields? To me seems like the 0x can be dropped. ... > + ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K); Seems too long even for 100 limit. Maybe: ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K); ?
On Tue, Jun 13, 2023 at 03:00:02PM +0200, Rasmus Villemoes wrote: > 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. > v2 changes: A nit-pick regarding to the process. You used In-reply-to email header and this a bit inconvenient if you operate with a threads in MUA, for example, I would like to delete old thread, but in this case it automatically marks v2 for deletion (I'm using classical mutt).
On 13/06/2023 15:00, Rasmus Villemoes wrote: > 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. > > v2 changes: Do not attach (thread) your patchsets to some other threads (unrelated or older versions). This buries them deep in the mailbox and might interfere with applying entire sets. Best regards, Krzysztof
On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 03:00:07PM +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". > > A couple of nit-picks below. > > ... > > > +static int isl12022_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > > +{ > > + struct regmap *regmap = dev_get_drvdata(dev); > > + u32 user = 0, val; > > This assignment can be done in the actual case below. > > > + int ret; > > + > > + switch (cmd) { > > + case RTC_VL_READ: > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > + if (ret < 0) > > I always feel uneasy with ' < 0' — Does positive error makes sense? > Is it even possible? OTOH if the entire driver uses this idiom... > oh well, let's make it consistent. > /** * regmap_read() - Read a value from a single register * * @map: Register map to read from * @reg: Register to be read from * @val: Pointer to store read value * * A value of zero will be returned on success, a negative errno will * be returned in error cases. */ > > + return ret; > > user = 0; > > The rationale to avoid potential mistakes in the future in case this function > will be expanded and user will be re-used. > > > + if (val & ISL12022_SR_LBAT85) > > + user |= RTC_VL_BACKUP_LOW; > > + > > + if (val & ISL12022_SR_LBAT75) > > + user |= RTC_VL_BACKUP_EMPTY; > > + > > + return put_user(user, (u32 __user *)arg); > > + > > + default: > > + return -ENOIOCTLCMD; > > + } > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On 13/06/2023 17.25, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 03:00:10PM +0200, Rasmus Villemoes wrote: >> If device tree implies that the chip's IRQ/F_OUT pin is used as a >> clock, expose that in the driver. For now, pretend it is a >> fixed-rate (32kHz) clock; if other use cases appear the driver can be >> updated to provide its own clk_ops etc. >> >> When the clock output is not used on a given board, one can prolong >> the battery life by ensuring that the FOx bits are 0. For the hardware >> I'm currently working on, the RTC draws 1.2uA with the FOx bits at >> their default 0001 value, dropping to 0.88uA when those bits are >> cleared. > > ... > >> +#define ISL12022_INT_FO_MASK GENMASK(3, 0) >> +#define ISL12022_INT_FO_OFF 0x0 >> +#define ISL12022_INT_FO_32K 0x1 > > A nit-pick. Are they decimal or bit fields? -ENOPARSE. A number is a number. Its representation in C code may be decimal or hexadecimal (or...). And sure, 0 and 0x0 are different spellings of the same thing. The data sheet lists the possible values in terms of individual bits, so I suppose I could even do 0b0000 and 0b0001, but that's too unusual (even if perfectly acceptable by gcc). > To me seems like the 0x can be dropped. Can, but won't, a single hex digit is more natural way to represent a four-bit field. >> + ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K); > > Seems too long even for 100 limit. > Maybe: > > ret = regmap_update_bits(regmap, ISL12022_REG_INT, > ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K); Sure. Rasmus
On Wed, Jun 14, 2023 at 12:51:47PM +0200, Rasmus Villemoes wrote: > On 13/06/2023 17.25, Andy Shevchenko wrote: > > On Tue, Jun 13, 2023 at 03:00:10PM +0200, Rasmus Villemoes wrote: ... > >> +#define ISL12022_INT_FO_MASK GENMASK(3, 0) > >> +#define ISL12022_INT_FO_OFF 0x0 > >> +#define ISL12022_INT_FO_32K 0x1 > > > > A nit-pick. Are they decimal or bit fields? > > -ENOPARSE. A number is a number. Its representation in C code may be > decimal or hexadecimal (or...). And sure, 0 and 0x0 are different > spellings of the same thing. The data sheet lists the possible values in > terms of individual bits, so I suppose I could even do 0b0000 and > 0b0001, but that's too unusual (even if perfectly acceptable by gcc). What does datasheet define? bits or the value in a 4-bit field? If bits, why don't you put it that way #define ISL12022_INT_FO_OFF 0 #define ISL12022_INT_FO_32K BIT(0) ? It's a nit-pick, of course, but the nuance is that proposed form might give a hint to the reader, current -- not. > > To me seems like the 0x can be dropped. > > Can, but won't, a single hex digit is more natural way to represent a > four-bit field.
On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: ... > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > + if (ret < 0) > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > Is it even possible? OTOH if the entire driver uses this idiom... > > oh well, let's make it consistent. > > /** > * regmap_read() - Read a value from a single register > * > * @map: Register map to read from > * @reg: Register to be read from > * @val: Pointer to store read value > * > * A value of zero will be returned on success, a negative errno will > * be returned in error cases. > */ I'm not sure what you meant by this. Yes, I know that there is no possibility that regmap API returns positive value. Do you mean that regmap API documentation is unclear? > > > + return ret;
On 14/06/2023 15:16:14+0300, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: > > ... > > > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > > + if (ret < 0) > > > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > > Is it even possible? OTOH if the entire driver uses this idiom... > > > oh well, let's make it consistent. > > > > /** > > * regmap_read() - Read a value from a single register > > * > > * @map: Register map to read from > > * @reg: Register to be read from > > * @val: Pointer to store read value > > * > > * A value of zero will be returned on success, a negative errno will > > * be returned in error cases. > > */ > > I'm not sure what you meant by this. Yes, I know that there is no > possibility that regmap API returns positive value. Do you mean that > regmap API documentation is unclear? No, I mean that you'd have to be clearer as to why you are uneasy with a test for a negative value when the function returns 0 for success and a negative value for an error. Else, this is pure bullying. > > > > > + return ret; > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Jun 14, 2023 at 03:50:36PM +0200, Alexandre Belloni wrote: > On 14/06/2023 15:16:14+0300, Andy Shevchenko wrote: > > On Tue, Jun 13, 2023 at 11:26:51PM +0200, Alexandre Belloni wrote: > > > On 13/06/2023 18:20:56+0300, Andy Shevchenko wrote: > > > > On Tue, Jun 13, 2023 at 03:00:07PM +0200, Rasmus Villemoes wrote: ... > > > > > + ret = regmap_read(regmap, ISL12022_REG_SR, &val); > > > > > + if (ret < 0) > > > > > > > > I always feel uneasy with ' < 0' — Does positive error makes sense? > > > > Is it even possible? OTOH if the entire driver uses this idiom... > > > > oh well, let's make it consistent. > > > > > > /** > > > * regmap_read() - Read a value from a single register > > > * > > > * @map: Register map to read from > > > * @reg: Register to be read from > > > * @val: Pointer to store read value > > > * > > > * A value of zero will be returned on success, a negative errno will > > > * be returned in error cases. > > > */ > > > > I'm not sure what you meant by this. Yes, I know that there is no > > possibility that regmap API returns positive value. Do you mean that > > regmap API documentation is unclear? > > No, I mean that you'd have to be clearer as to why you are uneasy with a > test for a negative value when the function returns 0 for success and a > negative value for an error. Else, this is pure bullying. From the perspective of the code reader, a person, who might have not known all the implementation details of the calls this kind of check will always puzzle about positive value. When reading such a code the following questions are arisen: 1) Can the positive return value be the case? 2) If so, what is the meaning of a such? 3) Why do we not care about it? All this can simply gone if we use ret = foo(...); if (ret) return ret; As it's clear that whatever is non-zero we accept as something to be promoted to the upper layer. I hope this explains my position. > > > > > + return ret;
On 14/06/2023 17.13, Andy Shevchenko wrote: > When reading such a code the following questions are arisen: > 1) Can the positive return value be the case? > 2) If so, what is the meaning of a such? > 3) Why do we not care about it? > > All this can simply gone if we use > > ret = foo(...); > if (ret) > return ret; > > As it's clear that whatever is non-zero we accept as something to be promoted > to the upper layer. I hope this explains my position. But we're in a context (in this case an ->ioctl method) where _our_ caller expects 0/-ESOMETHING, so returning something positive would be a bug - i.e., either way of spelling that if(), the reader must know that foo() also has those 0/-ESOMETHING semantics. I honestly didn't think much about it, but looking at the existing code and the stuff I add, all other places actually do 'if (ret)', so I've updated this site for consistency. Rasmus
On Thu, Jun 15, 2023 at 12:53:24PM +0200, Rasmus Villemoes wrote: > On 14/06/2023 17.13, Andy Shevchenko wrote: > > When reading such a code the following questions are arisen: > > 1) Can the positive return value be the case? > > 2) If so, what is the meaning of a such? > > 3) Why do we not care about it? > > > > All this can simply gone if we use > > > > ret = foo(...); > > if (ret) > > return ret; > > > > As it's clear that whatever is non-zero we accept as something to be promoted > > to the upper layer. I hope this explains my position. > > But we're in a context (in this case an ->ioctl method) where _our_ > caller expects 0/-ESOMETHING, so returning something positive would be a > bug - i.e., either way of spelling that if(), the reader must know that > foo() also has those 0/-ESOMETHING semantics. I totally understand this. But then it's either problem of regmap API documentation or API itself. I.o.w. not _your_ problem. > I honestly didn't think much about it, but looking at the existing code > and the stuff I add, all other places actually do 'if (ret)', so I've > updated this site for consistency. Thank you!
On 13/06/2023 21.06, Krzysztof Kozlowski wrote: > On 13/06/2023 15:00, Rasmus Villemoes wrote: >> 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. >> >> v2 changes: > > Do not attach (thread) your patchsets to some other threads (unrelated > or older versions). This buries them deep in the mailbox and might > interfere with applying entire sets. > Arrgh, I really didn't mean to do that with v3, but I reused the 'git send-email' from my shell history and overlooked that I had that --in-reply-to :( Sorry folks! 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. v2 changes: Patch 2: add Alexandre as maintainer [Rob's bot]. Patch 4: On arm64, <linux/bitfield.h> apparently ends up being included implicitly, but not so on arm [kernel test robot]. Use the more common post-increment in for loops [Andy]. Patch 5: Drop RTC_VL_CLR, just do RTC_VL_READ [Alexandre]. Patch 6: Set the TSE bit to trigger a manual detection, but drop the part reading the SR register and issuing a dev_warn() in case of low battery [Alexandre]. Patch 7: (Hopefully) properly describe the "at most one of interrupts and #clock-cells" [thanks Krzysztof]. Patch 8: Drop a useless dev_warn() in case clearing the FOx bits fails. 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 ioctl 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 | 69 ++++++++++ .../devicetree/bindings/rtc/trivial-rtc.yaml | 2 - drivers/rtc/rtc-isl12022.c | 118 +++++++++++++++++- 3 files changed, 181 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml