Re: [PATCH v4 09/11] hwmon: (amc6821) Convert to use regmap

From: Quentin Schulz
Date: Mon Jul 08 2024 - 06:59:04 EST

Hi Guenter,

On 7/5/24 11:35 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>
---
v4: Improve function comments
Better use of BIT(), FIELD_GET(), and FIELD_PREP()
Use min() instead of clamp_val() in fan_store() since it is already
known that the value is > 0
In pwm1_auto_point_pwm_store(), use '0' for unlimited to match
the original code
In set_slope_register(), set both temperature limit and
slope to avoid an extra register write operation when setting the
limit
In temp_auto_point_temp_store(), only read the other set of
temperature registers when writing the PSV temperature

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 | 822 +++++++++++++++++++---------------------
2 files changed, 381 insertions(+), 442 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..90efd6a0dfd3 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,158 @@ 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 0 on success or negative error code.
+ *
+ * temps returns set of three temperatures, in °C:
+ * temps[0]: Passive cooling temperature, applies to both channels
+ * temps[1]: Low temperature, start slope calculations
+ * temps[2]: High temperature
+ *
+ * Channel 0: local, channel 1: remote.
+ */
+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, &regval);
+ 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,
+ &regval);
+ if (err)
+ return err;
+ temps[1] = FIELD_GET(AMC6821_TEMP_LIMIT_MASK, regval) * 4;
- data->stat1 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT1);
- data->stat2 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT2);
+ regval = BIT(5) >> FIELD_GET(AMC6821_TEMP_SLOPE_MASK, regval);

BIT(5) doesn't have real meaning in the datasheet, what is in the datasheet though is that the slope is 32 / L-SLP[2:0] (well, you can more easily guess it from the datasheet than 0x20 or BIT(5) IMO).

[...]

+static inline int set_slope_register(struct regmap *regmap, int channel, u8 *temps)
{
- int dt;
- u8 tmp;
+ u8 regval = FIELD_PREP(AMC6821_TEMP_LIMIT_MASK, temps[1] / 4);
+ u8 tmp, dpwm;
+ int err, dt;
+ u32 pwm;
- dt = ptemp[2]-ptemp[1];
+ err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+ if (err)
+ return err;
+
+ dpwm = 255 - pwm;
+
+ dt = temps[2] - temps[1];
for (tmp = 4; tmp > 0; tmp--) {
- if (dt * (0x20 >> tmp) >= dpwm)
+ if (dt * (BIT(5) >> tmp) >= dpwm)

Ditto I think?

break;
}
- tmp |= (ptemp[1] & 0x7C) << 1;
- if (i2c_smbus_write_byte_data(client,
- reg, tmp)) {
- dev_err(&client->dev, "Register write error, aborting.\n");
- return -EIO;
- }
- return 0;
+ regval |= FIELD_PREP(AMC6821_TEMP_SLOPE_MASK, tmp);
+
+ return regmap_write(regmap,
+ channel ? AMC6821_REG_RTEMP_FAN_CTRL : AMC6821_REG_LTEMP_FAN_CTRL,
+ regval);
}
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;
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.
+ */
+ ret = amc6821_get_auto_point_temps(data->regmap, 1 - nr, otemps);

I would have been more comfortable with
nr ? 0 : 1

Only nitpicks, so:

Reviewed-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>

Thanks for making it easier for us to add some patches on top of AMC6821 soon, really appreciate it.

Quentin