Hi Guenter,
On 7/4/24 7:52 PM, Guenter Roeck wrote:
Use regmap for register accesses and caching.
While at it, use sysfs_emit() instead of sprintf() to write sysfs
attribute data, and remove spurious debug messages which would only
be seen as result of a bug in the code. Also make sure that error
codes are propagated and not replaced with -EIO.
While at it, introduce rounding of written temperature values and for
internal calculations to reduce deviation from written values and as
much as possible.
No functional change intended except for differences introduced by
rounding.
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
v3: Add more details to patch description
Cache all attributes
Introduce rounding when writing attributes and for some calculations
Always return error codes from regmap operations; never replace with
-EIO
v2: Drop another spurious debug message in this patch instead of patch 10
Add missing "select REGMAP_I2C" to Kconfig
Change misleading variable name from 'mask' to 'mode'.
Use sysfs_emit instead of sprintf everywhere
drivers/hwmon/Kconfig | 1 +
drivers/hwmon/amc6821.c | 812 ++++++++++++++++++----------------------
2 files changed, 373 insertions(+), 440 deletions(-)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..a8fa87a96e8f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
config SENSORS_AMC6821
tristate "Texas Instruments AMC6821"
depends on I2C
+ select REGMAP_I2C
help
If you say yes here you get support for the Texas Instruments
AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 295a9148779d..a5abd36a1430 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,15 +8,18 @@
* Copyright (C) 2007 Hans J. Koch <hjk@xxxxxxxxxxxx>
*/
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
#include <linux/bits.h>
#include <linux/err.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
#include <linux/i2c.h>
#include <linux/init.h>
-#include <linux/jiffies.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
/*
@@ -44,6 +47,7 @@ module_param(init, int, 0444);
#define AMC6821_REG_CONF4 0x04
#define AMC6821_REG_STAT1 0x02
#define AMC6821_REG_STAT2 0x03
+#define AMC6821_REG_TEMP_LO 0x06
#define AMC6821_REG_TDATA_LOW 0x08
#define AMC6821_REG_TDATA_HI 0x09
#define AMC6821_REG_LTEMP_HI 0x0A
@@ -61,11 +65,8 @@ module_param(init, int, 0444);
#define AMC6821_REG_DCY_LOW_TEMP 0x21
#define AMC6821_REG_TACH_LLIMITL 0x10
-#define AMC6821_REG_TACH_LLIMITH 0x11
#define AMC6821_REG_TACH_HLIMITL 0x12
-#define AMC6821_REG_TACH_HLIMITH 0x13
#define AMC6821_REG_TACH_SETTINGL 0x1e
-#define AMC6821_REG_TACH_SETTINGH 0x1f
#define AMC6821_CONF1_START BIT(0)
#define AMC6821_CONF1_FAN_INT_EN BIT(1)
@@ -108,6 +109,9 @@ module_param(init, int, 0444);
#define AMC6821_STAT2_L_THERM BIT(6)
#define AMC6821_STAT2_THERM_IN BIT(7)
+#define AMC6821_TEMP_SLOPE_MASK GENMASK(2, 0)
+#define AMC6821_TEMP_LIMIT_MASK GENMASK(7, 3)
+
enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
IDX_TEMP2_MAX, IDX_TEMP2_CRIT,
@@ -130,224 +134,155 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
AMC6821_REG_TACH_HLIMITL,
AMC6821_REG_TACH_SETTINGL, };
-static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
- AMC6821_REG_TACH_LLIMITH,
- AMC6821_REG_TACH_HLIMITH,
- AMC6821_REG_TACH_SETTINGH, };
-
/*
* Client data (each client gets its own)
*/
struct amc6821_data {
- struct i2c_client *client;
+ struct regmap *regmap;
struct mutex update_lock;
- bool valid; /* false until following fields are valid */
- unsigned long last_updated; /* in jiffies */
-
- /* register values */
- int temp[TEMP_IDX_LEN];
-
- u16 fan[FAN1_IDX_LEN];
- u8 fan1_pulses;
-
- u8 pwm1;
- u8 temp1_auto_point_temp[3];
- u8 temp2_auto_point_temp[3];
- u8 pwm1_auto_point_pwm[3];
- u8 pwm1_enable;
- u8 pwm1_auto_channels_temp;
-
- u8 stat1;
- u8 stat2;
};
-static struct amc6821_data *amc6821_update_device(struct device *dev)
+/*
+ * Return set of three temperatures:
It actually returns 0 if successful, negative errno otherwise (matches
regmap_* return values).
But it does write to temps array with those values.
Would be nice to say what we're expecting in channel, i.e. 0 for Remote
and 1 for Local.
+ * temps[0]: Passive cooling temperature, applies to both channels
+ * temps[1]: Low temperature, start slope calculations
+ * temps[2]: High temperature
+ */
IIUC, we have different units there, >> 3 (/4) °C for 0 and 2, but °C
for temps[1] ? If I didn't misunderstand, I think it's worth making it
explicit in the docs (or make them have the same unit).
+static int amc6821_get_auto_point_temps(struct regmap *regmap, int channel, u8 *temps)
{
- struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
- int timeout = HZ;
- u8 reg;
- int i;
+ u32 pwm, regval;
+ int err;
- mutex_lock(&data->update_lock);
+ err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+ if (err)
+ return err;
- if (time_after(jiffies, data->last_updated + timeout) ||
- !data->valid) {
+ err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, ®val);
+ if (err)
+ return err;
+ temps[0] = regval;
- for (i = 0; i < TEMP_IDX_LEN; i++)
- data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
- client, temp_reg[i]);
+ err = regmap_read(regmap,
+ channel ? AMC6821_REG_RTEMP_FAN_CTRL : AMC6821_REG_LTEMP_FAN_CTRL,
+ ®val);
+ if (err)
+ return err;
+ temps[1] = (regval & 0xF8) >> 1;
I think we want to use AMC6821_TEMP_LIMIT_MASK here instead of 0xF8?
I guess we could also use FIELD_GET?
- data->stat1 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT1);
- data->stat2 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT2);
+ regval &= 0x07;
I think we want to use AMC6821_TEMP_SLOPE_MASK instead of 0x07 here?
I guess we could also use FIELD_GET?
[...]
static ssize_t temp_auto_point_temp_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- struct i2c_client *client = data->client;
+ struct amc6821_data *data = dev_get_drvdata(dev);
int ix = to_sensor_dev_attr_2(attr)->index;
int nr = to_sensor_dev_attr_2(attr)->nr;
- u8 *ptemp;
- u8 reg;
- int dpwm;
+ struct regmap *regmap = data->regmap;
+ u8 temps[3], otemps[3];
long val;
- int ret = kstrtol(buf, 10, &val);
+ int ret;
+
+ ret = kstrtol(buf, 10, &val);
if (ret)
return ret;
- switch (nr) {
- case 1:
- ptemp = data->temp1_auto_point_temp;
- reg = AMC6821_REG_LTEMP_FAN_CTRL;
- break;
- case 2:
- ptemp = data->temp2_auto_point_temp;
- reg = AMC6821_REG_RTEMP_FAN_CTRL;
- break;
- default:
- dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
- return -EINVAL;
- }
-
mutex_lock(&data->update_lock);
- data->valid = false;
+
+ ret = amc6821_get_auto_point_temps(data->regmap, nr, temps);
+ if (ret)
+ goto unlock;
+ ret = amc6821_get_auto_point_temps(data->regmap, 1 - nr, otemps);
+ if (ret)
+ goto unlock;
We could reduce the scope of otemps since it's only used in the ix==0
case below.
switch (ix) {
case 0:
- ptemp[0] = clamp_val(val / 1000, 0,
- data->temp1_auto_point_temp[1]);
- ptemp[0] = clamp_val(ptemp[0], 0,
- data->temp2_auto_point_temp[1]);
- ptemp[0] = clamp_val(ptemp[0], 0, 63);
- if (i2c_smbus_write_byte_data(
- client,
- AMC6821_REG_PSV_TEMP,
- ptemp[0])) {
- dev_err(&client->dev,
- "Register write error, aborting.\n");
- count = -EIO;
- }
- goto EXIT;
+ /*
+ * Passive cooling temperature. Range limit against low limit
+ * of both channels.
+ */
+ val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 63000), 1000);
This was already in the original code, but I think 64°C should be doable
as well? The datasheet says:
"""
The PSV ranges from 0°C to +64°C.
"""
And there's a PSV8 bit we can write, meaning we can do (1 << 8) with a
step of 4°C which gives us 64°C? In a separate commit though, to not mix
too many fixes into one, making it easier for people to identify and
possibly revert them if necessary.
+ val = clamp_val(val, 0, min(temps[1], otemps[1]));
+ ret = regmap_write(regmap, AMC6821_REG_PSV_TEMP, val);
+ break;
case 1:
- ptemp[1] = clamp_val(val / 1000, (ptemp[0] & 0x7C) + 4, 124);
- ptemp[1] &= 0x7C;
- ptemp[2] = clamp_val(ptemp[2], ptemp[1] + 1, 255);
+ /*
+ * Low limit; must be between passive and high limit,
+ * and not exceed 124. Step size is 4 degrees C.
+ */
+ val = clamp_val(val, DIV_ROUND_UP(temps[0], 4) * 4000, 124000);
Oof. I think the issue is that we have different units for temps[0],
temps[1] and temps[2]?
temps[1] is in °C, while temps[0] is in °C >> 3 (so / 4) because we read
from PSV-Temp register directly, which only exposes PSV[8:3] and
PSV[2:0] are 0. I'm wondering if we shouldn't just have the same unit
when filled by amc6821_get_auto_point_temps?
temps[2] is also °C >> 3 (4°C step in the register). I think we would
benefit from having the same unit here to make it easier to do maths
with temps[1] and temps[0/2]. What do you think?
If we didn't have this °C >> 3 formula, we could simply divide by 1000
to get the value and then do the same maths for writing to the registers
(except a different offset for temps[0] than temps[1/2]).
+ temps[1] = DIV_ROUND_CLOSEST(val, 4000) * 4;
+ val = temps[1] / 4;
+ /* Auto-adjust high limit if necessary */
+ temps[2] = clamp_val(temps[2], temps[1] + 1, 255);
Is this why we didn't want 255 for temps[1]? Because then we could have
256 here?
+ ret = regmap_update_bits(regmap,
+ nr ? AMC6821_REG_RTEMP_FAN_CTRL
+ : AMC6821_REG_LTEMP_FAN_CTRL,
+ AMC6821_TEMP_LIMIT_MASK,
+ FIELD_PREP(AMC6821_TEMP_LIMIT_MASK, val));
+ if (ret)
+ break;
+ ret = set_slope_register(regmap, nr, temps);
I'm wondering if we shouldn't put the writes to the TEMP_LIMIT_MASK and
AMC6821_TEMP_SLOPE_MASK into the same regmap write, otherwise there's a
small timeframe during which the slope is not matching the TEMP_LIMIT. I
guess it's probably not that big of a deal but still bringing this up.
Cheers,
Quentin
