[PATCH 0/3] Lm90 Enhancements

[PATCH 0/3] Lm90 Enhancements

[Wei Ni]

This patch set enhance the lm90 driver,
it make the driver more readable and easier to use thermal framework.

This series is v1, based on [RFC 2/9], [RFC 3/9] and [RFC 4/9] submitted here:
http://thread.gmane.org/gmane.linux.power-management.general/31056

This series based on Rui's -thermal tree.

Changes from RFC:
1. change _show_temp() to read_temp(), _set_temp() to write_temp().
2. simply return value for the read_temp(), not use pointer.
3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT.

Wei Ni (3):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: (lm90) add support to handle irq
  hwmon: (lm90) use enums for the indexes of temp8 and temp11

 drivers/hwmon/lm90.c |  322 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 214 insertions(+), 108 deletions(-)

-- 
1.7.9.5

[lm-sensors] [PATCH 0/3] Lm90 Enhancements

[Wei Ni]

This patch set enhance the lm90 driver,
it make the driver more readable and easier to use thermal framework.

This series is v1, based on [RFC 2/9], [RFC 3/9] and [RFC 4/9] submitted here:
http://thread.gmane.org/gmane.linux.power-management.general/31056

This series based on Rui's -thermal tree.

Changes from RFC:
1. change _show_temp() to read_temp(), _set_temp() to write_temp().
2. simply return value for the read_temp(), not use pointer.
3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT.

Wei Ni (3):
  hwmon: (lm90) split set&show temp as common codes
  hwmon: (lm90) add support to handle irq
  hwmon: (lm90) use enums for the indexes of temp8 and temp11

 drivers/hwmon/lm90.c |  322 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 214 insertions(+), 108 deletions(-)

-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 1/3] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

Split set&show temp codes as common functions, so we can use it directly when
implement linux thermal framework.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/hwmon/lm90.c |  118 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 75 insertions(+), 43 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..2cb7f8e 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,37 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
  * Sysfs stuff
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int read_temp8(struct device *dev, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind == adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+		temp = temp_from_u8_adt7461(data, data->temp8[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[attr->index]);
+		temp = temp_from_u8(data->temp8[index]);
 	else
-		temp = temp_from_s8(data->temp8[attr->index]);
+		temp = temp_from_s8(data->temp8[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int temp;
+
+	temp = read_temp8(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static void write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -737,60 +745,76 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index == 3)
+	if (data->kind == lm99 && index == 3)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		data->temp8[nr] = temp_to_u8_adt7461(data, val);
+		data->temp8[index] = temp_to_u8_adt7461(data, val);
 	else if (data->kind == max6646)
-		data->temp8[nr] = temp_to_u8(val);
+		data->temp8[index] = temp_to_u8(val);
 	else
-		data->temp8[nr] = temp_to_s8(val);
+		data->temp8[index] = temp_to_s8(val);
 
-	lm90_select_remote_channel(client, data, nr >= 6);
-	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+	lm90_select_remote_channel(client, data, index >= 6);
+	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp8(dev, index, val);
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int read_temp11(struct device *dev, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind == adt7461)
-		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+		temp = temp_from_u16_adt7461(data, data->temp11[index]);
 	else if (data->kind == max6646)
-		temp = temp_from_u16(data->temp11[attr->index]);
+		temp = temp_from_u16(data->temp11[index]);
 	else
-		temp = temp_from_s16(data->temp11[attr->index]);
+		temp = temp_from_s16(data->temp11[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 &&  attr->index <= 2)
+	if (data->kind == lm99 &&  index <= 2)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int temp;
+
+	temp = read_temp11(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static void write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -804,17 +828,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
@@ -839,6 +854,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int nr = attr->nr;
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp11(dev, nr, index, val);
+
 	return count;
 }
 
-- 
1.7.9.5

[lm-sensors] [PATCH 1/3] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

Split set&show temp codes as common functions, so we can use it directly when
implement linux thermal framework.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |  118 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 75 insertions(+), 43 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..2cb7f8e 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,37 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
  * Sysfs stuff
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int read_temp8(struct device *dev, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind = adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+		temp = temp_from_u8_adt7461(data, data->temp8[index]);
 	else if (data->kind = max6646)
-		temp = temp_from_u8(data->temp8[attr->index]);
+		temp = temp_from_u8(data->temp8[index]);
 	else
-		temp = temp_from_s8(data->temp8[attr->index]);
+		temp = temp_from_s8(data->temp8[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind = lm99 && attr->index = 3)
+	if (data->kind = lm99 && index = 3)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+			  char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int temp;
+
+	temp = read_temp8(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static void write_temp8(struct device *dev, int index, long val)
 {
 	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -737,60 +745,76 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind = lm99 && attr->index = 3)
+	if (data->kind = lm99 && index = 3)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind = adt7461)
-		data->temp8[nr] = temp_to_u8_adt7461(data, val);
+		data->temp8[index] = temp_to_u8_adt7461(data, val);
 	else if (data->kind = max6646)
-		data->temp8[nr] = temp_to_u8(val);
+		data->temp8[index] = temp_to_u8(val);
 	else
-		data->temp8[nr] = temp_to_s8(val);
+		data->temp8[index] = temp_to_s8(val);
 
-	lm90_select_remote_channel(client, data, nr >= 6);
-	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+	lm90_select_remote_channel(client, data, index >= 6);
+	i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp8(dev, index, val);
+
 	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int read_temp11(struct device *dev, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
+	struct lm90_data *data = lm90_update_device(dev);
 
 	if (data->kind = adt7461)
-		temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+		temp = temp_from_u16_adt7461(data, data->temp11[index]);
 	else if (data->kind = max6646)
-		temp = temp_from_u16(data->temp11[attr->index]);
+		temp = temp_from_u16(data->temp11[index]);
 	else
-		temp = temp_from_s16(data->temp11[attr->index]);
+		temp = temp_from_s16(data->temp11[index]);
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind = lm99 &&  attr->index <= 2)
+	if (data->kind = lm99 &&  index <= 2)
 		temp += 16000;
 
+	return temp;
+}
+
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int temp;
+
+	temp = read_temp11(dev, attr->index);
+
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static void write_temp11(struct device *dev, int nr, int index, long val)
 {
 	struct {
 		u8 high;
@@ -804,17 +828,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind = lm99 && index <= 2)
@@ -839,6 +854,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	lm90_select_remote_channel(client, data, 0);
 
 	mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int nr = attr->nr;
+	int index = attr->index;
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	write_temp11(dev, nr, index, val);
+
 	return count;
 }
 
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

Add support to handle irq. When the temperature touch
the limit value, the driver can handle the interrupt.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2cb7f8e..fce9dfa 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/interrupt.h>
 
 /*
  * Addresses to scan
@@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static void lm90_alert(struct i2c_client *client, unsigned int flag);
+
+static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
+{
+	struct lm90_data *data = dev_id;
+	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+
+	lm90_alert(client, 0);
+
+	return IRQ_HANDLED;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
 		goto exit_remove_files;
 	}
 
+	if (client->irq >= 0) {
+		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
+		err = devm_request_threaded_irq(dev, client->irq,
+						NULL, lm90_irq_thread,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"lm90", data);
+		if (err < 0) {
+			dev_err(dev, "cannot request interrupt\n");
+			goto exit_remove_files;
+		}
+	}
+
 	return 0;
 
 exit_remove_files:
-- 
1.7.9.5

[lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

Add support to handle irq. When the temperature touch
the limit value, the driver can handle the interrupt.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2cb7f8e..fce9dfa 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/interrupt.h>
 
 /*
  * Addresses to scan
@@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
 }
 
+static void lm90_alert(struct i2c_client *client, unsigned int flag);
+
+static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
+{
+	struct lm90_data *data = dev_id;
+	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+
+	lm90_alert(client, 0);
+
+	return IRQ_HANDLED;
+}
+
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id)
 {
@@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
 		goto exit_remove_files;
 	}
 
+	if (client->irq >= 0) {
+		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
+		err = devm_request_threaded_irq(dev, client->irq,
+						NULL, lm90_irq_thread,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"lm90", data);
+		if (err < 0) {
+			dev_err(dev, "cannot request interrupt\n");
+			goto exit_remove_files;
+		}
+	}
+
 	return 0;
 
 exit_remove_files:
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 3/3] hwmon: (lm90) use enums for the indexes of temp8 and temp11

[Wei Ni]

Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.

Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/hwmon/lm90.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 114 insertions(+), 65 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index fce9dfa..97fe83a 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -297,6 +297,59 @@ static const struct lm90_params lm90_params[] = {
 };
 
 /*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+	TEMP8_LOCAL_LOW = 0,	/* 0: local low limit */
+	TEMP8_LOCAL_HIGH,	/* 1: local high limit */
+	TEMP8_LOCAL_CRIT,	/* 2: local critical limit */
+	TEMP8_REMOTE_CRIT,	/* 3: remote critical limit */
+	TEMP8_LOCAL_EMERG,	/* 4: local emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE_EMERG,	/* 5: remote emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE2_CRIT,	/* 6: remote 2 critical limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REMOTE2_EMERG,	/* 7: remote 2 emergency limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+	TEMP11_REMOTE_TEMP = 0,	/* 0: remote input */
+	TEMP11_REMOTE_LOW,	/* 1: remote low limit */
+	TEMP11_REMOTE_HIGH,	/* 2: remote high limit */
+	TEMP11_REMOTE_OFFSET,	/* 3: remote offset
+				 * (except max6646, max6657/58/59,
+				 *  and max6695/96)
+				 */
+	TEMP11_LOCAL_TEMP,	/* 4: local input */
+	TEMP11_REMOTE2_TEMP,	/* 5: remote 2 input (max6695/96 only) */
+	TEMP11_REMOTE2_LOW,	/* 6: remote 2 low limit (max6695/96 only) */
+	TEMP11_REMOTE2_HIGH,	/* 7: remote 2 high limit (max6695/96 only) */
+	TEMP11_REG_NUM
+};
+
+/*
+ * TEMP11 register NR
+ */
+enum lm90_temp11_reg_nr {
+	NR_CHAN_0_REMOTE_LOW = 0,	/* 0: channel 0, remote low limit */
+	NR_CHAN_0_REMOTE_HIGH,		/* 1: channel 0, remote high limit */
+	NR_CHAN_0_REMOTE_OFFSET,	/* 2: channel 0, remote offset */
+	NR_CHAN_1_REMOTE_LOW,		/* 3: channel 1, remote low limit */
+	NR_CHAN_1_REMOTE_HIGH,		/* 4: channel 1, remote high limit */
+	NR_NUM				/* number of the NRs for temp11 */
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -318,25 +371,8 @@ struct lm90_data {
 	u8 reg_local_ext;	/* local extension register offset */
 
 	/* registers values */
-	s8 temp8[8];	/* 0: local low limit
-			 * 1: local high limit
-			 * 2: local critical limit
-			 * 3: remote critical limit
-			 * 4: local emergency limit (max6659 and max6695/96)
-			 * 5: remote emergency limit (max6659 and max6695/96)
-			 * 6: remote 2 critical limit (max6695/96 only)
-			 * 7: remote 2 emergency limit (max6695/96 only)
-			 */
-	s16 temp11[8];	/* 0: remote input
-			 * 1: remote low limit
-			 * 2: remote high limit
-			 * 3: remote offset (except max6646, max6657/58/59,
-			 *		     and max6695/96)
-			 * 4: local input
-			 * 5: remote 2 input (max6695/96 only)
-			 * 6: remote 2 low limit (max6695/96 only)
-			 * 7: remote 2 high limit (max6695/96 only)
-			 */
+	s8 temp8[TEMP8_REG_NUM];
+	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
 };
@@ -478,37 +514,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		u8 alarms;
 
 		dev_dbg(&client->dev, "Updating lm90 data.\n");
-		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
-		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+				&data->temp8[TEMP8_LOCAL_LOW]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+				&data->temp8[TEMP8_LOCAL_HIGH]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+				&data->temp8[TEMP8_LOCAL_CRIT]);
+		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+				&data->temp8[TEMP8_REMOTE_CRIT]);
 		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
 
 		if (data->reg_local_ext) {
 			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
 				    data->reg_local_ext,
-				    &data->temp11[4]);
+				    &data->temp11[TEMP11_LOCAL_TEMP]);
 		} else {
 			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
 					  &h) == 0)
-				data->temp11[4] = h << 8;
+				data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
 		}
 		lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-			    LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
+			LM90_REG_R_REMOTE_TEMPL,
+			&data->temp11[TEMP11_REMOTE_TEMP]);
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
-			data->temp11[1] = h << 8;
+			data->temp11[TEMP11_REMOTE_LOW] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
 					  &l) == 0)
-				data->temp11[1] |= l;
+				data->temp11[TEMP11_REMOTE_LOW] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
-			data->temp11[2] = h << 8;
+			data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
 					  &l) == 0)
-				data->temp11[2] |= l;
+				data->temp11[TEMP11_REMOTE_HIGH] |= l;
 		}
 
 		if (data->flags & LM90_HAVE_OFFSET) {
@@ -516,13 +557,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 					  &h) == 0
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
 					  &l) == 0)
-				data->temp11[3] = (h << 8) | l;
+				data->temp11[TEMP11_REMOTE_OFFSET] =
+								(h << 8) | l;
 		}
 		if (data->flags & LM90_HAVE_EMERGENCY) {
 			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
-				      &data->temp8[4]);
+				      &data->temp8[TEMP8_LOCAL_EMERG]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[5]);
+				      &data->temp8[TEMP8_REMOTE_EMERG]);
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
 		data->alarms = alarms;	/* save as 16 bit value */
@@ -530,15 +572,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		if (data->kind == max6696) {
 			lm90_select_remote_channel(client, data, 1);
 			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
-				      &data->temp8[6]);
+				      &data->temp8[TEMP8_REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[7]);
+				      &data->temp8[TEMP8_REMOTE2_EMERG]);
 			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+					LM90_REG_R_REMOTE_TEMPL,
+					&data->temp11[TEMP11_REMOTE2_TEMP]);
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
-				data->temp11[6] = h << 8;
+				data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
-				data->temp11[7] = h << 8;
+				data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
 			lm90_select_remote_channel(client, data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -735,7 +778,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 
 static void write_temp8(struct device *dev, int index, long val)
 {
-	static const u8 reg[8] = {
+	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
@@ -821,7 +864,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
 		u8 high;
 		u8 low;
 		int channel;
-	} reg[5] = {
+	} reg[NR_NUM] = {
 		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
 		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
@@ -912,11 +955,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[2]);
+		temp = temp_from_u8_adt7461(data,
+					data->temp8[TEMP8_LOCAL_CRIT]);
 	else if (data->kind == max6646)
-		temp = temp_from_u8(data->temp8[2]);
+		temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
 	else
-		temp = temp_from_s8(data->temp8[2]);
+		temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -970,25 +1014,28 @@ static ssize_t set_update_interval(struct device *dev,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 0);
+	set_temp8, TEMP8_LOCAL_LOW);
 static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, 1);
+	set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 1);
+	set_temp8, TEMP8_LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, 2);
+	set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 2);
+	set_temp8, TEMP8_LOCAL_CRIT);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 3);
+	set_temp8, TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
-	set_temphyst, 2);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
+	set_temphyst, TEMP8_LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, 3);
+	set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1036,13 +1083,13 @@ static const struct attribute_group lm90_group = {
  * Additional attributes for devices with emergency sensors
  */
 static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 4);
+	set_temp8, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 5);
+	set_temp8, TEMP8_REMOTE_EMERG);
 static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 4);
+			  NULL, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 5);
+			  NULL, TEMP8_REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1072,18 +1119,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
 /*
  * Additional attributes for devices with 3 temperature sensors
  */
-static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
 static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, 6);
+	set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
 static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, 7);
+	set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
 static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 6);
-static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6);
+	set_temp8, TEMP8_REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE2_CRIT);
 static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 7);
+	set_temp8, TEMP8_REMOTE2_EMERG);
 static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 7);
+			  NULL, TEMP8_REMOTE2_EMERG);
 
 static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
 static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
-- 
1.7.9.5

[lm-sensors] [PATCH 3/3] hwmon: (lm90) use enums for the indexes of temp8 and temp11

[Wei Ni]

Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/hwmon/lm90.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 114 insertions(+), 65 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index fce9dfa..97fe83a 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -297,6 +297,59 @@ static const struct lm90_params lm90_params[] = {
 };
 
 /*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+	TEMP8_LOCAL_LOW = 0,	/* 0: local low limit */
+	TEMP8_LOCAL_HIGH,	/* 1: local high limit */
+	TEMP8_LOCAL_CRIT,	/* 2: local critical limit */
+	TEMP8_REMOTE_CRIT,	/* 3: remote critical limit */
+	TEMP8_LOCAL_EMERG,	/* 4: local emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE_EMERG,	/* 5: remote emergency limit
+				 * (max6659 and max6695/96)
+				 */
+	TEMP8_REMOTE2_CRIT,	/* 6: remote 2 critical limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REMOTE2_EMERG,	/* 7: remote 2 emergency limit
+				 * (max6695/96 only)
+				 */
+	TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+	TEMP11_REMOTE_TEMP = 0,	/* 0: remote input */
+	TEMP11_REMOTE_LOW,	/* 1: remote low limit */
+	TEMP11_REMOTE_HIGH,	/* 2: remote high limit */
+	TEMP11_REMOTE_OFFSET,	/* 3: remote offset
+				 * (except max6646, max6657/58/59,
+				 *  and max6695/96)
+				 */
+	TEMP11_LOCAL_TEMP,	/* 4: local input */
+	TEMP11_REMOTE2_TEMP,	/* 5: remote 2 input (max6695/96 only) */
+	TEMP11_REMOTE2_LOW,	/* 6: remote 2 low limit (max6695/96 only) */
+	TEMP11_REMOTE2_HIGH,	/* 7: remote 2 high limit (max6695/96 only) */
+	TEMP11_REG_NUM
+};
+
+/*
+ * TEMP11 register NR
+ */
+enum lm90_temp11_reg_nr {
+	NR_CHAN_0_REMOTE_LOW = 0,	/* 0: channel 0, remote low limit */
+	NR_CHAN_0_REMOTE_HIGH,		/* 1: channel 0, remote high limit */
+	NR_CHAN_0_REMOTE_OFFSET,	/* 2: channel 0, remote offset */
+	NR_CHAN_1_REMOTE_LOW,		/* 3: channel 1, remote low limit */
+	NR_CHAN_1_REMOTE_HIGH,		/* 4: channel 1, remote high limit */
+	NR_NUM				/* number of the NRs for temp11 */
+};
+
+/*
  * Client data (each client gets its own)
  */
 
@@ -318,25 +371,8 @@ struct lm90_data {
 	u8 reg_local_ext;	/* local extension register offset */
 
 	/* registers values */
-	s8 temp8[8];	/* 0: local low limit
-			 * 1: local high limit
-			 * 2: local critical limit
-			 * 3: remote critical limit
-			 * 4: local emergency limit (max6659 and max6695/96)
-			 * 5: remote emergency limit (max6659 and max6695/96)
-			 * 6: remote 2 critical limit (max6695/96 only)
-			 * 7: remote 2 emergency limit (max6695/96 only)
-			 */
-	s16 temp11[8];	/* 0: remote input
-			 * 1: remote low limit
-			 * 2: remote high limit
-			 * 3: remote offset (except max6646, max6657/58/59,
-			 *		     and max6695/96)
-			 * 4: local input
-			 * 5: remote 2 input (max6695/96 only)
-			 * 6: remote 2 low limit (max6695/96 only)
-			 * 7: remote 2 high limit (max6695/96 only)
-			 */
+	s8 temp8[TEMP8_REG_NUM];
+	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
 };
@@ -478,37 +514,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		u8 alarms;
 
 		dev_dbg(&client->dev, "Updating lm90 data.\n");
-		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
-		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
-		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+				&data->temp8[TEMP8_LOCAL_LOW]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+				&data->temp8[TEMP8_LOCAL_HIGH]);
+		lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+				&data->temp8[TEMP8_LOCAL_CRIT]);
+		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+				&data->temp8[TEMP8_REMOTE_CRIT]);
 		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
 
 		if (data->reg_local_ext) {
 			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
 				    data->reg_local_ext,
-				    &data->temp11[4]);
+				    &data->temp11[TEMP11_LOCAL_TEMP]);
 		} else {
 			if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
 					  &h) = 0)
-				data->temp11[4] = h << 8;
+				data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
 		}
 		lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-			    LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
+			LM90_REG_R_REMOTE_TEMPL,
+			&data->temp11[TEMP11_REMOTE_TEMP]);
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) = 0) {
-			data->temp11[1] = h << 8;
+			data->temp11[TEMP11_REMOTE_LOW] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
 					  &l) = 0)
-				data->temp11[1] |= l;
+				data->temp11[TEMP11_REMOTE_LOW] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) = 0) {
-			data->temp11[2] = h << 8;
+			data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
 			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
 					  &l) = 0)
-				data->temp11[2] |= l;
+				data->temp11[TEMP11_REMOTE_HIGH] |= l;
 		}
 
 		if (data->flags & LM90_HAVE_OFFSET) {
@@ -516,13 +557,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 					  &h) = 0
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
 					  &l) = 0)
-				data->temp11[3] = (h << 8) | l;
+				data->temp11[TEMP11_REMOTE_OFFSET] +								(h << 8) | l;
 		}
 		if (data->flags & LM90_HAVE_EMERGENCY) {
 			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
-				      &data->temp8[4]);
+				      &data->temp8[TEMP8_LOCAL_EMERG]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[5]);
+				      &data->temp8[TEMP8_REMOTE_EMERG]);
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
 		data->alarms = alarms;	/* save as 16 bit value */
@@ -530,15 +572,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		if (data->kind = max6696) {
 			lm90_select_remote_channel(client, data, 1);
 			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
-				      &data->temp8[6]);
+				      &data->temp8[TEMP8_REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
-				      &data->temp8[7]);
+				      &data->temp8[TEMP8_REMOTE2_EMERG]);
 			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
-				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+					LM90_REG_R_REMOTE_TEMPL,
+					&data->temp11[TEMP11_REMOTE2_TEMP]);
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
-				data->temp11[6] = h << 8;
+				data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
-				data->temp11[7] = h << 8;
+				data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
 			lm90_select_remote_channel(client, data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -735,7 +778,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 
 static void write_temp8(struct device *dev, int index, long val)
 {
-	static const u8 reg[8] = {
+	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
@@ -821,7 +864,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
 		u8 high;
 		u8 low;
 		int channel;
-	} reg[5] = {
+	} reg[NR_NUM] = {
 		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
 		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
 		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
@@ -912,11 +955,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
 
 	mutex_lock(&data->update_lock);
 	if (data->kind = adt7461)
-		temp = temp_from_u8_adt7461(data, data->temp8[2]);
+		temp = temp_from_u8_adt7461(data,
+					data->temp8[TEMP8_LOCAL_CRIT]);
 	else if (data->kind = max6646)
-		temp = temp_from_u8(data->temp8[2]);
+		temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
 	else
-		temp = temp_from_s8(data->temp8[2]);
+		temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
 	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -970,25 +1014,28 @@ static ssize_t set_update_interval(struct device *dev,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 0);
+	set_temp8, TEMP8_LOCAL_LOW);
 static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, 1);
+	set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 1);
+	set_temp8, TEMP8_LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, 2);
+	set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 2);
+	set_temp8, TEMP8_LOCAL_CRIT);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 3);
+	set_temp8, TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
-	set_temphyst, 2);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
+	set_temphyst, TEMP8_LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE_CRIT);
 static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, 3);
+	set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1036,13 +1083,13 @@ static const struct attribute_group lm90_group = {
  * Additional attributes for devices with emergency sensors
  */
 static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 4);
+	set_temp8, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 5);
+	set_temp8, TEMP8_REMOTE_EMERG);
 static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 4);
+			  NULL, TEMP8_LOCAL_EMERG);
 static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 5);
+			  NULL, TEMP8_REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1072,18 +1119,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
 /*
  * Additional attributes for devices with 3 temperature sensors
  */
-static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
+	NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
 static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, 6);
+	set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
 static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, 7);
+	set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
 static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 6);
-static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6);
+	set_temp8, TEMP8_REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+	TEMP8_REMOTE2_CRIT);
 static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, 7);
+	set_temp8, TEMP8_REMOTE2_EMERG);
 static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 7);
+			  NULL, TEMP8_REMOTE2_EMERG);
 
 static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
 static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Stephen Warren]

On 07/04/2013 01:57 AM, Wei Ni wrote:
> Add support to handle irq. When the temperature touch
> the limit value, the driver can handle the interrupt.

> +	if (client->irq >= 0) {

0 isn't a valid IRQ, so you can write that as simply if (client->irq).

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Stephen Warren]

On 07/04/2013 01:57 AM, Wei Ni wrote:
> Add support to handle irq. When the temperature touch
> the limit value, the driver can handle the interrupt.

> +	if (client->irq >= 0) {

0 isn't a valid IRQ, so you can write that as simply if (client->irq).

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Guenter Roeck]

On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
> On 07/04/2013 01:57 AM, Wei Ni wrote:
> > Add support to handle irq. When the temperature touch
> > the limit value, the driver can handle the interrupt.
> 
> > +	if (client->irq >= 0) {
> 
> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
> 
If I recall correctly, it is valid on some platforms.

Guenter

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Guenter Roeck]

On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
> On 07/04/2013 01:57 AM, Wei Ni wrote:
> > Add support to handle irq. When the temperature touch
> > the limit value, the driver can handle the interrupt.
> 
> > +	if (client->irq >= 0) {
> 
> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
> 
If I recall correctly, it is valid on some platforms.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Stephen Warren]

On 07/05/2013 01:47 PM, Guenter Roeck wrote:
> On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
>> On 07/04/2013 01:57 AM, Wei Ni wrote:
>>> Add support to handle irq. When the temperature touch
>>> the limit value, the driver can handle the interrupt.
>>
>>> +	if (client->irq >= 0) {
>>
>> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
>>
> If I recall correctly, it is valid on some platforms.

I thought ARM (just some ARM sub-architectures?) might have been the
last architecture, and even irrespective of that, we were trying not to
introduce any new code that relies on this strangeness, so it doesn't
propagate?

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Stephen Warren]

On 07/05/2013 01:47 PM, Guenter Roeck wrote:
> On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
>> On 07/04/2013 01:57 AM, Wei Ni wrote:
>>> Add support to handle irq. When the temperature touch
>>> the limit value, the driver can handle the interrupt.
>>
>>> +	if (client->irq >= 0) {
>>
>> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
>>
> If I recall correctly, it is valid on some platforms.

I thought ARM (just some ARM sub-architectures?) might have been the
last architecture, and even irrespective of that, we were trying not to
introduce any new code that relies on this strangeness, so it doesn't
propagate?

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Guenter Roeck]

On Mon, Jul 08, 2013 at 09:46:41AM -0600, Stephen Warren wrote:
> On 07/05/2013 01:47 PM, Guenter Roeck wrote:
> > On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
> >> On 07/04/2013 01:57 AM, Wei Ni wrote:
> >>> Add support to handle irq. When the temperature touch
> >>> the limit value, the driver can handle the interrupt.
> >>
> >>> +	if (client->irq >= 0) {
> >>
> >> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
> >>
> > If I recall correctly, it is valid on some platforms.
> 
> I thought ARM (just some ARM sub-architectures?) might have been the
> last architecture, and even irrespective of that, we were trying not to
> introduce any new code that relies on this strangeness, so it doesn't
> propagate?
> 
Sounds good to me. Another problem, though, may be that NO_IRQ is sometimes
defined as -1, sometimes as 0, sometimes as 0xffffffff, and sometimes as INT_MAX.
Which of course is another mess :(.

Guenter

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Guenter Roeck]

On Mon, Jul 08, 2013 at 09:46:41AM -0600, Stephen Warren wrote:
> On 07/05/2013 01:47 PM, Guenter Roeck wrote:
> > On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
> >> On 07/04/2013 01:57 AM, Wei Ni wrote:
> >>> Add support to handle irq. When the temperature touch
> >>> the limit value, the driver can handle the interrupt.
> >>
> >>> +	if (client->irq >= 0) {
> >>
> >> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
> >>
> > If I recall correctly, it is valid on some platforms.
> 
> I thought ARM (just some ARM sub-architectures?) might have been the
> last architecture, and even irrespective of that, we were trying not to
> introduce any new code that relies on this strangeness, so it doesn't
> propagate?
> 
Sounds good to me. Another problem, though, may be that NO_IRQ is sometimes
defined as -1, sometimes as 0, sometimes as 0xffffffff, and sometimes as INT_MAX.
Which of course is another mess :(.

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
> Add support to handle irq. When the temperature touch

Nit: This first sentence can be dropped. It merely repeats the subject.
And another nit: "irq" -> "IRQ"

> the limit value, the driver can handle the interrupt.
> 
> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2cb7f8e..fce9dfa 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
>  
>  /*
>   * Addresses to scan
> @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alert(struct i2c_client *client, unsigned int flag);

Any reason why the implementation of lm90_alert() can't be moved here.
Granted, it'll make the diff larger but at least it'll allow us to get
rid of the forward declaration.

> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +
> +	lm90_alert(client, 0);
> +
> +	return IRQ_HANDLED;
> +}

Isn't this potentially dangerous, since if the IRQ was shared, IRQ
processing will always stop after this handler. Shouldn't you check
whether the device actually triggered an interrupt (by reading the
interrupt status register)?

> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
>  		goto exit_remove_files;
>  	}
>  
> +	if (client->irq >= 0) {
> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);

Nit: "irq" -> "IRQ"

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2004 bytes --]

On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
> Add support to handle irq. When the temperature touch

Nit: This first sentence can be dropped. It merely repeats the subject.
And another nit: "irq" -> "IRQ"

> the limit value, the driver can handle the interrupt.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2cb7f8e..fce9dfa 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
>  
>  /*
>   * Addresses to scan
> @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>  }
>  
> +static void lm90_alert(struct i2c_client *client, unsigned int flag);

Any reason why the implementation of lm90_alert() can't be moved here.
Granted, it'll make the diff larger but at least it'll allow us to get
rid of the forward declaration.

> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> +	struct lm90_data *data = dev_id;
> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> +
> +	lm90_alert(client, 0);
> +
> +	return IRQ_HANDLED;
> +}

Isn't this potentially dangerous, since if the IRQ was shared, IRQ
processing will always stop after this handler. Shouldn't you check
whether the device actually triggered an interrupt (by reading the
interrupt status register)?

> +
>  static int lm90_probe(struct i2c_client *client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
>  		goto exit_remove_files;
>  	}
>  
> +	if (client->irq >= 0) {
> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);

Nit: "irq" -> "IRQ"

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Jean Delvare]

Hi Thierry,

Thanks for the review, it is greatly appreciated. A couple additions...

On Tue, 9 Jul 2013 08:15:15 +0200, Thierry Reding wrote:
> On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
> > Add support to handle irq. When the temperature touch
> 
> Nit: This first sentence can be dropped. It merely repeats the subject.

Note that I have no problem with that.

> > (...)
> > @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
> >  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> >  }
> >  
> > +static void lm90_alert(struct i2c_client *client, unsigned int flag);
> 
> Any reason why the implementation of lm90_alert() can't be moved here.
> Granted, it'll make the diff larger but at least it'll allow us to get
> rid of the forward declaration.

Yes, please avoid forward declarations as much as possible.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Jean Delvare]

Hi Thierry,

Thanks for the review, it is greatly appreciated. A couple additions...

On Tue, 9 Jul 2013 08:15:15 +0200, Thierry Reding wrote:
> On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
> > Add support to handle irq. When the temperature touch
> 
> Nit: This first sentence can be dropped. It merely repeats the subject.

Note that I have no problem with that.

> > (...)
> > @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
> >  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> >  }
> >  
> > +static void lm90_alert(struct i2c_client *client, unsigned int flag);
> 
> Any reason why the implementation of lm90_alert() can't be moved here.
> Granted, it'll make the diff larger but at least it'll allow us to get
> rid of the forward declaration.

Yes, please avoid forward declarations as much as possible.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 07/09/2013 04:06 AM, Guenter Roeck wrote:
> On Mon, Jul 08, 2013 at 09:46:41AM -0600, Stephen Warren wrote:
>> On 07/05/2013 01:47 PM, Guenter Roeck wrote:
>>> On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
>>>> On 07/04/2013 01:57 AM, Wei Ni wrote:
>>>>> Add support to handle irq. When the temperature touch
>>>>> the limit value, the driver can handle the interrupt.
>>>>
>>>>> +	if (client->irq >= 0) {
>>>>
>>>> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
>>>>
>>> If I recall correctly, it is valid on some platforms.
>>
>> I thought ARM (just some ARM sub-architectures?) might have been the
>> last architecture, and even irrespective of that, we were trying not to
>> introduce any new code that relies on this strangeness, so it doesn't
>> propagate?
>>
> Sounds good to me. Another problem, though, may be that NO_IRQ is sometimes
> defined as -1, sometimes as 0, sometimes as 0xffffffff, and sometimes as INT_MAX.
> Which of course is another mess :(.
> 
> Guenter
> 

Ok, so I will use the "if (client->irq)".

Thanks.
Wei.

> 

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 07/09/2013 04:06 AM, Guenter Roeck wrote:
> On Mon, Jul 08, 2013 at 09:46:41AM -0600, Stephen Warren wrote:
>> On 07/05/2013 01:47 PM, Guenter Roeck wrote:
>>> On Fri, Jul 05, 2013 at 11:35:05AM -0600, Stephen Warren wrote:
>>>> On 07/04/2013 01:57 AM, Wei Ni wrote:
>>>>> Add support to handle irq. When the temperature touch
>>>>> the limit value, the driver can handle the interrupt.
>>>>
>>>>> +	if (client->irq >= 0) {
>>>>
>>>> 0 isn't a valid IRQ, so you can write that as simply if (client->irq).
>>>>
>>> If I recall correctly, it is valid on some platforms.
>>
>> I thought ARM (just some ARM sub-architectures?) might have been the
>> last architecture, and even irrespective of that, we were trying not to
>> introduce any new code that relies on this strangeness, so it doesn't
>> propagate?
>>
> Sounds good to me. Another problem, though, may be that NO_IRQ is sometimes
> defined as -1, sometimes as 0, sometimes as 0xffffffff, and sometimes as INT_MAX.
> Which of course is another mess :(.
> 
> Guenter
> 

Ok, so I will use the "if (client->irq)".

Thanks.
Wei.

> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 07/09/2013 02:15 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
>> Add support to handle irq. When the temperature touch
> 
> Nit: This first sentence can be dropped. It merely repeats the subject.
> And another nit: "irq" -> "IRQ"

Ok, I will update in my next version.

> 
>> the limit value, the driver can handle the interrupt.
>>
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 2cb7f8e..fce9dfa 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,7 @@
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/interrupt.h>
>>  
>>  /*
>>   * Addresses to scan
>> @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>  }
>>  
>> +static void lm90_alert(struct i2c_client *client, unsigned int flag);
> 
> Any reason why the implementation of lm90_alert() can't be moved here.
> Granted, it'll make the diff larger but at least it'll allow us to get
> rid of the forward declaration.

Ok, you are right, I will move it.

> 
>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>> +{
>> +	struct lm90_data *data = dev_id;
>> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>> +
>> +	lm90_alert(client, 0);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> Isn't this potentially dangerous, since if the IRQ was shared, IRQ
> processing will always stop after this handler. Shouldn't you check
> whether the device actually triggered an interrupt (by reading the
> interrupt status register)?

Yes, it has potential dangerous, I didn't consider it carefully.
I will check the interrupt status.

> 
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
>>  		goto exit_remove_files;
>>  	}
>>  
>> +	if (client->irq >= 0) {
>> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
> 
> Nit: "irq" -> "IRQ"

Ok, got it.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
> 

Re: [lm-sensors] [PATCH 2/3] hwmon: (lm90) add support to handle irq

[Wei Ni]

On 07/09/2013 02:15 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jul 04, 2013 at 03:57:49PM +0800, Wei Ni wrote:
>> Add support to handle irq. When the temperature touch
> 
> Nit: This first sentence can be dropped. It merely repeats the subject.
> And another nit: "irq" -> "IRQ"

Ok, I will update in my next version.

> 
>> the limit value, the driver can handle the interrupt.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/hwmon/lm90.c |   25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 2cb7f8e..fce9dfa 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,7 @@
>>  #include <linux/err.h>
>>  #include <linux/mutex.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/interrupt.h>
>>  
>>  /*
>>   * Addresses to scan
>> @@ -1423,6 +1424,18 @@ static void lm90_init_client(struct i2c_client *client)
>>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>>  }
>>  
>> +static void lm90_alert(struct i2c_client *client, unsigned int flag);
> 
> Any reason why the implementation of lm90_alert() can't be moved here.
> Granted, it'll make the diff larger but at least it'll allow us to get
> rid of the forward declaration.

Ok, you are right, I will move it.

> 
>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>> +{
>> +	struct lm90_data *data = dev_id;
>> +	struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>> +
>> +	lm90_alert(client, 0);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> Isn't this potentially dangerous, since if the IRQ was shared, IRQ
> processing will always stop after this handler. Shouldn't you check
> whether the device actually triggered an interrupt (by reading the
> interrupt status register)?

Yes, it has potential dangerous, I didn't consider it carefully.
I will check the interrupt status.

> 
>> +
>>  static int lm90_probe(struct i2c_client *client,
>>  		      const struct i2c_device_id *id)
>>  {
>> @@ -1499,6 +1512,18 @@ static int lm90_probe(struct i2c_client *client,
>>  		goto exit_remove_files;
>>  	}
>>  
>> +	if (client->irq >= 0) {
>> +		dev_dbg(dev, "lm90 irq: %d\n", client->irq);
> 
> Nit: "irq" -> "IRQ"

Ok, got it.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors