[RFC PATCH v2 0/4] hwmon: (jc42) regmap conversion and resume fix

[RFC PATCH v2 0/4] hwmon: (jc42) regmap conversion and resume fix

[Martin Blumenstingl]

Hello,

this is a follow-up to the comments I got from Guenter on v1 of my patch
from [0] titled:
  "hwmon: (jc42) Restore the min/max/critical temperatures on resume"
There Guenter suggested: "The best solution would probably be to convert
the driver to use regmap and let regmap handle the caching". That's the
goal of this series - in addition to fixing the original resume issue
(see patch #3 - which was the reason for v1 of this series).

Changes since v1 at [0]:
- marked as RFC
- added patches for regmap (patch #1) and regcache (patch #2) conversion
- patch #3 has been updated to use regcache for restoring the register
  values during system resume (this was originally patch 1/1)
- added another patch to remove caching of the temperature register


[0] https://lore.kernel.org/linux-hwmon/20221019214108.220319-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (4):
  hwmon: (jc42) Convert register access to use an I2C regmap
  hwmon: (jc42) Convert to regmap's built-in caching
  hwmon: (jc42) Restore the min/max/critical temperatures on resume
  hwmon: (jc42) Don't cache the temperature register

 drivers/hwmon/Kconfig |   1 +
 drivers/hwmon/jc42.c  | 224 +++++++++++++++++++++++-------------------
 2 files changed, 125 insertions(+), 100 deletions(-)

-- 
2.38.1

[RFC PATCH v2 1/4] hwmon: (jc42) Convert register access to use an I2C regmap

[Martin Blumenstingl]

Switch the jc42 driver to use an I2C regmap to access the registers.
This is done in preparation for improving the caching of registers and
to restore the cached limits during system resume.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/Kconfig |   1 +
 drivers/hwmon/jc42.c  | 102 +++++++++++++++++++++++++-----------------
 2 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ac3daaf59ce..d3bccc8176c5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -799,6 +799,7 @@ config SENSORS_IT87
 config SENSORS_JC42
 	tristate "JEDEC JC42.4 compliant memory module temperature sensors"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here, you get support for JEDEC JC42.4 compliant
 	  temperature sensors, which are used on many DDR3 memory modules for
diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 30888feaf589..329a80264556 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -19,6 +19,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = {
@@ -216,7 +217,7 @@ static const u8 temp_regs[t_num_temp] = {
 
 /* Each client has this additional data */
 struct jc42_data {
-	struct i2c_client *client;
+	struct regmap	*regmap;
 	struct mutex	update_lock;	/* protect register access */
 	bool		extended;	/* true if extended range supported */
 	bool		valid;
@@ -251,19 +252,16 @@ static int jc42_temp_from_reg(s16 reg)
 static struct jc42_data *jc42_update_device(struct device *dev)
 {
 	struct jc42_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	struct jc42_data *ret = data;
-	int i, val;
+	unsigned int i, val;
+	int ret;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
 		for (i = 0; i < t_num_temp; i++) {
-			val = i2c_smbus_read_word_swapped(client, temp_regs[i]);
-			if (val < 0) {
-				ret = ERR_PTR(val);
+			ret = regmap_read(data->regmap, temp_regs[i], &val);
+			if (ret)
 				goto abort;
-			}
 			data->temp[i] = val;
 		}
 		data->last_updated = jiffies;
@@ -271,7 +269,7 @@ static struct jc42_data *jc42_update_device(struct device *dev)
 	}
 abort:
 	mutex_unlock(&data->update_lock);
-	return ret;
+	return ret ? ERR_PTR(ret) : data;
 }
 
 static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
@@ -326,7 +324,6 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long val)
 {
 	struct jc42_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int diff, hyst;
 	int ret;
 
@@ -335,18 +332,18 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 	switch (attr) {
 	case hwmon_temp_min:
 		data->temp[t_min] = jc42_temp_to_reg(val, data->extended);
-		ret = i2c_smbus_write_word_swapped(client, temp_regs[t_min],
-						   data->temp[t_min]);
+		ret = regmap_write(data->regmap, temp_regs[t_min],
+				   data->temp[t_min]);
 		break;
 	case hwmon_temp_max:
 		data->temp[t_max] = jc42_temp_to_reg(val, data->extended);
-		ret = i2c_smbus_write_word_swapped(client, temp_regs[t_max],
-						   data->temp[t_max]);
+		ret = regmap_write(data->regmap, temp_regs[t_max],
+				   data->temp[t_max]);
 		break;
 	case hwmon_temp_crit:
 		data->temp[t_crit] = jc42_temp_to_reg(val, data->extended);
-		ret = i2c_smbus_write_word_swapped(client, temp_regs[t_crit],
-						   data->temp[t_crit]);
+		ret = regmap_write(data->regmap, temp_regs[t_crit],
+				   data->temp[t_crit]);
 		break;
 	case hwmon_temp_crit_hyst:
 		/*
@@ -368,9 +365,8 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		}
 		data->config = (data->config & ~JC42_CFG_HYST_MASK) |
 				(hyst << JC42_CFG_HYST_SHIFT);
-		ret = i2c_smbus_write_word_swapped(data->client,
-						   JC42_REG_CONFIG,
-						   data->config);
+		ret = regmap_write(data->regmap, JC42_REG_CONFIG,
+				   data->config);
 		break;
 	default:
 		ret = -EOPNOTSUPP;
@@ -470,51 +466,79 @@ static const struct hwmon_chip_info jc42_chip_info = {
 	.info = jc42_info,
 };
 
+static bool jc42_readable_reg(struct device *dev, unsigned int reg)
+{
+	return (reg >= JC42_REG_CAP && reg <= JC42_REG_DEVICEID) ||
+		reg == JC42_REG_SMBUS;
+}
+
+static bool jc42_writable_reg(struct device *dev, unsigned int reg)
+{
+	return (reg >= JC42_REG_CONFIG && reg <= JC42_REG_TEMP_CRITICAL) ||
+		reg == JC42_REG_SMBUS;
+}
+
+static bool jc42_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg == JC42_REG_CONFIG || reg == JC42_REG_TEMP;
+}
+
+static const struct regmap_config jc42_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.max_register = JC42_REG_SMBUS,
+	.writeable_reg = jc42_writable_reg,
+	.readable_reg = jc42_readable_reg,
+	.volatile_reg = jc42_volatile_reg,
+};
+
 static int jc42_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
+	unsigned int config, cap;
 	struct jc42_data *data;
-	int config, cap;
+	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct jc42_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, &jc42_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
-	cap = i2c_smbus_read_word_swapped(client, JC42_REG_CAP);
-	if (cap < 0)
-		return cap;
+	ret = regmap_read(data->regmap, JC42_REG_CAP, &cap);
+	if (ret)
+		return ret;
 
 	data->extended = !!(cap & JC42_CAP_RANGE);
 
 	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
-		int smbus;
-
 		/*
 		 * Not all chips support this register, but from a
 		 * quick read of various datasheets no chip appears
 		 * incompatible with the below attempt to disable
 		 * the timeout. And the whole thing is opt-in...
 		 */
-		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
-		if (smbus < 0)
-			return smbus;
-		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
-					     smbus | SMBUS_STMOUT);
+		ret = regmap_set_bits(data->regmap, JC42_REG_SMBUS,
+				      SMBUS_STMOUT);
+		if (ret)
+			return ret;
 	}
 
-	config = i2c_smbus_read_word_swapped(client, JC42_REG_CONFIG);
-	if (config < 0)
-		return config;
+	ret = regmap_read(data->regmap, JC42_REG_CONFIG, &config);
+	if (ret)
+		return ret;
 
 	data->orig_config = config;
 	if (config & JC42_CFG_SHUTDOWN) {
 		config &= ~JC42_CFG_SHUTDOWN;
-		i2c_smbus_write_word_swapped(client, JC42_REG_CONFIG, config);
+		regmap_write(data->regmap, JC42_REG_CONFIG, config);
 	}
 	data->config = config;
 
@@ -535,7 +559,7 @@ static void jc42_remove(struct i2c_client *client)
 
 		config = (data->orig_config & ~JC42_CFG_HYST_MASK)
 		  | (data->config & JC42_CFG_HYST_MASK);
-		i2c_smbus_write_word_swapped(client, JC42_REG_CONFIG, config);
+		regmap_write(data->regmap, JC42_REG_CONFIG, config);
 	}
 }
 
@@ -546,8 +570,7 @@ static int jc42_suspend(struct device *dev)
 	struct jc42_data *data = dev_get_drvdata(dev);
 
 	data->config |= JC42_CFG_SHUTDOWN;
-	i2c_smbus_write_word_swapped(data->client, JC42_REG_CONFIG,
-				     data->config);
+	regmap_write(data->regmap, JC42_REG_CONFIG, data->config);
 	return 0;
 }
 
@@ -556,8 +579,7 @@ static int jc42_resume(struct device *dev)
 	struct jc42_data *data = dev_get_drvdata(dev);
 
 	data->config &= ~JC42_CFG_SHUTDOWN;
-	i2c_smbus_write_word_swapped(data->client, JC42_REG_CONFIG,
-				     data->config);
+	regmap_write(data->regmap, JC42_REG_CONFIG, data->config);
 	return 0;
 }
 
-- 
2.38.1

[RFC PATCH v2 2/4] hwmon: (jc42) Convert to regmap's built-in caching

[Martin Blumenstingl]

Move over to regmap's built-in caching instead of adding a custom
caching implementation. This works for JC42_REG_TEMP_UPPER,
JC42_REG_TEMP_LOWER and JC42_REG_TEMP_CRITICAL as these values never
change except when explicitly written. For JC42_REG_TEMP a cache
variable is still kept as regmap cannot cache this register (because
it's volatile, meaning it can change at any time).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/jc42.c | 97 ++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 43 deletions(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 329a80264556..3f524ab5451c 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -200,21 +200,6 @@ static struct jc42_chips jc42_chips[] = {
 	{ STM_MANID, STTS3000_DEVID, STTS3000_DEVID_MASK },
 };
 
-enum temp_index {
-	t_input = 0,
-	t_crit,
-	t_min,
-	t_max,
-	t_num_temp
-};
-
-static const u8 temp_regs[t_num_temp] = {
-	[t_input] = JC42_REG_TEMP,
-	[t_crit] = JC42_REG_TEMP_CRITICAL,
-	[t_min] = JC42_REG_TEMP_LOWER,
-	[t_max] = JC42_REG_TEMP_UPPER,
-};
-
 /* Each client has this additional data */
 struct jc42_data {
 	struct regmap	*regmap;
@@ -224,7 +209,7 @@ struct jc42_data {
 	unsigned long	last_updated;	/* In jiffies */
 	u16		orig_config;	/* original configuration */
 	u16		config;		/* current configuration */
-	u16		temp[t_num_temp];/* Temperatures */
+	u16		temp;		/* Cached temperature register value */
 };
 
 #define JC42_TEMP_MIN_EXTENDED	(-40000)
@@ -252,18 +237,17 @@ static int jc42_temp_from_reg(s16 reg)
 static struct jc42_data *jc42_update_device(struct device *dev)
 {
 	struct jc42_data *data = dev_get_drvdata(dev);
-	unsigned int i, val;
+	unsigned int val;
 	int ret;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		for (i = 0; i < t_num_temp; i++) {
-			ret = regmap_read(data->regmap, temp_regs[i], &val);
-			if (ret)
-				goto abort;
-			data->temp[i] = val;
-		}
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &val);
+		if (ret)
+			goto abort;
+
+		data->temp = val;
 		data->last_updated = jiffies;
 		data->valid = true;
 	}
@@ -276,44 +260,67 @@ static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
 		     u32 attr, int channel, long *val)
 {
 	struct jc42_data *data = jc42_update_device(dev);
-	int temp, hyst;
+	unsigned int regval;
+	int ret, temp, hyst;
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	switch (attr) {
 	case hwmon_temp_input:
-		*val = jc42_temp_from_reg(data->temp[t_input]);
+		*val = jc42_temp_from_reg(data->temp);
 		return 0;
 	case hwmon_temp_min:
-		*val = jc42_temp_from_reg(data->temp[t_min]);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_LOWER, &regval);
+		if (ret)
+			return ret;
+
+		*val = jc42_temp_from_reg(regval);
 		return 0;
 	case hwmon_temp_max:
-		*val = jc42_temp_from_reg(data->temp[t_max]);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_UPPER, &regval);
+		if (ret)
+			return ret;
+
+		*val = jc42_temp_from_reg(regval);
 		return 0;
 	case hwmon_temp_crit:
-		*val = jc42_temp_from_reg(data->temp[t_crit]);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
+				  &regval);
+		if (ret)
+			return ret;
+
+		*val = jc42_temp_from_reg(regval);
 		return 0;
 	case hwmon_temp_max_hyst:
-		temp = jc42_temp_from_reg(data->temp[t_max]);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_UPPER, &regval);
+		if (ret)
+			return ret;
+
+		temp = jc42_temp_from_reg(regval);
 		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
 						>> JC42_CFG_HYST_SHIFT];
 		*val = temp - hyst;
 		return 0;
 	case hwmon_temp_crit_hyst:
-		temp = jc42_temp_from_reg(data->temp[t_crit]);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
+				  &regval);
+		if (ret)
+			return ret;
+
+		temp = jc42_temp_from_reg(regval);
 		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
 						>> JC42_CFG_HYST_SHIFT];
 		*val = temp - hyst;
 		return 0;
 	case hwmon_temp_min_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_MIN_BIT) & 1;
+		*val = (data->temp >> JC42_ALARM_MIN_BIT) & 1;
 		return 0;
 	case hwmon_temp_max_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_MAX_BIT) & 1;
+		*val = (data->temp >> JC42_ALARM_MAX_BIT) & 1;
 		return 0;
 	case hwmon_temp_crit_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_CRIT_BIT) & 1;
+		*val = (data->temp >> JC42_ALARM_CRIT_BIT) & 1;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -324,6 +331,7 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long val)
 {
 	struct jc42_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
 	int diff, hyst;
 	int ret;
 
@@ -331,21 +339,23 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 
 	switch (attr) {
 	case hwmon_temp_min:
-		data->temp[t_min] = jc42_temp_to_reg(val, data->extended);
-		ret = regmap_write(data->regmap, temp_regs[t_min],
-				   data->temp[t_min]);
+		ret = regmap_write(data->regmap, JC42_REG_TEMP_LOWER,
+				   jc42_temp_to_reg(val, data->extended));
 		break;
 	case hwmon_temp_max:
-		data->temp[t_max] = jc42_temp_to_reg(val, data->extended);
-		ret = regmap_write(data->regmap, temp_regs[t_max],
-				   data->temp[t_max]);
+		ret = regmap_write(data->regmap, JC42_REG_TEMP_UPPER,
+				   jc42_temp_to_reg(val, data->extended));
 		break;
 	case hwmon_temp_crit:
-		data->temp[t_crit] = jc42_temp_to_reg(val, data->extended);
-		ret = regmap_write(data->regmap, temp_regs[t_crit],
-				   data->temp[t_crit]);
+		ret = regmap_write(data->regmap, JC42_REG_TEMP_CRITICAL,
+				   jc42_temp_to_reg(val, data->extended));
 		break;
 	case hwmon_temp_crit_hyst:
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
+				  &regval);
+		if (ret)
+			return ret;
+
 		/*
 		 * JC42.4 compliant chips only support four hysteresis values.
 		 * Pick best choice and go from there.
@@ -353,7 +363,7 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		val = clamp_val(val, (data->extended ? JC42_TEMP_MIN_EXTENDED
 						     : JC42_TEMP_MIN) - 6000,
 				JC42_TEMP_MAX);
-		diff = jc42_temp_from_reg(data->temp[t_crit]) - val;
+		diff = jc42_temp_from_reg(regval) - val;
 		hyst = 0;
 		if (diff > 0) {
 			if (diff < 2250)
@@ -491,6 +501,7 @@ static const struct regmap_config jc42_regmap_config = {
 	.writeable_reg = jc42_writable_reg,
 	.readable_reg = jc42_readable_reg,
 	.volatile_reg = jc42_volatile_reg,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static int jc42_probe(struct i2c_client *client)
-- 
2.38.1

[RFC PATCH v2 3/4] hwmon: (jc42) Restore the min/max/critical temperatures on resume

[Martin Blumenstingl]

The JC42 compatible thermal sensor on Kingston KSM32ES8/16ME DIMMs
(using Micron E-Die) is an ST Microelectronics STTS2004 (manufacturer
0x104a, device 0x2201). It does not keep the previously programmed
minimum, maximum and critical temperatures after system suspend and
resume (which is a shutdown / startup cycle for the JC42 temperature
sensor). This results in an alarm on system resume because the hardware
default for these values is 0°C (so any environment temperature greater
than 0°C will trigger the alarm).

Example before system suspend:
  jc42-i2c-0-1a
  Adapter: SMBus PIIX4 adapter port 0 at 0b00
  temp1:        +34.8°C  (low  =  +0.0°C)
                         (high = +85.0°C, hyst = +85.0°C)
                         (crit = +95.0°C, hyst = +95.0°C)

Example after system resume (without this change):
  jc42-i2c-0-1a
  Adapter: SMBus PIIX4 adapter port 0 at 0b00
  temp1:        +34.8°C  (low  =  +0.0°C)             ALARM (HIGH, CRIT)
                         (high =  +0.0°C, hyst =  +0.0°C)
                         (crit =  +0.0°C, hyst =  +0.0°C)

Apply the cached values from the JC42_REG_TEMP_UPPER,
JC42_REG_TEMP_LOWER, JC42_REG_TEMP_CRITICAL and JC42_REG_SMBUS (where
the SMBUS register is not related to this issue but a side-effect of
using regcache_sync() during system resume with the previously
cached/programmed values. This fixes the alarm due to the hardware
defaults of 0°C because the previously applied limits (set by userspace)
are re-applied on system resume.

Fixes: 175c490c9e7f ("hwmon: (jc42) Add support for STTS2004 and AT30TSE004")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/jc42.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 3f524ab5451c..61311483a5c6 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -582,6 +582,10 @@ static int jc42_suspend(struct device *dev)
 
 	data->config |= JC42_CFG_SHUTDOWN;
 	regmap_write(data->regmap, JC42_REG_CONFIG, data->config);
+
+	regcache_cache_only(data->regmap, true);
+	regcache_mark_dirty(data->regmap);
+
 	return 0;
 }
 
@@ -589,9 +593,13 @@ static int jc42_resume(struct device *dev)
 {
 	struct jc42_data *data = dev_get_drvdata(dev);
 
+	regcache_cache_only(data->regmap, false);
+
 	data->config &= ~JC42_CFG_SHUTDOWN;
 	regmap_write(data->regmap, JC42_REG_CONFIG, data->config);
-	return 0;
+
+	/* Restore cached register values to hardware */
+	return regcache_sync(data->regmap);
 }
 
 static const struct dev_pm_ops jc42_dev_pm_ops = {
-- 
2.38.1

[RFC PATCH v2 4/4] hwmon: (jc42) Don't cache the temperature register

[Martin Blumenstingl]

Now that we're utilizing regmap and it's regcache for the
minimum/maximum/critical temperature registers the only cached register
that's left is the actual temperature register. Drop the custom cache
implementation as it just complicates things.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/jc42.c | 59 ++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 61311483a5c6..52a60eb0791b 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -203,13 +203,10 @@ static struct jc42_chips jc42_chips[] = {
 /* Each client has this additional data */
 struct jc42_data {
 	struct regmap	*regmap;
-	struct mutex	update_lock;	/* protect register access */
 	bool		extended;	/* true if extended range supported */
 	bool		valid;
-	unsigned long	last_updated;	/* In jiffies */
 	u16		orig_config;	/* original configuration */
 	u16		config;		/* current configuration */
-	u16		temp;		/* Cached temperature register value */
 };
 
 #define JC42_TEMP_MIN_EXTENDED	(-40000)
@@ -234,41 +231,20 @@ static int jc42_temp_from_reg(s16 reg)
 	return reg * 125 / 2;
 }
 
-static struct jc42_data *jc42_update_device(struct device *dev)
-{
-	struct jc42_data *data = dev_get_drvdata(dev);
-	unsigned int val;
-	int ret;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		ret = regmap_read(data->regmap, JC42_REG_TEMP, &val);
-		if (ret)
-			goto abort;
-
-		data->temp = val;
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-abort:
-	mutex_unlock(&data->update_lock);
-	return ret ? ERR_PTR(ret) : data;
-}
-
 static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
 		     u32 attr, int channel, long *val)
 {
-	struct jc42_data *data = jc42_update_device(dev);
+	struct jc42_data *data = dev_get_drvdata(dev);
 	unsigned int regval;
 	int ret, temp, hyst;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
 	switch (attr) {
 	case hwmon_temp_input:
-		*val = jc42_temp_from_reg(data->temp);
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = jc42_temp_from_reg(regval);
 		return 0;
 	case hwmon_temp_min:
 		ret = regmap_read(data->regmap, JC42_REG_TEMP_LOWER, &regval);
@@ -314,13 +290,25 @@ static int jc42_read(struct device *dev, enum hwmon_sensor_types type,
 		*val = temp - hyst;
 		return 0;
 	case hwmon_temp_min_alarm:
-		*val = (data->temp >> JC42_ALARM_MIN_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_MIN_BIT) & 1;
 		return 0;
 	case hwmon_temp_max_alarm:
-		*val = (data->temp >> JC42_ALARM_MAX_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_MAX_BIT) & 1;
 		return 0;
 	case hwmon_temp_crit_alarm:
-		*val = (data->temp >> JC42_ALARM_CRIT_BIT) & 1;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			return ret;
+
+		*val = (regval >> JC42_ALARM_CRIT_BIT) & 1;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -335,8 +323,6 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 	int diff, hyst;
 	int ret;
 
-	mutex_lock(&data->update_lock);
-
 	switch (attr) {
 	case hwmon_temp_min:
 		ret = regmap_write(data->regmap, JC42_REG_TEMP_LOWER,
@@ -383,8 +369,6 @@ static int jc42_write(struct device *dev, enum hwmon_sensor_types type,
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return ret;
 }
 
@@ -521,7 +505,6 @@ static int jc42_probe(struct i2c_client *client)
 		return PTR_ERR(data->regmap);
 
 	i2c_set_clientdata(client, data);
-	mutex_init(&data->update_lock);
 
 	ret = regmap_read(data->regmap, JC42_REG_CAP, &cap);
 	if (ret)
-- 
2.38.1

Re: [RFC PATCH v2 2/4] hwmon: (jc42) Convert to regmap's built-in caching

[Guenter Roeck]

On Thu, Oct 20, 2022 at 11:03:18PM +0200, Martin Blumenstingl wrote:
> Move over to regmap's built-in caching instead of adding a custom
> caching implementation. This works for JC42_REG_TEMP_UPPER,
> JC42_REG_TEMP_LOWER and JC42_REG_TEMP_CRITICAL as these values never
> change except when explicitly written. For JC42_REG_TEMP a cache
> variable is still kept as regmap cannot cache this register (because
> it's volatile, meaning it can change at any time).
> 

Just drop that one as well, together with jc42_update_device(),
and read the temperature directly where needed. In practice
caching of 'hot' registers isn't really worth the trouble.

Thanks,
Guenter

> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/hwmon/jc42.c | 97 ++++++++++++++++++++++++--------------------
>  1 file changed, 54 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 329a80264556..3f524ab5451c 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -200,21 +200,6 @@ static struct jc42_chips jc42_chips[] = {
>  	{ STM_MANID, STTS3000_DEVID, STTS3000_DEVID_MASK },
>  };
>  
> -enum temp_index {
> -	t_input = 0,
> -	t_crit,
> -	t_min,
> -	t_max,
> -	t_num_temp
> -};
> -
> -static const u8 temp_regs[t_num_temp] = {
> -	[t_input] = JC42_REG_TEMP,
> -	[t_crit] = JC42_REG_TEMP_CRITICAL,
> -	[t_min] = JC42_REG_TEMP_LOWER,
> -	[t_max] = JC42_REG_TEMP_UPPER,
> -};
> -
>  /* Each client has this additional data */
>  struct jc42_data {
>  	struct regmap	*regmap;
> @@ -224,7 +209,7 @@ struct jc42_data {
>  	unsigned long	last_updated;	/* In jiffies */
>  	u16		orig_config;	/* original configuration */
>  	u16		config;		/* current configuration */
> -	u16		temp[t_num_temp];/* Temperatures */
> +	u16		temp;		/* Cached temperature register value */

Re: [RFC PATCH v2 1/4] hwmon: (jc42) Convert register access to use an I2C regmap

[Guenter Roeck]

On Thu, Oct 20, 2022 at 11:03:17PM +0200, Martin Blumenstingl wrote:
> Switch the jc42 driver to use an I2C regmap to access the registers.
> This is done in preparation for improving the caching of registers and
> to restore the cached limits during system resume.
> 

I would suggest to combine patch 1 and 2 and drop local caching entirely
in a single patch.

Thanks,
Guenter

Re: [RFC PATCH v2 4/4] hwmon: (jc42) Don't cache the temperature register

[Guenter Roeck]

On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
> Now that we're utilizing regmap and it's regcache for the
> minimum/maximum/critical temperature registers the only cached register
> that's left is the actual temperature register. Drop the custom cache
> implementation as it just complicates things.
> 

Ah, you got there eventually. Just combine this patch into the first
patch of the series. No need to keep separate patches, especially since
a lot of the code changed in patch 1 and 2 is just thrown away here.

That reminds me, though: Make sure that the alarm bits are not dropped
after reading the temperature (running the 'sensors' command with
alarms active should do). I have some JC42 chips here and will do the
same.

Thanks,
Guenter

Re: [RFC PATCH v2 0/4] hwmon: (jc42) regmap conversion and resume fix

[Guenter Roeck]

On Thu, Oct 20, 2022 at 11:03:16PM +0200, Martin Blumenstingl wrote:
> Hello,
> 
> this is a follow-up to the comments I got from Guenter on v1 of my patch
> from [0] titled:
>   "hwmon: (jc42) Restore the min/max/critical temperatures on resume"
> There Guenter suggested: "The best solution would probably be to convert
> the driver to use regmap and let regmap handle the caching". That's the
> goal of this series - in addition to fixing the original resume issue
> (see patch #3 - which was the reason for v1 of this series).
> 
> Changes since v1 at [0]:
> - marked as RFC
> - added patches for regmap (patch #1) and regcache (patch #2) conversion
> - patch #3 has been updated to use regcache for restoring the register
>   values during system resume (this was originally patch 1/1)
> - added another patch to remove caching of the temperature register
> 


Great, excellent work. As mentioned in the patches, please combine
patches 1, 2, and 4 into a single patch. Also, drop the RFC.

Thanks,
Guenter

Re: [RFC PATCH v2 4/4] hwmon: (jc42) Don't cache the temperature register

[Martin Blumenstingl]

Hi Guenter,

On Fri, Oct 21, 2022 at 12:14 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
> > Now that we're utilizing regmap and it's regcache for the
> > minimum/maximum/critical temperature registers the only cached register
> > that's left is the actual temperature register. Drop the custom cache
> > implementation as it just complicates things.
> >
>
> Ah, you got there eventually. Just combine this patch into the first
> patch of the series. No need to keep separate patches, especially since
> a lot of the code changed in patch 1 and 2 is just thrown away here.
Thanks again for the quick response and for the great feedback. I'll
combine the patches tomorrow and send a v3!

> That reminds me, though: Make sure that the alarm bits are not dropped
> after reading the temperature (running the 'sensors' command with
> alarms active should do). I have some JC42 chips here and will do the
> same.
I configured below ambient high and crit temperatures:
  jc42-i2c-0-1a
  Adapter: SMBus PIIX4 adapter port 0 at 0b00
  temp1:        +35.0°C  (low  =  +0.0°C)                  ALARM (HIGH, CRIT)
                        (high = +25.0°C, hyst = +25.0°C)
                        (crit = +30.0°C, hyst = +30.0°C)

Then I ran "sensors" three times in a row.
The output of all "sensors" commands is the same, meaning all of them
show the ALARM (HIGH, CRIT) part.

Do you want me to mention this somewhere (for example in the
cover-letter or the new patch #1)?


Best regards,
Martin

Re: [RFC PATCH v2 4/4] hwmon: (jc42) Don't cache the temperature register

[Guenter Roeck]

On 10/20/22 15:22, Martin Blumenstingl wrote:
> Hi Guenter,
> 
> On Fri, Oct 21, 2022 at 12:14 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Thu, Oct 20, 2022 at 11:03:20PM +0200, Martin Blumenstingl wrote:
>>> Now that we're utilizing regmap and it's regcache for the
>>> minimum/maximum/critical temperature registers the only cached register
>>> that's left is the actual temperature register. Drop the custom cache
>>> implementation as it just complicates things.
>>>
>>
>> Ah, you got there eventually. Just combine this patch into the first
>> patch of the series. No need to keep separate patches, especially since
>> a lot of the code changed in patch 1 and 2 is just thrown away here.
> Thanks again for the quick response and for the great feedback. I'll
> combine the patches tomorrow and send a v3!
> 
>> That reminds me, though: Make sure that the alarm bits are not dropped
>> after reading the temperature (running the 'sensors' command with
>> alarms active should do). I have some JC42 chips here and will do the
>> same.
> I configured below ambient high and crit temperatures:
>    jc42-i2c-0-1a
>    Adapter: SMBus PIIX4 adapter port 0 at 0b00
>    temp1:        +35.0°C  (low  =  +0.0°C)                  ALARM (HIGH, CRIT)
>                          (high = +25.0°C, hyst = +25.0°C)
>                          (crit = +30.0°C, hyst = +30.0°C)
> 
> Then I ran "sensors" three times in a row.
> The output of all "sensors" commands is the same, meaning all of them
> show the ALARM (HIGH, CRIT) part.
> 

Excellent!

> Do you want me to mention this somewhere (for example in the
> cover-letter or the new patch #1)?
> 
Yes, please mention it in the cover letter.

Background for the question: Some older sensor chips (granted, typically
20+ years old) tend to reset the alarm status after reading it, only
to set it again after the next measurement cycle.

Thanks,
Guenter