diff mbox series

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

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

Commit Message

Rasmus Villemoes June 15, 2023, 10:58 a.m. UTC 
  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(+)

Comments

Andy Shevchenko June 15, 2023, 11:11 a.m. UTC | #1 
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);
> +}
Rasmus Villemoes June 19, 2023, 7:27 a.m. UTC | #2 
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
diff mbox series

Patch

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