[PATCH v4 0/2] hwmon: (jc42) regmap conversion and resume fix

[PATCH v4 0/2] 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).

Guenter suggested:
> Make sure that the alarm bits are not dropped after reading the
> temperature (running the 'sensors' command with alarms active should
> do)
I configured the limits to be below the case temperature on my system
(as the jc42 sensor - a ST Microelectronics STTS2004 - is part of the
DIMMs) and ran sensors three times in a row. The output is the same for
all runs:
  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)
My conclusion is that the alarm bit is not dropped after reading the
temperature.


Changes since v3 at [2]:
- re-add the update_lock mutex as Guenter spotted that it's still
  needed in jc42_write() - and later on we found out that jc42_read()
  also requires it (previously it was part of the now removed
  jc42_update_device()). This affects patch #1
- added Guenter's Reviewed-by to patch #2 (thank you!)

Changes since v2 at [1]:
- squashed patches #1, #2 and #4 into the new patch #1 (without any
  other changes to content in jc42.c)
- patch #3 has no changes other than it's numbering (see previous
  change)
- dropped RFC prefix

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/
[1] https://lore.kernel.org/linux-hwmon/20221020210320.1624617-1-martin.blumenstingl@googlemail.com/
[2] https://lore.kernel.org/linux-hwmon/20221021165000.1865615-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (2):
  hwmon: (jc42) Convert register access and caching to regmap/regcache
  hwmon: (jc42) Restore the min/max/critical temperatures on resume

 drivers/hwmon/Kconfig |   1 +
 drivers/hwmon/jc42.c  | 243 ++++++++++++++++++++++++------------------
 2 files changed, 141 insertions(+), 103 deletions(-)

-- 
2.38.1

[PATCH v4 1/2] hwmon: (jc42) Convert register access and caching to regmap/regcache

[Martin Blumenstingl]

Switch the jc42 driver to use an I2C regmap to access the registers.
Also 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. The cache For JC42_REG_TEMP is
dropped (regmap can't cache it because it's volatile, meaning it can
change at any time) as well for simplicity and consistency with other
drivers.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/hwmon/Kconfig |   1 +
 drivers/hwmon/jc42.c  | 233 ++++++++++++++++++++++++------------------
 2 files changed, 132 insertions(+), 102 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..355639d208d0 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[] = {
@@ -199,31 +200,14 @@ 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 i2c_client *client;
 	struct mutex	update_lock;	/* protect register access */
+	struct regmap	*regmap;
 	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[t_num_temp];/* Temperatures */
 };
 
 #define JC42_TEMP_MIN_EXTENDED	(-40000)
@@ -248,85 +232,102 @@ 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);
-	struct i2c_client *client = data->client;
-	struct jc42_data *ret = data;
-	int i, val;
-
-	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);
-				goto abort;
-			}
-			data->temp[i] = val;
-		}
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-abort:
-	mutex_unlock(&data->update_lock);
-	return ret;
-}
-
 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;
+	struct jc42_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int ret, temp, hyst;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	mutex_lock(&data->update_lock);
 
 	switch (attr) {
 	case hwmon_temp_input:
-		*val = jc42_temp_from_reg(data->temp[t_input]);
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			break;
+
+		*val = jc42_temp_from_reg(regval);
+		break;
 	case hwmon_temp_min:
-		*val = jc42_temp_from_reg(data->temp[t_min]);
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_LOWER, &regval);
+		if (ret)
+			break;
+
+		*val = jc42_temp_from_reg(regval);
+		break;
 	case hwmon_temp_max:
-		*val = jc42_temp_from_reg(data->temp[t_max]);
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_UPPER, &regval);
+		if (ret)
+			break;
+
+		*val = jc42_temp_from_reg(regval);
+		break;
 	case hwmon_temp_crit:
-		*val = jc42_temp_from_reg(data->temp[t_crit]);
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
+				  &regval);
+		if (ret)
+			break;
+
+		*val = jc42_temp_from_reg(regval);
+		break;
 	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)
+			break;
+
+		temp = jc42_temp_from_reg(regval);
 		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
 						>> JC42_CFG_HYST_SHIFT];
 		*val = temp - hyst;
-		return 0;
+		break;
 	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)
+			break;
+
+		temp = jc42_temp_from_reg(regval);
 		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
 						>> JC42_CFG_HYST_SHIFT];
 		*val = temp - hyst;
-		return 0;
+		break;
 	case hwmon_temp_min_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_MIN_BIT) & 1;
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			break;
+
+		*val = (regval >> JC42_ALARM_MIN_BIT) & 1;
+		break;
 	case hwmon_temp_max_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_MAX_BIT) & 1;
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			break;
+
+		*val = (regval >> JC42_ALARM_MAX_BIT) & 1;
+		break;
 	case hwmon_temp_crit_alarm:
-		*val = (data->temp[t_input] >> JC42_ALARM_CRIT_BIT) & 1;
-		return 0;
+		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
+		if (ret)
+			break;
+
+		*val = (regval >> JC42_ALARM_CRIT_BIT) & 1;
+		break;
 	default:
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		break;
 	}
+
+	mutex_unlock(&data->update_lock);
+
+	return ret;
 }
 
 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;
+	unsigned int regval;
 	int diff, hyst;
 	int ret;
 
@@ -334,21 +335,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 = i2c_smbus_write_word_swapped(client, 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 = i2c_smbus_write_word_swapped(client, 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 = i2c_smbus_write_word_swapped(client, 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.
@@ -356,7 +359,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)
@@ -368,9 +371,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 +472,80 @@ 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,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 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 +566,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 +577,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 +586,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

Re: [PATCH v4 1/2] hwmon: (jc42) Convert register access and caching to regmap/regcache

[Guenter Roeck]

On Sun, Oct 23, 2022 at 11:31:56PM +0200, Martin Blumenstingl wrote:
> Switch the jc42 driver to use an I2C regmap to access the registers.
> Also 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. The cache For JC42_REG_TEMP is
> dropped (regmap can't cache it because it's volatile, meaning it can
> change at any time) as well for simplicity and consistency with other
> drivers.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/Kconfig |   1 +
>  drivers/hwmon/jc42.c  | 233 ++++++++++++++++++++++++------------------
>  2 files changed, 132 insertions(+), 102 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..355639d208d0 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[] = {
> @@ -199,31 +200,14 @@ 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 i2c_client *client;
>  	struct mutex	update_lock;	/* protect register access */
> +	struct regmap	*regmap;
>  	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[t_num_temp];/* Temperatures */
>  };
>  
>  #define JC42_TEMP_MIN_EXTENDED	(-40000)
> @@ -248,85 +232,102 @@ 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);
> -	struct i2c_client *client = data->client;
> -	struct jc42_data *ret = data;
> -	int i, val;
> -
> -	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);
> -				goto abort;
> -			}
> -			data->temp[i] = val;
> -		}
> -		data->last_updated = jiffies;
> -		data->valid = true;
> -	}
> -abort:
> -	mutex_unlock(&data->update_lock);
> -	return ret;
> -}
> -
>  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;
> +	struct jc42_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int ret, temp, hyst;
>  
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	mutex_lock(&data->update_lock);
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> -		*val = jc42_temp_from_reg(data->temp[t_input]);
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = jc42_temp_from_reg(regval);
> +		break;
>  	case hwmon_temp_min:
> -		*val = jc42_temp_from_reg(data->temp[t_min]);
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP_LOWER, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = jc42_temp_from_reg(regval);
> +		break;
>  	case hwmon_temp_max:
> -		*val = jc42_temp_from_reg(data->temp[t_max]);
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP_UPPER, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = jc42_temp_from_reg(regval);
> +		break;
>  	case hwmon_temp_crit:
> -		*val = jc42_temp_from_reg(data->temp[t_crit]);
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP_CRITICAL,
> +				  &regval);
> +		if (ret)
> +			break;
> +
> +		*val = jc42_temp_from_reg(regval);
> +		break;
>  	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)
> +			break;
> +
> +		temp = jc42_temp_from_reg(regval);
>  		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
>  						>> JC42_CFG_HYST_SHIFT];
>  		*val = temp - hyst;
> -		return 0;
> +		break;
>  	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)
> +			break;
> +
> +		temp = jc42_temp_from_reg(regval);
>  		hyst = jc42_hysteresis[(data->config & JC42_CFG_HYST_MASK)
>  						>> JC42_CFG_HYST_SHIFT];
>  		*val = temp - hyst;
> -		return 0;
> +		break;
>  	case hwmon_temp_min_alarm:
> -		*val = (data->temp[t_input] >> JC42_ALARM_MIN_BIT) & 1;
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = (regval >> JC42_ALARM_MIN_BIT) & 1;
> +		break;
>  	case hwmon_temp_max_alarm:
> -		*val = (data->temp[t_input] >> JC42_ALARM_MAX_BIT) & 1;
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = (regval >> JC42_ALARM_MAX_BIT) & 1;
> +		break;
>  	case hwmon_temp_crit_alarm:
> -		*val = (data->temp[t_input] >> JC42_ALARM_CRIT_BIT) & 1;
> -		return 0;
> +		ret = regmap_read(data->regmap, JC42_REG_TEMP, &regval);
> +		if (ret)
> +			break;
> +
> +		*val = (regval >> JC42_ALARM_CRIT_BIT) & 1;
> +		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		break;
>  	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
>  }
>  
>  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;
> +	unsigned int regval;
>  	int diff, hyst;
>  	int ret;
>  
> @@ -334,21 +335,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 = i2c_smbus_write_word_swapped(client, 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 = i2c_smbus_write_word_swapped(client, 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 = i2c_smbus_write_word_swapped(client, 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.
> @@ -356,7 +359,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)
> @@ -368,9 +371,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 +472,80 @@ 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,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
>  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 +566,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 +577,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 +586,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;
>  }
>  

[PATCH v4 2/2] 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")
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
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 355639d208d0..0554b41c32bc 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -578,6 +578,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;
 }
 
@@ -585,9 +589,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

Re: [PATCH v4 2/2] hwmon: (jc42) Restore the min/max/critical temperatures on resume

[Guenter Roeck]

On Sun, Oct 23, 2022 at 11:31:57PM +0200, Martin Blumenstingl wrote:
> 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")
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  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 355639d208d0..0554b41c32bc 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -578,6 +578,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;
>  }
>  
> @@ -585,9 +589,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 = {