| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7a829bf2-c5f6-4234-a224-52328045f581@cherry.de>
Date: Mon, 1 Jul 2024 15:01:57 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap
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@...ck-us.net>
> ---
> 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
Powered by blists - more mailing lists