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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <015776ca-a9e0-470f-bbf5-39cec4147c6b@cherry.de>
Date: Wed, 3 Jul 2024 18:19:15 +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 v2 09/11] hwmon: (amc6821) Convert to use regmap

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@...ck-us.net>
> ---
> 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@...sjkoch.de>
>    */
>   
> +#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.

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.

> +	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;
> +	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, &regval);
> +	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, &regval);
> +	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;
> +	int err;
>   
> -	return sprintf(buf, "%d\n", data->temp[ix] * 1000);
> +	err = regmap_read(data->regmap, temp_reg[ix], &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", sign_extend32(regval, 7) * 1000);
>   }
>   
>   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;
> +	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);

Was this an intended removal? I think we can afford keeping it in?

>   		return -EINVAL;
>   	}
> -	if (flag)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	err = regmap_read(data->regmap, reg, &regval);
> +	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;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> +	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;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", regval);
>   }
>   
>   static ssize_t pwm1_store(struct device *dev,
> @@ -355,24 +299,43 @@ 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;
>   	u8 val;
>   	int ret = kstrtou8(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   
> -	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;
> +
> +	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,
> @@ -380,49 +343,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 mode;
> +	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;
> +		mode = 0;
>   		break;
>   	case 2:
> -		config &= ~AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mode = AMC6821_CONF1_FDRC1;
>   		break;
>   	case 3:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mode = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
>   		break;
>   	case 4:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config &= ~AMC6821_CONF1_FDRC1;
> +		mode = 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,
> +				 mode);
> +	if (err)
> +		return err;
> +
>   	return count;
>   }
>   
> @@ -430,26 +381,45 @@ 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 sysfs_emit(buf, "%d\n", val);
>   }
>   
>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>   					 struct device_attribute *devattr,
>   					 char *buf)
>   {
> +	struct amc6821_data *data = dev_get_drvdata(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);
> +
>   	switch (nr) {
>   	case 1:
> -		return sprintf(buf, "%d\n",
> -			data->temp1_auto_point_temp[ix] * 1000);
> +		return sysfs_emit(buf, "%d\n",
> +				  data->temp1_auto_point_temp[ix] * 1000);
>   	case 2:
> -		return sprintf(buf, "%d\n",
> -			data->temp2_auto_point_temp[ix] * 1000);
> +		return sysfs_emit(buf, "%d\n",
> +				  data->temp2_auto_point_temp[ix] * 1000);
>   	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);

Ditto.

>   		return -EINVAL;
>   	}
>   }
> @@ -458,44 +428,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);
>   	int ix = to_sensor_dev_attr_2(attr)->index;
>   	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	struct regmap *regmap = data->regmap;
>   	u8 *ptemp;
>   	u8 reg;
> -	int dpwm;
>   	long val;
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
> @@ -511,12 +496,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		reg = AMC6821_REG_RTEMP_FAN_CTRL;
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);

Ditto.

>   		return -EINVAL;
>   	}
>   
>   	mutex_lock(&data->update_lock);
> -	data->valid = false;
>   
>   	switch (ix) {
>   	case 0:
> @@ -525,13 +508,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:
> @@ -543,12 +522,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		ptemp[2] = clamp_val(val / 1000, ptemp[1]+1, 255);
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);

Ditto.

>   		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:
> @@ -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?

>   	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?

> +		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?

Looks good to me otherwise.

Cheers,
Quentin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ