Re: [PATCH v2 09/11] hwmon: (amc6821) Convert to use regmap
From: Guenter Roeck
Date: Wed Jul 03 2024 - 16:55:53 EST
On 7/3/24 09:19, Quentin Schulz wrote:
Hi Guenter,
On 7/1/24 11:23 PM, Guenter Roeck wrote:
Use regmap for register accesses and for most 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.
No functional change intended.
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
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 | 713 ++++++++++++++++++----------------------
2 files changed, 329 insertions(+), 385 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 028998d3bedf..3fe0bfeac843 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,15 +8,16 @@
* Copyright (C) 2007 Hans J. Koch <hjk@xxxxxxxxxxxx>
*/
+#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/module.h>
#include <linux/mutex.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
/*
@@ -44,6 +45,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 +63,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)
@@ -130,224 +129,169 @@ 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)
+static int amc6821_init_auto_point_data(struct amc6821_data *data)
{
- struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
- int timeout = HZ;
- u8 reg;
- int i;
+ struct regmap *regmap = data->regmap;
+ u32 pwm, regval;
+ int err;
- mutex_lock(&data->update_lock);
+ err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
I think this is incorrect logic.
amc6821_init_auto_point_data is only called once in init_client, in probe. While we currently do not write to AMC6821_REG_DCY_LOW_TEMP, we could in the future. But writing to it would desynchronise the auto_point_temp values for the new value of the register.
We do write the register, in pwm1_auto_point_pwm_store().
I suggest we put the logic into a function and return the value for a given temp_auto_point (1/2 currently) so that we are calling this function instead of a member in the struct so that we are never caching it unknowingly (regmap would do it for us in any case). It's a bit odd anyway to have only **those** values be cached in the struct as members and migrating everything else.
Yes, I know, that is a bit odd. Essentially I was too lazy to clean that up.
But, yes, you are correct, that results in the cached values being wrong
after AMC6821_REG_DCY_LOW_TEMP is updated. Back to the drawing board,
and thanks for finding this.
[ ... ]
default:
- dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
Was this an intended removal? I think we can afford keeping it in?
Yes, it is intentional. It (and the others below) indicate a coding
error, which would have been found during development. While it does make
sense to keep the default: case to make the compiler happy, it doesn't
make sense to keep the pointless message in the code.
@@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
const char *buf, size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
- int dpwm;
+ struct regmap *regmap = data->regmap;
u8 val;
- int ret = kstrtou8(buf, 10, &val);
+ int ret;
+
+ ret = kstrtou8(buf, 10, &val);
Not sure this cosmetic change is worth it? Or maybe squash with an earlier commit?
That slipped in, I think from the first patch of the series. I'll move it there.
if (ret)
return ret;
@@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
return -EINVAL;
mutex_lock(&data->update_lock);
- data->pwm1_auto_point_pwm[1] = val;
- if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
- data->pwm1_auto_point_pwm[1])) {
- dev_err(&client->dev, "Register write error, aborting.\n");
- count = -EIO;
- goto EXIT;
+ ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
+ if (ret)
I think we're missing a count = something here?
Yes.
+ goto unlock;
+
+ ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
+ data->temp1_auto_point_temp);
+ if (ret) {
+ count = ret;
In some places, we replace set_slope_register return code with -EIO (like it was) and sometimes we propagate it, like here. Is there some reason for this or can we have some kind of consistency across the code base here?
Thanks for noticing. The code should be consistent and always propagate the error code.
I'll fix it.
Thanks,
Guenter