Hi Guenter,
On 6/28/24 5:13 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.
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
drivers/hwmon/amc6821.c | 711 +++++++++++++++++++---------------------
1 file changed, 330 insertions(+), 381 deletions(-)
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index ebffc9393c3d..6ffab4288134 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -14,9 +14,9 @@
#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 +44,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 +62,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)
@@ -73,7 +71,7 @@ module_param(init, int, 0444);
#define AMC6821_CONF1_PWMINV BIT(3)
#define AMC6821_CONF1_FAN_FAULT_EN BIT(4)
#define AMC6821_CONF1_FDRC0 BIT(5)
-#define AMC6821_CONF1_FDRC1 BIT(7)
+#define AMC6821_CONF1_FDRC1 BIT(6)
This should have been squashed with a previous commit.
#define AMC6821_CONF1_THERMOVIE BIT(7)
#define AMC6821_CONF2_PWM_EN BIT(0)
@@ -130,224 +128,170 @@ 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);
+ 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;
+ data->temp1_auto_point_temp[0] = regval;
+ data->temp2_auto_point_temp[0] = data->temp1_auto_point_temp[0];
- 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, AMC6821_REG_LTEMP_FAN_CTRL, ®val);
+ if (err)
+ return err;
+ data->temp1_auto_point_temp[1] = (regval & 0xF8) >> 1;
- data->stat1 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT1);
- data->stat2 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_STAT2);
+ regval &= 0x07;
+ regval = 0x20 >> regval;
+ if (regval)
+ data->temp1_auto_point_temp[2] =
+ data->temp1_auto_point_temp[1] +
+ (255 - pwm) / regval;
+ else
+ data->temp1_auto_point_temp[2] = 255;
- data->pwm1 = i2c_smbus_read_byte_data(client,
- AMC6821_REG_DCY);
- for (i = 0; i < FAN1_IDX_LEN; i++) {
- data->fan[i] = i2c_smbus_read_byte_data(
- client,
- fan_reg_low[i]);
- data->fan[i] += i2c_smbus_read_byte_data(
- client,
- fan_reg_hi[i]) << 8;
- }
- data->fan1_pulses = i2c_smbus_read_byte_data(client,
- AMC6821_REG_CONF4);
- data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
+ err = regmap_read(regmap, AMC6821_REG_RTEMP_FAN_CTRL, ®val);
+ if (err)
+ return err;
- data->pwm1_auto_point_pwm[0] = 0;
- data->pwm1_auto_point_pwm[2] = 255;
- data->pwm1_auto_point_pwm[1] = i2c_smbus_read_byte_data(client,
- AMC6821_REG_DCY_LOW_TEMP);
+ data->temp2_auto_point_temp[1] = (regval & 0xF8) >> 1;
+ regval &= 0x07;
+ regval = 0x20 >> regval;
- data->temp1_auto_point_temp[0] =
- i2c_smbus_read_byte_data(client,
- AMC6821_REG_PSV_TEMP);
- data->temp2_auto_point_temp[0] =
- data->temp1_auto_point_temp[0];
- reg = i2c_smbus_read_byte_data(client,
- AMC6821_REG_LTEMP_FAN_CTRL);
- data->temp1_auto_point_temp[1] = (reg & 0xF8) >> 1;
- reg &= 0x07;
- reg = 0x20 >> reg;
- if (reg > 0)
- data->temp1_auto_point_temp[2] =
- data->temp1_auto_point_temp[1] +
- (data->pwm1_auto_point_pwm[2] -
- data->pwm1_auto_point_pwm[1]) / reg;
- else
- data->temp1_auto_point_temp[2] = 255;
+ if (regval)
+ data->temp2_auto_point_temp[2] =
+ data->temp2_auto_point_temp[1] +
+ (255 - pwm) / regval;
+ else
+ data->temp2_auto_point_temp[2] = 255;
- reg = i2c_smbus_read_byte_data(client,
- AMC6821_REG_RTEMP_FAN_CTRL);
- data->temp2_auto_point_temp[1] = (reg & 0xF8) >> 1;
- reg &= 0x07;
- reg = 0x20 >> reg;
- if (reg > 0)
- data->temp2_auto_point_temp[2] =
- data->temp2_auto_point_temp[1] +
- (data->pwm1_auto_point_pwm[2] -
- data->pwm1_auto_point_pwm[1]) / reg;
- else
- data->temp2_auto_point_temp[2] = 255;
-
- reg = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
- reg = (reg >> 5) & 0x3;
- switch (reg) {
- case 0: /*open loop: software sets pwm1*/
- data->pwm1_auto_channels_temp = 0;
- data->pwm1_enable = 1;
- break;
- case 2: /*closed loop: remote T (temp2)*/
- data->pwm1_auto_channels_temp = 2;
- data->pwm1_enable = 2;
- break;
- case 3: /*closed loop: local and remote T (temp2)*/
- data->pwm1_auto_channels_temp = 3;
- data->pwm1_enable = 3;
- break;
- case 1: /*
- * semi-open loop: software sets rpm, chip controls
- * pwm1
- */
- data->pwm1_auto_channels_temp = 0;
- data->pwm1_enable = 4;
- break;
- }
-
- data->last_updated = jiffies;
- data->valid = true;
- }
- mutex_unlock(&data->update_lock);
- return data;
+ return 0;
}
static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
+ struct amc6821_data *data = dev_get_drvdata(dev);
int ix = to_sensor_dev_attr(devattr)->index;
+ u32 regval;
Why not a u8 directly here? We know single reads are going to return a
u8 so no need to store more?
I'm not too fluent in type conversion, but maybe even an s8 since this
is actually what's stored in the register?
+ int err;
- return sprintf(buf, "%d\n", data->temp[ix] * 1000);
+ err = regmap_read(data->regmap, temp_reg[ix], ®val);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
The type casting choice is odd here. Considering we'll be printing an
int and that 1000 cannot be stored in an int8_t, maybe just cast it to
an int directly?
}
static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
int ix = to_sensor_dev_attr(attr)->index;
long val;
+ int err;
int ret = kstrtol(buf, 10, &val);
if (ret)
return ret;
val = clamp_val(val / 1000, -128, 127);
- mutex_lock(&data->update_lock);
- data->temp[ix] = val;
- if (i2c_smbus_write_byte_data(client, temp_reg[ix], data->temp[ix])) {
- dev_err(&client->dev, "Register write error, aborting.\n");
- count = -EIO;
- }
- mutex_unlock(&data->update_lock);
+ err = regmap_write(data->regmap, temp_reg[ix], val);
+ if (err)
+ return err;
+
return count;
}
static ssize_t temp_alarm_show(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
+ struct amc6821_data *data = dev_get_drvdata(dev);
int ix = to_sensor_dev_attr(devattr)->index;
- u8 flag;
+ u32 regval, mask, reg;
Why not u8 for regval?
+ int err;
switch (ix) {
case IDX_TEMP1_MIN:
- flag = data->stat1 & AMC6821_STAT1_LTL;
+ reg = AMC6821_REG_STAT1;
+ mask = AMC6821_STAT1_LTL;
break;
case IDX_TEMP1_MAX:
- flag = data->stat1 & AMC6821_STAT1_LTH;
+ reg = AMC6821_REG_STAT1;
+ mask = AMC6821_STAT1_LTH;
break;
case IDX_TEMP1_CRIT:
- flag = data->stat2 & AMC6821_STAT2_LTC;
+ reg = AMC6821_REG_STAT2;
+ mask = AMC6821_STAT2_LTC;
break;
case IDX_TEMP2_MIN:
- flag = data->stat1 & AMC6821_STAT1_RTL;
+ reg = AMC6821_REG_STAT1;
+ mask = AMC6821_STAT1_RTL;
break;
case IDX_TEMP2_MAX:
- flag = data->stat1 & AMC6821_STAT1_RTH;
+ reg = AMC6821_REG_STAT1;
+ mask = AMC6821_STAT1_RTH;
break;
case IDX_TEMP2_CRIT:
- flag = data->stat2 & AMC6821_STAT2_RTC;
+ reg = AMC6821_REG_STAT2;
+ mask = AMC6821_STAT2_RTC;
break;
default:
dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
return -EINVAL;
}
- if (flag)
- return sprintf(buf, "1");
- else
- return sprintf(buf, "0");
+ err = regmap_read(data->regmap, reg, ®val);
+ if (err)
+ return err;
+ return sysfs_emit(buf, "%d\n", !!(regval & mask));
}
static ssize_t temp2_fault_show(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- if (data->stat1 & AMC6821_STAT1_RTF)
- return sprintf(buf, "1");
- else
- return sprintf(buf, "0");
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ u32 regval;
Ditto.
+ int err;
+
+ err = regmap_read(data->regmap, AMC6821_REG_STAT1, ®val);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
}
static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- return sprintf(buf, "%d\n", data->pwm1);
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ u32 regval;
Ditto.
+ int err;
+
+ err = regmap_read(data->regmap, AMC6821_REG_DCY, ®val);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%d\n", regval);
}
static ssize_t pwm1_store(struct device *dev,
@@ -355,7 +299,6 @@ static ssize_t pwm1_store(struct device *dev,
size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
long val;
int ret = kstrtol(buf, 10, &val);
if (ret)
@@ -364,18 +307,38 @@ static ssize_t pwm1_store(struct device *dev,
if (val < 0 || val > 255)
return -EINVAL;
- mutex_lock(&data->update_lock);
- data->pwm1 = val;
- i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
- mutex_unlock(&data->update_lock);
+ ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
+ if (ret)
+ return ret;
+
return count;
}
static ssize_t pwm1_enable_show(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- return sprintf(buf, "%d\n", data->pwm1_enable);
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ int err;
+ u32 val;
Ditto.
+
+ err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+ if (err)
+ return err;
+ switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+ case 0:
+ val = 1; /* manual */
+ break;
+ case AMC6821_CONF1_FDRC0:
+ val = 4; /* target rpm (fan1_target) controlled */
+ break;
+ case AMC6821_CONF1_FDRC1:
+ val = 2; /* remote temp controlled */
+ break;
+ default:
+ val = 3; /* max(local, remote) temp controlled */
+ break;
+ }
+ return sysfs_emit(buf, "%d\n", val);
}
static ssize_t pwm1_enable_store(struct device *dev,
@@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
const char *buf, size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
long val;
- int config = kstrtol(buf, 10, &val);
- if (config)
- return config;
+ u32 mask;
Please rename to something else, e.g. val, as this is NOT used as a mask
but rather the value to write in the masked bitfield (which is hardcoded
to AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1.
+ int err;
- mutex_lock(&data->update_lock);
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
- count = config;
- goto unlock;
- }
+ err = kstrtol(buf, 10, &val);
+ if (err)
+ return err;
switch (val) {
case 1:
- config &= ~AMC6821_CONF1_FDRC0;
- config &= ~AMC6821_CONF1_FDRC1;
+ mask = 0;
break;
case 2:
- config &= ~AMC6821_CONF1_FDRC0;
- config |= AMC6821_CONF1_FDRC1;
+ mask = AMC6821_CONF1_FDRC1;
break;
case 3:
- config |= AMC6821_CONF1_FDRC0;
- config |= AMC6821_CONF1_FDRC1;
+ mask = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
break;
case 4:
- config |= AMC6821_CONF1_FDRC0;
- config &= ~AMC6821_CONF1_FDRC1;
+ mask = AMC6821_CONF1_FDRC0;
break;
default:
- count = -EINVAL;
- goto unlock;
+ return -EINVAL;
}
- if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF1, config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
- count = -EIO;
- }
-unlock:
- mutex_unlock(&data->update_lock);
+
+ err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
+ AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
+ mask);
+ if (err < 0)
Shouldn't we check for err != 0 (so err) instead to be consistent with
the rest of the code base in the driver?
+ return err;
+
return count;
}
@@ -433,8 +384,27 @@ static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- return sprintf(buf, "%d\n", data->pwm1_auto_channels_temp);
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ u32 val;
+ int err;
+
+ err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+ if (err)
+ return err;
+ switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+ case 0:
+ case AMC6821_CONF1_FDRC0:
+ val = 0; /* manual or target rpm controlled */
+ break;
+ case AMC6821_CONF1_FDRC1:
+ val = 2; /* remote temp controlled */
+ break;
+ default:
+ val = 3; /* max(local, remote) temp controlled */
+ break;
+ }
+
+ return sprintf(buf, "%d\n", val);
}
static ssize_t temp_auto_point_temp_show(struct device *dev,
@@ -443,7 +413,8 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
{
int ix = to_sensor_dev_attr_2(devattr)->index;
int nr = to_sensor_dev_attr_2(devattr)->nr;
- struct amc6821_data *data = amc6821_update_device(dev);
+ struct amc6821_data *data = dev_get_drvdata(dev);
+
switch (nr) {
case 1:
return sprintf(buf, "%d\n",
@@ -461,44 +432,59 @@ static ssize_t pwm1_auto_point_pwm_show(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
+ struct amc6821_data *data = dev_get_drvdata(dev);
int ix = to_sensor_dev_attr(devattr)->index;
- struct amc6821_data *data = amc6821_update_device(dev);
- return sprintf(buf, "%d\n", data->pwm1_auto_point_pwm[ix]);
+ u32 val;
+ int err;
+
+ switch (ix) {
+ case 0:
+ val = 0;
+ break;
+ case 1:
+ err = regmap_read(data->regmap, AMC6821_REG_DCY_LOW_TEMP, &val);
+ if (err)
+ return err;
+ break;
+ default:
+ val = 255;
+ break;
+ }
+ return sysfs_emit(buf, "%d\n", val);
}
-static inline ssize_t set_slope_register(struct i2c_client *client,
- u8 reg,
- u8 dpwm,
- u8 *ptemp)
+static inline int set_slope_register(struct regmap *regmap,
+ u8 reg, u8 *ptemp)
{
- int dt;
- u8 tmp;
+ 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 = ptemp[2] - ptemp[1];
for (tmp = 4; tmp > 0; tmp--) {
if (dt * (0x20 >> tmp) >= dpwm)
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;
+ return regmap_write(regmap, reg, tmp);
}
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);
+ struct regmap *regmap = data->regmap;
int ix = to_sensor_dev_attr_2(attr)->index;
int nr = to_sensor_dev_attr_2(attr)->nr;
u8 *ptemp;
u8 reg;
- int dpwm;
long val;
int ret = kstrtol(buf, 10, &val);
if (ret)
@@ -519,7 +505,6 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
}
mutex_lock(&data->update_lock);
- data->valid = false;
switch (ix) {
case 0:
@@ -528,13 +513,9 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
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;
+ if (regmap_write(regmap, AMC6821_REG_PSV_TEMP, ptemp[0])) {
+ dev_err(dev, "Register write error, aborting.\n");
+ count = -EIO;
}
goto EXIT;
case 1:
@@ -550,8 +531,7 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
count = -EINVAL;
goto EXIT;
}
- dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
- if (set_slope_register(client, reg, dpwm, ptemp))
+ if (set_slope_register(regmap, reg, ptemp))
count = -EIO;
EXIT:
@@ -564,38 +544,37 @@ 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;
- unsigned long val;
- int ret = kstrtoul(buf, 10, &val);
+ struct regmap *regmap = data->regmap;
+ long val;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &val);
if (ret)
return ret;
- if (val > 255)
+ if (val > 254)
I think this should have been squashed with an earlier commit?
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)
+ goto unlock;
+
+ ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
+ data->temp1_auto_point_temp);
+ if (ret) {
+ count = ret;
+ goto unlock;
}
- dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
- if (set_slope_register(client, AMC6821_REG_LTEMP_FAN_CTRL, dpwm,
- data->temp1_auto_point_temp)) {
- count = -EIO;
- goto EXIT;
- }
- if (set_slope_register(client, AMC6821_REG_RTEMP_FAN_CTRL, dpwm,
- data->temp2_auto_point_temp)) {
- count = -EIO;
- goto EXIT;
+ ret = set_slope_register(regmap, AMC6821_REG_RTEMP_FAN_CTRL,
+ data->temp2_auto_point_temp);
+ if (ret) {
+ count = ret;
+ goto unlock;
}
-EXIT:
- data->valid = false;
+unlock:
mutex_unlock(&data->update_lock);
return count;
}
@@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
+ struct amc6821_data *data = dev_get_drvdata(dev);
int ix = to_sensor_dev_attr(devattr)->index;
- if (0 == data->fan[ix])
- return sprintf(buf, "6000000");
- return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
+ u32 regval;
+ u8 regs[2];
Can't this be a u16 directly.......
+ int err;
+
+ err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
+ if (err)
+ return err;
+ regval = (regs[1] << 8) | regs[0];
+
..... to avoid doing this here?
+ return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
}
static ssize_t fan1_fault_show(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- if (data->stat1 & AMC6821_STAT1_FANS)
- return sprintf(buf, "1");
- else
- return sprintf(buf, "0");
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ u32 regval;
+ int err;
+
+ err = regmap_read(data->regmap, AMC6821_REG_STAT1, ®val);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
}
static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
- long val;
int ix = to_sensor_dev_attr(attr)->index;
- int ret = kstrtol(buf, 10, &val);
- if (ret)
- return ret;
- val = val < 1 ? 0xFFFF : 6000000 / val;
+ u8 regs[2];
Ditto.
+ long val;
+ int err;
+
+ err = kstrtol(buf, 10, &val);
+ if (err)
+ return err;
+
+ val = val < 1 ? 0xFFFF : 6000000 / val;
+ val = clamp_val(val, 0, 0xFFFF);
+
+ regs[0] = val & 0xff;
+ regs[1] = val >> 8;
+
to avoid this here.
+ err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
+ if (err)
+ return err;
- mutex_lock(&data->update_lock);
- data->fan[ix] = (u16)clamp_val(val, 0, 0xFFFF);
- if (i2c_smbus_write_byte_data(client, fan_reg_low[ix],
- data->fan[ix] & 0xFF)) {
- dev_err(&client->dev, "Register write error, aborting.\n");
- count = -EIO;
- goto EXIT;
- }
- if (i2c_smbus_write_byte_data(client,
- fan_reg_hi[ix], data->fan[ix] >> 8)) {
- dev_err(&client->dev, "Register write error, aborting.\n");
- count = -EIO;
- }
-EXIT:
- mutex_unlock(&data->update_lock);
return count;
}
static ssize_t fan1_pulses_show(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct amc6821_data *data = amc6821_update_device(dev);
- return sprintf(buf, "%d\n", data->fan1_pulses);
+ struct amc6821_data *data = dev_get_drvdata(dev);
+ u32 regval;
+ int err;
+
+ err = regmap_read(data->regmap, AMC6821_REG_CONF4, ®val);
+ if (err)
+ return err;
+
+ return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
}
static ssize_t fan1_pulses_store(struct device *dev,
@@ -662,40 +655,22 @@ static ssize_t fan1_pulses_store(struct device *dev,
size_t count)
{
struct amc6821_data *data = dev_get_drvdata(dev);
- struct i2c_client *client = data->client;
long val;
- int config = kstrtol(buf, 10, &val);
- if (config)
- return config;
+ int err;
+
+ err = kstrtol(buf, 10, &val);
+ if (err)
+ return err;
+
+ if (val != 2 && val != 4)
+ return -EINVAL;
+
+ err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
+ AMC6821_CONF4_PSPR,
+ val == 4 ? AMC6821_CONF4_PSPR : 0);
+ if (err)
+ return err;
- mutex_lock(&data->update_lock);
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
- count = config;
- goto EXIT;
- }
- switch (val) {
- case 2:
- config &= ~AMC6821_CONF4_PSPR;
- data->fan1_pulses = 2;
- break;
- case 4:
- config |= AMC6821_CONF4_PSPR;
- data->fan1_pulses = 4;
- break;
- default:
- count = -EINVAL;
- goto EXIT;
- }
- if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4, config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
- count = -EIO;
- }
-EXIT:
- mutex_unlock(&data->update_lock);
return count;
}
@@ -827,110 +802,84 @@ static int amc6821_detect(
return 0;
}
-static int amc6821_init_client(struct i2c_client *client)
+static int amc6821_init_client(struct amc6821_data *data)
{
- int config;
- int err = -EIO;
+ struct regmap *regmap = data->regmap;
+ int err;
+
+ err = amc6821_init_auto_point_data(data);
+ if (err)
+ return err;
if (init) {
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
-
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
- return err;
- }
-
- config |= AMC6821_CONF4_MODE;
-
- if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4,
- config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
+ err = regmap_set_bits(regmap, AMC6821_REG_CONF4, AMC6821_CONF4_MODE);
+ if (err)
return err;
- }
-
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF3);
-
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
+ err = regmap_clear_bits(regmap, AMC6821_REG_CONF3, AMC6821_CONF3_THERM_FAN_EN);
+ if (err)
return err;
- }
-
- dev_info(&client->dev, "Revision %d\n", config & 0x0f);
-
- config &= ~AMC6821_CONF3_THERM_FAN_EN;
-
- if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF3,
- config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
+ err = regmap_clear_bits(regmap, AMC6821_REG_CONF2,
+ AMC6821_CONF2_RTFIE |
+ AMC6821_CONF2_LTOIE |
+ AMC6821_CONF2_RTOIE);
+ if (err)
return err;
- }
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF2);
-
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
+ err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
+ AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
+ AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
+ AMC6821_CONF1_START |
+ (pwminv ? AMC6821_CONF1_PWMINV : 0));
+ if (err)
return err;
- }
-
- config &= ~AMC6821_CONF2_RTFIE;
- config &= ~AMC6821_CONF2_LTOIE;
- config &= ~AMC6821_CONF2_RTOIE;
- if (i2c_smbus_write_byte_data(client,
- AMC6821_REG_CONF2, config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
- return err;
- }
-
- config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-
- if (config < 0) {
- dev_err(&client->dev,
- "Error reading configuration register, aborting.\n");
- return err;
- }
-
- config &= ~AMC6821_CONF1_THERMOVIE;
- config &= ~AMC6821_CONF1_FANIE;
- config |= AMC6821_CONF1_START;
- if (pwminv)
- config |= AMC6821_CONF1_PWMINV;
- else
- config &= ~AMC6821_CONF1_PWMINV;
-
- if (i2c_smbus_write_byte_data(
- client, AMC6821_REG_CONF1, config)) {
- dev_err(&client->dev,
- "Configuration register write error, aborting.\n");
- return err;
- }
}
return 0;
}
+static bool amc6821_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case AMC6821_REG_STAT1:
+ case AMC6821_REG_STAT2:
+ case AMC6821_REG_TEMP_LO:
+ case AMC6821_REG_TDATA_LOW: > + case AMC6821_REG_LTEMP_HI:
+ case AMC6821_REG_RTEMP_HI:
+ case AMC6821_REG_TDATA_HI:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config amc6821_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = AMC6821_REG_CONF3,
+ .volatile_reg = amc6821_volatile_reg,
+ .cache_type = REGCACHE_MAPLE,
+};
+
static int amc6821_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct amc6821_data *data;
struct device *hwmon_dev;
+ struct regmap *regmap;
Save this variable by using data->client directly?
Cheers,
Quentin
