[PATCH 1/3] hwmon: (adm9240) Drop log messages from detect function

[PATCH 1/3] hwmon: (adm9240) Drop log messages from detect function

[Guenter Roeck]

Not detecting a chip in the detect function is normal and should not
generate any log messages, much less error messages.

Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/adm9240.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index cc3e0184e720..3bbdd662c9e4 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -730,26 +730,19 @@ static int adm9240_detect(struct i2c_client *new_client,
 		return -ENODEV;
 
 	/* verify chip: reg address should match i2c address */
-	if (i2c_smbus_read_byte_data(new_client, ADM9240_REG_I2C_ADDR)
-			!= address) {
-		dev_err(&adapter->dev, "detect fail: address match, 0x%02x\n",
-			address);
+	if (i2c_smbus_read_byte_data(new_client, ADM9240_REG_I2C_ADDR) != address)
 		return -ENODEV;
-	}
 
 	/* check known chip manufacturer */
 	man_id = i2c_smbus_read_byte_data(new_client, ADM9240_REG_MAN_ID);
-	if (man_id == 0x23) {
+	if (man_id == 0x23)
 		name = "adm9240";
-	} else if (man_id == 0xda) {
+	else if (man_id == 0xda)
 		name = "ds1780";
-	} else if (man_id == 0x01) {
+	else if (man_id == 0x01)
 		name = "lm81";
-	} else {
-		dev_err(&adapter->dev, "detect fail: unknown manuf, 0x%02x\n",
-			man_id);
+	else
 		return -ENODEV;
-	}
 
 	/* successful detect, print chip info */
 	die_rev = i2c_smbus_read_byte_data(new_client, ADM9240_REG_DIE_REV);
-- 
2.17.1

[PATCH 2/3] hwmon: (adm9240) Store i2c device instead of client in local data

[Guenter Roeck]

We only use the pointer to i2c_client to access &client->dev.
Store the device pointer directly instead of retrieving it
from i2c_client.

Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/adm9240.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 3bbdd662c9e4..7404082c7a3f 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -123,7 +123,7 @@ static inline unsigned int AOUT_FROM_REG(u8 reg)
 
 /* per client data */
 struct adm9240_data {
-	struct i2c_client *client;
+	struct device *dev;
 	struct regmap *regmap;
 	struct mutex update_lock;
 	char valid;
@@ -160,7 +160,7 @@ static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
 	err = regmap_write(data->regmap, ADM9240_REG_VID_FAN_DIV, reg);
 	if (err < 0)
 		return err;
-	dev_dbg(&data->client->dev,
+	dev_dbg(data->dev,
 		"fan%d clock divider changed from %u to %u\n",
 		nr + 1, 1 << old, 1 << fan_div);
 
@@ -507,7 +507,6 @@ static ssize_t fan_min_store(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct adm9240_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int nr = attr->index;
 	u8 new_div;
 	unsigned long val;
@@ -523,14 +522,14 @@ static ssize_t fan_min_store(struct device *dev,
 		data->fan_min[nr] = 255;
 		new_div = data->fan_div[nr];
 
-		dev_dbg(&client->dev, "fan%u low limit set disabled\n",
+		dev_dbg(data->dev, "fan%u low limit set disabled\n",
 				nr + 1);
 
 	} else if (val < 1350000 / (8 * 254)) {
 		new_div = 3;
 		data->fan_min[nr] = 254;
 
-		dev_dbg(&client->dev, "fan%u low limit set minimum %u\n",
+		dev_dbg(data->dev, "fan%u low limit set minimum %u\n",
 				nr + 1, FAN_FROM_REG(254, 1 << new_div));
 
 	} else {
@@ -546,7 +545,7 @@ static ssize_t fan_min_store(struct device *dev,
 
 		data->fan_min[nr] = new_min;
 
-		dev_dbg(&client->dev, "fan%u low limit set fan speed %u\n",
+		dev_dbg(data->dev, "fan%u low limit set fan speed %u\n",
 				nr + 1, FAN_FROM_REG(new_min, 1 << new_div));
 	}
 
@@ -663,7 +662,7 @@ static ssize_t alarm_store(struct device *dev, struct device_attribute *attr,
 	mutex_unlock(&data->update_lock);
 	if (err < 0)
 		return err;
-	dev_dbg(&data->client->dev, "chassis intrusion latch cleared\n");
+	dev_dbg(data->dev, "chassis intrusion latch cleared\n");
 
 	return count;
 }
@@ -755,7 +754,7 @@ static int adm9240_detect(struct i2c_client *new_client,
 	return 0;
 }
 
-static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *data)
+static int adm9240_init_client(struct adm9240_data *data)
 {
 	u8 conf, mode;
 	int err;
@@ -770,13 +769,13 @@ static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *d
 
 	data->vrm = vid_which_vrm(); /* need this to report vid as mV */
 
-	dev_info(&client->dev, "Using VRM: %d.%d\n", data->vrm / 10,
-			data->vrm % 10);
+	dev_info(data->dev, "Using VRM: %d.%d\n", data->vrm / 10,
+		 data->vrm % 10);
 
 	if (conf & 1) { /* measurement cycle running: report state */
 
-		dev_info(&client->dev, "status: config 0x%02x mode %u\n",
-				conf, mode);
+		dev_info(data->dev, "status: config 0x%02x mode %u\n",
+			 conf, mode);
 
 	} else { /* cold start: open limits before starting chip */
 		int i;
@@ -809,7 +808,7 @@ static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *d
 		if (err < 0)
 			return err;
 
-		dev_info(&client->dev,
+		dev_info(data->dev,
 			 "cold start: config was 0x%02x mode %u\n", conf, mode);
 	}
 
@@ -834,13 +833,13 @@ static int adm9240_probe(struct i2c_client *new_client)
 	if (!data)
 		return -ENOMEM;
 
-	data->client = new_client;
+	data->dev = dev;
 	mutex_init(&data->update_lock);
 	data->regmap = devm_regmap_init_i2c(new_client, &adm9240_regmap_config);
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
-	err = adm9240_init_client(new_client, data);
+	err = adm9240_init_client(data);
 	if (err < 0)
 		return err;
 
-- 
2.17.1

[PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Guenter Roeck]

Also use regmap for register caching. This change reduces code and
data size by more than 40%.

While at it, fixed some warnings reported by checkpatch.

Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/adm9240.c | 940 +++++++++++++++++++---------------------
 1 file changed, 445 insertions(+), 495 deletions(-)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 7404082c7a3f..5677263bcf0d 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -28,6 +28,7 @@
  * LM81 extended temp reading not implemented
  */
 
+#include <linux/bits.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -37,7 +38,6 @@
 #include <linux/hwmon-vid.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
-#include <linux/jiffies.h>
 #include <linux/regmap.h>
 
 /* Addresses to scan */
@@ -126,29 +126,15 @@ struct adm9240_data {
 	struct device *dev;
 	struct regmap *regmap;
 	struct mutex update_lock;
-	char valid;
-	unsigned long last_updated_measure;
-	unsigned long last_updated_config;
-
-	u8 in[6];		/* ro	in0_input */
-	u8 in_max[6];		/* rw	in0_max */
-	u8 in_min[6];		/* rw	in0_min */
-	u8 fan[2];		/* ro	fan1_input */
-	u8 fan_min[2];		/* rw	fan1_min */
+
 	u8 fan_div[2];		/* rw	fan1_div, read-only accessor */
-	s16 temp;		/* ro	temp1_input, 9-bit sign-extended */
-	s8 temp_max[2];		/* rw	0 -> temp_max, 1 -> temp_max_hyst */
-	u16 alarms;		/* ro	alarms */
-	u8 aout;		/* rw	aout_output */
-	u8 vid;			/* ro	vid */
 	u8 vrm;			/* --	vrm set on startup, no accessor */
 };
 
 /* write new fan div, callers must hold data->update_lock */
-static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
-		u8 fan_div)
+static int adm9240_write_fan_div(struct adm9240_data *data, int channel, u8 fan_div)
 {
-	unsigned int reg, old, shift = (nr + 2) * 2;
+	unsigned int reg, old, shift = (channel + 2) * 2;
 	int err;
 
 	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &reg);
@@ -161,335 +147,12 @@ static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
 	if (err < 0)
 		return err;
 	dev_dbg(data->dev,
-		"fan%d clock divider changed from %u to %u\n",
-		nr + 1, 1 << old, 1 << fan_div);
+		"fan%d clock divider changed from %lu to %lu\n",
+		channel + 1, BIT(old), BIT(fan_div));
 
 	return 0;
 }
 
-static int adm9240_update_measure(struct adm9240_data *data)
-{
-	unsigned int val;
-	u8 regs[2];
-	int err;
-	int i;
-
-	err = regmap_bulk_read(data->regmap, ADM9240_REG_IN(0), &data->in[0], 6);
-	if (err < 0)
-		return err;
-	err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
-	if (err < 0)
-		return err;
-
-	data->alarms = regs[0] | regs[1] << 8;
-
-	/*
-	 * read temperature: assume temperature changes less than
-	 * 0.5'C per two measurement cycles thus ignore possible
-	 * but unlikely aliasing error on lsb reading. --Grant
-	 */
-	err = regmap_read(data->regmap, ADM9240_REG_TEMP, &val);
-	if (err < 0)
-		return err;
-	data->temp = val << 8;
-	err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &val);
-	if (err < 0)
-		return err;
-	data->temp |= val;
-
-	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN(0),
-			       &data->fan[0], 2);
-	if (err < 0)
-		return err;
-
-	for (i = 0; i < 2; i++) { /* read fans */
-		/* adjust fan clock divider on overflow */
-		if (data->valid && data->fan[i] == 255 &&
-				data->fan_div[i] < 3) {
-
-			err = adm9240_write_fan_div(data, i,
-					++data->fan_div[i]);
-			if (err < 0)
-				return err;
-
-			/* adjust fan_min if active, but not to 0 */
-			if (data->fan_min[i] < 255 &&
-					data->fan_min[i] >= 2)
-				data->fan_min[i] /= 2;
-		}
-	}
-
-	return 0;
-}
-
-static int adm9240_update_config(struct adm9240_data *data)
-{
-	unsigned int val;
-	int i;
-	int err;
-
-	for (i = 0; i < 6; i++) {
-		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MIN(i),
-				      &data->in_min[i], 1);
-		if (err < 0)
-			return err;
-		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MAX(i),
-				      &data->in_max[i], 1);
-		if (err < 0)
-			return err;
-	}
-	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN_MIN(0),
-				      &data->fan_min[0], 2);
-	if (err < 0)
-		return err;
-	err = regmap_bulk_read(data->regmap, ADM9240_REG_TEMP_MAX(0),
-				      &data->temp_max[0], 2);
-	if (err < 0)
-		return err;
-
-	/* read fan divs and 5-bit VID */
-	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &val);
-	if (err < 0)
-		return err;
-	data->fan_div[0] = (val >> 4) & 3;
-	data->fan_div[1] = (val >> 6) & 3;
-	data->vid = val & 0x0f;
-	err = regmap_read(data->regmap, ADM9240_REG_VID4, &val);
-	if (err < 0)
-		return err;
-	data->vid |= (val & 1) << 4;
-	/* read analog out */
-	err = regmap_raw_read(data->regmap, ADM9240_REG_ANALOG_OUT,
-			      &data->aout, 1);
-
-	return err;
-}
-
-static struct adm9240_data *adm9240_update_device(struct device *dev)
-{
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	int err;
-
-	mutex_lock(&data->update_lock);
-
-	/* minimum measurement cycle: 1.75 seconds */
-	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
-			|| !data->valid) {
-		err = adm9240_update_measure(data);
-		if (err < 0) {
-			data->valid = 0;
-			mutex_unlock(&data->update_lock);
-			return ERR_PTR(err);
-		}
-		data->last_updated_measure = jiffies;
-	}
-
-	/* minimum config reading cycle: 300 seconds */
-	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
-			|| !data->valid) {
-		err = adm9240_update_config(data);
-		if (err < 0) {
-			data->valid = 0;
-			mutex_unlock(&data->update_lock);
-			return ERR_PTR(err);
-		}
-		data->last_updated_config = jiffies;
-		data->valid = 1;
-	}
-	mutex_unlock(&data->update_lock);
-	return data;
-}
-
-/*** sysfs accessors ***/
-
-/* temperature */
-static ssize_t temp1_input_show(struct device *dev,
-				struct device_attribute *dummy, char *buf)
-{
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", data->temp / 128 * 500); /* 9-bit value */
-}
-
-static ssize_t max_show(struct device *dev, struct device_attribute *devattr,
-			char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
-}
-
-static ssize_t max_store(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	mutex_lock(&data->update_lock);
-	data->temp_max[attr->index] = TEMP_TO_REG(val);
-	err = regmap_write(data->regmap, ADM9240_REG_TEMP_MAX(attr->index),
-			   data->temp_max[attr->index]);
-	mutex_unlock(&data->update_lock);
-	return err < 0 ? err : count;
-}
-
-static DEVICE_ATTR_RO(temp1_input);
-static SENSOR_DEVICE_ATTR_RW(temp1_max, max, 0);
-static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, max, 1);
-
-/* voltage */
-static ssize_t in_show(struct device *dev, struct device_attribute *devattr,
-		       char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[attr->index],
-				attr->index));
-}
-
-static ssize_t in_min_show(struct device *dev,
-			   struct device_attribute *devattr, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[attr->index],
-				attr->index));
-}
-
-static ssize_t in_max_show(struct device *dev,
-			   struct device_attribute *devattr, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[attr->index],
-				attr->index));
-}
-
-static ssize_t in_min_store(struct device *dev,
-			    struct device_attribute *devattr, const char *buf,
-			    size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	unsigned long val;
-	int err;
-
-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
-
-	mutex_lock(&data->update_lock);
-	data->in_min[attr->index] = IN_TO_REG(val, attr->index);
-	err = regmap_write(data->regmap, ADM9240_REG_IN_MIN(attr->index),
-			   data->in_min[attr->index]);
-	mutex_unlock(&data->update_lock);
-	return err < 0 ? err : count;
-}
-
-static ssize_t in_max_store(struct device *dev,
-			    struct device_attribute *devattr, const char *buf,
-			    size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	unsigned long val;
-	int err;
-
-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
-
-	mutex_lock(&data->update_lock);
-	data->in_max[attr->index] = IN_TO_REG(val, attr->index);
-	err = regmap_write(data->regmap, ADM9240_REG_IN_MAX(attr->index),
-			   data->in_max[attr->index]);
-	mutex_unlock(&data->update_lock);
-	return err < 0 ? err : count;
-}
-
-static SENSOR_DEVICE_ATTR_RO(in0_input, in, 0);
-static SENSOR_DEVICE_ATTR_RW(in0_min, in_min, 0);
-static SENSOR_DEVICE_ATTR_RW(in0_max, in_max, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_input, in, 1);
-static SENSOR_DEVICE_ATTR_RW(in1_min, in_min, 1);
-static SENSOR_DEVICE_ATTR_RW(in1_max, in_max, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_input, in, 2);
-static SENSOR_DEVICE_ATTR_RW(in2_min, in_min, 2);
-static SENSOR_DEVICE_ATTR_RW(in2_max, in_max, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_input, in, 3);
-static SENSOR_DEVICE_ATTR_RW(in3_min, in_min, 3);
-static SENSOR_DEVICE_ATTR_RW(in3_max, in_max, 3);
-static SENSOR_DEVICE_ATTR_RO(in4_input, in, 4);
-static SENSOR_DEVICE_ATTR_RW(in4_min, in_min, 4);
-static SENSOR_DEVICE_ATTR_RW(in4_max, in_max, 4);
-static SENSOR_DEVICE_ATTR_RO(in5_input, in, 5);
-static SENSOR_DEVICE_ATTR_RW(in5_min, in_min, 5);
-static SENSOR_DEVICE_ATTR_RW(in5_max, in_max, 5);
-
-/* fans */
-static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
-			char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
-				1 << data->fan_div[attr->index]));
-}
-
-static ssize_t fan_min_show(struct device *dev,
-			    struct device_attribute *devattr, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[attr->index],
-				1 << data->fan_div[attr->index]));
-}
-
-static ssize_t fan_div_show(struct device *dev,
-			    struct device_attribute *devattr, char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
-}
-
 /*
  * set fan speed low limit:
  *
@@ -501,37 +164,25 @@ static ssize_t fan_div_show(struct device *dev,
  * - otherwise: select fan clock divider to suit fan speed low limit,
  *   measurement code may adjust registers to ensure fan speed reading
  */
-static ssize_t fan_min_store(struct device *dev,
-			     struct device_attribute *devattr,
-			     const char *buf, size_t count)
+static int adm9240_fan_min_write(struct adm9240_data *data, int channel, long val)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	int nr = attr->index;
 	u8 new_div;
-	unsigned long val;
+	u8 fan_min;
 	int err;
 
-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
-
 	mutex_lock(&data->update_lock);
 
 	if (!val) {
-		data->fan_min[nr] = 255;
-		new_div = data->fan_div[nr];
-
-		dev_dbg(data->dev, "fan%u low limit set disabled\n",
-				nr + 1);
+		fan_min = 255;
+		new_div = data->fan_div[channel];
 
+		dev_dbg(data->dev, "fan%u low limit set disabled\n", channel + 1);
 	} else if (val < 1350000 / (8 * 254)) {
 		new_div = 3;
-		data->fan_min[nr] = 254;
+		fan_min = 254;
 
 		dev_dbg(data->dev, "fan%u low limit set minimum %u\n",
-				nr + 1, FAN_FROM_REG(254, 1 << new_div));
-
+			channel + 1, FAN_FROM_REG(254, BIT(new_div)));
 	} else {
 		unsigned int new_min = 1350000 / val;
 
@@ -543,87 +194,55 @@ static ssize_t fan_min_store(struct device *dev,
 		if (!new_min) /* keep > 0 */
 			new_min++;
 
-		data->fan_min[nr] = new_min;
+		fan_min = new_min;
 
 		dev_dbg(data->dev, "fan%u low limit set fan speed %u\n",
-				nr + 1, FAN_FROM_REG(new_min, 1 << new_div));
+			channel + 1, FAN_FROM_REG(new_min, BIT(new_div)));
 	}
 
-	if (new_div != data->fan_div[nr]) {
-		data->fan_div[nr] = new_div;
-		adm9240_write_fan_div(data, nr, new_div);
+	if (new_div != data->fan_div[channel]) {
+		data->fan_div[channel] = new_div;
+		adm9240_write_fan_div(data, channel, new_div);
 	}
-	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(nr),
-			   data->fan_min[nr]);
+	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(channel), fan_min);
 
 	mutex_unlock(&data->update_lock);
-	return err < 0 ? err : count;
-}
-
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
-static SENSOR_DEVICE_ATTR_RW(fan1_min, fan_min, 0);
-static SENSOR_DEVICE_ATTR_RO(fan1_div, fan_div, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
-static SENSOR_DEVICE_ATTR_RW(fan2_min, fan_min, 1);
-static SENSOR_DEVICE_ATTR_RO(fan2_div, fan_div, 1);
-
-/* alarms */
-static ssize_t alarms_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct adm9240_data *data = adm9240_update_device(dev);
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	return sprintf(buf, "%u\n", data->alarms);
+	return err;
 }
-static DEVICE_ATTR_RO(alarms);
-
-static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
-			  char *buf)
-{
-	int bitnr = to_sensor_dev_attr(attr)->index;
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
 
-	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
-}
-static SENSOR_DEVICE_ATTR_RO(in0_alarm, alarm, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_alarm, alarm, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_alarm, alarm, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_alarm, alarm, 3);
-static SENSOR_DEVICE_ATTR_RO(in4_alarm, alarm, 8);
-static SENSOR_DEVICE_ATTR_RO(in5_alarm, alarm, 9);
-static SENSOR_DEVICE_ATTR_RO(temp1_alarm, alarm, 4);
-static SENSOR_DEVICE_ATTR_RO(fan1_alarm, alarm, 6);
-static SENSOR_DEVICE_ATTR_RO(fan2_alarm, alarm, 7);
-
-/* vid */
 static ssize_t cpu0_vid_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
-	struct adm9240_data *data = adm9240_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+	u8 vid;
 
-	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
+	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
+	if (err < 0)
+		return err;
+	vid = regval & 0x0f;
+	err = regmap_read(data->regmap, ADM9240_REG_VID4, &regval);
+	if (err < 0)
+		return err;
+	vid |= (regval & 1) << 4;
+	return sprintf(buf, "%d\n", vid_from_reg(vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
 
-/* analog output */
 static ssize_t aout_output_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct adm9240_data *data = adm9240_update_device(dev);
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	err = regmap_read(data->regmap, ADM9240_REG_ANALOG_OUT, &regval);
+	if (err)
+		return err;
 
-	return sprintf(buf, "%d\n", AOUT_FROM_REG(data->aout));
+	return sprintf(buf, "%d\n", AOUT_FROM_REG(regval));
 }
 
 static ssize_t aout_output_store(struct device *dev,
@@ -638,76 +257,13 @@ static ssize_t aout_output_store(struct device *dev,
 	if (err)
 		return err;
 
-	mutex_lock(&data->update_lock);
-	data->aout = AOUT_TO_REG(val);
-	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, data->aout);
-	mutex_unlock(&data->update_lock);
+	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, AOUT_TO_REG(val));
 	return err < 0 ? err : count;
 }
 static DEVICE_ATTR_RW(aout_output);
 
-static ssize_t alarm_store(struct device *dev, struct device_attribute *attr,
-			   const char *buf, size_t count)
-{
-	struct adm9240_data *data = dev_get_drvdata(dev);
-	unsigned long val;
-	int err;
-
-	if (kstrtoul(buf, 10, &val) || val != 0)
-		return -EINVAL;
-
-	mutex_lock(&data->update_lock);
-	err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
-	data->valid = 0;		/* Force cache refresh */
-	mutex_unlock(&data->update_lock);
-	if (err < 0)
-		return err;
-	dev_dbg(data->dev, "chassis intrusion latch cleared\n");
-
-	return count;
-}
-static SENSOR_DEVICE_ATTR_RW(intrusion0_alarm, alarm, 12);
-
 static struct attribute *adm9240_attrs[] = {
-	&sensor_dev_attr_in0_input.dev_attr.attr,
-	&sensor_dev_attr_in0_min.dev_attr.attr,
-	&sensor_dev_attr_in0_max.dev_attr.attr,
-	&sensor_dev_attr_in0_alarm.dev_attr.attr,
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in1_min.dev_attr.attr,
-	&sensor_dev_attr_in1_max.dev_attr.attr,
-	&sensor_dev_attr_in1_alarm.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in2_min.dev_attr.attr,
-	&sensor_dev_attr_in2_max.dev_attr.attr,
-	&sensor_dev_attr_in2_alarm.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in3_min.dev_attr.attr,
-	&sensor_dev_attr_in3_max.dev_attr.attr,
-	&sensor_dev_attr_in3_alarm.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
-	&sensor_dev_attr_in4_min.dev_attr.attr,
-	&sensor_dev_attr_in4_max.dev_attr.attr,
-	&sensor_dev_attr_in4_alarm.dev_attr.attr,
-	&sensor_dev_attr_in5_input.dev_attr.attr,
-	&sensor_dev_attr_in5_min.dev_attr.attr,
-	&sensor_dev_attr_in5_max.dev_attr.attr,
-	&sensor_dev_attr_in5_alarm.dev_attr.attr,
-	&dev_attr_temp1_input.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	&sensor_dev_attr_fan1_div.dev_attr.attr,
-	&sensor_dev_attr_fan1_min.dev_attr.attr,
-	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan2_input.dev_attr.attr,
-	&sensor_dev_attr_fan2_div.dev_attr.attr,
-	&sensor_dev_attr_fan2_min.dev_attr.attr,
-	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
-	&dev_attr_alarms.attr,
 	&dev_attr_aout_output.attr,
-	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
 	&dev_attr_cpu0_vid.attr,
 	NULL
 };
@@ -749,13 +305,14 @@ static int adm9240_detect(struct i2c_client *new_client,
 		 man_id == 0x23 ? "ADM9240" :
 		 man_id == 0xda ? "DS1780" : "LM81", die_rev);
 
-	strlcpy(info->type, name, I2C_NAME_SIZE);
+	strscpy(info->type, name, I2C_NAME_SIZE);
 
 	return 0;
 }
 
 static int adm9240_init_client(struct adm9240_data *data)
 {
+	unsigned int regval;
 	u8 conf, mode;
 	int err;
 
@@ -792,13 +349,13 @@ static int adm9240_init_client(struct adm9240_data *data)
 		}
 		for (i = 0; i < 2; i++) {
 			err = regmap_write(data->regmap,
-					ADM9240_REG_FAN_MIN(i), 255);
+					   ADM9240_REG_FAN_MIN(i), 255);
 			if (err < 0)
 				return err;
 		}
 		for (i = 0; i < 2; i++) {
 			err = regmap_write(data->regmap,
-					ADM9240_REG_TEMP_MAX(i), 127);
+					   ADM9240_REG_TEMP_MAX(i), 127);
 			if (err < 0)
 				return err;
 		}
@@ -812,19 +369,413 @@ static int adm9240_init_client(struct adm9240_data *data)
 			 "cold start: config was 0x%02x mode %u\n", conf, mode);
 	}
 
+	/* read fan divs */
+	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
+	if (err < 0)
+		return err;
+	data->fan_div[0] = (regval >> 4) & 3;
+	data->fan_div[1] = (regval >> 6) & 3;
+	return 0;
+}
+
+static int adm9240_chip_read(struct device *dev, u32 attr, long *val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	u8 regs[2];
+	int err;
+
+	switch (attr) {
+	case hwmon_chip_alarms:
+		err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
+		if (err < 0)
+			return err;
+		*val = regs[0] | regs[1] << 8;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_intrusion_read(struct device *dev, u32 attr, long *val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_intrusion_alarm:
+		err = regmap_read(data->regmap, ADM9240_REG_INT(1), &regval);
+		if (err < 0)
+			return err;
+		*val = !!(regval & BIT(4));
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_intrusion_write(struct device *dev, u32 attr, long val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	int err;
+
+	switch (attr) {
+	case hwmon_intrusion_alarm:
+		if (val)
+			return -EINVAL;
+		err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
+		if (err < 0)
+			return err;
+		dev_dbg(data->dev, "chassis intrusion latch cleared\n");
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_in_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int reg;
+	int err;
+
+	switch (attr) {
+	case hwmon_in_input:
+		reg = ADM9240_REG_IN(channel);
+		break;
+	case hwmon_in_min:
+		reg = ADM9240_REG_IN_MIN(channel);
+		break;
+	case hwmon_in_max:
+		reg = ADM9240_REG_IN_MAX(channel);
+		break;
+	case hwmon_in_alarm:
+		if (channel < 4) {
+			reg = ADM9240_REG_INT(0);
+		} else {
+			reg = ADM9240_REG_INT(1);
+			channel -= 4;
+		}
+		err = regmap_read(data->regmap, reg, &regval);
+		if (err < 0)
+			return err;
+		*val = !!(regval & BIT(channel));
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+	err = regmap_read(data->regmap, reg, &regval);
+	if (err < 0)
+		return err;
+	*val = IN_FROM_REG(regval, channel);
 	return 0;
 }
 
+static int adm9240_in_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	int reg;
+
+	switch (attr) {
+	case hwmon_in_min:
+		reg = ADM9240_REG_IN_MIN(channel);
+		break;
+	case hwmon_in_max:
+		reg = ADM9240_REG_IN(channel);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return regmap_write(data->regmap, reg, IN_TO_REG(val, channel));
+}
+
+static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), &regval);
+		if (err < 0)
+			return err;
+		if (regval == 255 && data->fan_div[channel] < 3) {
+			/* adjust fan clock divider on overflow */
+			err = adm9240_write_fan_div(data, channel,
+						    ++data->fan_div[channel]);
+			if (err)
+				return err;
+		}
+		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
+		break;
+	case hwmon_fan_div:
+		*val = BIT(data->fan_div[channel]);
+		break;
+	case hwmon_fan_min:
+		err = regmap_read(data->regmap, ADM9240_REG_FAN_MIN(channel), &regval);
+		if (err < 0)
+			return err;
+		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
+		break;
+	case hwmon_fan_alarm:
+		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
+		if (err < 0)
+			return err;
+		*val = !!(regval & BIT(channel + 6));
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_fan_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	int err;
+
+	switch (attr) {
+	case hwmon_fan_min:
+		err = adm9240_fan_min_write(data, channel, val);
+		if (err < 0)
+			return err;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_temp_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err, temp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		err = regmap_read(data->regmap, ADM9240_REG_TEMP, &regval);
+		if (err < 0)
+			return err;
+		temp = regval << 1;
+		err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &regval);
+		if (err < 0)
+			return err;
+		temp |= regval >> 7;
+		*val = sign_extend32(temp, 8) * 500;
+		break;
+	case hwmon_temp_max:
+		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(0), &regval);
+		if (err < 0)
+			return err;
+		*val = (s8)regval * 1000;
+		break;
+	case hwmon_temp_max_hyst:
+		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(1), &regval);
+		if (err < 0)
+			return err;
+		*val = (s8)regval * 1000;
+		break;
+	case hwmon_temp_alarm:
+		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
+		if (err < 0)
+			return err;
+		*val = !!(regval & BIT(4));
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int adm9240_temp_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct adm9240_data *data = dev_get_drvdata(dev);
+	int reg;
+
+	switch (attr) {
+	case hwmon_temp_max:
+		reg = ADM9240_REG_TEMP_MAX(0);
+		break;
+	case hwmon_temp_max_hyst:
+		reg = ADM9240_REG_TEMP_MAX(1);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return regmap_write(data->regmap, reg, TEMP_TO_REG(val));
+}
+
+static int adm9240_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return adm9240_chip_read(dev, attr, val);
+	case hwmon_intrusion:
+		return adm9240_intrusion_read(dev, attr, val);
+	case hwmon_in:
+		return adm9240_in_read(dev, attr, channel, val);
+	case hwmon_fan:
+		return adm9240_fan_read(dev, attr, channel, val);
+	case hwmon_temp:
+		return adm9240_temp_read(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int adm9240_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+			 int channel, long val)
+{
+	switch (type) {
+	case hwmon_intrusion:
+		return adm9240_intrusion_write(dev, attr, val);
+	case hwmon_in:
+		return adm9240_in_write(dev, attr, channel, val);
+	case hwmon_fan:
+		return adm9240_fan_write(dev, attr, channel, val);
+	case hwmon_temp:
+		return adm9240_temp_write(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t adm9240_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	umode_t mode = 0;
+
+	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_alarms:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_intrusion:
+		switch (attr) {
+		case hwmon_intrusion_alarm:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp:
+		case hwmon_temp_alarm:
+			mode = 0444;
+			break;
+		case hwmon_temp_max:
+		case hwmon_temp_max_hyst:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_div:
+		case hwmon_fan_alarm:
+			mode = 0444;
+			break;
+		case hwmon_fan_min:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+		case hwmon_in_alarm:
+			mode = 0444;
+			break;
+		case hwmon_in_min:
+		case hwmon_in_max:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return mode;
+}
+
+static const struct hwmon_ops adm9240_hwmon_ops = {
+	.is_visible = adm9240_is_visible,
+	.read = adm9240_read,
+	.write = adm9240_write,
+};
+
+static const struct hwmon_channel_info *adm9240_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS),
+	HWMON_CHANNEL_INFO(intrusion, HWMON_INTRUSION_ALARM),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST | HWMON_T_ALARM),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM,
+			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM),
+	NULL
+};
+
+static const struct hwmon_chip_info adm9240_chip_info = {
+	.ops = &adm9240_hwmon_ops,
+	.info = adm9240_info,
+};
+
+static bool adm9240_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ADM9240_REG_IN(0) ... ADM9240_REG_IN(5):
+	case ADM9240_REG_FAN(0) ... ADM9240_REG_FAN(1):
+	case ADM9240_REG_INT(0) ... ADM9240_REG_INT(1):
+	case ADM9240_REG_TEMP:
+	case ADM9240_REG_TEMP_CONF:
+	case ADM9240_REG_VID_FAN_DIV:
+	case ADM9240_REG_VID4:
+	case ADM9240_REG_ANALOG_OUT:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static const struct regmap_config adm9240_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.use_single_read = true,
 	.use_single_write = true,
+	.volatile_reg = adm9240_volatile_reg,
 };
 
-static int adm9240_probe(struct i2c_client *new_client)
+static int adm9240_probe(struct i2c_client *client)
 {
-	struct device *dev = &new_client->dev;
+	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct adm9240_data *data;
 	int err;
@@ -835,7 +786,7 @@ static int adm9240_probe(struct i2c_client *new_client)
 
 	data->dev = dev;
 	mutex_init(&data->update_lock);
-	data->regmap = devm_regmap_init_i2c(new_client, &adm9240_regmap_config);
+	data->regmap = devm_regmap_init_i2c(client, &adm9240_regmap_config);
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
@@ -843,10 +794,9 @@ static int adm9240_probe(struct i2c_client *new_client)
 	if (err < 0)
 		return err;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
-							   new_client->name,
-							   data,
-							   adm9240_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							 &adm9240_chip_info,
+							 adm9240_groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.17.1

Re: [PATCH 1/3] hwmon: (adm9240) Drop log messages from detect function

[Chris Packham]

On 11/03/21 8:33 pm, Guenter Roeck wrote:
> Not detecting a chip in the detect function is normal and should not
> generate any log messages, much less error messages.
>
> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>   drivers/hwmon/adm9240.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index cc3e0184e720..3bbdd662c9e4 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -730,26 +730,19 @@ static int adm9240_detect(struct i2c_client *new_client,
>   		return -ENODEV;
>   
>   	/* verify chip: reg address should match i2c address */
> -	if (i2c_smbus_read_byte_data(new_client, ADM9240_REG_I2C_ADDR)
> -			!= address) {
> -		dev_err(&adapter->dev, "detect fail: address match, 0x%02x\n",
> -			address);
> +	if (i2c_smbus_read_byte_data(new_client, ADM9240_REG_I2C_ADDR) != address)
>   		return -ENODEV;
> -	}
>   
>   	/* check known chip manufacturer */
>   	man_id = i2c_smbus_read_byte_data(new_client, ADM9240_REG_MAN_ID);
> -	if (man_id == 0x23) {
> +	if (man_id == 0x23)
>   		name = "adm9240";
> -	} else if (man_id == 0xda) {
> +	else if (man_id == 0xda)
>   		name = "ds1780";
> -	} else if (man_id == 0x01) {
> +	else if (man_id == 0x01)
>   		name = "lm81";
> -	} else {
> -		dev_err(&adapter->dev, "detect fail: unknown manuf, 0x%02x\n",
> -			man_id);
> +	else
>   		return -ENODEV;
> -	}
>   
>   	/* successful detect, print chip info */
>   	die_rev = i2c_smbus_read_byte_data(new_client, ADM9240_REG_DIE_REV);

Re: [PATCH 2/3] hwmon: (adm9240) Store i2c device instead of client in local data

[Chris Packham]

On 11/03/21 8:33 pm, Guenter Roeck wrote:
> We only use the pointer to i2c_client to access &client->dev.
> Store the device pointer directly instead of retrieving it
> from i2c_client.
>
> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> ---
>   drivers/hwmon/adm9240.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 3bbdd662c9e4..7404082c7a3f 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -123,7 +123,7 @@ static inline unsigned int AOUT_FROM_REG(u8 reg)
>   
>   /* per client data */
>   struct adm9240_data {
> -	struct i2c_client *client;
> +	struct device *dev;
>   	struct regmap *regmap;
>   	struct mutex update_lock;
>   	char valid;
> @@ -160,7 +160,7 @@ static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
>   	err = regmap_write(data->regmap, ADM9240_REG_VID_FAN_DIV, reg);
>   	if (err < 0)
>   		return err;
> -	dev_dbg(&data->client->dev,
> +	dev_dbg(data->dev,
>   		"fan%d clock divider changed from %u to %u\n",
>   		nr + 1, 1 << old, 1 << fan_div);
>   
> @@ -507,7 +507,6 @@ static ssize_t fan_min_store(struct device *dev,
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct adm9240_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	int nr = attr->index;
>   	u8 new_div;
>   	unsigned long val;
> @@ -523,14 +522,14 @@ static ssize_t fan_min_store(struct device *dev,
>   		data->fan_min[nr] = 255;
>   		new_div = data->fan_div[nr];
>   
> -		dev_dbg(&client->dev, "fan%u low limit set disabled\n",
> +		dev_dbg(data->dev, "fan%u low limit set disabled\n",
>   				nr + 1);
>   
>   	} else if (val < 1350000 / (8 * 254)) {
>   		new_div = 3;
>   		data->fan_min[nr] = 254;
>   
> -		dev_dbg(&client->dev, "fan%u low limit set minimum %u\n",
> +		dev_dbg(data->dev, "fan%u low limit set minimum %u\n",
>   				nr + 1, FAN_FROM_REG(254, 1 << new_div));
>   
>   	} else {
> @@ -546,7 +545,7 @@ static ssize_t fan_min_store(struct device *dev,
>   
>   		data->fan_min[nr] = new_min;
>   
> -		dev_dbg(&client->dev, "fan%u low limit set fan speed %u\n",
> +		dev_dbg(data->dev, "fan%u low limit set fan speed %u\n",
>   				nr + 1, FAN_FROM_REG(new_min, 1 << new_div));
>   	}
>   
> @@ -663,7 +662,7 @@ static ssize_t alarm_store(struct device *dev, struct device_attribute *attr,
>   	mutex_unlock(&data->update_lock);
>   	if (err < 0)
>   		return err;
> -	dev_dbg(&data->client->dev, "chassis intrusion latch cleared\n");
> +	dev_dbg(data->dev, "chassis intrusion latch cleared\n");
>   
>   	return count;
>   }
> @@ -755,7 +754,7 @@ static int adm9240_detect(struct i2c_client *new_client,
>   	return 0;
>   }
>   
> -static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *data)
> +static int adm9240_init_client(struct adm9240_data *data)
>   {
>   	u8 conf, mode;
>   	int err;
> @@ -770,13 +769,13 @@ static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *d
>   
>   	data->vrm = vid_which_vrm(); /* need this to report vid as mV */
>   
> -	dev_info(&client->dev, "Using VRM: %d.%d\n", data->vrm / 10,
> -			data->vrm % 10);
> +	dev_info(data->dev, "Using VRM: %d.%d\n", data->vrm / 10,
> +		 data->vrm % 10);
>   
>   	if (conf & 1) { /* measurement cycle running: report state */
>   
> -		dev_info(&client->dev, "status: config 0x%02x mode %u\n",
> -				conf, mode);
> +		dev_info(data->dev, "status: config 0x%02x mode %u\n",
> +			 conf, mode);
>   
>   	} else { /* cold start: open limits before starting chip */
>   		int i;
> @@ -809,7 +808,7 @@ static int adm9240_init_client(struct i2c_client *client, struct adm9240_data *d
>   		if (err < 0)
>   			return err;
>   
> -		dev_info(&client->dev,
> +		dev_info(data->dev,
>   			 "cold start: config was 0x%02x mode %u\n", conf, mode);
>   	}
>   
> @@ -834,13 +833,13 @@ static int adm9240_probe(struct i2c_client *new_client)
>   	if (!data)
>   		return -ENOMEM;
>   
> -	data->client = new_client;
> +	data->dev = dev;
>   	mutex_init(&data->update_lock);
>   	data->regmap = devm_regmap_init_i2c(new_client, &adm9240_regmap_config);
>   	if (IS_ERR(data->regmap))
>   		return PTR_ERR(data->regmap);
>   
> -	err = adm9240_init_client(new_client, data);
> +	err = adm9240_init_client(data);
>   	if (err < 0)
>   		return err;
>   

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Chris Packham]

On 11/03/21 8:33 pm, Guenter Roeck wrote:
> Also use regmap for register caching. This change reduces code and
> data size by more than 40%.
>
> While at it, fixed some warnings reported by checkpatch.
>
> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>   drivers/hwmon/adm9240.c | 940 +++++++++++++++++++---------------------
>   1 file changed, 445 insertions(+), 495 deletions(-)
>
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 7404082c7a3f..5677263bcf0d 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -28,6 +28,7 @@
>    * LM81 extended temp reading not implemented
>    */
>   
> +#include <linux/bits.h>
>   #include <linux/init.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
> @@ -37,7 +38,6 @@
>   #include <linux/hwmon-vid.h>
>   #include <linux/err.h>
>   #include <linux/mutex.h>
> -#include <linux/jiffies.h>
>   #include <linux/regmap.h>
>   
>   /* Addresses to scan */
> @@ -126,29 +126,15 @@ struct adm9240_data {
>   	struct device *dev;
>   	struct regmap *regmap;
>   	struct mutex update_lock;
> -	char valid;
> -	unsigned long last_updated_measure;
> -	unsigned long last_updated_config;
> -
> -	u8 in[6];		/* ro	in0_input */
> -	u8 in_max[6];		/* rw	in0_max */
> -	u8 in_min[6];		/* rw	in0_min */
> -	u8 fan[2];		/* ro	fan1_input */
> -	u8 fan_min[2];		/* rw	fan1_min */
> +
>   	u8 fan_div[2];		/* rw	fan1_div, read-only accessor */
> -	s16 temp;		/* ro	temp1_input, 9-bit sign-extended */
> -	s8 temp_max[2];		/* rw	0 -> temp_max, 1 -> temp_max_hyst */
> -	u16 alarms;		/* ro	alarms */
> -	u8 aout;		/* rw	aout_output */
> -	u8 vid;			/* ro	vid */
>   	u8 vrm;			/* --	vrm set on startup, no accessor */
>   };
>   
>   /* write new fan div, callers must hold data->update_lock */
> -static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
> -		u8 fan_div)
> +static int adm9240_write_fan_div(struct adm9240_data *data, int channel, u8 fan_div)
>   {
> -	unsigned int reg, old, shift = (nr + 2) * 2;
> +	unsigned int reg, old, shift = (channel + 2) * 2;
>   	int err;
>   
>   	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &reg);
> @@ -161,335 +147,12 @@ static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
>   	if (err < 0)
>   		return err;
>   	dev_dbg(data->dev,
> -		"fan%d clock divider changed from %u to %u\n",
> -		nr + 1, 1 << old, 1 << fan_div);
> +		"fan%d clock divider changed from %lu to %lu\n",
> +		channel + 1, BIT(old), BIT(fan_div));
>   
>   	return 0;
>   }
>   
> -static int adm9240_update_measure(struct adm9240_data *data)
> -{
> -	unsigned int val;
> -	u8 regs[2];
> -	int err;
> -	int i;
> -
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_IN(0), &data->in[0], 6);
> -	if (err < 0)
> -		return err;
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
> -	if (err < 0)
> -		return err;
> -
> -	data->alarms = regs[0] | regs[1] << 8;
> -
> -	/*
> -	 * read temperature: assume temperature changes less than
> -	 * 0.5'C per two measurement cycles thus ignore possible
> -	 * but unlikely aliasing error on lsb reading. --Grant
> -	 */
> -	err = regmap_read(data->regmap, ADM9240_REG_TEMP, &val);
> -	if (err < 0)
> -		return err;
> -	data->temp = val << 8;
> -	err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &val);
> -	if (err < 0)
> -		return err;
> -	data->temp |= val;
> -
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN(0),
> -			       &data->fan[0], 2);
> -	if (err < 0)
> -		return err;
> -
> -	for (i = 0; i < 2; i++) { /* read fans */
> -		/* adjust fan clock divider on overflow */
> -		if (data->valid && data->fan[i] == 255 &&
> -				data->fan_div[i] < 3) {
> -
> -			err = adm9240_write_fan_div(data, i,
> -					++data->fan_div[i]);
> -			if (err < 0)
> -				return err;
> -
> -			/* adjust fan_min if active, but not to 0 */
> -			if (data->fan_min[i] < 255 &&
> -					data->fan_min[i] >= 2)
> -				data->fan_min[i] /= 2;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static int adm9240_update_config(struct adm9240_data *data)
> -{
> -	unsigned int val;
> -	int i;
> -	int err;
> -
> -	for (i = 0; i < 6; i++) {
> -		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MIN(i),
> -				      &data->in_min[i], 1);
> -		if (err < 0)
> -			return err;
> -		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MAX(i),
> -				      &data->in_max[i], 1);
> -		if (err < 0)
> -			return err;
> -	}
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN_MIN(0),
> -				      &data->fan_min[0], 2);
> -	if (err < 0)
> -		return err;
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_TEMP_MAX(0),
> -				      &data->temp_max[0], 2);
> -	if (err < 0)
> -		return err;
> -
> -	/* read fan divs and 5-bit VID */
> -	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &val);
> -	if (err < 0)
> -		return err;
> -	data->fan_div[0] = (val >> 4) & 3;
> -	data->fan_div[1] = (val >> 6) & 3;
> -	data->vid = val & 0x0f;
> -	err = regmap_read(data->regmap, ADM9240_REG_VID4, &val);
> -	if (err < 0)
> -		return err;
> -	data->vid |= (val & 1) << 4;
> -	/* read analog out */
> -	err = regmap_raw_read(data->regmap, ADM9240_REG_ANALOG_OUT,
> -			      &data->aout, 1);
> -
> -	return err;
> -}
> -
> -static struct adm9240_data *adm9240_update_device(struct device *dev)
> -{
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	int err;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	/* minimum measurement cycle: 1.75 seconds */
> -	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
> -			|| !data->valid) {
> -		err = adm9240_update_measure(data);
> -		if (err < 0) {
> -			data->valid = 0;
> -			mutex_unlock(&data->update_lock);
> -			return ERR_PTR(err);
> -		}
> -		data->last_updated_measure = jiffies;
> -	}
> -
> -	/* minimum config reading cycle: 300 seconds */
> -	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
> -			|| !data->valid) {
> -		err = adm9240_update_config(data);
> -		if (err < 0) {
> -			data->valid = 0;
> -			mutex_unlock(&data->update_lock);
> -			return ERR_PTR(err);
> -		}
> -		data->last_updated_config = jiffies;
> -		data->valid = 1;
> -	}
> -	mutex_unlock(&data->update_lock);
> -	return data;
> -}
> -
> -/*** sysfs accessors ***/
> -
> -/* temperature */
> -static ssize_t temp1_input_show(struct device *dev,
> -				struct device_attribute *dummy, char *buf)
> -{
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", data->temp / 128 * 500); /* 9-bit value */
> -}
> -
> -static ssize_t max_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
> -}
> -
> -static ssize_t max_store(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->temp_max[attr->index] = TEMP_TO_REG(val);
> -	err = regmap_write(data->regmap, ADM9240_REG_TEMP_MAX(attr->index),
> -			   data->temp_max[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static DEVICE_ATTR_RO(temp1_input);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max, max, 0);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, max, 1);
> -
> -/* voltage */
> -static ssize_t in_show(struct device *dev, struct device_attribute *devattr,
> -		       char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_min_show(struct device *dev,
> -			   struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_max_show(struct device *dev,
> -			   struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_min_store(struct device *dev,
> -			    struct device_attribute *devattr, const char *buf,
> -			    size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->in_min[attr->index] = IN_TO_REG(val, attr->index);
> -	err = regmap_write(data->regmap, ADM9240_REG_IN_MIN(attr->index),
> -			   data->in_min[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static ssize_t in_max_store(struct device *dev,
> -			    struct device_attribute *devattr, const char *buf,
> -			    size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->in_max[attr->index] = IN_TO_REG(val, attr->index);
> -	err = regmap_write(data->regmap, ADM9240_REG_IN_MAX(attr->index),
> -			   data->in_max[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static SENSOR_DEVICE_ATTR_RO(in0_input, in, 0);
> -static SENSOR_DEVICE_ATTR_RW(in0_min, in_min, 0);
> -static SENSOR_DEVICE_ATTR_RW(in0_max, in_max, 0);
> -static SENSOR_DEVICE_ATTR_RO(in1_input, in, 1);
> -static SENSOR_DEVICE_ATTR_RW(in1_min, in_min, 1);
> -static SENSOR_DEVICE_ATTR_RW(in1_max, in_max, 1);
> -static SENSOR_DEVICE_ATTR_RO(in2_input, in, 2);
> -static SENSOR_DEVICE_ATTR_RW(in2_min, in_min, 2);
> -static SENSOR_DEVICE_ATTR_RW(in2_max, in_max, 2);
> -static SENSOR_DEVICE_ATTR_RO(in3_input, in, 3);
> -static SENSOR_DEVICE_ATTR_RW(in3_min, in_min, 3);
> -static SENSOR_DEVICE_ATTR_RW(in3_max, in_max, 3);
> -static SENSOR_DEVICE_ATTR_RO(in4_input, in, 4);
> -static SENSOR_DEVICE_ATTR_RW(in4_min, in_min, 4);
> -static SENSOR_DEVICE_ATTR_RW(in4_max, in_max, 4);
> -static SENSOR_DEVICE_ATTR_RO(in5_input, in, 5);
> -static SENSOR_DEVICE_ATTR_RW(in5_min, in_min, 5);
> -static SENSOR_DEVICE_ATTR_RW(in5_max, in_max, 5);
> -
> -/* fans */
> -static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
> -				1 << data->fan_div[attr->index]));
> -}
> -
> -static ssize_t fan_min_show(struct device *dev,
> -			    struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[attr->index],
> -				1 << data->fan_div[attr->index]));
> -}
> -
> -static ssize_t fan_div_show(struct device *dev,
> -			    struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
> -}
> -
>   /*
>    * set fan speed low limit:
>    *
> @@ -501,37 +164,25 @@ static ssize_t fan_div_show(struct device *dev,
>    * - otherwise: select fan clock divider to suit fan speed low limit,
>    *   measurement code may adjust registers to ensure fan speed reading
>    */
> -static ssize_t fan_min_store(struct device *dev,
> -			     struct device_attribute *devattr,
> -			     const char *buf, size_t count)
> +static int adm9240_fan_min_write(struct adm9240_data *data, int channel, long val)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	int nr = attr->index;
>   	u8 new_div;
> -	unsigned long val;
> +	u8 fan_min;
>   	int err;
>   
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
>   	mutex_lock(&data->update_lock);
>   
>   	if (!val) {
> -		data->fan_min[nr] = 255;
> -		new_div = data->fan_div[nr];
> -
> -		dev_dbg(data->dev, "fan%u low limit set disabled\n",
> -				nr + 1);
> +		fan_min = 255;
> +		new_div = data->fan_div[channel];
>   
> +		dev_dbg(data->dev, "fan%u low limit set disabled\n", channel + 1);
>   	} else if (val < 1350000 / (8 * 254)) {
>   		new_div = 3;
> -		data->fan_min[nr] = 254;
> +		fan_min = 254;
>   
>   		dev_dbg(data->dev, "fan%u low limit set minimum %u\n",
> -				nr + 1, FAN_FROM_REG(254, 1 << new_div));
> -
> +			channel + 1, FAN_FROM_REG(254, BIT(new_div)));
>   	} else {
>   		unsigned int new_min = 1350000 / val;
>   
> @@ -543,87 +194,55 @@ static ssize_t fan_min_store(struct device *dev,
>   		if (!new_min) /* keep > 0 */
>   			new_min++;
>   
> -		data->fan_min[nr] = new_min;
> +		fan_min = new_min;
>   
>   		dev_dbg(data->dev, "fan%u low limit set fan speed %u\n",
> -				nr + 1, FAN_FROM_REG(new_min, 1 << new_div));
> +			channel + 1, FAN_FROM_REG(new_min, BIT(new_div)));
>   	}
>   
> -	if (new_div != data->fan_div[nr]) {
> -		data->fan_div[nr] = new_div;
> -		adm9240_write_fan_div(data, nr, new_div);
> +	if (new_div != data->fan_div[channel]) {
> +		data->fan_div[channel] = new_div;
> +		adm9240_write_fan_div(data, channel, new_div);
>   	}
> -	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(nr),
> -			   data->fan_min[nr]);
> +	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(channel), fan_min);
>   
>   	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
> -static SENSOR_DEVICE_ATTR_RW(fan1_min, fan_min, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan1_div, fan_div, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
> -static SENSOR_DEVICE_ATTR_RW(fan2_min, fan_min, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan2_div, fan_div, 1);
> -
> -/* alarms */
> -static ssize_t alarms_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct adm9240_data *data = adm9240_update_device(dev);
>   
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%u\n", data->alarms);
> +	return err;
>   }
> -static DEVICE_ATTR_RO(alarms);
> -
> -static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
> -			  char *buf)
> -{
> -	int bitnr = to_sensor_dev_attr(attr)->index;
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
>   
> -	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
> -}
> -static SENSOR_DEVICE_ATTR_RO(in0_alarm, alarm, 0);
> -static SENSOR_DEVICE_ATTR_RO(in1_alarm, alarm, 1);
> -static SENSOR_DEVICE_ATTR_RO(in2_alarm, alarm, 2);
> -static SENSOR_DEVICE_ATTR_RO(in3_alarm, alarm, 3);
> -static SENSOR_DEVICE_ATTR_RO(in4_alarm, alarm, 8);
> -static SENSOR_DEVICE_ATTR_RO(in5_alarm, alarm, 9);
> -static SENSOR_DEVICE_ATTR_RO(temp1_alarm, alarm, 4);
> -static SENSOR_DEVICE_ATTR_RO(fan1_alarm, alarm, 6);
> -static SENSOR_DEVICE_ATTR_RO(fan2_alarm, alarm, 7);
> -
> -/* vid */
>   static ssize_t cpu0_vid_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)
>   {
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +	u8 vid;
>   
> -	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
> +	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
> +	if (err < 0)
> +		return err;
> +	vid = regval & 0x0f;
> +	err = regmap_read(data->regmap, ADM9240_REG_VID4, &regval);
> +	if (err < 0)
> +		return err;
> +	vid |= (regval & 1) << 4;
> +	return sprintf(buf, "%d\n", vid_from_reg(vid, data->vrm));
>   }
>   static DEVICE_ATTR_RO(cpu0_vid);
>   
> -/* analog output */
>   static ssize_t aout_output_show(struct device *dev,
>   				struct device_attribute *attr, char *buf)
>   {
> -	struct adm9240_data *data = adm9240_update_device(dev);
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
>   
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	err = regmap_read(data->regmap, ADM9240_REG_ANALOG_OUT, &regval);
> +	if (err)
> +		return err;
>   
> -	return sprintf(buf, "%d\n", AOUT_FROM_REG(data->aout));
> +	return sprintf(buf, "%d\n", AOUT_FROM_REG(regval));
>   }
>   
>   static ssize_t aout_output_store(struct device *dev,
> @@ -638,76 +257,13 @@ static ssize_t aout_output_store(struct device *dev,
>   	if (err)
>   		return err;
>   
> -	mutex_lock(&data->update_lock);
> -	data->aout = AOUT_TO_REG(val);
> -	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, data->aout);
> -	mutex_unlock(&data->update_lock);
> +	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, AOUT_TO_REG(val));
>   	return err < 0 ? err : count;
>   }
>   static DEVICE_ATTR_RW(aout_output);
>   
> -static ssize_t alarm_store(struct device *dev, struct device_attribute *attr,
> -			   const char *buf, size_t count)
> -{
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	if (kstrtoul(buf, 10, &val) || val != 0)
> -		return -EINVAL;
> -
> -	mutex_lock(&data->update_lock);
> -	err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
> -	data->valid = 0;		/* Force cache refresh */
> -	mutex_unlock(&data->update_lock);
> -	if (err < 0)
> -		return err;
> -	dev_dbg(data->dev, "chassis intrusion latch cleared\n");
> -
> -	return count;
> -}
> -static SENSOR_DEVICE_ATTR_RW(intrusion0_alarm, alarm, 12);
> -
>   static struct attribute *adm9240_attrs[] = {
> -	&sensor_dev_attr_in0_input.dev_attr.attr,
> -	&sensor_dev_attr_in0_min.dev_attr.attr,
> -	&sensor_dev_attr_in0_max.dev_attr.attr,
> -	&sensor_dev_attr_in0_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in1_input.dev_attr.attr,
> -	&sensor_dev_attr_in1_min.dev_attr.attr,
> -	&sensor_dev_attr_in1_max.dev_attr.attr,
> -	&sensor_dev_attr_in1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in2_input.dev_attr.attr,
> -	&sensor_dev_attr_in2_min.dev_attr.attr,
> -	&sensor_dev_attr_in2_max.dev_attr.attr,
> -	&sensor_dev_attr_in2_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in3_input.dev_attr.attr,
> -	&sensor_dev_attr_in3_min.dev_attr.attr,
> -	&sensor_dev_attr_in3_max.dev_attr.attr,
> -	&sensor_dev_attr_in3_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in4_input.dev_attr.attr,
> -	&sensor_dev_attr_in4_min.dev_attr.attr,
> -	&sensor_dev_attr_in4_max.dev_attr.attr,
> -	&sensor_dev_attr_in4_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in5_input.dev_attr.attr,
> -	&sensor_dev_attr_in5_min.dev_attr.attr,
> -	&sensor_dev_attr_in5_max.dev_attr.attr,
> -	&sensor_dev_attr_in5_alarm.dev_attr.attr,
> -	&dev_attr_temp1_input.attr,
> -	&sensor_dev_attr_temp1_max.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> -	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,
> -	&sensor_dev_attr_fan1_div.dev_attr.attr,
> -	&sensor_dev_attr_fan1_min.dev_attr.attr,
> -	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,
> -	&sensor_dev_attr_fan2_div.dev_attr.attr,
> -	&sensor_dev_attr_fan2_min.dev_attr.attr,
> -	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
> -	&dev_attr_alarms.attr,
>   	&dev_attr_aout_output.attr,
> -	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
>   	&dev_attr_cpu0_vid.attr,
>   	NULL
>   };
> @@ -749,13 +305,14 @@ static int adm9240_detect(struct i2c_client *new_client,
>   		 man_id == 0x23 ? "ADM9240" :
>   		 man_id == 0xda ? "DS1780" : "LM81", die_rev);
>   
> -	strlcpy(info->type, name, I2C_NAME_SIZE);
> +	strscpy(info->type, name, I2C_NAME_SIZE);
>   
>   	return 0;
>   }
>   
>   static int adm9240_init_client(struct adm9240_data *data)
>   {
> +	unsigned int regval;
>   	u8 conf, mode;
>   	int err;
>   
> @@ -792,13 +349,13 @@ static int adm9240_init_client(struct adm9240_data *data)
>   		}
>   		for (i = 0; i < 2; i++) {
>   			err = regmap_write(data->regmap,
> -					ADM9240_REG_FAN_MIN(i), 255);
> +					   ADM9240_REG_FAN_MIN(i), 255);
>   			if (err < 0)
>   				return err;
>   		}
>   		for (i = 0; i < 2; i++) {
>   			err = regmap_write(data->regmap,
> -					ADM9240_REG_TEMP_MAX(i), 127);
> +					   ADM9240_REG_TEMP_MAX(i), 127);
>   			if (err < 0)
>   				return err;
>   		}
> @@ -812,19 +369,413 @@ static int adm9240_init_client(struct adm9240_data *data)
>   			 "cold start: config was 0x%02x mode %u\n", conf, mode);
>   	}
>   
> +	/* read fan divs */
> +	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
> +	if (err < 0)
> +		return err;
> +	data->fan_div[0] = (regval >> 4) & 3;
> +	data->fan_div[1] = (regval >> 6) & 3;
> +	return 0;
> +}
> +
> +static int adm9240_chip_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	u8 regs[2];
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_chip_alarms:
> +		err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
> +		if (err < 0)
> +			return err;
> +		*val = regs[0] | regs[1] << 8;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_intrusion_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_intrusion_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(1), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(4));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_intrusion_write(struct device *dev, u32 attr, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_intrusion_alarm:
> +		if (val)
> +			return -EINVAL;
> +		err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
> +		if (err < 0)
> +			return err;
> +		dev_dbg(data->dev, "chassis intrusion latch cleared\n");
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_in_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int reg;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = ADM9240_REG_IN(channel);
> +		break;
> +	case hwmon_in_min:
> +		reg = ADM9240_REG_IN_MIN(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADM9240_REG_IN_MAX(channel);
> +		break;
> +	case hwmon_in_alarm:
> +		if (channel < 4) {
> +			reg = ADM9240_REG_INT(0);
> +		} else {
> +			reg = ADM9240_REG_INT(1);
> +			channel -= 4;
> +		}
> +		err = regmap_read(data->regmap, reg, &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(channel));
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	err = regmap_read(data->regmap, reg, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = IN_FROM_REG(regval, channel);
>   	return 0;
>   }
>   
> +static int adm9240_in_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		reg = ADM9240_REG_IN_MIN(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADM9240_REG_IN(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return regmap_write(data->regmap, reg, IN_TO_REG(val, channel));
> +}
> +
> +static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), &regval);
> +		if (err < 0)
> +			return err;
> +		if (regval == 255 && data->fan_div[channel] < 3) {
> +			/* adjust fan clock divider on overflow */
> +			err = adm9240_write_fan_div(data, channel,
> +						    ++data->fan_div[channel]);
> +			if (err)
> +				return err;
> +		}
> +		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
> +		break;
> +	case hwmon_fan_div:
> +		*val = BIT(data->fan_div[channel]);
> +		break;
> +	case hwmon_fan_min:
> +		err = regmap_read(data->regmap, ADM9240_REG_FAN_MIN(channel), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
> +		break;
> +	case hwmon_fan_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(channel + 6));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_fan_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_fan_min:
> +		err = adm9240_fan_min_write(data, channel, val);
> +		if (err < 0)
> +			return err;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_temp_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err, temp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP, &regval);
> +		if (err < 0)
> +			return err;
> +		temp = regval << 1;
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &regval);
> +		if (err < 0)
> +			return err;
> +		temp |= regval >> 7;
> +		*val = sign_extend32(temp, 8) * 500;
> +		break;
> +	case hwmon_temp_max:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = (s8)regval * 1000;
> +		break;
> +	case hwmon_temp_max_hyst:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(1), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = (s8)regval * 1000;
> +		break;
> +	case hwmon_temp_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(4));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_temp_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		reg = ADM9240_REG_TEMP_MAX(0);
> +		break;
> +	case hwmon_temp_max_hyst:
> +		reg = ADM9240_REG_TEMP_MAX(1);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return regmap_write(data->regmap, reg, TEMP_TO_REG(val));
> +}
> +
> +static int adm9240_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return adm9240_chip_read(dev, attr, val);
> +	case hwmon_intrusion:
> +		return adm9240_intrusion_read(dev, attr, val);
> +	case hwmon_in:
> +		return adm9240_in_read(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return adm9240_fan_read(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return adm9240_temp_read(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int adm9240_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			 int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_intrusion:
> +		return adm9240_intrusion_write(dev, attr, val);
> +	case hwmon_in:
> +		return adm9240_in_write(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return adm9240_fan_write(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return adm9240_temp_write(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t adm9240_is_visible(const void *_data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	umode_t mode = 0;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_alarms:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_intrusion:
> +		switch (attr) {
> +		case hwmon_intrusion_alarm:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp:
> +		case hwmon_temp_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_temp_max:
> +		case hwmon_temp_max_hyst:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_div:
> +		case hwmon_fan_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_fan_min:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_in_min:
> +		case hwmon_in_max:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return mode;
> +}
> +
> +static const struct hwmon_ops adm9240_hwmon_ops = {
> +	.is_visible = adm9240_is_visible,
> +	.read = adm9240_read,
> +	.write = adm9240_write,
> +};
> +
> +static const struct hwmon_channel_info *adm9240_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS),
> +	HWMON_CHANNEL_INFO(intrusion, HWMON_INTRUSION_ALARM),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST | HWMON_T_ALARM),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM,
> +			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info adm9240_chip_info = {
> +	.ops = &adm9240_hwmon_ops,
> +	.info = adm9240_info,
> +};
> +
> +static bool adm9240_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ADM9240_REG_IN(0) ... ADM9240_REG_IN(5):
> +	case ADM9240_REG_FAN(0) ... ADM9240_REG_FAN(1):
> +	case ADM9240_REG_INT(0) ... ADM9240_REG_INT(1):
> +	case ADM9240_REG_TEMP:
> +	case ADM9240_REG_TEMP_CONF:
> +	case ADM9240_REG_VID_FAN_DIV:
> +	case ADM9240_REG_VID4:
> +	case ADM9240_REG_ANALOG_OUT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>   static const struct regmap_config adm9240_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.use_single_read = true,
>   	.use_single_write = true,
> +	.volatile_reg = adm9240_volatile_reg,
>   };
>   
> -static int adm9240_probe(struct i2c_client *new_client)
> +static int adm9240_probe(struct i2c_client *client)
>   {
> -	struct device *dev = &new_client->dev;
> +	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct adm9240_data *data;
>   	int err;
> @@ -835,7 +786,7 @@ static int adm9240_probe(struct i2c_client *new_client)
>   
>   	data->dev = dev;
>   	mutex_init(&data->update_lock);
> -	data->regmap = devm_regmap_init_i2c(new_client, &adm9240_regmap_config);
> +	data->regmap = devm_regmap_init_i2c(client, &adm9240_regmap_config);
>   	if (IS_ERR(data->regmap))
>   		return PTR_ERR(data->regmap);
>   
> @@ -843,10 +794,9 @@ static int adm9240_probe(struct i2c_client *new_client)
>   	if (err < 0)
>   		return err;
>   
> -	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   new_client->name,
> -							   data,
> -							   adm9240_groups);
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> +							 &adm9240_chip_info,
> +							 adm9240_groups);
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Chris Packham]

Hi Guenter,

On 11/03/21 8:33 pm, Guenter Roeck wrote:
> Also use regmap for register caching. This change reduces code and
> data size by more than 40%.
>
> While at it, fixed some warnings reported by checkpatch.
>
> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

I've just been informed by our QA team that it looks like the 
configuration of limits (e.g. by writing to sysfs) has been broken. 
Probably by this change. I'm just starting to dig into it now but I 
though I'd give you a heads up.

> ---
>   drivers/hwmon/adm9240.c | 940 +++++++++++++++++++---------------------
>   1 file changed, 445 insertions(+), 495 deletions(-)
>
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 7404082c7a3f..5677263bcf0d 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -28,6 +28,7 @@
>    * LM81 extended temp reading not implemented
>    */
>   
> +#include <linux/bits.h>
>   #include <linux/init.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
> @@ -37,7 +38,6 @@
>   #include <linux/hwmon-vid.h>
>   #include <linux/err.h>
>   #include <linux/mutex.h>
> -#include <linux/jiffies.h>
>   #include <linux/regmap.h>
>   
>   /* Addresses to scan */
> @@ -126,29 +126,15 @@ struct adm9240_data {
>   	struct device *dev;
>   	struct regmap *regmap;
>   	struct mutex update_lock;
> -	char valid;
> -	unsigned long last_updated_measure;
> -	unsigned long last_updated_config;
> -
> -	u8 in[6];		/* ro	in0_input */
> -	u8 in_max[6];		/* rw	in0_max */
> -	u8 in_min[6];		/* rw	in0_min */
> -	u8 fan[2];		/* ro	fan1_input */
> -	u8 fan_min[2];		/* rw	fan1_min */
> +
>   	u8 fan_div[2];		/* rw	fan1_div, read-only accessor */
> -	s16 temp;		/* ro	temp1_input, 9-bit sign-extended */
> -	s8 temp_max[2];		/* rw	0 -> temp_max, 1 -> temp_max_hyst */
> -	u16 alarms;		/* ro	alarms */
> -	u8 aout;		/* rw	aout_output */
> -	u8 vid;			/* ro	vid */
>   	u8 vrm;			/* --	vrm set on startup, no accessor */
>   };
>   
>   /* write new fan div, callers must hold data->update_lock */
> -static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
> -		u8 fan_div)
> +static int adm9240_write_fan_div(struct adm9240_data *data, int channel, u8 fan_div)
>   {
> -	unsigned int reg, old, shift = (nr + 2) * 2;
> +	unsigned int reg, old, shift = (channel + 2) * 2;
>   	int err;
>   
>   	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &reg);
> @@ -161,335 +147,12 @@ static int adm9240_write_fan_div(struct adm9240_data *data, int nr,
>   	if (err < 0)
>   		return err;
>   	dev_dbg(data->dev,
> -		"fan%d clock divider changed from %u to %u\n",
> -		nr + 1, 1 << old, 1 << fan_div);
> +		"fan%d clock divider changed from %lu to %lu\n",
> +		channel + 1, BIT(old), BIT(fan_div));
>   
>   	return 0;
>   }
>   
> -static int adm9240_update_measure(struct adm9240_data *data)
> -{
> -	unsigned int val;
> -	u8 regs[2];
> -	int err;
> -	int i;
> -
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_IN(0), &data->in[0], 6);
> -	if (err < 0)
> -		return err;
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
> -	if (err < 0)
> -		return err;
> -
> -	data->alarms = regs[0] | regs[1] << 8;
> -
> -	/*
> -	 * read temperature: assume temperature changes less than
> -	 * 0.5'C per two measurement cycles thus ignore possible
> -	 * but unlikely aliasing error on lsb reading. --Grant
> -	 */
> -	err = regmap_read(data->regmap, ADM9240_REG_TEMP, &val);
> -	if (err < 0)
> -		return err;
> -	data->temp = val << 8;
> -	err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &val);
> -	if (err < 0)
> -		return err;
> -	data->temp |= val;
> -
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN(0),
> -			       &data->fan[0], 2);
> -	if (err < 0)
> -		return err;
> -
> -	for (i = 0; i < 2; i++) { /* read fans */
> -		/* adjust fan clock divider on overflow */
> -		if (data->valid && data->fan[i] == 255 &&
> -				data->fan_div[i] < 3) {
> -
> -			err = adm9240_write_fan_div(data, i,
> -					++data->fan_div[i]);
> -			if (err < 0)
> -				return err;
> -
> -			/* adjust fan_min if active, but not to 0 */
> -			if (data->fan_min[i] < 255 &&
> -					data->fan_min[i] >= 2)
> -				data->fan_min[i] /= 2;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static int adm9240_update_config(struct adm9240_data *data)
> -{
> -	unsigned int val;
> -	int i;
> -	int err;
> -
> -	for (i = 0; i < 6; i++) {
> -		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MIN(i),
> -				      &data->in_min[i], 1);
> -		if (err < 0)
> -			return err;
> -		err = regmap_raw_read(data->regmap, ADM9240_REG_IN_MAX(i),
> -				      &data->in_max[i], 1);
> -		if (err < 0)
> -			return err;
> -	}
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_FAN_MIN(0),
> -				      &data->fan_min[0], 2);
> -	if (err < 0)
> -		return err;
> -	err = regmap_bulk_read(data->regmap, ADM9240_REG_TEMP_MAX(0),
> -				      &data->temp_max[0], 2);
> -	if (err < 0)
> -		return err;
> -
> -	/* read fan divs and 5-bit VID */
> -	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &val);
> -	if (err < 0)
> -		return err;
> -	data->fan_div[0] = (val >> 4) & 3;
> -	data->fan_div[1] = (val >> 6) & 3;
> -	data->vid = val & 0x0f;
> -	err = regmap_read(data->regmap, ADM9240_REG_VID4, &val);
> -	if (err < 0)
> -		return err;
> -	data->vid |= (val & 1) << 4;
> -	/* read analog out */
> -	err = regmap_raw_read(data->regmap, ADM9240_REG_ANALOG_OUT,
> -			      &data->aout, 1);
> -
> -	return err;
> -}
> -
> -static struct adm9240_data *adm9240_update_device(struct device *dev)
> -{
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	int err;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	/* minimum measurement cycle: 1.75 seconds */
> -	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
> -			|| !data->valid) {
> -		err = adm9240_update_measure(data);
> -		if (err < 0) {
> -			data->valid = 0;
> -			mutex_unlock(&data->update_lock);
> -			return ERR_PTR(err);
> -		}
> -		data->last_updated_measure = jiffies;
> -	}
> -
> -	/* minimum config reading cycle: 300 seconds */
> -	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
> -			|| !data->valid) {
> -		err = adm9240_update_config(data);
> -		if (err < 0) {
> -			data->valid = 0;
> -			mutex_unlock(&data->update_lock);
> -			return ERR_PTR(err);
> -		}
> -		data->last_updated_config = jiffies;
> -		data->valid = 1;
> -	}
> -	mutex_unlock(&data->update_lock);
> -	return data;
> -}
> -
> -/*** sysfs accessors ***/
> -
> -/* temperature */
> -static ssize_t temp1_input_show(struct device *dev,
> -				struct device_attribute *dummy, char *buf)
> -{
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", data->temp / 128 * 500); /* 9-bit value */
> -}
> -
> -static ssize_t max_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", data->temp_max[attr->index] * 1000);
> -}
> -
> -static ssize_t max_store(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	long val;
> -	int err;
> -
> -	err = kstrtol(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->temp_max[attr->index] = TEMP_TO_REG(val);
> -	err = regmap_write(data->regmap, ADM9240_REG_TEMP_MAX(attr->index),
> -			   data->temp_max[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static DEVICE_ATTR_RO(temp1_input);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max, max, 0);
> -static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, max, 1);
> -
> -/* voltage */
> -static ssize_t in_show(struct device *dev, struct device_attribute *devattr,
> -		       char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_min_show(struct device *dev,
> -			   struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_max_show(struct device *dev,
> -			   struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[attr->index],
> -				attr->index));
> -}
> -
> -static ssize_t in_min_store(struct device *dev,
> -			    struct device_attribute *devattr, const char *buf,
> -			    size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->in_min[attr->index] = IN_TO_REG(val, attr->index);
> -	err = regmap_write(data->regmap, ADM9240_REG_IN_MIN(attr->index),
> -			   data->in_min[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static ssize_t in_max_store(struct device *dev,
> -			    struct device_attribute *devattr, const char *buf,
> -			    size_t count)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	data->in_max[attr->index] = IN_TO_REG(val, attr->index);
> -	err = regmap_write(data->regmap, ADM9240_REG_IN_MAX(attr->index),
> -			   data->in_max[attr->index]);
> -	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static SENSOR_DEVICE_ATTR_RO(in0_input, in, 0);
> -static SENSOR_DEVICE_ATTR_RW(in0_min, in_min, 0);
> -static SENSOR_DEVICE_ATTR_RW(in0_max, in_max, 0);
> -static SENSOR_DEVICE_ATTR_RO(in1_input, in, 1);
> -static SENSOR_DEVICE_ATTR_RW(in1_min, in_min, 1);
> -static SENSOR_DEVICE_ATTR_RW(in1_max, in_max, 1);
> -static SENSOR_DEVICE_ATTR_RO(in2_input, in, 2);
> -static SENSOR_DEVICE_ATTR_RW(in2_min, in_min, 2);
> -static SENSOR_DEVICE_ATTR_RW(in2_max, in_max, 2);
> -static SENSOR_DEVICE_ATTR_RO(in3_input, in, 3);
> -static SENSOR_DEVICE_ATTR_RW(in3_min, in_min, 3);
> -static SENSOR_DEVICE_ATTR_RW(in3_max, in_max, 3);
> -static SENSOR_DEVICE_ATTR_RO(in4_input, in, 4);
> -static SENSOR_DEVICE_ATTR_RW(in4_min, in_min, 4);
> -static SENSOR_DEVICE_ATTR_RW(in4_max, in_max, 4);
> -static SENSOR_DEVICE_ATTR_RO(in5_input, in, 5);
> -static SENSOR_DEVICE_ATTR_RW(in5_min, in_min, 5);
> -static SENSOR_DEVICE_ATTR_RW(in5_max, in_max, 5);
> -
> -/* fans */
> -static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
> -				1 << data->fan_div[attr->index]));
> -}
> -
> -static ssize_t fan_min_show(struct device *dev,
> -			    struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[attr->index],
> -				1 << data->fan_div[attr->index]));
> -}
> -
> -static ssize_t fan_div_show(struct device *dev,
> -			    struct device_attribute *devattr, char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
> -}
> -
>   /*
>    * set fan speed low limit:
>    *
> @@ -501,37 +164,25 @@ static ssize_t fan_div_show(struct device *dev,
>    * - otherwise: select fan clock divider to suit fan speed low limit,
>    *   measurement code may adjust registers to ensure fan speed reading
>    */
> -static ssize_t fan_min_store(struct device *dev,
> -			     struct device_attribute *devattr,
> -			     const char *buf, size_t count)
> +static int adm9240_fan_min_write(struct adm9240_data *data, int channel, long val)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	int nr = attr->index;
>   	u8 new_div;
> -	unsigned long val;
> +	u8 fan_min;
>   	int err;
>   
> -	err = kstrtoul(buf, 10, &val);
> -	if (err)
> -		return err;
> -
>   	mutex_lock(&data->update_lock);
>   
>   	if (!val) {
> -		data->fan_min[nr] = 255;
> -		new_div = data->fan_div[nr];
> -
> -		dev_dbg(data->dev, "fan%u low limit set disabled\n",
> -				nr + 1);
> +		fan_min = 255;
> +		new_div = data->fan_div[channel];
>   
> +		dev_dbg(data->dev, "fan%u low limit set disabled\n", channel + 1);
>   	} else if (val < 1350000 / (8 * 254)) {
>   		new_div = 3;
> -		data->fan_min[nr] = 254;
> +		fan_min = 254;
>   
>   		dev_dbg(data->dev, "fan%u low limit set minimum %u\n",
> -				nr + 1, FAN_FROM_REG(254, 1 << new_div));
> -
> +			channel + 1, FAN_FROM_REG(254, BIT(new_div)));
>   	} else {
>   		unsigned int new_min = 1350000 / val;
>   
> @@ -543,87 +194,55 @@ static ssize_t fan_min_store(struct device *dev,
>   		if (!new_min) /* keep > 0 */
>   			new_min++;
>   
> -		data->fan_min[nr] = new_min;
> +		fan_min = new_min;
>   
>   		dev_dbg(data->dev, "fan%u low limit set fan speed %u\n",
> -				nr + 1, FAN_FROM_REG(new_min, 1 << new_div));
> +			channel + 1, FAN_FROM_REG(new_min, BIT(new_div)));
>   	}
>   
> -	if (new_div != data->fan_div[nr]) {
> -		data->fan_div[nr] = new_div;
> -		adm9240_write_fan_div(data, nr, new_div);
> +	if (new_div != data->fan_div[channel]) {
> +		data->fan_div[channel] = new_div;
> +		adm9240_write_fan_div(data, channel, new_div);
>   	}
> -	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(nr),
> -			   data->fan_min[nr]);
> +	err = regmap_write(data->regmap, ADM9240_REG_FAN_MIN(channel), fan_min);
>   
>   	mutex_unlock(&data->update_lock);
> -	return err < 0 ? err : count;
> -}
> -
> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
> -static SENSOR_DEVICE_ATTR_RW(fan1_min, fan_min, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan1_div, fan_div, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
> -static SENSOR_DEVICE_ATTR_RW(fan2_min, fan_min, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan2_div, fan_div, 1);
> -
> -/* alarms */
> -static ssize_t alarms_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct adm9240_data *data = adm9240_update_device(dev);
>   
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	return sprintf(buf, "%u\n", data->alarms);
> +	return err;
>   }
> -static DEVICE_ATTR_RO(alarms);
> -
> -static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
> -			  char *buf)
> -{
> -	int bitnr = to_sensor_dev_attr(attr)->index;
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
>   
> -	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
> -}
> -static SENSOR_DEVICE_ATTR_RO(in0_alarm, alarm, 0);
> -static SENSOR_DEVICE_ATTR_RO(in1_alarm, alarm, 1);
> -static SENSOR_DEVICE_ATTR_RO(in2_alarm, alarm, 2);
> -static SENSOR_DEVICE_ATTR_RO(in3_alarm, alarm, 3);
> -static SENSOR_DEVICE_ATTR_RO(in4_alarm, alarm, 8);
> -static SENSOR_DEVICE_ATTR_RO(in5_alarm, alarm, 9);
> -static SENSOR_DEVICE_ATTR_RO(temp1_alarm, alarm, 4);
> -static SENSOR_DEVICE_ATTR_RO(fan1_alarm, alarm, 6);
> -static SENSOR_DEVICE_ATTR_RO(fan2_alarm, alarm, 7);
> -
> -/* vid */
>   static ssize_t cpu0_vid_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)
>   {
> -	struct adm9240_data *data = adm9240_update_device(dev);
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +	u8 vid;
>   
> -	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
> +	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
> +	if (err < 0)
> +		return err;
> +	vid = regval & 0x0f;
> +	err = regmap_read(data->regmap, ADM9240_REG_VID4, &regval);
> +	if (err < 0)
> +		return err;
> +	vid |= (regval & 1) << 4;
> +	return sprintf(buf, "%d\n", vid_from_reg(vid, data->vrm));
>   }
>   static DEVICE_ATTR_RO(cpu0_vid);
>   
> -/* analog output */
>   static ssize_t aout_output_show(struct device *dev,
>   				struct device_attribute *attr, char *buf)
>   {
> -	struct adm9240_data *data = adm9240_update_device(dev);
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
>   
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	err = regmap_read(data->regmap, ADM9240_REG_ANALOG_OUT, &regval);
> +	if (err)
> +		return err;
>   
> -	return sprintf(buf, "%d\n", AOUT_FROM_REG(data->aout));
> +	return sprintf(buf, "%d\n", AOUT_FROM_REG(regval));
>   }
>   
>   static ssize_t aout_output_store(struct device *dev,
> @@ -638,76 +257,13 @@ static ssize_t aout_output_store(struct device *dev,
>   	if (err)
>   		return err;
>   
> -	mutex_lock(&data->update_lock);
> -	data->aout = AOUT_TO_REG(val);
> -	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, data->aout);
> -	mutex_unlock(&data->update_lock);
> +	err = regmap_write(data->regmap, ADM9240_REG_ANALOG_OUT, AOUT_TO_REG(val));
>   	return err < 0 ? err : count;
>   }
>   static DEVICE_ATTR_RW(aout_output);
>   
> -static ssize_t alarm_store(struct device *dev, struct device_attribute *attr,
> -			   const char *buf, size_t count)
> -{
> -	struct adm9240_data *data = dev_get_drvdata(dev);
> -	unsigned long val;
> -	int err;
> -
> -	if (kstrtoul(buf, 10, &val) || val != 0)
> -		return -EINVAL;
> -
> -	mutex_lock(&data->update_lock);
> -	err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
> -	data->valid = 0;		/* Force cache refresh */
> -	mutex_unlock(&data->update_lock);
> -	if (err < 0)
> -		return err;
> -	dev_dbg(data->dev, "chassis intrusion latch cleared\n");
> -
> -	return count;
> -}
> -static SENSOR_DEVICE_ATTR_RW(intrusion0_alarm, alarm, 12);
> -
>   static struct attribute *adm9240_attrs[] = {
> -	&sensor_dev_attr_in0_input.dev_attr.attr,
> -	&sensor_dev_attr_in0_min.dev_attr.attr,
> -	&sensor_dev_attr_in0_max.dev_attr.attr,
> -	&sensor_dev_attr_in0_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in1_input.dev_attr.attr,
> -	&sensor_dev_attr_in1_min.dev_attr.attr,
> -	&sensor_dev_attr_in1_max.dev_attr.attr,
> -	&sensor_dev_attr_in1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in2_input.dev_attr.attr,
> -	&sensor_dev_attr_in2_min.dev_attr.attr,
> -	&sensor_dev_attr_in2_max.dev_attr.attr,
> -	&sensor_dev_attr_in2_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in3_input.dev_attr.attr,
> -	&sensor_dev_attr_in3_min.dev_attr.attr,
> -	&sensor_dev_attr_in3_max.dev_attr.attr,
> -	&sensor_dev_attr_in3_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in4_input.dev_attr.attr,
> -	&sensor_dev_attr_in4_min.dev_attr.attr,
> -	&sensor_dev_attr_in4_max.dev_attr.attr,
> -	&sensor_dev_attr_in4_alarm.dev_attr.attr,
> -	&sensor_dev_attr_in5_input.dev_attr.attr,
> -	&sensor_dev_attr_in5_min.dev_attr.attr,
> -	&sensor_dev_attr_in5_max.dev_attr.attr,
> -	&sensor_dev_attr_in5_alarm.dev_attr.attr,
> -	&dev_attr_temp1_input.attr,
> -	&sensor_dev_attr_temp1_max.dev_attr.attr,
> -	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> -	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,
> -	&sensor_dev_attr_fan1_div.dev_attr.attr,
> -	&sensor_dev_attr_fan1_min.dev_attr.attr,
> -	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,
> -	&sensor_dev_attr_fan2_div.dev_attr.attr,
> -	&sensor_dev_attr_fan2_min.dev_attr.attr,
> -	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
> -	&dev_attr_alarms.attr,
>   	&dev_attr_aout_output.attr,
> -	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,
>   	&dev_attr_cpu0_vid.attr,
>   	NULL
>   };
> @@ -749,13 +305,14 @@ static int adm9240_detect(struct i2c_client *new_client,
>   		 man_id == 0x23 ? "ADM9240" :
>   		 man_id == 0xda ? "DS1780" : "LM81", die_rev);
>   
> -	strlcpy(info->type, name, I2C_NAME_SIZE);
> +	strscpy(info->type, name, I2C_NAME_SIZE);
>   
>   	return 0;
>   }
>   
>   static int adm9240_init_client(struct adm9240_data *data)
>   {
> +	unsigned int regval;
>   	u8 conf, mode;
>   	int err;
>   
> @@ -792,13 +349,13 @@ static int adm9240_init_client(struct adm9240_data *data)
>   		}
>   		for (i = 0; i < 2; i++) {
>   			err = regmap_write(data->regmap,
> -					ADM9240_REG_FAN_MIN(i), 255);
> +					   ADM9240_REG_FAN_MIN(i), 255);
>   			if (err < 0)
>   				return err;
>   		}
>   		for (i = 0; i < 2; i++) {
>   			err = regmap_write(data->regmap,
> -					ADM9240_REG_TEMP_MAX(i), 127);
> +					   ADM9240_REG_TEMP_MAX(i), 127);
>   			if (err < 0)
>   				return err;
>   		}
> @@ -812,19 +369,413 @@ static int adm9240_init_client(struct adm9240_data *data)
>   			 "cold start: config was 0x%02x mode %u\n", conf, mode);
>   	}
>   
> +	/* read fan divs */
> +	err = regmap_read(data->regmap, ADM9240_REG_VID_FAN_DIV, &regval);
> +	if (err < 0)
> +		return err;
> +	data->fan_div[0] = (regval >> 4) & 3;
> +	data->fan_div[1] = (regval >> 6) & 3;
> +	return 0;
> +}
> +
> +static int adm9240_chip_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	u8 regs[2];
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_chip_alarms:
> +		err = regmap_bulk_read(data->regmap, ADM9240_REG_INT(0), &regs, 2);
> +		if (err < 0)
> +			return err;
> +		*val = regs[0] | regs[1] << 8;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_intrusion_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_intrusion_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(1), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(4));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_intrusion_write(struct device *dev, u32 attr, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_intrusion_alarm:
> +		if (val)
> +			return -EINVAL;
> +		err = regmap_write(data->regmap, ADM9240_REG_CHASSIS_CLEAR, 0x80);
> +		if (err < 0)
> +			return err;
> +		dev_dbg(data->dev, "chassis intrusion latch cleared\n");
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_in_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int reg;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = ADM9240_REG_IN(channel);
> +		break;
> +	case hwmon_in_min:
> +		reg = ADM9240_REG_IN_MIN(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADM9240_REG_IN_MAX(channel);
> +		break;
> +	case hwmon_in_alarm:
> +		if (channel < 4) {
> +			reg = ADM9240_REG_INT(0);
> +		} else {
> +			reg = ADM9240_REG_INT(1);
> +			channel -= 4;
> +		}
> +		err = regmap_read(data->regmap, reg, &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(channel));
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	err = regmap_read(data->regmap, reg, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = IN_FROM_REG(regval, channel);
>   	return 0;
>   }
>   
> +static int adm9240_in_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		reg = ADM9240_REG_IN_MIN(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADM9240_REG_IN(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return regmap_write(data->regmap, reg, IN_TO_REG(val, channel));
> +}
> +
> +static int adm9240_fan_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), &regval);
> +		if (err < 0)
> +			return err;
> +		if (regval == 255 && data->fan_div[channel] < 3) {
> +			/* adjust fan clock divider on overflow */
> +			err = adm9240_write_fan_div(data, channel,
> +						    ++data->fan_div[channel]);
> +			if (err)
> +				return err;
> +		}
> +		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
> +		break;
> +	case hwmon_fan_div:
> +		*val = BIT(data->fan_div[channel]);
> +		break;
> +	case hwmon_fan_min:
> +		err = regmap_read(data->regmap, ADM9240_REG_FAN_MIN(channel), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = FAN_FROM_REG(regval, BIT(data->fan_div[channel]));
> +		break;
> +	case hwmon_fan_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(channel + 6));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_fan_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_fan_min:
> +		err = adm9240_fan_min_write(data, channel, val);
> +		if (err < 0)
> +			return err;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_temp_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err, temp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP, &regval);
> +		if (err < 0)
> +			return err;
> +		temp = regval << 1;
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_CONF, &regval);
> +		if (err < 0)
> +			return err;
> +		temp |= regval >> 7;
> +		*val = sign_extend32(temp, 8) * 500;
> +		break;
> +	case hwmon_temp_max:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = (s8)regval * 1000;
> +		break;
> +	case hwmon_temp_max_hyst:
> +		err = regmap_read(data->regmap, ADM9240_REG_TEMP_MAX(1), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = (s8)regval * 1000;
> +		break;
> +	case hwmon_temp_alarm:
> +		err = regmap_read(data->regmap, ADM9240_REG_INT(0), &regval);
> +		if (err < 0)
> +			return err;
> +		*val = !!(regval & BIT(4));
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int adm9240_temp_write(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct adm9240_data *data = dev_get_drvdata(dev);
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		reg = ADM9240_REG_TEMP_MAX(0);
> +		break;
> +	case hwmon_temp_max_hyst:
> +		reg = ADM9240_REG_TEMP_MAX(1);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return regmap_write(data->regmap, reg, TEMP_TO_REG(val));
> +}
> +
> +static int adm9240_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return adm9240_chip_read(dev, attr, val);
> +	case hwmon_intrusion:
> +		return adm9240_intrusion_read(dev, attr, val);
> +	case hwmon_in:
> +		return adm9240_in_read(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return adm9240_fan_read(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return adm9240_temp_read(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int adm9240_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +			 int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_intrusion:
> +		return adm9240_intrusion_write(dev, attr, val);
> +	case hwmon_in:
> +		return adm9240_in_write(dev, attr, channel, val);
> +	case hwmon_fan:
> +		return adm9240_fan_write(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return adm9240_temp_write(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t adm9240_is_visible(const void *_data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	umode_t mode = 0;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_alarms:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_intrusion:
> +		switch (attr) {
> +		case hwmon_intrusion_alarm:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp:
> +		case hwmon_temp_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_temp_max:
> +		case hwmon_temp_max_hyst:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_div:
> +		case hwmon_fan_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_fan_min:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_alarm:
> +			mode = 0444;
> +			break;
> +		case hwmon_in_min:
> +		case hwmon_in_max:
> +			mode = 0644;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return mode;
> +}
> +
> +static const struct hwmon_ops adm9240_hwmon_ops = {
> +	.is_visible = adm9240_is_visible,
> +	.read = adm9240_read,
> +	.write = adm9240_write,
> +};
> +
> +static const struct hwmon_channel_info *adm9240_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS),
> +	HWMON_CHANNEL_INFO(intrusion, HWMON_INTRUSION_ALARM),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST | HWMON_T_ALARM),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_ALARM),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM,
> +			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_DIV | HWMON_F_ALARM),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info adm9240_chip_info = {
> +	.ops = &adm9240_hwmon_ops,
> +	.info = adm9240_info,
> +};
> +
> +static bool adm9240_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ADM9240_REG_IN(0) ... ADM9240_REG_IN(5):
> +	case ADM9240_REG_FAN(0) ... ADM9240_REG_FAN(1):
> +	case ADM9240_REG_INT(0) ... ADM9240_REG_INT(1):
> +	case ADM9240_REG_TEMP:
> +	case ADM9240_REG_TEMP_CONF:
> +	case ADM9240_REG_VID_FAN_DIV:
> +	case ADM9240_REG_VID4:
> +	case ADM9240_REG_ANALOG_OUT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>   static const struct regmap_config adm9240_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.use_single_read = true,
>   	.use_single_write = true,
> +	.volatile_reg = adm9240_volatile_reg,
>   };
>   
> -static int adm9240_probe(struct i2c_client *new_client)
> +static int adm9240_probe(struct i2c_client *client)
>   {
> -	struct device *dev = &new_client->dev;
> +	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct adm9240_data *data;
>   	int err;
> @@ -835,7 +786,7 @@ static int adm9240_probe(struct i2c_client *new_client)
>   
>   	data->dev = dev;
>   	mutex_init(&data->update_lock);
> -	data->regmap = devm_regmap_init_i2c(new_client, &adm9240_regmap_config);
> +	data->regmap = devm_regmap_init_i2c(client, &adm9240_regmap_config);
>   	if (IS_ERR(data->regmap))
>   		return PTR_ERR(data->regmap);
>   
> @@ -843,10 +794,9 @@ static int adm9240_probe(struct i2c_client *new_client)
>   	if (err < 0)
>   		return err;
>   
> -	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   new_client->name,
> -							   data,
> -							   adm9240_groups);
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> +							 &adm9240_chip_info,
> +							 adm9240_groups);
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Guenter Roeck]

On 5/12/21 2:54 PM, Chris Packham wrote:
> Hi Guenter,
> 
> On 11/03/21 8:33 pm, Guenter Roeck wrote:
>> Also use regmap for register caching. This change reduces code and
>> data size by more than 40%.
>>
>> While at it, fixed some warnings reported by checkpatch.
>>
>> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> I've just been informed by our QA team that it looks like the
> configuration of limits (e.g. by writing to sysfs) has been broken.
> Probably by this change. I'm just starting to dig into it now but I
> though I'd give you a heads up.
> 

Thanks for the heads up.

It looks like voltage maximum writes use the wrong register,
ADM9240_REG_IN instead of ADM9240_REG_IN_MAX.
Odd, I'd have assumed that my module test code catches that.
I'll have to check why it doesn't.

Anyway, anything more specific ?

Thanks,
Guenter

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Guenter Roeck]

On 5/12/21 3:09 PM, Guenter Roeck wrote:
> On 5/12/21 2:54 PM, Chris Packham wrote:
>> Hi Guenter,
>>
>> On 11/03/21 8:33 pm, Guenter Roeck wrote:
>>> Also use regmap for register caching. This change reduces code and
>>> data size by more than 40%.
>>>
>>> While at it, fixed some warnings reported by checkpatch.
>>>
>>> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>
>> I've just been informed by our QA team that it looks like the
>> configuration of limits (e.g. by writing to sysfs) has been broken.
>> Probably by this change. I'm just starting to dig into it now but I
>> though I'd give you a heads up.
>>
> 
> Thanks for the heads up.
> 
> It looks like voltage maximum writes use the wrong register,
> ADM9240_REG_IN instead of ADM9240_REG_IN_MAX.
> Odd, I'd have assumed that my module test code catches that.
> I'll have to check why it doesn't.
> 

Yes, turns out my module test script does not catch that situation.
It tries to find the value range and determines that there is no range
(because all writes are into the wrong register). I'll have to fix that.

> Anyway, anything more specific ?
> 
I'll wait for your response before submitting a patch.

Guenter

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Chris Packham]

On 13/05/21 10:35 am, Guenter Roeck wrote:
> On 5/12/21 3:09 PM, Guenter Roeck wrote:
>> On 5/12/21 2:54 PM, Chris Packham wrote:
>>> Hi Guenter,
>>>
>>> On 11/03/21 8:33 pm, Guenter Roeck wrote:
>>>> Also use regmap for register caching. This change reduces code and
>>>> data size by more than 40%.
>>>>
>>>> While at it, fixed some warnings reported by checkpatch.
>>>>
>>>> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> I've just been informed by our QA team that it looks like the
>>> configuration of limits (e.g. by writing to sysfs) has been broken.
>>> Probably by this change. I'm just starting to dig into it now but I
>>> though I'd give you a heads up.
>>>
>>
>> Thanks for the heads up.
>>
>> It looks like voltage maximum writes use the wrong register,
>> ADM9240_REG_IN instead of ADM9240_REG_IN_MAX.
>> Odd, I'd have assumed that my module test code catches that.
>> I'll have to check why it doesn't.
>>
>
> Yes, turns out my module test script does not catch that situation.
> It tries to find the value range and determines that there is no range
> (because all writes are into the wrong register). I'll have to fix that.
>
>> Anyway, anything more specific ?
>>
> I'll wait for your response before submitting a patch.
>
I agree that the writes to max aren't working. Haven't checked min.

[root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
3586
[root@awplus flash]# echo 1097 >/sys/class/hwmon/hwmon0/in5_max
[root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
3586

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Guenter Roeck]

On 5/12/21 3:41 PM, Chris Packham wrote:
> 
> On 13/05/21 10:35 am, Guenter Roeck wrote:
>> On 5/12/21 3:09 PM, Guenter Roeck wrote:
>>> On 5/12/21 2:54 PM, Chris Packham wrote:
>>>> Hi Guenter,
>>>>
>>>> On 11/03/21 8:33 pm, Guenter Roeck wrote:
>>>>> Also use regmap for register caching. This change reduces code and
>>>>> data size by more than 40%.
>>>>>
>>>>> While at it, fixed some warnings reported by checkpatch.
>>>>>
>>>>> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>
>>>> I've just been informed by our QA team that it looks like the
>>>> configuration of limits (e.g. by writing to sysfs) has been broken.
>>>> Probably by this change. I'm just starting to dig into it now but I
>>>> though I'd give you a heads up.
>>>>
>>>
>>> Thanks for the heads up.
>>>
>>> It looks like voltage maximum writes use the wrong register,
>>> ADM9240_REG_IN instead of ADM9240_REG_IN_MAX.
>>> Odd, I'd have assumed that my module test code catches that.
>>> I'll have to check why it doesn't.
>>>
>>
>> Yes, turns out my module test script does not catch that situation.
>> It tries to find the value range and determines that there is no range
>> (because all writes are into the wrong register). I'll have to fix that.
>>
>>> Anyway, anything more specific ?
>>>
>> I'll wait for your response before submitting a patch.
>>
> I agree that the writes to max aren't working. Haven't checked min.
> 
> [root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
> 3586
> [root@awplus flash]# echo 1097 >/sys/class/hwmon/hwmon0/in5_max
> [root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
> 3586
> 

Anything else ? I got a patch ready to fix that, but I would prefer to
fix everything in one go if possible. My (fixed) module test script
doesn't pick up other problems, but obviously we can't really trust it.

Thanks,
Guenter

Re: [PATCH 3/3] hwmon: (adm9240) Convert to devm_hwmon_device_register_with_info API

[Chris Packham]

On 13/05/21 10:51 am, Guenter Roeck wrote:
> On 5/12/21 3:41 PM, Chris Packham wrote:
>>
>> On 13/05/21 10:35 am, Guenter Roeck wrote:
>>> On 5/12/21 3:09 PM, Guenter Roeck wrote:
>>>> On 5/12/21 2:54 PM, Chris Packham wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> On 11/03/21 8:33 pm, Guenter Roeck wrote:
>>>>>> Also use regmap for register caching. This change reduces code and
>>>>>> data size by more than 40%.
>>>>>>
>>>>>> While at it, fixed some warnings reported by checkpatch.
>>>>>>
>>>>>> Cc: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>
>>>>> I've just been informed by our QA team that it looks like the
>>>>> configuration of limits (e.g. by writing to sysfs) has been broken.
>>>>> Probably by this change. I'm just starting to dig into it now but I
>>>>> though I'd give you a heads up.
>>>>>
>>>>
>>>> Thanks for the heads up.
>>>>
>>>> It looks like voltage maximum writes use the wrong register,
>>>> ADM9240_REG_IN instead of ADM9240_REG_IN_MAX.
>>>> Odd, I'd have assumed that my module test code catches that.
>>>> I'll have to check why it doesn't.
>>>>
>>>
>>> Yes, turns out my module test script does not catch that situation.
>>> It tries to find the value range and determines that there is no range
>>> (because all writes are into the wrong register). I'll have to fix 
>>> that.
>>>
>>>> Anyway, anything more specific ?
>>>>
>>> I'll wait for your response before submitting a patch.
>>>
>> I agree that the writes to max aren't working. Haven't checked min.
>>
>> [root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
>> 3586
>> [root@awplus flash]# echo 1097 >/sys/class/hwmon/hwmon0/in5_max
>> [root@awplus flash]# cat /sys/class/hwmon/hwmon0/in5_max
>> 3586
>>
>
> Anything else ? I got a patch ready to fix that, but I would prefer to
> fix everything in one go if possible. My (fixed) module test script
> doesn't pick up other problems, but obviously we can't really trust it.
>
Everything else seems OK, I think it was just the max. That was 
certainly the only thing the QA team was complaining about.

Thanks for the quick work.

> Thanks,
> Guenter