[RFC PATCH 1/2] hwmon: adt7411: update to new hwmon registration API

[RFC PATCH 1/2] hwmon: adt7411: update to new hwmon registration API

[Michael Walle]

This is also a preparation for to support more properties like min, max and
alarm.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/hwmon/adt7411.c | 300 +++++++++++++++++++++++++++++-------------------
 1 file changed, 179 insertions(+), 121 deletions(-)

diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
index 812fbc0..2f44cdc 100644
--- a/drivers/hwmon/adt7411.c
+++ b/drivers/hwmon/adt7411.c
@@ -55,7 +55,7 @@ struct adt7411_data {
 	struct mutex device_lock;	/* for "atomic" device accesses */
 	struct mutex update_lock;
 	unsigned long next_update;
-	int vref_cached;
+	long vref_cached;
 	struct i2c_client *client;
 	bool use_ext_temp;
 };
@@ -114,85 +114,6 @@ static int adt7411_modify_bit(struct i2c_client *client, u8 reg, u8 bit,
 	return ret;
 }
 
-static ssize_t adt7411_show_vdd(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct adt7411_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int ret = adt7411_read_10_bit(client, ADT7411_REG_INT_TEMP_VDD_LSB,
-			ADT7411_REG_VDD_MSB, 2);
-
-	return ret < 0 ? ret : sprintf(buf, "%u\n", ret * 7000 / 1024);
-}
-
-static ssize_t adt7411_show_temp(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	int nr = to_sensor_dev_attr(attr)->index;
-	struct adt7411_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int val;
-	struct {
-		u8 low;
-		u8 high;
-	} reg[2] = {
-		{ ADT7411_REG_INT_TEMP_VDD_LSB, ADT7411_REG_INT_TEMP_MSB },
-		{ ADT7411_REG_EXT_TEMP_AIN14_LSB,
-		  ADT7411_REG_EXT_TEMP_AIN1_MSB },
-	};
-
-	val = adt7411_read_10_bit(client, reg[nr].low, reg[nr].high, 0);
-	if (val < 0)
-		return val;
-
-	val = val & 0x200 ? val - 0x400 : val; /* 10 bit signed */
-
-	return sprintf(buf, "%d\n", val * 250);
-}
-
-static ssize_t adt7411_show_input(struct device *dev,
-				  struct device_attribute *attr, char *buf)
-{
-	int nr = to_sensor_dev_attr(attr)->index;
-	struct adt7411_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int val;
-	u8 lsb_reg, lsb_shift;
-
-	mutex_lock(&data->update_lock);
-	if (time_after_eq(jiffies, data->next_update)) {
-		val = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
-		if (val < 0)
-			goto exit_unlock;
-
-		if (val & ADT7411_CFG3_REF_VDD) {
-			val = adt7411_read_10_bit(client,
-					ADT7411_REG_INT_TEMP_VDD_LSB,
-					ADT7411_REG_VDD_MSB, 2);
-			if (val < 0)
-				goto exit_unlock;
-
-			data->vref_cached = val * 7000 / 1024;
-		} else {
-			data->vref_cached = 2250;
-		}
-
-		data->next_update = jiffies + HZ;
-	}
-
-	lsb_reg = ADT7411_REG_EXT_TEMP_AIN14_LSB + (nr >> 2);
-	lsb_shift = 2 * (nr & 0x03);
-	val = adt7411_read_10_bit(client, lsb_reg,
-			ADT7411_REG_EXT_TEMP_AIN1_MSB + nr, lsb_shift);
-	if (val < 0)
-		goto exit_unlock;
-
-	val = sprintf(buf, "%u\n", val * data->vref_cached / 1024);
- exit_unlock:
-	mutex_unlock(&data->update_lock);
-	return val;
-}
-
 static ssize_t adt7411_show_bit(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -228,65 +149,156 @@ static ssize_t adt7411_set_bit(struct device *dev,
 	return ret < 0 ? ret : count;
 }
 
-
 #define ADT7411_BIT_ATTR(__name, __reg, __bit) \
 	SENSOR_DEVICE_ATTR_2(__name, S_IRUGO | S_IWUSR, adt7411_show_bit, \
 	adt7411_set_bit, __bit, __reg)
 
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adt7411_show_temp, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, adt7411_show_temp, NULL, 1);
-static DEVICE_ATTR(in0_input, S_IRUGO, adt7411_show_vdd, NULL);
-static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, adt7411_show_input, NULL, 0);
-static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, adt7411_show_input, NULL, 1);
-static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, adt7411_show_input, NULL, 2);
-static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, adt7411_show_input, NULL, 3);
-static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, adt7411_show_input, NULL, 4);
-static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, adt7411_show_input, NULL, 5);
-static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, adt7411_show_input, NULL, 6);
-static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, adt7411_show_input, NULL, 7);
 static ADT7411_BIT_ATTR(no_average, ADT7411_REG_CFG2, ADT7411_CFG2_DISABLE_AVG);
 static ADT7411_BIT_ATTR(fast_sampling, ADT7411_REG_CFG3, ADT7411_CFG3_ADC_CLK_225);
 static ADT7411_BIT_ATTR(adc_ref_vdd, ADT7411_REG_CFG3, ADT7411_CFG3_REF_VDD);
 
 static struct attribute *adt7411_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&dev_attr_in0_input.attr,
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
-	&sensor_dev_attr_in5_input.dev_attr.attr,
-	&sensor_dev_attr_in6_input.dev_attr.attr,
-	&sensor_dev_attr_in7_input.dev_attr.attr,
-	&sensor_dev_attr_in8_input.dev_attr.attr,
 	&sensor_dev_attr_no_average.dev_attr.attr,
 	&sensor_dev_attr_fast_sampling.dev_attr.attr,
 	&sensor_dev_attr_adc_ref_vdd.dev_attr.attr,
 	NULL
 };
+ATTRIBUTE_GROUPS(adt7411);
 
-static umode_t adt7411_attrs_visible(struct kobject *kobj,
-				     struct attribute *attr, int index)
+static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
 {
-	struct device *dev = container_of(kobj, struct device, kobj);
 	struct adt7411_data *data = dev_get_drvdata(dev);
-	bool visible = true;
+	struct i2c_client *client = data->client;
+	int ret;
 
-	if (attr == &sensor_dev_attr_temp2_input.dev_attr.attr)
-		visible = data->use_ext_temp;
-	else if (attr == &sensor_dev_attr_in1_input.dev_attr.attr ||
-		 attr == &sensor_dev_attr_in2_input.dev_attr.attr)
-		visible = !data->use_ext_temp;
+	switch (attr) {
+	case hwmon_in_input:
+		ret = adt7411_read_10_bit(client, ADT7411_REG_INT_TEMP_VDD_LSB,
+				ADT7411_REG_VDD_MSB, 2);
+		if (ret < 0)
+			return ret;
+		*val = ret * 7000 / 1024;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 
-	return visible ? attr->mode : 0;
+	int ret;
+	int lsb_reg, lsb_shift;
+	int nr = channel - 1;
+
+	mutex_lock(&data->update_lock);
+	if (time_after_eq(jiffies, data->next_update)) {
+		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
+		if (ret < 0)
+			goto exit_unlock;
+
+		if (ret & ADT7411_CFG3_REF_VDD) {
+			ret = adt7411_read_in_vdd(dev, hwmon_in_input,
+						  &data->vref_cached);
+			if (ret < 0)
+				goto exit_unlock;
+		} else {
+			data->vref_cached = 2250;
+		}
+
+		data->next_update = jiffies + HZ;
+	}
+
+	switch (attr) {
+	case hwmon_in_input:
+		lsb_reg = ADT7411_REG_EXT_TEMP_AIN14_LSB + (nr >> 2);
+		lsb_shift = 2 * (nr & 0x03);
+		ret = adt7411_read_10_bit(client, lsb_reg,
+				ADT7411_REG_EXT_TEMP_AIN1_MSB + nr, lsb_shift);
+		if (ret < 0)
+			goto exit_unlock;
+		*val = ret * data->vref_cached / 1024;
+		ret = 0;
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+ exit_unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
 }
 
-static const struct attribute_group adt7411_group = {
-	.attrs = adt7411_attrs,
-	.is_visible = adt7411_attrs_visible,
-};
-__ATTRIBUTE_GROUPS(adt7411);
+static int adt7411_read_in(struct device *dev, u32 attr, int channel,
+			   long *val)
+{
+	if (channel == 0)
+		return adt7411_read_in_vdd(dev, attr, val);
+	else
+		return adt7411_read_in_chan(dev, attr, channel, val);
+}
+
+static int adt7411_read_temp(struct device *dev, u32 attr, int channel,
+			     long *val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret, regl, regh;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		regl = channel ? ADT7411_REG_EXT_TEMP_AIN14_LSB :
+				 ADT7411_REG_INT_TEMP_VDD_LSB;
+		regh = channel ? ADT7411_REG_EXT_TEMP_AIN1_MSB :
+				 ADT7411_REG_INT_TEMP_MSB;
+		ret = adt7411_read_10_bit(client, regl, regh, 0);
+		if (ret < 0)
+			return ret;
+		ret = ret & 0x200 ? ret - 0x400 : ret; /* 10 bit signed */
+		*val = ret * 250;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int adt7411_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_in:
+		return adt7411_read_in(dev, attr, channel, val);
+	case hwmon_temp:
+		return adt7411_read_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t adt7411_is_visible(const void *_data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	const struct adt7411_data *data = _data;
+
+	switch (type) {
+	case hwmon_in:
+		if (channel > 0 && channel < 3)
+			return data->use_ext_temp ? 0 : S_IRUGO;
+		else
+			return S_IRUGO;
+	case hwmon_temp:
+		if (channel == 1)
+			return data->use_ext_temp ? S_IRUGO : 0;
+		else
+			return S_IRUGO;
+	default:
+		return 0;
+	}
+}
 
 static int adt7411_detect(struct i2c_client *client,
 			  struct i2c_board_info *info)
@@ -358,6 +370,51 @@ static int adt7411_init_device(struct adt7411_data *data)
 	return i2c_smbus_write_byte_data(data->client, ADT7411_REG_CFG1, val);
 }
 
+static const u32 adt7411_in_config[] = {
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info adt7411_in = {
+	.type = hwmon_in,
+	.config = adt7411_in_config,
+};
+
+static const u32 adt7411_temp_config[] = {
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info adt7411_temp = {
+	.type = hwmon_temp,
+	.config = adt7411_temp_config,
+};
+
+static const struct hwmon_channel_info *adt7411_info[] = {
+	&adt7411_in,
+	&adt7411_temp,
+	NULL
+};
+
+static const struct hwmon_ops adt7411_hwmon_ops = {
+	.is_visible = adt7411_is_visible,
+	.read = adt7411_read,
+};
+
+static const struct hwmon_chip_info adt7411_chip_info = {
+	.ops = &adt7411_hwmon_ops,
+	.info = adt7411_info,
+};
+
 static int adt7411_probe(struct i2c_client *client,
 				   const struct i2c_device_id *id)
 {
@@ -382,9 +439,10 @@ static int adt7411_probe(struct i2c_client *client,
 	/* force update on first occasion */
 	data->next_update = jiffies;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data,
-							   adt7411_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data,
+							 &adt7411_chip_info,
+							 adt7411_groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.1.4

[RFC PATCH 2/2] hwmon: adt7411: add min, max and alarm attributes

[Michael Walle]

This patch adds support for the min, max and alarm attributes of the
voltage and temperature channels. Additionally, the temp2_fault attribute
is supported which indicates a fault of the external temperature diode.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/hwmon/adt7411.c | 306 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 271 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
index 2f44cdc..c6351b8 100644
--- a/drivers/hwmon/adt7411.c
+++ b/drivers/hwmon/adt7411.c
@@ -21,6 +21,21 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/slab.h>
 
+#define ADT7411_REG_STAT_1			0x00
+#define ADT7411_STAT_1_INT_TEMP_HIGH		(1 << 0)
+#define ADT7411_STAT_1_INT_TEMP_LOW		(1 << 1)
+#define ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1	(1 << 2)
+#define ADT7411_STAT_1_EXT_TEMP_LOW		(1 << 3)
+#define ADT7411_STAT_1_EXT_TEMP_FAULT		(1 << 4)
+#define ADT7411_STAT_1_AIN2			(1 << 5)
+#define ADT7411_STAT_1_AIN3			(1 << 6)
+#define ADT7411_STAT_1_AIN4			(1 << 7)
+#define ADT7411_REG_STAT_2			0x01
+#define ADT7411_STAT_2_AIN5			(1 << 0)
+#define ADT7411_STAT_2_AIN6			(1 << 1)
+#define ADT7411_STAT_2_AIN7			(1 << 2)
+#define ADT7411_STAT_2_AIN8			(1 << 3)
+#define ADT7411_STAT_2_VDD			(1 << 4)
 #define ADT7411_REG_INT_TEMP_VDD_LSB		0x03
 #define ADT7411_REG_EXT_TEMP_AIN14_LSB		0x04
 #define ADT7411_REG_VDD_MSB			0x06
@@ -43,6 +58,17 @@
 #define ADT7411_CFG3_RESERVED_BIT3		(1 << 3)
 #define ADT7411_CFG3_REF_VDD			(1 << 4)
 
+#define ADT7411_REG_VDD_HIGH			0x23
+#define ADT7411_REG_VDD_LOW			0x24
+#define ADT7411_REG_TEMP_HIGH(nr)		(0x25 + 2 * (nr))
+#define ADT7411_REG_TEMP_LOW(nr)		(0x26 + 2 * (nr))
+#define ADT7411_REG_IN_HIGH(nr)		((nr) > 1 \
+						  ? 0x2b + 2 * ((nr)-2) \
+						  : 0x27)
+#define ADT7411_REG_IN_LOW(nr)			((nr) > 1 \
+						  ? 0x2c + 2 * ((nr)-2) \
+						  : 0x28)
+
 #define ADT7411_REG_DEVICE_ID			0x4d
 #define ADT7411_REG_MANUFACTURER_ID		0x4e
 
@@ -51,6 +77,30 @@
 
 static const unsigned short normal_i2c[] = { 0x48, 0x4a, 0x4b, I2C_CLIENT_END };
 
+static const u8 adt7411_in_alarm_reg[] = {
+	ADT7411_REG_STAT_2,
+	ADT7411_REG_STAT_1,
+	ADT7411_REG_STAT_1,
+	ADT7411_REG_STAT_1,
+	ADT7411_REG_STAT_1,
+	ADT7411_REG_STAT_2,
+	ADT7411_REG_STAT_2,
+	ADT7411_REG_STAT_2,
+	ADT7411_REG_STAT_2,
+};
+
+static const u8 adt7411_in_alarm_bits[] = {
+	ADT7411_STAT_2_VDD,
+	ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1,
+	ADT7411_STAT_1_AIN2,
+	ADT7411_STAT_1_AIN3,
+	ADT7411_STAT_1_AIN4,
+	ADT7411_STAT_2_AIN5,
+	ADT7411_STAT_2_AIN6,
+	ADT7411_STAT_2_AIN7,
+	ADT7411_STAT_2_AIN8,
+};
+
 struct adt7411_data {
 	struct mutex device_lock;	/* for "atomic" device accesses */
 	struct mutex update_lock;
@@ -165,6 +215,19 @@ static struct attribute *adt7411_attrs[] = {
 };
 ATTRIBUTE_GROUPS(adt7411);
 
+static int adt7411_read_in_alarm(struct device *dev, int channel, long *val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, adt7411_in_alarm_reg[channel]);
+	if (ret < 0)
+		return ret;
+	*val = !!(ret & adt7411_in_alarm_bits[channel]);
+	return 0;
+}
+
 static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
 {
 	struct adt7411_data *data = dev_get_drvdata(dev);
@@ -179,32 +242,40 @@ static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
 			return ret;
 		*val = ret * 7000 / 1024;
 		return 0;
+	case hwmon_in_min:
+		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_LOW);
+		if (ret < 0)
+			return ret;
+		*val = ret * 7000 / 256;
+		return 0;
+	case hwmon_in_max:
+		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_HIGH);
+		if (ret < 0)
+			return ret;
+		*val = ret * 7000 / 256;
+	case hwmon_in_alarm:
+		return adt7411_read_in_alarm(dev, 0, val);
 	default:
 		return -EOPNOTSUPP;
 	}
 }
 
-static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
-				long *val)
+static int adt7411_update_vref(struct device *dev)
 {
 	struct adt7411_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
+	int val;
 
-	int ret;
-	int lsb_reg, lsb_shift;
-	int nr = channel - 1;
-
-	mutex_lock(&data->update_lock);
 	if (time_after_eq(jiffies, data->next_update)) {
-		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
-		if (ret < 0)
-			goto exit_unlock;
+		val = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
+		if (val < 0)
+			return val;
 
-		if (ret & ADT7411_CFG3_REF_VDD) {
-			ret = adt7411_read_in_vdd(dev, hwmon_in_input,
+		if (val & ADT7411_CFG3_REF_VDD) {
+			val = adt7411_read_in_vdd(dev, hwmon_in_input,
 						  &data->vref_cached);
-			if (ret < 0)
-				goto exit_unlock;
+			if (val < 0)
+				return val;
 		} else {
 			data->vref_cached = 2250;
 		}
@@ -212,6 +283,24 @@ static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
 		data->next_update = jiffies + HZ;
 	}
 
+	return 0;
+}
+
+static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+
+	int ret;
+	int reg, lsb_reg, lsb_shift;
+	int nr = channel - 1;
+
+	mutex_lock(&data->update_lock);
+	ret = adt7411_update_vref(dev);
+	if (ret < 0)
+		goto exit_unlock;
+
 	switch (attr) {
 	case hwmon_in_input:
 		lsb_reg = ADT7411_REG_EXT_TEMP_AIN14_LSB + (nr >> 2);
@@ -223,6 +312,20 @@ static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
 		*val = ret * data->vref_cached / 1024;
 		ret = 0;
 		break;
+	case hwmon_in_min:
+	case hwmon_in_max:
+		reg = (attr == hwmon_in_min)
+			? ADT7411_REG_IN_LOW(channel)
+			: ADT7411_REG_IN_HIGH(channel);
+		ret = i2c_smbus_read_byte_data(client, reg);
+		if (ret < 0)
+			goto exit_unlock;
+		*val = ret * data->vref_cached / 256;
+		ret = 0;
+		break;
+	case hwmon_in_alarm:
+		ret = adt7411_read_in_alarm(dev, channel, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -241,12 +344,41 @@ static int adt7411_read_in(struct device *dev, u32 attr, int channel,
 		return adt7411_read_in_chan(dev, attr, channel, val);
 }
 
+
+static int adt7411_read_temp_alarm(struct device *dev, u32 attr, int channel,
+				   long *val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret, bit;
+
+	ret = i2c_smbus_read_byte_data(client, ADT7411_REG_STAT_1);
+	if (ret < 0)
+		return ret;
+
+	switch (attr) {
+	case hwmon_temp_min_alarm:
+		bit = channel ? ADT7411_STAT_1_EXT_TEMP_LOW
+			      : ADT7411_STAT_1_INT_TEMP_LOW;
+		break;
+	case hwmon_temp_max_alarm:
+		bit = channel ? ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1
+			      : ADT7411_STAT_1_INT_TEMP_HIGH;
+		break;
+	case hwmon_temp_fault:
+		bit = ADT7411_STAT_1_EXT_TEMP_FAULT;
+		break;
+	}
+	*val = !!(ret & bit);
+	return 0;
+}
+
 static int adt7411_read_temp(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
 	struct adt7411_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int ret, regl, regh;
+	int ret, reg, regl, regh;
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -260,6 +392,21 @@ static int adt7411_read_temp(struct device *dev, u32 attr, int channel,
 		ret = ret & 0x200 ? ret - 0x400 : ret; /* 10 bit signed */
 		*val = ret * 250;
 		return 0;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+		reg = (attr == hwmon_temp_min)
+			? ADT7411_REG_TEMP_LOW(channel)
+			: ADT7411_REG_TEMP_HIGH(channel);
+		ret = i2c_smbus_read_byte_data(client, reg);
+		if (ret < 0)
+			return ret;
+		ret = ret & 0x80 ? ret - 0x100 : ret; /* 8 bit signed */
+		*val = ret * 1000;
+		return 0;
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_fault:
+		return adt7411_read_temp_alarm(dev, attr, channel, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -278,26 +425,112 @@ static int adt7411_read(struct device *dev, enum hwmon_sensor_types type,
 	}
 }
 
+static int adt7411_write_in(struct device *dev, u32 attr, int channel,
+			    long val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret, reg;
+
+	mutex_lock(&data->update_lock);
+	ret = adt7411_update_vref(dev);
+	if (ret < 0)
+		goto exit_unlock;
+	val = DIV_ROUND_CLOSEST(val * 256, data->vref_cached);
+	val = clamp_val(val, 0, 255);
+
+	switch (attr) {
+	case hwmon_in_min:
+		reg = ADT7411_REG_IN_LOW(channel);
+		break;
+	case hwmon_in_max:
+		reg = ADT7411_REG_IN_HIGH(channel);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		goto exit_unlock;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, reg, val);
+ exit_unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static int adt7411_write_temp(struct device *dev, u32 attr, int channel,
+			      long val)
+{
+	struct adt7411_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int reg;
+
+	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = clamp_val(val, -128, 127);
+	val = val < 0 ? 0x100 + val : val;
+
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = ADT7411_REG_TEMP_LOW(channel);
+		break;
+	case hwmon_temp_max:
+		reg = ADT7411_REG_TEMP_HIGH(channel);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return i2c_smbus_write_byte_data(client, reg, val);
+}
+
+static int adt7411_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_in:
+		return adt7411_write_in(dev, attr, channel, val);
+	case hwmon_temp:
+		return adt7411_write_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t adt7411_is_visible(const void *_data,
 				  enum hwmon_sensor_types type,
 				  u32 attr, int channel)
 {
 	const struct adt7411_data *data = _data;
+	bool visible;
 
 	switch (type) {
 	case hwmon_in:
-		if (channel > 0 && channel < 3)
-			return data->use_ext_temp ? 0 : S_IRUGO;
-		else
-			return S_IRUGO;
+		visible = channel == 0 || channel >= 2 || !data->use_ext_temp;
+		switch (attr) {
+		case hwmon_in_input:
+		case hwmon_in_alarm:
+			return visible ? S_IRUGO : 0;
+		case hwmon_in_min:
+		case hwmon_in_max:
+			return visible ? S_IRUGO | S_IWUSR : 0;
+		}
+		break;
 	case hwmon_temp:
-		if (channel == 1)
-			return data->use_ext_temp ? S_IRUGO : 0;
-		else
-			return S_IRUGO;
+		visible = channel == 0 || data->use_ext_temp;
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+		case hwmon_temp_fault:
+			return visible ? S_IRUGO : 0;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+			return visible ? S_IRUGO | S_IWUSR : 0;
+		}
+		break;
 	default:
-		return 0;
+		break;
 	}
+	return 0;
 }
 
 static int adt7411_detect(struct i2c_client *client,
@@ -371,15 +604,15 @@ static int adt7411_init_device(struct adt7411_data *data)
 }
 
 static const u32 adt7411_in_config[] = {
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
-	HWMON_I_INPUT,
+	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_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,
 	0
 };
 
@@ -389,8 +622,10 @@ static const struct hwmon_channel_info adt7411_in = {
 };
 
 static const u32 adt7411_temp_config[] = {
-	HWMON_T_INPUT,
-	HWMON_T_INPUT,
+	HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MIN_ALARM |
+		HWMON_T_MAX | HWMON_T_MAX_ALARM,
+	HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MIN_ALARM |
+		HWMON_T_MAX | HWMON_T_MAX_ALARM | HWMON_T_FAULT,
 	0
 };
 
@@ -408,6 +643,7 @@ static const struct hwmon_channel_info *adt7411_info[] = {
 static const struct hwmon_ops adt7411_hwmon_ops = {
 	.is_visible = adt7411_is_visible,
 	.read = adt7411_read,
+	.write = adt7411_write,
 };
 
 static const struct hwmon_chip_info adt7411_chip_info = {
-- 
2.1.4

Re: [RFC PATCH 1/2] hwmon: adt7411: update to new hwmon registration API

[Michael Walle]

Am 2016-10-14 11:43, schrieb Michael Walle:
> This is also a preparation for to support more properties like min, max 
> and
> alarm.
> 

ping? :)

-michael

Re: [RFC PATCH 1/2] hwmon: adt7411: update to new hwmon registration API

[Guenter Roeck]

On 11/10/2016 05:04 AM, Michael Walle wrote:
> Am 2016-10-14 11:43, schrieb Michael Walle:
>> This is also a preparation for to support more properties like min, max and
>> alarm.
>>
>
> ping? :)
>
Sorry, I didn't have much time lately. I'll try to get to it shortly.

Guenter

Re: [RFC,1/2] hwmon: adt7411: update to new hwmon registration API

[Guenter Roeck]

On Fri, Oct 14, 2016 at 11:43:34AM +0200, Michael Walle wrote:
> This is also a preparation for to support more properties like min, max and
> alarm.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Passed my unit test. Applied to -next.

Thanks,
Guenter

Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

[Guenter Roeck]

Hi Michael,

On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
> This patch adds support for the min, max and alarm attributes of the
> voltage and temperature channels. Additionally, the temp2_fault attribute
> is supported which indicates a fault of the external temperature diode.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Sorry for the late reply. Mostly looks ok.
Couple of comments below.

Guenter

> ---
>  drivers/hwmon/adt7411.c | 306 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 271 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 2f44cdc..c6351b8 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c
> @@ -21,6 +21,21 @@
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/slab.h>
>  
> +#define ADT7411_REG_STAT_1			0x00
> +#define ADT7411_STAT_1_INT_TEMP_HIGH		(1 << 0)
> +#define ADT7411_STAT_1_INT_TEMP_LOW		(1 << 1)
> +#define ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1	(1 << 2)
> +#define ADT7411_STAT_1_EXT_TEMP_LOW		(1 << 3)
> +#define ADT7411_STAT_1_EXT_TEMP_FAULT		(1 << 4)
> +#define ADT7411_STAT_1_AIN2			(1 << 5)
> +#define ADT7411_STAT_1_AIN3			(1 << 6)
> +#define ADT7411_STAT_1_AIN4			(1 << 7)
> +#define ADT7411_REG_STAT_2			0x01
> +#define ADT7411_STAT_2_AIN5			(1 << 0)
> +#define ADT7411_STAT_2_AIN6			(1 << 1)
> +#define ADT7411_STAT_2_AIN7			(1 << 2)
> +#define ADT7411_STAT_2_AIN8			(1 << 3)
> +#define ADT7411_STAT_2_VDD			(1 << 4)

Please use BIT().

>  #define ADT7411_REG_INT_TEMP_VDD_LSB		0x03
>  #define ADT7411_REG_EXT_TEMP_AIN14_LSB		0x04
>  #define ADT7411_REG_VDD_MSB			0x06
> @@ -43,6 +58,17 @@
>  #define ADT7411_CFG3_RESERVED_BIT3		(1 << 3)
>  #define ADT7411_CFG3_REF_VDD			(1 << 4)
>  
> +#define ADT7411_REG_VDD_HIGH			0x23
> +#define ADT7411_REG_VDD_LOW			0x24
> +#define ADT7411_REG_TEMP_HIGH(nr)		(0x25 + 2 * (nr))
> +#define ADT7411_REG_TEMP_LOW(nr)		(0x26 + 2 * (nr))
> +#define ADT7411_REG_IN_HIGH(nr)		((nr) > 1 \
> +						  ? 0x2b + 2 * ((nr)-2) \
> +						  : 0x27)
> +#define ADT7411_REG_IN_LOW(nr)			((nr) > 1 \
> +						  ? 0x2c + 2 * ((nr)-2) \
> +						  : 0x28)
> +
>  #define ADT7411_REG_DEVICE_ID			0x4d
>  #define ADT7411_REG_MANUFACTURER_ID		0x4e
>  
> @@ -51,6 +77,30 @@
>  
>  static const unsigned short normal_i2c[] = { 0x48, 0x4a, 0x4b, I2C_CLIENT_END };
>  
> +static const u8 adt7411_in_alarm_reg[] = {
> +	ADT7411_REG_STAT_2,
> +	ADT7411_REG_STAT_1,
> +	ADT7411_REG_STAT_1,
> +	ADT7411_REG_STAT_1,
> +	ADT7411_REG_STAT_1,
> +	ADT7411_REG_STAT_2,
> +	ADT7411_REG_STAT_2,
> +	ADT7411_REG_STAT_2,
> +	ADT7411_REG_STAT_2,
> +};
> +
> +static const u8 adt7411_in_alarm_bits[] = {
> +	ADT7411_STAT_2_VDD,
> +	ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1,
> +	ADT7411_STAT_1_AIN2,
> +	ADT7411_STAT_1_AIN3,
> +	ADT7411_STAT_1_AIN4,
> +	ADT7411_STAT_2_AIN5,
> +	ADT7411_STAT_2_AIN6,
> +	ADT7411_STAT_2_AIN7,
> +	ADT7411_STAT_2_AIN8,
> +};
> +
>  struct adt7411_data {
>  	struct mutex device_lock;	/* for "atomic" device accesses */
>  	struct mutex update_lock;
> @@ -165,6 +215,19 @@ static struct attribute *adt7411_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(adt7411);
>  
> +static int adt7411_read_in_alarm(struct device *dev, int channel, long *val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, adt7411_in_alarm_reg[channel]);
> +	if (ret < 0)
> +		return ret;
> +	*val = !!(ret & adt7411_in_alarm_bits[channel]);
> +	return 0;
> +}
> +
>  static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
>  {
>  	struct adt7411_data *data = dev_get_drvdata(dev);
> @@ -179,32 +242,40 @@ static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
>  			return ret;
>  		*val = ret * 7000 / 1024;
>  		return 0;
> +	case hwmon_in_min:
> +		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_LOW);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret * 7000 / 256;
> +		return 0;
> +	case hwmon_in_max:
> +		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_HIGH);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret * 7000 / 256;
> +	case hwmon_in_alarm:
> +		return adt7411_read_in_alarm(dev, 0, val);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  }
>  
> -static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
> -				long *val)
> +static int adt7411_update_vref(struct device *dev)
>  {
>  	struct adt7411_data *data = dev_get_drvdata(dev);
>  	struct i2c_client *client = data->client;
> +	int val;
>  
> -	int ret;
> -	int lsb_reg, lsb_shift;
> -	int nr = channel - 1;
> -
> -	mutex_lock(&data->update_lock);
>  	if (time_after_eq(jiffies, data->next_update)) {
> -		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
> -		if (ret < 0)
> -			goto exit_unlock;
> +		val = i2c_smbus_read_byte_data(client, ADT7411_REG_CFG3);
> +		if (val < 0)
> +			return val;
>  
> -		if (ret & ADT7411_CFG3_REF_VDD) {
> -			ret = adt7411_read_in_vdd(dev, hwmon_in_input,
> +		if (val & ADT7411_CFG3_REF_VDD) {
> +			val = adt7411_read_in_vdd(dev, hwmon_in_input,
>  						  &data->vref_cached);
> -			if (ret < 0)
> -				goto exit_unlock;
> +			if (val < 0)
> +				return val;
>  		} else {
>  			data->vref_cached = 2250;
>  		}
> @@ -212,6 +283,24 @@ static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
>  		data->next_update = jiffies + HZ;
>  	}
>  
> +	return 0;
> +}
> +
> +static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
> +				long *val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +
> +	int ret;
> +	int reg, lsb_reg, lsb_shift;
> +	int nr = channel - 1;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = adt7411_update_vref(dev);
> +	if (ret < 0)
> +		goto exit_unlock;
> +
>  	switch (attr) {
>  	case hwmon_in_input:
>  		lsb_reg = ADT7411_REG_EXT_TEMP_AIN14_LSB + (nr >> 2);
> @@ -223,6 +312,20 @@ static int adt7411_read_in_chan(struct device *dev, u32 attr, int channel,
>  		*val = ret * data->vref_cached / 1024;
>  		ret = 0;
>  		break;
> +	case hwmon_in_min:
> +	case hwmon_in_max:
> +		reg = (attr == hwmon_in_min)
> +			? ADT7411_REG_IN_LOW(channel)
> +			: ADT7411_REG_IN_HIGH(channel);
> +		ret = i2c_smbus_read_byte_data(client, reg);
> +		if (ret < 0)
> +			goto exit_unlock;
> +		*val = ret * data->vref_cached / 256;
> +		ret = 0;
> +		break;
> +	case hwmon_in_alarm:
> +		ret = adt7411_read_in_alarm(dev, channel, val);
> +		break;
>  	default:
>  		ret = -EOPNOTSUPP;
>  		break;
> @@ -241,12 +344,41 @@ static int adt7411_read_in(struct device *dev, u32 attr, int channel,
>  		return adt7411_read_in_chan(dev, attr, channel, val);
>  }
>  
> +
> +static int adt7411_read_temp_alarm(struct device *dev, u32 attr, int channel,
> +				   long *val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret, bit;
> +
> +	ret = i2c_smbus_read_byte_data(client, ADT7411_REG_STAT_1);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (attr) {
> +	case hwmon_temp_min_alarm:
> +		bit = channel ? ADT7411_STAT_1_EXT_TEMP_LOW
> +			      : ADT7411_STAT_1_INT_TEMP_LOW;
> +		break;
> +	case hwmon_temp_max_alarm:
> +		bit = channel ? ADT7411_STAT_1_EXT_TEMP_HIGH_AIN1
> +			      : ADT7411_STAT_1_INT_TEMP_HIGH;
> +		break;
> +	case hwmon_temp_fault:
> +		bit = ADT7411_STAT_1_EXT_TEMP_FAULT;
> +		break;
> +	}
> +	*val = !!(ret & bit);

gcc complains that bit may be uninitialized. Please add the missing
default: above. Sure, that won't happen from the code flow, but the
warning can easily be avoided.

> +	return 0;
> +}
> +
>  static int adt7411_read_temp(struct device *dev, u32 attr, int channel,
>  			     long *val)
>  {
>  	struct adt7411_data *data = dev_get_drvdata(dev);
>  	struct i2c_client *client = data->client;
> -	int ret, regl, regh;
> +	int ret, reg, regl, regh;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> @@ -260,6 +392,21 @@ static int adt7411_read_temp(struct device *dev, u32 attr, int channel,
>  		ret = ret & 0x200 ? ret - 0x400 : ret; /* 10 bit signed */
>  		*val = ret * 250;
>  		return 0;
> +	case hwmon_temp_min:
> +	case hwmon_temp_max:
> +		reg = (attr == hwmon_temp_min)
> +			? ADT7411_REG_TEMP_LOW(channel)
> +			: ADT7411_REG_TEMP_HIGH(channel);
> +		ret = i2c_smbus_read_byte_data(client, reg);
> +		if (ret < 0)
> +			return ret;
> +		ret = ret & 0x80 ? ret - 0x100 : ret; /* 8 bit signed */
> +		*val = ret * 1000;
> +		return 0;
> +	case hwmon_temp_min_alarm:
> +	case hwmon_temp_max_alarm:
> +	case hwmon_temp_fault:
> +		return adt7411_read_temp_alarm(dev, attr, channel, val);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -278,26 +425,112 @@ static int adt7411_read(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  }
>  
> +static int adt7411_write_in(struct device *dev, u32 attr, int channel,
> +			    long val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret, reg;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = adt7411_update_vref(dev);
> +	if (ret < 0)
> +		goto exit_unlock;
> +	val = DIV_ROUND_CLOSEST(val * 256, data->vref_cached);
> +	val = clamp_val(val, 0, 255);
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		reg = ADT7411_REG_IN_LOW(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADT7411_REG_IN_HIGH(channel);
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		goto exit_unlock;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(client, reg, val);
> + exit_unlock:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static int adt7411_write_temp(struct device *dev, u32 attr, int channel,
> +			      long val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int reg;
> +
> +	val = DIV_ROUND_CLOSEST(val, 1000);
> +	val = clamp_val(val, -128, 127);
> +	val = val < 0 ? 0x100 + val : val;

Does this add any value ? It doesn't change the low byte.

> +
> +	switch (attr) {
> +	case hwmon_temp_min:
> +		reg = ADT7411_REG_TEMP_LOW(channel);
> +		break;
> +	case hwmon_temp_max:
> +		reg = ADT7411_REG_TEMP_HIGH(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return i2c_smbus_write_byte_data(client, reg, val);
> +}
> +
> +static int adt7411_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return adt7411_write_in(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return adt7411_write_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static umode_t adt7411_is_visible(const void *_data,
>  				  enum hwmon_sensor_types type,
>  				  u32 attr, int channel)
>  {
>  	const struct adt7411_data *data = _data;
> +	bool visible;
>  
>  	switch (type) {
>  	case hwmon_in:
> -		if (channel > 0 && channel < 3)
> -			return data->use_ext_temp ? 0 : S_IRUGO;
> -		else
> -			return S_IRUGO;
> +		visible = channel == 0 || channel >= 2 || !data->use_ext_temp;

in2 is now visible even if external temperature is measured.
This is not correct. Yes, one can read the register, but the 
external pin (AIN2) is connected to the temperature sensor.

> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_alarm:
> +			return visible ? S_IRUGO : 0;
> +		case hwmon_in_min:
> +		case hwmon_in_max:
> +			return visible ? S_IRUGO | S_IWUSR : 0;
> +		}
> +		break;
>  	case hwmon_temp:
> -		if (channel == 1)
> -			return data->use_ext_temp ? S_IRUGO : 0;
> -		else
> -			return S_IRUGO;
> +		visible = channel == 0 || data->use_ext_temp;
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_min_alarm:
> +		case hwmon_temp_max_alarm:
> +		case hwmon_temp_fault:
> +			return visible ? S_IRUGO : 0;
> +		case hwmon_temp_min:
> +		case hwmon_temp_max:
> +			return visible ? S_IRUGO | S_IWUSR : 0;
> +		}
> +		break;
>  	default:
> -		return 0;
> +		break;
>  	}
> +	return 0;
>  }
>  
>  static int adt7411_detect(struct i2c_client *client,
> @@ -371,15 +604,15 @@ static int adt7411_init_device(struct adt7411_data *data)
>  }
>  
>  static const u32 adt7411_in_config[] = {
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> -	HWMON_I_INPUT,
> +	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_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,
>  	0
>  };
>  
> @@ -389,8 +622,10 @@ static const struct hwmon_channel_info adt7411_in = {
>  };
>  
>  static const u32 adt7411_temp_config[] = {
> -	HWMON_T_INPUT,
> -	HWMON_T_INPUT,
> +	HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MIN_ALARM |
> +		HWMON_T_MAX | HWMON_T_MAX_ALARM,
> +	HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MIN_ALARM |
> +		HWMON_T_MAX | HWMON_T_MAX_ALARM | HWMON_T_FAULT,
>  	0
>  };
>  
> @@ -408,6 +643,7 @@ static const struct hwmon_channel_info *adt7411_info[] = {
>  static const struct hwmon_ops adt7411_hwmon_ops = {
>  	.is_visible = adt7411_is_visible,
>  	.read = adt7411_read,
> +	.write = adt7411_write,
>  };
>  
>  static const struct hwmon_chip_info adt7411_chip_info = {

Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

[Guenter Roeck]

On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
> This patch adds support for the min, max and alarm attributes of the
> voltage and temperature channels. Additionally, the temp2_fault attribute
> is supported which indicates a fault of the external temperature diode.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/hwmon/adt7411.c | 306 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 271 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 2f44cdc..c6351b8 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c
[ ... ]
>  static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
>  {
>  	struct adt7411_data *data = dev_get_drvdata(dev);
> @@ -179,32 +242,40 @@ static int adt7411_read_in_vdd(struct device *dev, u32 attr, long *val)
>  			return ret;
>  		*val = ret * 7000 / 1024;
>  		return 0;
> +	case hwmon_in_min:
> +		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_LOW);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret * 7000 / 256;
> +		return 0;
> +	case hwmon_in_max:
> +		ret = i2c_smbus_read_byte_data(client, ADT7411_REG_VDD_HIGH);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret * 7000 / 256;

		return 0;

> +	case hwmon_in_alarm:
> +		return adt7411_read_in_alarm(dev, 0, val);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  }

Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

[Guenter Roeck]

On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
> This patch adds support for the min, max and alarm attributes of the
> voltage and temperature channels. Additionally, the temp2_fault attribute
> is supported which indicates a fault of the external temperature diode.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/hwmon/adt7411.c | 306 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 271 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 2f44cdc..c6351b8 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c

[ ... ]

>  
> +static int adt7411_write_in(struct device *dev, u32 attr, int channel,
> +			    long val)
> +{
> +	struct adt7411_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret, reg;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = adt7411_update_vref(dev);
> +	if (ret < 0)
> +		goto exit_unlock;
> +	val = DIV_ROUND_CLOSEST(val * 256, data->vref_cached);
> +	val = clamp_val(val, 0, 255);
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		reg = ADT7411_REG_IN_LOW(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = ADT7411_REG_IN_HIGH(channel);
> +		break;

This is also used to set the vdd limits, but it does not write into the vdd
limit registers. ADT7411_REG_IN_HIGH(0) == ADT7411_REG_IN_HIGH(1) == 0x27,
but it should be 0x23. Same for the low limit register.

Guenter

Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

[Michael Walle]

Am 2016-11-19 19:05, schrieb Guenter Roeck:
> Hi Michael,
> 
> On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
>> This patch adds support for the min, max and alarm attributes of the
>> voltage and temperature channels. Additionally, the temp2_fault 
>> attribute
>> is supported which indicates a fault of the external temperature 
>> diode.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> Sorry for the late reply. Mostly looks ok.
> Couple of comments below.

thanks for the review. I will send an updated version, soon.


[snip]

>> +static int adt7411_write_temp(struct device *dev, u32 attr, int 
>> channel,
>> +			      long val)
>> +{
>> +	struct adt7411_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	int reg;
>> +
>> +	val = DIV_ROUND_CLOSEST(val, 1000);
>> +	val = clamp_val(val, -128, 127);
>> +	val = val < 0 ? 0x100 + val : val;
> 
> Does this add any value ? It doesn't change the low byte.

mh? if val is negative 256 will be added.


>>  static umode_t adt7411_is_visible(const void *_data,
>>  				  enum hwmon_sensor_types type,
>>  				  u32 attr, int channel)
>>  {
>>  	const struct adt7411_data *data = _data;
>> +	bool visible;
>> 
>>  	switch (type) {
>>  	case hwmon_in:
>> -		if (channel > 0 && channel < 3)
>> -			return data->use_ext_temp ? 0 : S_IRUGO;
>> -		else
>> -			return S_IRUGO;
>> +		visible = channel == 0 || channel >= 2 || !data->use_ext_temp;
> 
> in2 is now visible even if external temperature is measured.
> This is not correct. Yes, one can read the register, but the
> external pin (AIN2) is connected to the temperature sensor.

i guess

   visible = channel == 0 || channel >= 3 || !data->use_ext_temp;

makes more sense.

-michael

Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

[Guenter Roeck]

On Mon, Nov 21, 2016 at 05:53:01PM +0100, Michael Walle wrote:
> Am 2016-11-19 19:05, schrieb Guenter Roeck:
> >Hi Michael,
> >
> >On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
> >>This patch adds support for the min, max and alarm attributes of the
> >>voltage and temperature channels. Additionally, the temp2_fault
> >>attribute
> >>is supported which indicates a fault of the external temperature diode.
> >>
> >>Signed-off-by: Michael Walle <michael@walle.cc>
> >
> >Sorry for the late reply. Mostly looks ok.
> >Couple of comments below.
> 
> thanks for the review. I will send an updated version, soon.
> 
> 
> [snip]
> 
> >>+static int adt7411_write_temp(struct device *dev, u32 attr, int
> >>channel,
> >>+			      long val)
> >>+{
> >>+	struct adt7411_data *data = dev_get_drvdata(dev);
> >>+	struct i2c_client *client = data->client;
> >>+	int reg;
> >>+
> >>+	val = DIV_ROUND_CLOSEST(val, 1000);
> >>+	val = clamp_val(val, -128, 127);
> >>+	val = val < 0 ? 0x100 + val : val;
> >
> >Does this add any value ? It doesn't change the low byte.
> 
> mh? if val is negative 256 will be added.
> 

What changes for the low byte if you add 0x100 to val ?

0xXXXXXX00 + 0x100 -> 0xYYYYYY00
0xXXXXXX01 + 0x100 -> 0xYYYYYY01
..
0xXXXXXXff + 0x100 -> 0xYYYYYYff

or

0x000000xx + 0x100 = 0x000001xx
0x000001xx + 0x100 = 0x000002xx
..
0xfffffexx + 0x100 = 0xffffffxx
0xffffffxx + 0x100 = 0x000000xx

> 
> >> static umode_t adt7411_is_visible(const void *_data,
> >> 				  enum hwmon_sensor_types type,
> >> 				  u32 attr, int channel)
> >> {
> >> 	const struct adt7411_data *data = _data;
> >>+	bool visible;
> >>
> >> 	switch (type) {
> >> 	case hwmon_in:
> >>-		if (channel > 0 && channel < 3)
> >>-			return data->use_ext_temp ? 0 : S_IRUGO;
> >>-		else
> >>-			return S_IRUGO;
> >>+		visible = channel == 0 || channel >= 2 || !data->use_ext_temp;
> >
> >in2 is now visible even if external temperature is measured.
> >This is not correct. Yes, one can read the register, but the
> >external pin (AIN2) is connected to the temperature sensor.
> 
> i guess
> 
>   visible = channel == 0 || channel >= 3 || !data->use_ext_temp;
> 
Correct.

Guenter