diff mbox series

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

Message ID 20230613130011.305589-9-linux@rasmusvillemoes.dk
State New
Headers
Series rtc: isl12022: battery backup voltage and clock support |

Commit Message

Rasmus Villemoes June 13, 2023, 1 p.m. UTC 
  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(+)

Comments

Andy Shevchenko June 13, 2023, 3:25 p.m. UTC | #1 
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);

?
kernel test robot June 13, 2023, 7:10 p.m. UTC | #2 
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
kernel test robot June 14, 2023, 5:27 a.m. UTC | #3 
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!
Rasmus Villemoes June 14, 2023, 10:51 a.m. UTC | #4 
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
Andy Shevchenko June 14, 2023, 12:13 p.m. UTC | #5 
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.
diff mbox series

Patch

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