[PATCH 0/8] rtc: isl12022: battery backup voltage and clock support

[PATCH 0/8] rtc: isl12022: battery backup voltage and clock support

[Rasmus Villemoes]

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

-- 
2.37.2

[PATCH 1/8] rtc: isl12022: remove wrong warning for low battery level

[Rasmus Villemoes]

There are multiple problems with this warning.

First of all, it triggers way too often, in fact nearly on every boot,
because the SR_LBAT85/SR_LBAT75 bits have another meaning when in
battery backup mode. Quoting from the data sheet:

  LOW BATTERY INDICATOR 85% BIT (LBAT85)

  In Normal Mode (VDD), this bit indicates when the battery level has
  dropped below the pre-selected trip levels. [...] The LBAT85
  detection happens automatically once every minute when seconds
  register reaches 59.

  In Battery Mode (VBAT), this bit indicates the device has entered
  into battery mode by polling once every 10 minutes. The LBAT85
  detection happens automatically once when the minute register
  reaches x9h or x0h minutes.

Similar wording applies to the LBAT75 bit.

This means that if the device is powered off for more than 10 minutes,
the LBAT85 bit is guaranteed to be set. Upon power-on, unless we're
close enough to the end of a minute and/or the boot is slow enough
that the second register passes 59, the LBAT85 bit is still set when
the kernel (or early userspace) reads the RTC to set the system's
wallclock time.

Another minor problem is with the bit logic. If the 75% level is
reached, logically we're also below 85%, so both bits would most
likely be set. So even if the battery is below 75%, the warning would
still say "voltage dropped below 85%".

A third problem is that the driver and current DT binding offer no way
to indicate the nominal battery level and/or settings of the Battery
Level Monitor Trip Bits. Since the default value of the VB85TP[2:0] and
VB75TP[2:0] bits are 000, this means the actual setting of the
LBAT85/LBAT75 bits in VDD mode doesn't happen until the battery is below
2.125V/1.875V, which for a standard 3V battery is way too late.

A fourth problem is emitting this warning from ->read_time:
util-linux' hwclock will, in the absence of support for getting an
interrupt when the seconds counter is updated, issue
ioctl(RTC_RD_TIME) in a busy-loop until it sees a change in the
seconds field. In that case, if the battery low bits are set (either
genuinely, more than a minute after boot, due to the battery actually
being low, or as above, bogusly shortly after boot), the kernel log is
swamped with hundreds of identical warnings.

Subsequent patches will add such bindings and driver support, and also
proper support for RTC_VL_READ. For now, remove the broken warning.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/rtc/rtc-isl12022.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index e68a79b5e00e..ebd66b835cef 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -141,12 +141,6 @@ static int isl12022_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (ret)
 		return ret;
 
-	if (buf[ISL12022_REG_SR] & (ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75)) {
-		dev_warn(dev,
-			 "voltage dropped below %u%%, date and time is not reliable.\n",
-			 buf[ISL12022_REG_SR] & ISL12022_SR_LBAT85 ? 85 : 75);
-	}
-
 	dev_dbg(dev,
 		"raw data is sec=%02x, min=%02x, hr=%02x, mday=%02x, mon=%02x, year=%02x, wday=%02x, sr=%02x, int=%02x",
 		buf[ISL12022_REG_SC],
-- 
2.37.2

[PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rasmus Villemoes]

Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
intersil,isl12022.yaml file, in preparation for adding more bindings.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 2 files changed, 42 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
new file mode 100644
index 000000000000..899c5edc72e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/intersil,isl12022.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intersil ISL12022 Real-time Clock
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    const: isil,isl12022
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@6f {
+            compatible = "isil,isl12022";
+            reg = <0x6f>;
+            interrupts-extended = <&gpio1 5 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index a3603e638c37..b062c64266a6 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -45,8 +45,6 @@ properties:
       - isil,isl1208
       # Intersil ISL1218 Low Power RTC with Battery Backed SRAM
       - isil,isl1218
-      # Intersil ISL12022 Real-time Clock
-      - isil,isl12022
       # Loongson-2K Socs/LS7A bridge Real-time Clock
       - loongson,ls2x-rtc
       # Real Time Clock Module with I2C-Bus
-- 
2.37.2

[PATCH 3/8] dt-bindings: rtc: isl12022: add bindings for battery alarm trip levels

[Rasmus Villemoes]

The isl12022 has a built-in support for monitoring the voltage of the
backup battery, and setting bits in the status register when that
voltage drops below two predetermined levels (usually 85% and 75% of
the nominal voltage). However, since it can operate at wide range of
battery voltages (2.5V - 5.5V), one must configure those trip levels
according to which battery is used on a given board.

Add bindings for defining these two trip levels. While the register
and bit names suggest that they should correspond to 85% and 75% of
the nominal battery voltage, the data sheet also says

  There are total of 7 levels that could be selected for the first
  alarm. Any of the of levels could be selected as the first alarm
  with no reference as to nominal Battery voltage level.

Hence this provides the hardware designer the ability to choose values
based on the discharge characteristics of the battery chosen for the
given product, rather than just having one battery-microvolt property
and having the driver choose levels close to 0.85/0.75 times that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/rtc/intersil,isl12022.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
index 899c5edc72e4..1e85a9c8945b 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -19,6 +19,18 @@ properties:
   interrupts:
     maxItems: 1
 
+  isil,trip-level85-microvolt:
+    description: |
+      The battery voltage at which the first alarm should trigger
+      (normally ~85% of nominal V_BAT).
+    enum: [2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000]
+
+  isil,trip-level75-microvolt:
+    description: |
+      The battery voltage at which the second alarm should trigger
+      (normally ~75% of nominal V_BAT).
+    enum: [1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000]
+
 required:
   - compatible
   - reg
@@ -36,6 +48,8 @@ examples:
             compatible = "isil,isl12022";
             reg = <0x6f>;
             interrupts-extended = <&gpio1 5 IRQ_TYPE_LEVEL_LOW>;
+            isil,trip-level85-microvolt = <2550000>;
+            isil,trip-level75-microvolt = <2250000>;
         };
     };
 
-- 
2.37.2

[PATCH 4/8] rtc: isl12022: add support for trip level DT bindings

[Rasmus Villemoes]

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.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/rtc/rtc-isl12022.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index ebd66b835cef..cb8f1d92e116 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -31,6 +31,8 @@
 #define ISL12022_REG_SR		0x07
 #define ISL12022_REG_INT	0x08
 
+#define ISL12022_REG_PWR_VBAT	0x0a
+
 #define ISL12022_REG_BETA	0x0d
 #define ISL12022_REG_TEMP_L	0x28
 
@@ -42,6 +44,9 @@
 
 #define ISL12022_INT_WRTC	(1 << 6)
 
+#define ISL12022_REG_VB85_MASK	GENMASK(5, 3)
+#define ISL12022_REG_VB75_MASK	GENMASK(2, 0)
+
 #define ISL12022_BETA_TSE	(1 << 7)
 
 static umode_t isl12022_hwmon_is_visible(const void *data,
@@ -209,6 +214,35 @@ static const struct regmap_config regmap_config = {
 	.use_single_write = true,
 };
 
+static const u32 trip_level85[] = { 2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000 };
+static const u32 trip_level75[] = { 1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000 };
+
+static void isl12022_set_trip_levels(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 level85 = 0, level75 = 0;
+	int ret, x85, x75;
+	u8 val, mask;
+
+	device_property_read_u32(dev, "isil,trip-level85-microvolt", &level85);
+	device_property_read_u32(dev, "isil,trip-level75-microvolt", &level75);
+
+	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;
+
+	val = FIELD_PREP(ISL12022_REG_VB85_MASK, x85) | FIELD_PREP(ISL12022_REG_VB75_MASK, x75);
+	mask = ISL12022_REG_VB85_MASK | ISL12022_REG_VB75_MASK;
+
+	ret = regmap_update_bits(regmap, ISL12022_REG_PWR_VBAT, mask, val);
+	if (ret)
+		dev_warn(dev, "unable to set battery alarm levels: %d\n", ret);
+}
+
 static int isl12022_probe(struct i2c_client *client)
 {
 	struct rtc_device *rtc;
@@ -225,6 +259,7 @@ static int isl12022_probe(struct i2c_client *client)
 
 	dev_set_drvdata(&client->dev, regmap);
 
+	isl12022_set_trip_levels(&client->dev);
 	isl12022_hwmon_register(&client->dev);
 
 	rtc = devm_rtc_allocate_device(&client->dev);
-- 
2.37.2

[PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Rasmus Villemoes]

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);
+
+	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

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

[Rasmus Villemoes]

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);
+		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);
+	}
 }
 
 static int isl12022_probe(struct i2c_client *client)
-- 
2.37.2

[PATCH 7/8] dt-bindings: rtc: isl12022: add #clock-cells property

[Rasmus Villemoes]

The isl12022 has a dual-purpose irq/f_out pin, which can either be
used as an interrupt or clock output.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

I've tried to express the fact that the pin can either be used
for interrupts or as a clock source, so that at most one of
"interrupts" and "#clock-cells" can be present, but I don't really
have any idea if this is the proper way to do that.

 .../devicetree/bindings/rtc/intersil,isl12022.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
index 1e85a9c8945b..345abed9234f 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -19,6 +19,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  '#clock-cells':
+    const: 0
+
   isil,trip-level85-microvolt:
     description: |
       The battery voltage at which the first alarm should trigger
@@ -35,6 +38,13 @@ required:
   - compatible
   - reg
 
+if:
+  properties:
+    '#clock-cells'
+then:
+  properties:
+    interrupts: false
+
 unevaluatedProperties: false
 
 examples:
-- 
2.37.2

[PATCH 8/8] rtc: isl12022: implement support for the #clock-cells DT property

[Rasmus Villemoes]

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.

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 690dbb446d1a..0054300b744b 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
@@ -43,6 +44,9 @@
 #define ISL12022_SR_LBAT75	(1 << 1)
 
 #define ISL12022_INT_WRTC	(1 << 6)
+#define ISL12022_INT_FO_MASK	GENMASK(3, 0)
+#define ISL12022_INT_FO_OFF	0x0
+#define ISL12022_INT_FO_32K	0x1
 
 #define ISL12022_REG_VB85_MASK	GENMASK(5, 3)
 #define ISL12022_REG_VB75_MASK	GENMASK(2, 0)
@@ -255,6 +259,38 @@ static const struct regmap_config regmap_config = {
 	.use_single_write = true,
 };
 
+static int isl12022_register_clock(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	struct clk_hw *hw;
+	int ret;
+
+	if (!device_property_present(dev, "#clock-cells")) {
+		/*
+		 * Disabling the F_OUT pin reduces the power
+		 * consumption in battery mode by ~25%.
+		 */
+		ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK,
+					 ISL12022_INT_FO_OFF);
+		if (ret)
+			dev_warn(dev, "failed to clear FOx bits in INT register: %d", ret);
+		return 0;
+	}
+
+	/*
+	 * For now, only support a fixed clock of 32768Hz (the reset default).
+	 */
+	ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K);
+	if (ret)
+		return ret;
+
+	hw = devm_clk_hw_register_fixed_rate(dev, "isl12022_32kHz", NULL, 0, 32768);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+}
+
 static const u32 trip_level85[] = { 2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000 };
 static const u32 trip_level75[] = { 1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000 };
 
@@ -305,6 +341,7 @@ static int isl12022_probe(struct i2c_client *client)
 {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
+	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -317,6 +354,10 @@ static int isl12022_probe(struct i2c_client *client)
 
 	dev_set_drvdata(&client->dev, regmap);
 
+	ret = isl12022_register_clock(&client->dev);
+	if (ret)
+		return ret;
+
 	isl12022_set_trip_levels(&client->dev);
 	isl12022_hwmon_register(&client->dev);
 
-- 
2.37.2

Re: [PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rob Herring]

On Mon, 12 Jun 2023 13:30:52 +0200, Rasmus Villemoes wrote:
> Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
> intersil,isl12022.yaml file, in preparation for adding more bindings.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  2 files changed, 42 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 62, in <module>
    ret |= check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 31, in check_doc
    for error in sorted(dtschema.DTValidator.iter_schema_errors(testtree), key=lambda e: e.linecol):
  File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line 736, in iter_schema_errors
    cls.annotate_error(error, meta_schema, error.schema_path)
  File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line 712, in annotate_error
    schema = schema[p]
KeyError: 'type'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml: 'maintainers' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230612113059.247275-3-linux@rasmusvillemoes.dk

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Rasmus Villemoes]

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

Re: [PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rasmus Villemoes]

On 12/06/2023 14.26, Rob Herring wrote:
> 
> On Mon, 12 Jun 2023 13:30:52 +0200, Rasmus Villemoes wrote:
>> Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
>> intersil,isl12022.yaml file, in preparation for adding more bindings.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
>>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>>  2 files changed, 42 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml: 'maintainers' is a required property
> 	hint: Metaschema for devicetree binding documentation
> 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#

Hm ok. Can/should I copy the value from the trivial-rtc.yaml? Alexandre,
would that be ok with you?

Is there some simple way to do that dt_binding_check for a single file
or just a few? It seems to take forever to run on the whole tree.

Rasmus

Re: [PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Alexandre Belloni]

On 12/06/2023 14:36:03+0200, Rasmus Villemoes wrote:
> On 12/06/2023 14.26, Rob Herring wrote:
> > 
> > On Mon, 12 Jun 2023 13:30:52 +0200, Rasmus Villemoes wrote:
> >> Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
> >> intersil,isl12022.yaml file, in preparation for adding more bindings.
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
> >>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
> >>  2 files changed, 42 insertions(+), 2 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> >>
> > 
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > 
> > yamllint warnings/errors:
> > 
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml: 'maintainers' is a required property
> > 	hint: Metaschema for devicetree binding documentation
> > 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
> 
> Hm ok. Can/should I copy the value from the trivial-rtc.yaml? Alexandre,
> would that be ok with you?
> 

Yes

> Is there some simple way to do that dt_binding_check for a single file
> or just a few? It seems to take forever to run on the whole tree.
> 

The kernel documentation has this example:

make dt_binding_check DT_SCHEMA_FILES=trivial-devices.yaml


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rob Herring]

On Mon, Jun 12, 2023 at 02:36:03PM +0200, Rasmus Villemoes wrote:
> On 12/06/2023 14.26, Rob Herring wrote:
> > 
> > On Mon, 12 Jun 2023 13:30:52 +0200, Rasmus Villemoes wrote:
> >> Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
> >> intersil,isl12022.yaml file, in preparation for adding more bindings.
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
> >>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
> >>  2 files changed, 42 insertions(+), 2 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> >>
> > 
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > 
> > yamllint warnings/errors:
> > 
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml: 'maintainers' is a required property
> > 	hint: Metaschema for devicetree binding documentation
> > 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
> 
> Hm ok. Can/should I copy the value from the trivial-rtc.yaml? Alexandre,
> would that be ok with you?

Alexandre agreed, but in general the maintainer here should be someone 
that has the h/w and/or cares about it, not subsystem maintainers.

Rob

Re: [PATCH 4/8] rtc: isl12022: add support for trip level DT bindings

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Andy Shevchenko]

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

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Alexandre Belloni]

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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/8] rtc: isl12022: add support for trip level DT bindings

[kernel test robot]

Hi Rasmus,

kernel test robot noticed the following build errors:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on robh/for-next linus/master v6.4-rc6 next-20230609]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rasmus-Villemoes/rtc-isl12022-remove-wrong-warning-for-low-battery-level/20230612-211434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20230612113059.247275-5-linux%40rasmusvillemoes.dk
patch subject: [PATCH 4/8] rtc: isl12022: add support for trip level DT bindings
config: arm-randconfig-r012-20230612 (https://download.01.org/0day-ci/archive/20230613/202306130116.8PIDa21J-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        git remote add abelloni https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
        git fetch abelloni rtc-next
        git checkout abelloni/rtc-next
        b4 shazam https://lore.kernel.org/r/20230612113059.247275-5-linux@rasmusvillemoes.dk
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/rtc/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306130116.8PIDa21J-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/rtc/rtc-isl12022.c:238:8: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     238 |         val = FIELD_PREP(ISL12022_REG_VB85_MASK, x85) | FIELD_PREP(ISL12022_REG_VB75_MASK, x75);
         |               ^
   1 error generated.


vim +/FIELD_PREP +238 drivers/rtc/rtc-isl12022.c

   219	
   220	static void isl12022_set_trip_levels(struct device *dev)
   221	{
   222		struct regmap *regmap = dev_get_drvdata(dev);
   223		u32 level85 = 0, level75 = 0;
   224		int ret, x85, x75;
   225		u8 val, mask;
   226	
   227		device_property_read_u32(dev, "isil,trip-level85-microvolt", &level85);
   228		device_property_read_u32(dev, "isil,trip-level75-microvolt", &level75);
   229	
   230		for (x85 = 0; x85 < ARRAY_SIZE(trip_level85) - 1; ++x85)
   231			if (level85 <= trip_level85[x85])
   232				break;
   233	
   234		for (x75 = 0; x75 < ARRAY_SIZE(trip_level75) - 1; ++x75)
   235			if (level75 <= trip_level75[x75])
   236				break;
   237	
 > 238		val = FIELD_PREP(ISL12022_REG_VB85_MASK, x85) | FIELD_PREP(ISL12022_REG_VB75_MASK, x75);
   239		mask = ISL12022_REG_VB85_MASK | ISL12022_REG_VB75_MASK;
   240	
   241		ret = regmap_update_bits(regmap, ISL12022_REG_PWR_VBAT, mask, val);
   242		if (ret)
   243			dev_warn(dev, "unable to set battery alarm levels: %d\n", ret);
   244	}
   245	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/8] rtc: isl12022: add support for trip level DT bindings

[kernel test robot]

Hi Rasmus,

kernel test robot noticed the following build errors:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on robh/for-next linus/master v6.4-rc6 next-20230609]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rasmus-Villemoes/rtc-isl12022-remove-wrong-warning-for-low-battery-level/20230612-211434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20230612113059.247275-5-linux%40rasmusvillemoes.dk
patch subject: [PATCH 4/8] rtc: isl12022: add support for trip level DT bindings
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230613/202306130201.ai7ck1mx-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add abelloni https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
        git fetch abelloni rtc-next
        git checkout abelloni/rtc-next
        b4 shazam https://lore.kernel.org/r/20230612113059.247275-5-linux@rasmusvillemoes.dk
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306130201.ai7ck1mx-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/rtc/rtc-isl12022.c: In function 'isl12022_set_trip_levels':
>> drivers/rtc/rtc-isl12022.c:238:15: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     238 |         val = FIELD_PREP(ISL12022_REG_VB85_MASK, x85) | FIELD_PREP(ISL12022_REG_VB75_MASK, x75);
         |               ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_PREP +238 drivers/rtc/rtc-isl12022.c

   219	
   220	static void isl12022_set_trip_levels(struct device *dev)
   221	{
   222		struct regmap *regmap = dev_get_drvdata(dev);
   223		u32 level85 = 0, level75 = 0;
   224		int ret, x85, x75;
   225		u8 val, mask;
   226	
   227		device_property_read_u32(dev, "isil,trip-level85-microvolt", &level85);
   228		device_property_read_u32(dev, "isil,trip-level75-microvolt", &level75);
   229	
   230		for (x85 = 0; x85 < ARRAY_SIZE(trip_level85) - 1; ++x85)
   231			if (level85 <= trip_level85[x85])
   232				break;
   233	
   234		for (x75 = 0; x75 < ARRAY_SIZE(trip_level75) - 1; ++x75)
   235			if (level75 <= trip_level75[x75])
   236				break;
   237	
 > 238		val = FIELD_PREP(ISL12022_REG_VB85_MASK, x85) | FIELD_PREP(ISL12022_REG_VB75_MASK, x75);
   239		mask = ISL12022_REG_VB85_MASK | ISL12022_REG_VB75_MASK;
   240	
   241		ret = regmap_update_bits(regmap, ISL12022_REG_PWR_VBAT, mask, val);
   242		if (ret)
   243			dev_warn(dev, "unable to set battery alarm levels: %d\n", ret);
   244	}
   245	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Rasmus Villemoes]

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

Re: [PATCH 7/8] dt-bindings: rtc: isl12022: add #clock-cells property

[Krzysztof Kozlowski]

On 12/06/2023 13:30, Rasmus Villemoes wrote:
> The isl12022 has a dual-purpose irq/f_out pin, which can either be
> used as an interrupt or clock output.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---


> +
>    isil,trip-level85-microvolt:
>      description: |
>        The battery voltage at which the first alarm should trigger
> @@ -35,6 +38,13 @@ required:
>    - compatible
>    - reg
>  
> +if:
> +  properties:
> +    '#clock-cells'
> +then:
> +  properties:
> +    interrupts: false

https://elixir.bootlin.com/linux/v6.2-rc3/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174



Best regards,
Krzysztof

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

[Rasmus Villemoes]

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

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

[Rasmus Villemoes]

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

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Rasmus Villemoes]

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

Re: [PATCH 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rasmus Villemoes]

On 12/06/2023 16.20, Rob Herring wrote:
> On Mon, Jun 12, 2023 at 02:36:03PM +0200, Rasmus Villemoes wrote:
>> On 12/06/2023 14.26, Rob Herring wrote:
>>>
>>> On Mon, 12 Jun 2023 13:30:52 +0200, Rasmus Villemoes wrote:
>>>> Move the isil,isl12022 RTC bindings from trivial-rtc.yaml into its own
>>>> intersil,isl12022.yaml file, in preparation for adding more bindings.
>>>>
>>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>>> ---
>>>>  .../bindings/rtc/intersil,isl12022.yaml       | 42 +++++++++++++++++++
>>>>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>>>>  2 files changed, 42 insertions(+), 2 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>>>
>>>
>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>
>>> yamllint warnings/errors:
>>>
>>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml: 'maintainers' is a required property
>>> 	hint: Metaschema for devicetree binding documentation
>>> 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
>>
>> Hm ok. Can/should I copy the value from the trivial-rtc.yaml? Alexandre,
>> would that be ok with you?
> 
> Alexandre agreed, but in general the maintainer here should be someone 
> that has the h/w and/or cares about it, not subsystem maintainers.

OK. Right now I have the hardware and care about it because I've been
hired to work on it.

Incidentally, my backlog for this project/product also contains
upstreaming of a new gpiochip driver and DT bindings. I assume I should
just list myself as maintainer in that new .yaml file, even if I can't
promise to have time to review changes and/or even hardware to test on
12 months from now.

Rasmus

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

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 5/8] rtc: isl12022: implement RTC_VL_READ and RTC_VL_CLR ioctls

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 0/8] rtc: isl12022: battery backup voltage and clock support

[Rasmus Villemoes]

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

-- 
2.37.2

[PATCH v2 1/8] rtc: isl12022: remove wrong warning for low battery level

[Rasmus Villemoes]

There are multiple problems with this warning.

First of all, it triggers way too often, in fact nearly on every boot,
because the SR_LBAT85/SR_LBAT75 bits have another meaning when in
battery backup mode. Quoting from the data sheet:

  LOW BATTERY INDICATOR 85% BIT (LBAT85)

  In Normal Mode (VDD), this bit indicates when the battery level has
  dropped below the pre-selected trip levels. [...] The LBAT85
  detection happens automatically once every minute when seconds
  register reaches 59.

  In Battery Mode (VBAT), this bit indicates the device has entered
  into battery mode by polling once every 10 minutes. The LBAT85
  detection happens automatically once when the minute register
  reaches x9h or x0h minutes.

Similar wording applies to the LBAT75 bit.

This means that if the device is powered off for more than 10 minutes,
the LBAT85 bit is guaranteed to be set. Upon power-on, unless we're
close enough to the end of a minute and/or the boot is slow enough
that the second register passes 59, the LBAT85 bit is still set when
the kernel (or early userspace) reads the RTC to set the system's
wallclock time.

Another minor problem is with the bit logic. If the 75% level is
reached, logically we're also below 85%, so both bits would most
likely be set. So even if the battery is below 75%, the warning would
still say "voltage dropped below 85%".

A third problem is that the driver and current DT binding offer no way
to indicate the nominal battery level and/or settings of the Battery
Level Monitor Trip Bits. Since the default value of the VB85TP[2:0] and
VB75TP[2:0] bits are 000, this means the actual setting of the
LBAT85/LBAT75 bits in VDD mode doesn't happen until the battery is below
2.125V/1.875V, which for a standard 3V battery is way too late.

A fourth problem is emitting this warning from ->read_time:
util-linux' hwclock will, in the absence of support for getting an
interrupt when the seconds counter is updated, issue
ioctl(RTC_RD_TIME) in a busy-loop until it sees a change in the
seconds field. In that case, if the battery low bits are set (either
genuinely, more than a minute after boot, due to the battery actually
being low, or as above, bogusly shortly after boot), the kernel log is
swamped with hundreds of identical warnings.

Subsequent patches will add such bindings and driver support, and also
proper support for RTC_VL_READ. For now, remove the broken warning.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/rtc/rtc-isl12022.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index e68a79b5e00e..ebd66b835cef 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -141,12 +141,6 @@ static int isl12022_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (ret)
 		return ret;
 
-	if (buf[ISL12022_REG_SR] & (ISL12022_SR_LBAT85 | ISL12022_SR_LBAT75)) {
-		dev_warn(dev,
-			 "voltage dropped below %u%%, date and time is not reliable.\n",
-			 buf[ISL12022_REG_SR] & ISL12022_SR_LBAT85 ? 85 : 75);
-	}
-
 	dev_dbg(dev,
 		"raw data is sec=%02x, min=%02x, hr=%02x, mday=%02x, mon=%02x, year=%02x, wday=%02x, sr=%02x, int=%02x",
 		buf[ISL12022_REG_SC],
-- 
2.37.2

[PATCH v2 2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file

[Rasmus Villemoes]