Message-ID: <e38ceaf9-c904-41f1-a32d-14a150c92f30@cherry.de>
Date: Mon, 8 Jul 2024 12:58:45 +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 v4 09/11] hwmon: (amc6821) Convert to use regmap

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

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

Quentin

Powered by blists - more mailing lists