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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> +
> 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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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 <[email protected]>
+
+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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
On 13/06/2023 15:00, Rasmus Villemoes wrote:
> The current handling of the low-battery bits in the status register is
> wrong. The first six patches fix that and implement proper support for
> RTC_VL_READ.
>
> The last two patches allow describing the isl12022 as a clock
> provider, for now just as a fixed 32kHz clock. They are also
> tangentially related to the backup battery, in that when the isl12022
> is not used as a clock source, one can save some power consumption in
> battery mode by setting the FOx bits to 0.
>
> v2 changes:
Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.
Best regards,
Krzysztof
On 13/06/2023 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 <[email protected]>
> ---
> .../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
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/[email protected]/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/[email protected]
# 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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> ---
> .../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 <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
>> ---
>> .../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
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
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
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
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
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/[email protected]/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/[email protected]
# 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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
On 13/06/2023 17.25, Andy Shevchenko wrote:
> On Tue, Jun 13, 2023 at 03:00:10PM +0200, Rasmus Villemoes wrote:
>> If device tree implies that the chip's IRQ/F_OUT pin is used as a
>> clock, expose that in the driver. For now, pretend it is a
>> fixed-rate (32kHz) clock; if other use cases appear the driver can be
>> updated to provide its own clk_ops etc.
>>
>> When the clock output is not used on a given board, one can prolong
>> the battery life by ensuring that the FOx bits are 0. For the hardware
>> I'm currently working on, the RTC draws 1.2uA with the FOx bits at
>> their default 0001 value, dropping to 0.88uA when those bits are
>> cleared.
>
> ...
>
>> +#define ISL12022_INT_FO_MASK GENMASK(3, 0)
>> +#define ISL12022_INT_FO_OFF 0x0
>> +#define ISL12022_INT_FO_32K 0x1
>
> A nit-pick. Are they decimal or bit fields?
-ENOPARSE. A number is a number. Its representation in C code may be
decimal or hexadecimal (or...). And sure, 0 and 0x0 are different
spellings of the same thing. The data sheet lists the possible values in
terms of individual bits, so I suppose I could even do 0b0000 and
0b0001, but that's too unusual (even if perfectly acceptable by gcc).
> To me seems like the 0x can be dropped.
Can, but won't, a single hex digit is more natural way to represent a
four-bit field.
>> + ret = regmap_update_bits(regmap, ISL12022_REG_INT, ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K);
>
> Seems too long even for 100 limit.
> Maybe:
>
> ret = regmap_update_bits(regmap, ISL12022_REG_INT,
> ISL12022_INT_FO_MASK, ISL12022_INT_FO_32K);
Sure.
Rasmus
On 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
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
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
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
On 14/06/2023 17.13, Andy Shevchenko wrote:
> When reading such a code the following questions are arisen:
> 1) Can the positive return value be the case?
> 2) If so, what is the meaning of a such?
> 3) Why do we not care about it?
>
> All this can simply gone if we use
>
> ret = foo(...);
> if (ret)
> return ret;
>
> As it's clear that whatever is non-zero we accept as something to be promoted
> to the upper layer. I hope this explains my position.
But we're in a context (in this case an ->ioctl method) where _our_
caller expects 0/-ESOMETHING, so returning something positive would be a
bug - i.e., either way of spelling that if(), the reader must know that
foo() also has those 0/-ESOMETHING semantics.
I honestly didn't think much about it, but looking at the existing code
and the stuff I add, all other places actually do 'if (ret)', so I've
updated this site for consistency.
Rasmus
On Thu, Jun 15, 2023 at 12:53:24PM +0200, Rasmus Villemoes wrote:
> On 14/06/2023 17.13, Andy Shevchenko wrote:
> > When reading such a code the following questions are arisen:
> > 1) Can the positive return value be the case?
> > 2) If so, what is the meaning of a such?
> > 3) Why do we not care about it?
> >
> > All this can simply gone if we use
> >
> > ret = foo(...);
> > if (ret)
> > return ret;
> >
> > As it's clear that whatever is non-zero we accept as something to be promoted
> > to the upper layer. I hope this explains my position.
>
> But we're in a context (in this case an ->ioctl method) where _our_
> caller expects 0/-ESOMETHING, so returning something positive would be a
> bug - i.e., either way of spelling that if(), the reader must know that
> foo() also has those 0/-ESOMETHING semantics.
I totally understand this. But then it's either problem of regmap API
documentation or API itself. I.o.w. not _your_ problem.
> I honestly didn't think much about it, but looking at the existing code
> and the stuff I add, all other places actually do 'if (ret)', so I've
> updated this site for consistency.
Thank you!
--
With Best Regards,
Andy Shevchenko
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/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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
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
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