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/[email protected]/
[1] https://lore.kernel.org/linux-hwmon/[email protected]/
[2] https://lore.kernel.org/linux-hwmon/[email protected]/
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
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 <[email protected]>
---
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, ®val);
+ 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, ®val);
+ 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, ®val);
+ 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,
+ ®val);
+ 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, ®val);
+ 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,
+ ®val);
+ 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, ®val);
+ 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, ®val);
+ 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, ®val);
+ 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,
+ ®val);
+ 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
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 <[email protected]>
Signed-off-by: Martin Blumenstingl <[email protected]>
---
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
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 <[email protected]>
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, ®val);
> + 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, ®val);
> + 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, ®val);
> + 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,
> + ®val);
> + 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, ®val);
> + 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,
> + ®val);
> + 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, ®val);
> + 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, ®val);
> + 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, ®val);
> + 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,
> + ®val);
> + 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;
> }
>
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 <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>
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 = {