[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

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

[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

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

[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

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

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

[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

[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

[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

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

[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

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

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       | 45 +++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 2 files changed, 45 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..7c1e638d657a
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -0,0 +1,45 @@
+# 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
+
+maintainers:
+  - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+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

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

[Krzysztof Kozlowski]

On 13/06/2023 15:00, 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       | 45 +++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  2 files changed, 45 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 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 7c1e638d657a..d5d3a687a34d 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -22,6 +22,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
@@ -39,6 +51,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

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

[Krzysztof Kozlowski]

On 13/06/2023 15:00, Rasmus Villemoes wrote:
> 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 7c1e638d657a..d5d3a687a34d 100644
> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> @@ -22,6 +22,18 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  isil,trip-level85-microvolt:

Why encoding level85 in the property name? Your commit msg (datasheet)
suggests this is quite flexible, so why it cannot be just list of two
trip levels - for first and second interrupt?

> +    description: |

Do not need '|' unless you need to preserve formatting.

Best regards,
Krzysztof

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

[Rasmus Villemoes]

On 13/06/2023 21.09, Krzysztof Kozlowski wrote:
> On 13/06/2023 15:00, Rasmus Villemoes wrote:
>> 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 7c1e638d657a..d5d3a687a34d 100644
>> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> @@ -22,6 +22,18 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> +  isil,trip-level85-microvolt:
> 
> Why encoding level85 in the property name? Your commit msg (datasheet)
> suggests this is quite flexible, so why it cannot be just list of two
> trip levels - for first and second interrupt?

Yeah, so I did consider just making it a two-element array
isil,trip-levels-microvolt. But then I didn't know how to express the
enum constraint, i.e. that the first must be one of the 2125000, ...,
4675000 values and the second one of the 1875000, ..., 4125000 ones. Is
that possible, without providing a list of 49 possible pairs? Or is it
sufficient to just write this out in prose?

I'm also happy to use other names for these. I just chose to use the 85
and 75 nomenclature because that matches the field names.

>> +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK.

Rasmus

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

[Krzysztof Kozlowski]

On 13/06/2023 21:51, Rasmus Villemoes wrote:
>>> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> index 7c1e638d657a..d5d3a687a34d 100644
>>> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> @@ -22,6 +22,18 @@ properties:
>>>    interrupts:
>>>      maxItems: 1
>>>  
>>> +  isil,trip-level85-microvolt:
>>
>> Why encoding level85 in the property name? Your commit msg (datasheet)
>> suggests this is quite flexible, so why it cannot be just list of two
>> trip levels - for first and second interrupt?
> 
> Yeah, so I did consider just making it a two-element array
> isil,trip-levels-microvolt. But then I didn't know how to express the
> enum constraint, i.e. that the first must be one of the 2125000, ...,
> 4675000 values and the second one of the 1875000, ..., 4125000 ones. Is
> that possible, without providing a list of 49 possible pairs? Or is it
> sufficient to just write this out in prose?

items:
  - enum: [ a, b, c ]
  - enum: [ f, d, e ]


Best regards,
Krzysztof

[PATCH v2 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 | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index ebd66b835cef..50bbd1fefad8 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
@@ -31,6 +32,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 +45,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 +215,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 +260,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 v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index 50bbd1fefad8..bf0d65643897 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -204,7 +204,33 @@ 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_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 user = 0, val;
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		ret = regmap_read(regmap, ISL12022_REG_SR, &val);
+		if (ret < 0)
+			return ret;
+
+		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;
+	}
+}
+
 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

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Alexandre Belloni]

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

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

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Alexandre Belloni]

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

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

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Rasmus Villemoes]

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

Re: [PATCH v2 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 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. Empirically, this does not immediately update the bits in
the status register (i.e., an immediate read of SR after this write
can still show stale values), but the update is done after a few
milliseconds, so certainly before the RTC device gets registered and
userspace has a chance of doing the ioctl() on this device.

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

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index bf0d65643897..44603169e575 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -268,6 +268,16 @@ static void isl12022_set_trip_levels(struct device *dev)
 	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);
+
+	/*
+	 * Force a write of the TSE bit in the BETA register, in order
+	 * to trigger an update of the LBAT75 and LBAT85 bits in the
+	 * status register. In battery backup mode, those bits have
+	 * another meaning, so without this, they may contain stale
+	 * values for up to a minute after power-on.
+	 */
+	regmap_write_bits(regmap, ISL12022_REG_BETA,
+			  ISL12022_BETA_TSE, ISL12022_BETA_TSE);
 }
 
 static int isl12022_probe(struct i2c_client *client)
-- 
2.37.2

[PATCH v2 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>
---
 .../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 d5d3a687a34d..a9ef68b5fdcd 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -11,6 +11,13 @@ maintainers:
 
 allOf:
   - $ref: rtc.yaml#
+  # If #clock-cells is present, interrupts must not be present
+  - if:
+      required:
+        - '#clock-cells'
+    then:
+      properties:
+        interrupts: false
 
 properties:
   compatible:
@@ -22,6 +29,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  '#clock-cells':
+    const: 0
+
   isil,trip-level85-microvolt:
     description: |
       The battery voltage at which the first alarm should trigger
-- 
2.37.2

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

[Krzysztof Kozlowski]

On 13/06/2023 15:00, 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>
> ---
>  .../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 d5d3a687a34d..a9ef68b5fdcd 100644
> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
> @@ -11,6 +11,13 @@ maintainers:
>  
>  allOf:
>    - $ref: rtc.yaml#
> +  # If #clock-cells is present, interrupts must not be present
> +  - if:
> +      required:
> +        - '#clock-cells'
> +    then:
> +      properties:
> +        interrupts: false

Entire allOf block should be like in example-schema, so before
unevaluatedProperties. Please put it in correct place in your first
patch so here it does not have to be moved.

Best regards,
Krzysztof

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

[Rasmus Villemoes]

On 13/06/2023 21.10, Krzysztof Kozlowski wrote:
> On 13/06/2023 15:00, Rasmus Villemoes wrote:

>> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> index d5d3a687a34d..a9ef68b5fdcd 100644
>> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>> @@ -11,6 +11,13 @@ maintainers:
>>  
>>  allOf:
>>    - $ref: rtc.yaml#
>> +  # If #clock-cells is present, interrupts must not be present
>> +  - if:
>> +      required:
>> +        - '#clock-cells'
>> +    then:
>> +      properties:
>> +        interrupts: false
> 
> Entire allOf block should be like in example-schema, so before
> unevaluatedProperties. Please put it in correct place in your first
> patch so here it does not have to be moved.
>

OK. That first patch was basically a copy-paste of c690048ed59b, and
e.g. ingenic,rtc.yaml has a similar non-trivial allOf block between
maintainers and properties. Is there somehow I could have known it
should be right before unevaluatedProperties?

Rasmus

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

[Krzysztof Kozlowski]

On 13/06/2023 22:25, Rasmus Villemoes wrote:
> On 13/06/2023 21.10, Krzysztof Kozlowski wrote:
>> On 13/06/2023 15:00, Rasmus Villemoes wrote:
> 
>>> diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> index d5d3a687a34d..a9ef68b5fdcd 100644
>>> --- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> +++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
>>> @@ -11,6 +11,13 @@ maintainers:
>>>  
>>>  allOf:
>>>    - $ref: rtc.yaml#
>>> +  # If #clock-cells is present, interrupts must not be present
>>> +  - if:
>>> +      required:
>>> +        - '#clock-cells'
>>> +    then:
>>> +      properties:
>>> +        interrupts: false
>>
>> Entire allOf block should be like in example-schema, so before
>> unevaluatedProperties. Please put it in correct place in your first
>> patch so here it does not have to be moved.
>>
> 
> OK. That first patch was basically a copy-paste of c690048ed59b, and
> e.g. ingenic,rtc.yaml has a similar non-trivial allOf block between
> maintainers and properties. Is there somehow I could have known it
> should be right before unevaluatedProperties?

The trivial - with a $ref - we keep often at the top. But once it starts
growing, should be at the bottom. Since you know it will grow, just put
it at the bottom (not total bottom, but like in example-schema, so after
required:).

Best regards,
Krzysztof

[PATCH v2 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 | 40 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index 44603169e575..3e69f1a5dc12 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -10,6 +10,7 @@
 
 #include <linux/bcd.h>
 #include <linux/bitfield.h>
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
@@ -44,6 +45,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)
@@ -241,6 +245,37 @@ 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%.
+		 */
+		regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK,
+				   ISL12022_INT_FO_OFF);
+
+		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", 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 };
 
@@ -284,6 +319,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;
@@ -296,6 +332,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 v2 8/8] rtc: isl12022: implement support for the #clock-cells DT property

[Andy Shevchenko]

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

?

-- 
With Best Regards,
Andy Shevchenko

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

[Rasmus Villemoes]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[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-20230613]
[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/20230613-210308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20230613130011.305589-9-linux%40rasmusvillemoes.dk
patch subject: [PATCH v2 8/8] rtc: isl12022: implement support for the #clock-cells DT property
config: hexagon-randconfig-r041-20230612 (https://download.01.org/0day-ci/archive/20230614/202306140237.9IMWa7cz-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
        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/20230613130011.305589-9-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=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

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/202306140237.9IMWa7cz-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: __clk_hw_register_fixed_rate
   >>> referenced by rtc-isl12022.c:272 (drivers/rtc/rtc-isl12022.c:272)
   >>>               drivers/rtc/rtc-isl12022.o:(isl12022_probe) in archive vmlinux.a
   >>> referenced by rtc-isl12022.c:272 (drivers/rtc/rtc-isl12022.c:272)
   >>>               drivers/rtc/rtc-isl12022.o:(isl12022_probe) in archive vmlinux.a

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

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

[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-20230613]
[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/20230613-210308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20230613130011.305589-9-linux%40rasmusvillemoes.dk
patch subject: [PATCH v2 8/8] rtc: isl12022: implement support for the #clock-cells DT property
config: i386-randconfig-i012-20230612 (https://download.01.org/0day-ci/archive/20230614/202306141318.xPzubJXo-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
        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/20230613130011.305589-9-linux@rasmusvillemoes.dk
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

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/202306141318.xPzubJXo-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__clk_hw_register_fixed_rate" [drivers/rtc/rtc-isl12022.ko] undefined!
>> ERROR: modpost: "of_clk_hw_simple_get" [drivers/rtc/rtc-isl12022.ko] undefined!
>> ERROR: modpost: "devm_of_clk_add_hw_provider" [drivers/rtc/rtc-isl12022.ko] undefined!

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

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

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

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

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

[Rasmus Villemoes]

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

[PATCH v3 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.

v3 changes:

Patch 2: move the allOf block further down, add R-b [Krzysztof]

Patch 3: change to a single property with two values [Krzysztof]

Patch 4: adjust implementation accordingly

Patch 5: move initialization of 'user' variable inside switch case,
use 'if (ret)' instead of 'if (ret < 0)' for consistency within the
driver [Andy]

Patch 7: semantically identical to v2, just context changes due to
changes in 2/8 and 3/8

Patch 8: only do the clock registration when CONFIG_COMMON_CLK [kernel
test robot]

v2: https://lore.kernel.org/lkml/20230613130011.305589-1-linux@rasmusvillemoes.dk/
v1: https://lore.kernel.org/lkml/20230612113059.247275-1-linux@rasmusvillemoes.dk/

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 binding
  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       |  64 +++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |   2 -
 drivers/rtc/rtc-isl12022.c                    | 126 +++++++++++++++++-
 3 files changed, 184 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml

-- 
2.37.2

[PATCH v3 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 v3 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.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../bindings/rtc/intersil,isl12022.yaml       | 45 +++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 2 files changed, 45 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..054d3fc649ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -0,0 +1,45 @@
+# 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
+
+maintainers:
+  - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+properties:
+  compatible:
+    const: isil,isl12022
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: rtc.yaml#
+
+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 v3 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       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
index 054d3fc649ba..3c6c07aaa78f 100644
--- a/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
+++ b/Documentation/devicetree/bindings/rtc/intersil,isl12022.yaml
@@ -19,6 +19,14 @@ properties:
   interrupts:
     maxItems: 1
 
+  isil,battery-trip-levels-microvolt:
+    description:
+      The battery voltages at which the first alarm and second alarm
+      should trigger (normally ~85% and ~75% of nominal V_BAT).
+    items:
+      - enum: [2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000]
+      - enum: [1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000]
+
 required:
   - compatible
   - reg
@@ -39,6 +47,7 @@ examples:
             compatible = "isil,isl12022";
             reg = <0x6f>;
             interrupts-extended = <&gpio1 5 IRQ_TYPE_LEVEL_LOW>;
+            isil,battery-trip-levels-microvolt = <2550000>, <2250000>;
         };
     };
 
-- 
2.37.2

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

[Krzysztof Kozlowski]

On 15/06/2023 12:58, Rasmus Villemoes wrote:
> 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


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Rasmus Villemoes]

Implement support for using the values given in the
isil,battery-trip-levels-microvolt property to set appropriate values
in the VB85TP/VB75TP bits in the PWR_VBAT register.

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

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index ebd66b835cef..6a757f0a4736 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
@@ -31,6 +32,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 +45,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 +215,38 @@ static const struct regmap_config regmap_config = {
 	.use_single_write = true,
 };
 
+static const u32 trip_levels[2][7] = {
+	{ 2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000 },
+	{ 1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000 },
+};
+
+static void isl12022_set_trip_levels(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 levels[2] = {0, 0};
+	int ret, i, j, x[2];
+	u8 val, mask;
+
+	device_property_read_u32_array(dev, "isil,battery-trip-levels-microvolt",
+				       levels, 2);
+
+	for (i = 0; i < 2; i++) {
+		for (j = 0; j < ARRAY_SIZE(trip_levels[i]) - 1; j++) {
+			if (levels[i] <= trip_levels[i][j])
+				break;
+		}
+		x[i] = j;
+	}
+
+	val = FIELD_PREP(ISL12022_REG_VB85_MASK, x[0]) |
+		FIELD_PREP(ISL12022_REG_VB75_MASK, x[1]);
+	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 +263,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

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

[Andy Shevchenko]

On Thu, Jun 15, 2023 at 12:58:22PM +0200, Rasmus Villemoes wrote:
> Implement support for using the values given in the
> isil,battery-trip-levels-microvolt property to set appropriate values
> in the VB85TP/VB75TP bits in the PWR_VBAT register.

A few nit-picks below.

...

> +static void isl12022_set_trip_levels(struct device *dev)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +	u32 levels[2] = {0, 0};

A nit, 0, 0 is not needed, {} will do the job.

> +	int ret, i, j, x[2];
> +	u8 val, mask;

BUILD_BUG_ON(ARRAY_SIZE(x) != ARRAY_SIZE(levels)) ?

> +	device_property_read_u32_array(dev, "isil,battery-trip-levels-microvolt",
> +				       levels, 2);

A nit, ARRAY_SIZE(levels) ?

> +	for (i = 0; i < 2; i++) {

ARRAY_SIZE(x) ?

> +		for (j = 0; j < ARRAY_SIZE(trip_levels[i]) - 1; j++) {
> +			if (levels[i] <= trip_levels[i][j])
> +				break;
> +		}
> +		x[i] = j;
> +	}
> +
> +	val = FIELD_PREP(ISL12022_REG_VB85_MASK, x[0]) |
> +		FIELD_PREP(ISL12022_REG_VB75_MASK, x[1]);
> +	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);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Rasmus Villemoes]

On 15/06/2023 13.11, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 12:58:22PM +0200, Rasmus Villemoes wrote:

>> +static void isl12022_set_trip_levels(struct device *dev)
>> +{
>> +	struct regmap *regmap = dev_get_drvdata(dev);
>> +	u32 levels[2] = {0, 0};
> 
> A nit, 0, 0 is not needed, {} will do the job.

So? I'm not code-golfing, and here I really want to initialize to 0 (or
any value lower than the first entries in the trip_levels[] arrays so
that, lacking the DT property, the code ends up using what are the chip
reset defaults).

>> +	int ret, i, j, x[2];
>> +	u8 val, mask;
> 
> BUILD_BUG_ON(ARRAY_SIZE(x) != ARRAY_SIZE(levels)) ?

BUILD_BUG_ON doesn't make sense when these things are declared within a
few lines of each other _and_ since they're sized based on properties of
the hardware we're dealing with, nobody would ever have a reason to
change either. So no, that would IMO make it harder to read, because one
would stop and think "why is this obvious thing asserted?".

>> +	device_property_read_u32_array(dev, "isil,battery-trip-levels-microvolt",
>> +				       levels, 2);
> 
> A nit, ARRAY_SIZE(levels) ?
> 
>> +	for (i = 0; i < 2; i++) {
> 
> ARRAY_SIZE(x) ?

I considered that, but really didn't think it improved readability. I'll
defer to Alexandre on whether to change this.

Rasmus

[PATCH v3 5/8] rtc: isl12022: implement RTC_VL_READ ioctl

[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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index 6a757f0a4736..a48456abdcb9 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -204,7 +204,34 @@ 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_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 user, val;
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		ret = regmap_read(regmap, ISL12022_REG_SR, &val);
+		if (ret)
+			return ret;
+
+		user = 0;
+		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;
+	}
+}
+
 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 v3 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. Empirically, this does not immediately update the bits in
the status register (i.e., an immediate read of SR after this write
can still show stale values), but the update is done after a few
milliseconds, so certainly before the RTC device gets registered and
userspace has a chance of doing the ioctl() on this device.

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

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index a48456abdcb9..916879b0388c 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -272,6 +272,16 @@ static void isl12022_set_trip_levels(struct device *dev)
 	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);
+
+	/*
+	 * Force a write of the TSE bit in the BETA register, in order
+	 * to trigger an update of the LBAT75 and LBAT85 bits in the
+	 * status register. In battery backup mode, those bits have
+	 * another meaning, so without this, they may contain stale
+	 * values for up to a minute after power-on.
+	 */
+	regmap_write_bits(regmap, ISL12022_REG_BETA,
+			  ISL12022_BETA_TSE, ISL12022_BETA_TSE);
 }
 
 static int isl12022_probe(struct i2c_client *client)
-- 
2.37.2

[PATCH v3 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>
---
 .../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 3c6c07aaa78f..c2d1441ef273 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,battery-trip-levels-microvolt:
     description:
       The battery voltages at which the first alarm and second alarm
@@ -33,6 +36,13 @@ required:
 
 allOf:
   - $ref: rtc.yaml#
+  # If #clock-cells is present, interrupts must not be present
+  - if:
+      required:
+        - '#clock-cells'
+    then:
+      properties:
+        interrupts: false
 
 unevaluatedProperties: false
 
-- 
2.37.2

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

[Krzysztof Kozlowski]

On 15/06/2023 12:58, 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>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 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 | 44 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index 916879b0388c..05f50ab0e69a 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -10,6 +10,7 @@
 
 #include <linux/bcd.h>
 #include <linux/bitfield.h>
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
@@ -44,6 +45,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)
@@ -242,6 +246,41 @@ 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%.
+		 */
+		regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK,
+				   ISL12022_INT_FO_OFF);
+
+		return 0;
+	}
+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK))
+		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", 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_levels[2][7] = {
 	{ 2125000, 2295000, 2550000, 2805000, 3060000, 4250000, 4675000 },
 	{ 1875000, 2025000, 2250000, 2475000, 2700000, 3750000, 4125000 },
@@ -288,6 +327,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;
@@ -300,6 +340,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 v3 0/8] rtc: isl12022: battery backup voltage and clock support

[Rasmus Villemoes]

On 15/06/2023 12.58, 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.

Ping. Any chance these could be picked up so they make it for v6.6?

Rasmus

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

[Rasmus Villemoes]

On 28/07/2023 16.31, Rasmus Villemoes wrote:
> On 15/06/2023 12.58, 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.
> 
> Ping. Any chance these could be picked up so they make it for v6.6?

Ping^2.

Rasmus

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

[Rasmus Villemoes]

On 03/08/2023 08.45, Rasmus Villemoes wrote:
> On 28/07/2023 16.31, Rasmus Villemoes wrote:
>> On 15/06/2023 12.58, 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.
>>
>> Ping. Any chance these could be picked up so they make it for v6.6?
> 
> Ping^2.

Ping^3.

Rasmus

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

[Alexandre Belloni]

On Thu, 15 Jun 2023 12:58:18 +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.
> 
> [...]

Applied, thanks!

[1/8] rtc: isl12022: remove wrong warning for low battery level
      commit: 4d6af37cafad69ff93f62db80d5a3daa9ac3223f
[2/8] dt-bindings: rtc: Move isil,isl12022 from trivial-rtc.yaml into own schema file
      commit: ffc005280a47030d16cbbf3105c75d3562dba5a8
[3/8] dt-bindings: rtc: isl12022: add bindings for battery alarm trip levels
      commit: 69b569c124ffa698de25d039018fe86313c46c84
[4/8] rtc: isl12022: add support for trip level DT binding
      commit: 2caeb566baabb65add7d99ca6d8bfd566fe91582
[5/8] rtc: isl12022: implement RTC_VL_READ ioctl
      commit: eccebd813874b748ac4e79a9fe4c7290117ad3be
[6/8] rtc: isl12022: trigger battery level detection during probe
      commit: a11b6c460620f7fb5fae4c3aee5a5ba2e1e1129b
[7/8] dt-bindings: rtc: isl12022: add #clock-cells property
      commit: ab246c897be0bdf981f776399ca62b5ec4b8138f
[8/8] rtc: isl12022: implement support for the #clock-cells DT property
      commit: d57d12db774820819d0e591548a56b5cfc95f82a

Best regards,

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