[PATCH 0/3] lm90: device tree and thermal zone support

[PATCH 0/3] lm90: device tree and thermal zone support

[Stéphan Kochen]

This patchset adds support to the lm90 driver for initialising sensor
parameters from a device tree, as well as using the sensor in thermal zones
specified in a device tree.

The patchset is extracted from an ongoing personal project to get mainline
running on the Ouya console. The git repository is available at:

  https://github.com/stephank/linux.git

This particular patchset can also be pulled from the 'lm90-of' branch there.

The Ouya has a single GPIO fan, and an ON NCT1008 temperature sensor. With this
patchset, the kernel can do active cooling through a thermal zone, and set
sensible sensor alert parameters without user-space assistance.

There's still something amiss with alerts, though. When the temperature exceeds
one of the bounds, the machine bogs down and the log is flooded with warning
messages. (An interrupt flood? I'm not too concerned at this point, active
cooling should keep us far from the configured sensor alert bounds during
normal operation.)

Please note that these are my very first kernel patches. Hoping I get it right
the first time!

Stéphan Kochen (3):
  lm90: separate register accessors from sysfs
  lm90: initialize parameters from devicetree
  lm90: register with thermal zones

 Documentation/devicetree/bindings/hwmon/lm90.txt |  48 ++
 drivers/hwmon/lm90.c                             | 618 +++++++++++++++--------
 2 files changed, 453 insertions(+), 213 deletions(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] lm90: separate register accessors from sysfs

[Stéphan Kochen]

Add additional support functions `lm90_get_*` and `lm90_set_*` extracted
from the sysfs routines. The sysfs routines are now just stubs wrapping
the new support functions.

This way we have a consistent register access interface, which can be
used from other integrations down the line. To avoid confusion, all
sysfs methods are now prefixed `lm90_sysfs_*`.

To accomplish this, the `lm90_update_device` signature has changed and
is now `lm90_update`, taking data directly. It no longer serves the
dual purpose of update function *and* driver data getter.

`lm90_set_convrate`, `lm90_select_remote_channel` and `lm90_init_client`
also had redundant i2c client parameters, which have been removed.

Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
---
 drivers/hwmon/lm90.c | 511 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 300 insertions(+), 211 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c9ff08d..88daf72 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -473,10 +473,10 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
  * various registers have different meanings as a result of selecting a
  * non-default remote channel.
  */
-static inline void lm90_select_remote_channel(struct i2c_client *client,
-					      struct lm90_data *data,
-					      int channel)
+static inline void lm90_select_remote_channel(
+		struct lm90_data *data, int channel)
 {
+	struct i2c_client *client = data->client;
 	u8 config;
 
 	if (data->kind == max6696) {
@@ -490,32 +490,10 @@ static inline void lm90_select_remote_channel(struct i2c_client *client,
 }
 
 /*
- * Set conversion rate.
- * client->update_lock must be held when calling this function (unless we are
- * in detection or initialization steps).
+ * Update all current register values
  */
-static void lm90_set_convrate(struct i2c_client *client, struct lm90_data *data,
-			      unsigned int interval)
-{
-	int i;
-	unsigned int update_interval;
-
-	/* Shift calculations to avoid rounding errors */
-	interval <<= 6;
-
-	/* find the nearest update rate */
-	for (i = 0, update_interval = LM90_MAX_CONVRATE_MS << 6;
-	     i < data->max_convrate; i++, update_interval >>= 1)
-		if (interval >= update_interval * 3 / 4)
-			break;
-
-	i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
-	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
-}
-
-static struct lm90_data *lm90_update_device(struct device *dev)
+static void lm90_update(struct lm90_data *data)
 {
-	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
 	unsigned long next_update;
 
@@ -583,7 +561,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		data->alarms = alarms;	/* save as 16 bit value */
 
 		if (data->kind == max6696) {
-			lm90_select_remote_channel(client, data, 1);
+			lm90_select_remote_channel(data, 1);
 			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
 				      &data->temp8[REMOTE2_CRIT]);
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
@@ -595,7 +573,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 				data->temp11[REMOTE2_LOW] = h << 8;
 			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
 				data->temp11[REMOTE2_HIGH] = h << 8;
-			lm90_select_remote_channel(client, data, 0);
+			lm90_select_remote_channel(data, 0);
 
 			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
 					   &alarms))
@@ -624,8 +602,6 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 	}
 
 	mutex_unlock(&data->update_lock);
-
-	return data;
 }
 
 /*
@@ -756,32 +732,31 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
 }
 
 /*
- * Sysfs stuff
+ * Register accessors. Getters trigger an update. Setters have locked variants
+ * that require update_lock to be held.
  */
 
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
-			  char *buf)
+static int lm90_get_temp8(struct lm90_data *data, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
+	lm90_update(data);
+
 	if (data->kind == adt7461 || data->kind == tmp451)
-		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 sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static void lm90_set_temp8_locked(struct lm90_data *data, int index, long val)
 {
 	static const u8 reg[TEMP8_REG_NUM] = {
 		LM90_REG_W_LOCAL_LOW,
@@ -794,90 +769,73 @@ 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 lm90_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->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->kind == tmp451)
-		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, 0);
+	lm90_select_remote_channel(data, index >= 6);
+	i2c_smbus_write_byte_data(data->client, reg[index], data->temp8[index]);
+	lm90_select_remote_channel(data, 0);
+}
 
+static void lm90_set_temp8(struct lm90_data *data, int index, long val)
+{
+	mutex_lock(&data->update_lock);
+	lm90_set_temp8_locked(data, index, val);
 	mutex_unlock(&data->update_lock);
-	return count;
 }
 
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
-			   char *buf)
+static int lm90_get_temp11(struct lm90_data *data, int index)
 {
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
+	lm90_update(data);
+
 	if (data->kind == adt7461 || data->kind == tmp451)
-		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 sprintf(buf, "%d\n", temp);
+	return temp;
 }
 
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-			  const char *buf, size_t count)
+static void lm90_set_temp11_locked(struct lm90_data *data, int index, long val)
 {
-	struct {
-		u8 high;
-		u8 low;
-		int channel;
-	} reg[5] = {
-		{ 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 },
-		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 1 },
-		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
-	};
-
-	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
-	struct lm90_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int nr = attr->nr;
-	int index = attr->index;
-	long val;
-	int err;
+	u8 high, low, channel;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
+#define SELECT_REGS(_reg, _channel) { \
+	high = LM90_REG_W_##_reg##H; \
+	low = LM90_REG_W_##_reg##L; \
+	channel = _channel; \
+}
+	switch (index) {
+	case REMOTE_LOW:    SELECT_REGS(REMOTE_LOW,  0); break;
+	case REMOTE_HIGH:   SELECT_REGS(REMOTE_HIGH, 0); break;
+	case REMOTE_OFFSET: SELECT_REGS(REMOTE_OFFS, 0); break;
+	case REMOTE2_LOW:   SELECT_REGS(REMOTE_LOW,  1); break;
+	case REMOTE2_HIGH:  SELECT_REGS(REMOTE_HIGH, 1); break;
+	default: return;
+	}
+#undef SELECT_REGS
 
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
 		val -= 16000;
 
-	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461 || data->kind == tmp451)
 		data->temp11[index] = temp_to_u16_adt7461(data, val);
 	else if (data->kind == max6646)
@@ -887,54 +845,46 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	else
 		data->temp11[index] = temp_to_s8(val) << 8;
 
-	lm90_select_remote_channel(client, data, reg[nr].channel);
-	i2c_smbus_write_byte_data(client, reg[nr].high,
-				  data->temp11[index] >> 8);
+	lm90_select_remote_channel(data, channel);
+	i2c_smbus_write_byte_data(client, high,
+			data->temp11[index] >> 8);
 	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
-		i2c_smbus_write_byte_data(client, reg[nr].low,
-					  data->temp11[index] & 0xff);
-	lm90_select_remote_channel(client, data, 0);
+		i2c_smbus_write_byte_data(client, low,
+				data->temp11[index] & 0xff);
+	lm90_select_remote_channel(data, 0);
+}
 
+static void lm90_set_temp11(struct lm90_data *data, int index, long val)
+{
+	mutex_lock(&data->update_lock);
+	lm90_set_temp11_locked(data, index, val);
 	mutex_unlock(&data->update_lock);
-	return count;
 }
 
-static ssize_t show_temphyst(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
+static ssize_t lm90_get_temphyst(struct lm90_data *data, int index)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
+	lm90_update(data);
+
 	if (data->kind == adt7461 || data->kind == tmp451)
-		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 sprintf(buf, "%d\n", temp - temp_from_s8(data->temp_hyst));
+	return temp - temp_from_s8(data->temp_hyst);
 }
 
-static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
-			    const char *buf, size_t count)
+static void lm90_set_temphyst_locked(struct lm90_data *data, long val)
 {
-	struct lm90_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	long val;
-	int err;
 	int temp;
 
-	err = kstrtol(buf, 10, &val);
-	if (err < 0)
-		return err;
-
-	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461 || data->kind == tmp451)
 		temp = temp_from_u8_adt7461(data, data->temp8[LOCAL_CRIT]);
 	else if (data->kind == max6646)
@@ -943,43 +893,167 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
 		temp = temp_from_s8(data->temp8[LOCAL_CRIT]);
 
 	data->temp_hyst = hyst_to_reg(temp - val);
-	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
-				  data->temp_hyst);
+	i2c_smbus_write_byte_data(data->client, LM90_REG_W_TCRIT_HYST,
+			data->temp_hyst);
+}
+
+static void lm90_set_temphyst(struct lm90_data *data, long val)
+{
+	mutex_lock(&data->update_lock);
+	lm90_set_temphyst_locked(data, val);
+	mutex_unlock(&data->update_lock);
+}
+
+static void lm90_set_convrate_locked(struct lm90_data *data, unsigned int val)
+{
+	int i;
+	unsigned int update_interval;
+
+	/* Shift calculations to avoid rounding errors */
+	val <<= 6;
+
+	/* find the nearest update rate */
+	for (i = 0, update_interval = LM90_MAX_CONVRATE_MS << 6;
+	     i < data->max_convrate; i++, update_interval >>= 1)
+		if (val >= update_interval * 3 / 4)
+			break;
+
+	i2c_smbus_write_byte_data(data->client, LM90_REG_W_CONVRATE, i);
+	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
+}
+
+static void lm90_set_convrate(struct lm90_data *data, unsigned int val)
+{
+	mutex_lock(&data->update_lock);
+	lm90_set_convrate_locked(data, val);
 	mutex_unlock(&data->update_lock);
+}
+
+/*
+ * Sysfs stuff
+ */
+
+static ssize_t lm90_sysfs_show_temp8(
+		struct device *dev, struct device_attribute *devattr,
+		char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", lm90_get_temp8(data, attr->index));
+}
+
+static ssize_t lm90_sysfs_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);
+	struct lm90_data *data = dev_get_drvdata(dev);
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	lm90_set_temp8(data, attr->index, val);
+
 	return count;
 }
 
-static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
-			   char *buf)
+static ssize_t lm90_sysfs_show_temp11(
+		struct device *dev, struct device_attribute *devattr,
+		char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", lm90_get_temp11(data, attr->index));
+}
+
+static ssize_t lm90_sysfs_set_temp11(
+		struct device *dev, struct device_attribute *devattr,
+		const char *buf, size_t count)
 {
-	struct lm90_data *data = lm90_update_device(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct lm90_data *data = dev_get_drvdata(dev);
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	lm90_set_temp11(data, attr->index, val);
+
+	return count;
+}
+
+static ssize_t lm90_sysfs_show_temphyst(
+		struct device *dev, struct device_attribute *devattr,
+		char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", lm90_get_temphyst(data, attr->index));
+}
+
+static ssize_t lm90_sysfs_set_temphyst(
+		struct device *dev, struct device_attribute *dummy,
+		const char *buf, size_t count)
+{
+	struct lm90_data *data = dev_get_drvdata(dev);
+	long val;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	lm90_set_temphyst(data, val);
+
+	return count;
+}
+
+static ssize_t lm90_sysfs_show_alarms(
+		struct device *dev, struct device_attribute *dummy,
+		char *buf)
+{
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	lm90_update(data);
+
 	return sprintf(buf, "%d\n", data->alarms);
 }
 
-static ssize_t show_alarm(struct device *dev, struct device_attribute
-			  *devattr, char *buf)
+static ssize_t lm90_sysfs_show_alarm(
+		struct device *dev, struct device_attribute *devattr,
+		char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct lm90_data *data = lm90_update_device(dev);
+	struct lm90_data *data = dev_get_drvdata(dev);
 	int bitnr = attr->index;
 
+	lm90_update(data);
+
 	return sprintf(buf, "%d\n", (data->alarms >> bitnr) & 1);
 }
 
-static ssize_t show_update_interval(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static ssize_t lm90_sysfs_show_update_interval(
+		struct device *dev, struct device_attribute *attr,
+		char *buf)
 {
 	struct lm90_data *data = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", data->update_interval);
 }
 
-static ssize_t set_update_interval(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static ssize_t lm90_sysfs_set_update_interval(
+		struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
 {
 	struct lm90_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long val;
 	int err;
 
@@ -987,49 +1061,55 @@ static ssize_t set_update_interval(struct device *dev,
 	if (err)
 		return err;
 
-	mutex_lock(&data->update_lock);
-	lm90_set_convrate(client, data, clamp_val(val, 0, 100000));
-	mutex_unlock(&data->update_lock);
+	lm90_set_convrate(data, clamp_val(val, 0, 100000));
 
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
-	0, LOCAL_TEMP);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
-	0, REMOTE_TEMP);
-static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, LOCAL_LOW);
-static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 0, REMOTE_LOW);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, LOCAL_HIGH);
-static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1, REMOTE_HIGH);
-static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, LOCAL_CRIT);
-static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, REMOTE_CRIT);
-static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
-	set_temphyst, LOCAL_CRIT);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
-	REMOTE_CRIT);
-static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2, REMOTE_OFFSET);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
+	lm90_sysfs_show_temp11, NULL, LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO,
+	lm90_sysfs_show_temp11, NULL, REMOTE_TEMP);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_LOW);
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_LOW);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_HIGH);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_HIGH);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE_CRIT);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temphyst, lm90_sysfs_set_temphyst, LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO,
+	lm90_sysfs_show_temphyst, NULL, REMOTE_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_offset, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_OFFSET);
 
 /* Individual alarm files */
-static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
-static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL, 2);
-static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO, show_alarm, NULL, 3);
-static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO, show_alarm, NULL, 4);
-static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 5);
-static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 6);
 /* Raw alarm file for compatibility */
-static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+static DEVICE_ATTR(alarms, S_IRUGO,
+	lm90_sysfs_show_alarms, NULL);
 
-static DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR, show_update_interval,
-		   set_update_interval);
+static DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
+	lm90_sysfs_show_update_interval, lm90_sysfs_set_update_interval);
 
 static struct attribute *lm90_attributes[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
@@ -1071,14 +1151,14 @@ static const struct attribute_group lm90_temp2_offset_group = {
 /*
  * Additional attributes for devices with emergency sensors
  */
-static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, LOCAL_EMERG);
-static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, REMOTE_EMERG);
-static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, LOCAL_EMERG);
-static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, REMOTE_EMERG);
+static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_EMERG);
+static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE_EMERG);
+static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO,
+	lm90_sysfs_show_temphyst, NULL, LOCAL_EMERG);
+static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO,
+	lm90_sysfs_show_temphyst, NULL, REMOTE_EMERG);
 
 static struct attribute *lm90_emergency_attributes[] = {
 	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1092,8 +1172,10 @@ static const struct attribute_group lm90_emergency_group = {
 	.attrs = lm90_emergency_attributes,
 };
 
-static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO, show_alarm, NULL, 15);
-static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO, show_alarm, NULL, 13);
+static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 15);
+static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 13);
 
 static struct attribute *lm90_emergency_alarm_attributes[] = {
 	&sensor_dev_attr_temp1_emergency_alarm.dev_attr.attr,
@@ -1108,26 +1190,31 @@ 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, REMOTE2_TEMP);
-static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3, REMOTE2_LOW);
-static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 4, REMOTE2_HIGH);
-static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, REMOTE2_CRIT);
-static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
-	REMOTE2_CRIT);
-static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
-	set_temp8, REMOTE2_EMERG);
-static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
-			  NULL, 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);
-static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
-static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
-static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO, show_alarm, NULL, 14);
+static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO,
+	lm90_sysfs_show_temp11, NULL, REMOTE2_TEMP);
+static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE2_LOW);
+static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE2_HIGH);
+static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO,
+	lm90_sysfs_show_temphyst, NULL, REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE2_EMERG);
+static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO,
+	lm90_sysfs_show_temphyst, NULL, REMOTE2_EMERG);
+
+static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 10);
+static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 11);
+static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 12);
+static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO,
+	lm90_sysfs_show_alarm, NULL, 14);
 
 static struct attribute *lm90_temp3_attributes[] = {
 	&sensor_dev_attr_temp3_input.dev_attr.attr,
@@ -1151,15 +1238,15 @@ static const struct attribute_group lm90_temp3_group = {
 };
 
 /* pec used for ADM1032 only */
-static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
-			char *buf)
+static ssize_t lm90_sysfs_show_pec(struct device *dev,
+		struct device_attribute *dummy, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
 }
 
-static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
-		       const char *buf, size_t count)
+static ssize_t lm90_sysfs_set_pec(struct device *dev,
+		struct device_attribute *dummy, const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	long val;
@@ -1183,7 +1270,8 @@ static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
 	return count;
 }
 
-static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
+static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO,
+	lm90_sysfs_show_pec, lm90_sysfs_set_pec);
 
 /*
  * Real code
@@ -1413,8 +1501,9 @@ static void lm90_restore_conf(struct i2c_client *client, struct lm90_data *data)
 				  data->config_orig);
 }
 
-static void lm90_init_client(struct i2c_client *client, struct lm90_data *data)
+static void lm90_init_client(struct lm90_data *data)
 {
+	struct i2c_client *client = data->client;
 	u8 config, convrate;
 
 	if (lm90_read_reg(client, LM90_REG_R_CONVRATE, &convrate) < 0) {
@@ -1426,7 +1515,7 @@ static void lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 	/*
 	 * Start the conversions.
 	 */
-	lm90_set_convrate(client, data, 500);	/* 500ms; 2Hz conversion rate */
+	lm90_set_convrate_locked(data, 500);  /* 500ms / 2Hz  */
 	if (lm90_read_reg(client, LM90_REG_R_CONFIG1, &config) < 0) {
 		dev_warn(&client->dev, "Initialization failed!\n");
 		return;
@@ -1557,7 +1646,7 @@ static int lm90_probe(struct i2c_client *client,
 	data->max_convrate = lm90_params[data->kind].max_convrate;
 
 	/* Initialize the LM90 chip */
-	lm90_init_client(client, data);
+	lm90_init_client(data);
 
 	/* Register sysfs hooks */
 	data->groups[groups++] = &lm90_group;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] lm90: separate register accessors from sysfs

[Guenter Roeck]

On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> Add additional support functions `lm90_get_*` and `lm90_set_*` extracted
> from the sysfs routines. The sysfs routines are now just stubs wrapping
> the new support functions.
>
> This way we have a consistent register access interface, which can be
> used from other integrations down the line. To avoid confusion, all
> sysfs methods are now prefixed `lm90_sysfs_*`.
>
> To accomplish this, the `lm90_update_device` signature has changed and
> is now `lm90_update`, taking data directly. It no longer serves the
> dual purpose of update function *and* driver data getter.
>
> `lm90_set_convrate`, `lm90_select_remote_channel` and `lm90_init_client`
> also had redundant i2c client parameters, which have been removed.
>
> Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
> ---
>   drivers/hwmon/lm90.c | 511 ++++++++++++++++++++++++++++++---------------------
>   1 file changed, 300 insertions(+), 211 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index c9ff08d..88daf72 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -473,10 +473,10 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
>    * various registers have different meanings as a result of selecting a
>    * non-default remote channel.
>    */
> -static inline void lm90_select_remote_channel(struct i2c_client *client,
> -					      struct lm90_data *data,
> -					      int channel)
> +static inline void lm90_select_remote_channel(
> +		struct lm90_data *data, int channel)

Very much personal preference here, unrelated change, and violates coding style.

Please only one logical change per patch. If you think it is worthwhile to remove
the client argument, prepare a separate patch with only that change, and show
that it reduces code size. Also please make sure that your patch follows
coding style.

>   {
> +	struct i2c_client *client = data->client;
>   	u8 config;
>
>   	if (data->kind == max6696) {
> @@ -490,32 +490,10 @@ static inline void lm90_select_remote_channel(struct i2c_client *client,
>   }
>
>   /*
> - * Set conversion rate.
> - * client->update_lock must be held when calling this function (unless we are
> - * in detection or initialization steps).
> + * Update all current register values
>    */
> -static void lm90_set_convrate(struct i2c_client *client, struct lm90_data *data,
> -			      unsigned int interval)
> -{
> -	int i;
> -	unsigned int update_interval;
> -
> -	/* Shift calculations to avoid rounding errors */
> -	interval <<= 6;
> -
> -	/* find the nearest update rate */
> -	for (i = 0, update_interval = LM90_MAX_CONVRATE_MS << 6;
> -	     i < data->max_convrate; i++, update_interval >>= 1)
> -		if (interval >= update_interval * 3 / 4)
> -			break;
> -
> -	i2c_smbus_write_byte_data(client, LM90_REG_W_CONVRATE, i);
> -	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
> -}
> -
> -static struct lm90_data *lm90_update_device(struct device *dev)
> +static void lm90_update(struct lm90_data *data)
>   {
> -	struct lm90_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
>   	unsigned long next_update;
>
> @@ -583,7 +561,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>   		data->alarms = alarms;	/* save as 16 bit value */
>
>   		if (data->kind == max6696) {
> -			lm90_select_remote_channel(client, data, 1);
> +			lm90_select_remote_channel(data, 1);
>   			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
>   				      &data->temp8[REMOTE2_CRIT]);
>   			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> @@ -595,7 +573,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>   				data->temp11[REMOTE2_LOW] = h << 8;
>   			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
>   				data->temp11[REMOTE2_HIGH] = h << 8;
> -			lm90_select_remote_channel(client, data, 0);
> +			lm90_select_remote_channel(data, 0);
>
>   			if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
>   					   &alarms))
> @@ -624,8 +602,6 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>   	}
>
>   	mutex_unlock(&data->update_lock);
> -
> -	return data;
>   }
>
>   /*
> @@ -756,32 +732,31 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
>   }
>
>   /*
> - * Sysfs stuff
> + * Register accessors. Getters trigger an update. Setters have locked variants
> + * that require update_lock to be held.
>    */
>
> -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> -			  char *buf)
> +static int lm90_get_temp8(struct lm90_data *data, int index)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
>   	int temp;
>
> +	lm90_update(data);
> +
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		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 sprintf(buf, "%d\n", temp);
> +	return temp;
>   }
>
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static void lm90_set_temp8_locked(struct lm90_data *data, int index, long val)
>   {
>   	static const u8 reg[TEMP8_REG_NUM] = {
>   		LM90_REG_W_LOCAL_LOW,
> @@ -794,90 +769,73 @@ 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 lm90_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->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->kind == tmp451)
> -		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, 0);
> +	lm90_select_remote_channel(data, index >= 6);
> +	i2c_smbus_write_byte_data(data->client, reg[index], data->temp8[index]);
> +	lm90_select_remote_channel(data, 0);
> +}
>
> +static void lm90_set_temp8(struct lm90_data *data, int index, long val)
> +{
> +	mutex_lock(&data->update_lock);
> +	lm90_set_temp8_locked(data, index, val);
>   	mutex_unlock(&data->update_lock);
> -	return count;
>   }
>
> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> -			   char *buf)
> +static int lm90_get_temp11(struct lm90_data *data, int index)
>   {
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
>   	int temp;
>
> +	lm90_update(data);
> +
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		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 sprintf(buf, "%d\n", temp);
> +	return temp;
>   }
>
> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> -			  const char *buf, size_t count)
> +static void lm90_set_temp11_locked(struct lm90_data *data, int index, long val)
>   {
> -	struct {
> -		u8 high;
> -		u8 low;
> -		int channel;
> -	} reg[5] = {
> -		{ 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 },
> -		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 1 },
> -		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
> -	};

Another set of personal preference changes.

> -
> -	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> -	struct lm90_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	int nr = attr->nr;
> -	int index = attr->index;
> -	long val;
> -	int err;
> +	u8 high, low, channel;
>
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
> +#define SELECT_REGS(_reg, _channel) { \
> +	high = LM90_REG_W_##_reg##H; \
> +	low = LM90_REG_W_##_reg##L; \
> +	channel = _channel; \
> +}

We have been trying to get rid of function defines such as this one.
Most of the time it increases code size, and reduces readability.
Plus, it increases the amount of time we have to spend reviewing
the code to ensure it is correct.

> +	switch (index) {
> +	case REMOTE_LOW:    SELECT_REGS(REMOTE_LOW,  0); break;
> +	case REMOTE_HIGH:   SELECT_REGS(REMOTE_HIGH, 0); break;
> +	case REMOTE_OFFSET: SELECT_REGS(REMOTE_OFFS, 0); break;
> +	case REMOTE2_LOW:   SELECT_REGS(REMOTE_LOW,  1); break;
> +	case REMOTE2_HIGH:  SELECT_REGS(REMOTE_HIGH, 1); break;
> +	default: return;

... and, in this case, introduces checkpatch errors.

This patch is clearly more than what its headline says. It is
too complex to review as single patch. Please split into
logical changes, and explain why you make those changes.

Thanks,
Guenter


> +	}
> +#undef SELECT_REGS
>
>   	/* +16 degrees offset for temp2 for the LM99 */
>   	if (data->kind == lm99 && index <= 2)
>   		val -= 16000;
>
> -	mutex_lock(&data->update_lock);
>   	if (data->kind == adt7461 || data->kind == tmp451)
>   		data->temp11[index] = temp_to_u16_adt7461(data, val);
>   	else if (data->kind == max6646)
> @@ -887,54 +845,46 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>   	else
>   		data->temp11[index] = temp_to_s8(val) << 8;
>
> -	lm90_select_remote_channel(client, data, reg[nr].channel);
> -	i2c_smbus_write_byte_data(client, reg[nr].high,
> -				  data->temp11[index] >> 8);
> +	lm90_select_remote_channel(data, channel);
> +	i2c_smbus_write_byte_data(client, high,
> +			data->temp11[index] >> 8);
>   	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
> -		i2c_smbus_write_byte_data(client, reg[nr].low,
> -					  data->temp11[index] & 0xff);
> -	lm90_select_remote_channel(client, data, 0);
> +		i2c_smbus_write_byte_data(client, low,
> +				data->temp11[index] & 0xff);
> +	lm90_select_remote_channel(data, 0);
> +}
>
> +static void lm90_set_temp11(struct lm90_data *data, int index, long val)
> +{
> +	mutex_lock(&data->update_lock);
> +	lm90_set_temp11_locked(data, index, val);
>   	mutex_unlock(&data->update_lock);
> -	return count;
>   }
>
> -static ssize_t show_temphyst(struct device *dev,
> -			     struct device_attribute *devattr,
> -			     char *buf)
> +static ssize_t lm90_get_temphyst(struct lm90_data *data, int index)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
>   	int temp;
>
> +	lm90_update(data);
> +
>   	if (data->kind == adt7461 || data->kind == tmp451)
> -		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 sprintf(buf, "%d\n", temp - temp_from_s8(data->temp_hyst));
> +	return temp - temp_from_s8(data->temp_hyst);
>   }
>
> -static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
> -			    const char *buf, size_t count)
> +static void lm90_set_temphyst_locked(struct lm90_data *data, long val)
>   {
> -	struct lm90_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	long val;
> -	int err;
>   	int temp;
>
> -	err = kstrtol(buf, 10, &val);
> -	if (err < 0)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
>   	if (data->kind == adt7461 || data->kind == tmp451)
>   		temp = temp_from_u8_adt7461(data, data->temp8[LOCAL_CRIT]);
>   	else if (data->kind == max6646)
> @@ -943,43 +893,167 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
>   		temp = temp_from_s8(data->temp8[LOCAL_CRIT]);
>
>   	data->temp_hyst = hyst_to_reg(temp - val);
> -	i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
> -				  data->temp_hyst);
> +	i2c_smbus_write_byte_data(data->client, LM90_REG_W_TCRIT_HYST,
> +			data->temp_hyst);
> +}
> +
> +static void lm90_set_temphyst(struct lm90_data *data, long val)
> +{
> +	mutex_lock(&data->update_lock);
> +	lm90_set_temphyst_locked(data, val);
> +	mutex_unlock(&data->update_lock);
> +}
> +
> +static void lm90_set_convrate_locked(struct lm90_data *data, unsigned int val)
> +{
> +	int i;
> +	unsigned int update_interval;
> +
> +	/* Shift calculations to avoid rounding errors */
> +	val <<= 6;
> +
> +	/* find the nearest update rate */
> +	for (i = 0, update_interval = LM90_MAX_CONVRATE_MS << 6;
> +	     i < data->max_convrate; i++, update_interval >>= 1)
> +		if (val >= update_interval * 3 / 4)
> +			break;
> +
> +	i2c_smbus_write_byte_data(data->client, LM90_REG_W_CONVRATE, i);
> +	data->update_interval = DIV_ROUND_CLOSEST(update_interval, 64);
> +}
> +
> +static void lm90_set_convrate(struct lm90_data *data, unsigned int val)
> +{
> +	mutex_lock(&data->update_lock);
> +	lm90_set_convrate_locked(data, val);
>   	mutex_unlock(&data->update_lock);
> +}
> +
> +/*
> + * Sysfs stuff
> + */
> +
> +static ssize_t lm90_sysfs_show_temp8(
> +		struct device *dev, struct device_attribute *devattr,
> +		char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", lm90_get_temp8(data, attr->index));
> +}
> +
> +static ssize_t lm90_sysfs_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);
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	lm90_set_temp8(data, attr->index, val);
> +
>   	return count;
>   }
>
> -static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
> -			   char *buf)
> +static ssize_t lm90_sysfs_show_temp11(
> +		struct device *dev, struct device_attribute *devattr,
> +		char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", lm90_get_temp11(data, attr->index));
> +}
> +
> +static ssize_t lm90_sysfs_set_temp11(
> +		struct device *dev, struct device_attribute *devattr,
> +		const char *buf, size_t count)
>   {
> -	struct lm90_data *data = lm90_update_device(dev);
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	lm90_set_temp11(data, attr->index, val);
> +
> +	return count;
> +}
> +
> +static ssize_t lm90_sysfs_show_temphyst(
> +		struct device *dev, struct device_attribute *devattr,
> +		char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", lm90_get_temphyst(data, attr->index));
> +}
> +
> +static ssize_t lm90_sysfs_set_temphyst(
> +		struct device *dev, struct device_attribute *dummy,
> +		const char *buf, size_t count)
> +{
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +	long val;
> +	int err;
> +
> +	err = kstrtol(buf, 10, &val);
> +	if (err < 0)
> +		return err;
> +
> +	lm90_set_temphyst(data, val);
> +
> +	return count;
> +}
> +
> +static ssize_t lm90_sysfs_show_alarms(
> +		struct device *dev, struct device_attribute *dummy,
> +		char *buf)
> +{
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	lm90_update(data);
> +
>   	return sprintf(buf, "%d\n", data->alarms);
>   }
>
> -static ssize_t show_alarm(struct device *dev, struct device_attribute
> -			  *devattr, char *buf)
> +static ssize_t lm90_sysfs_show_alarm(
> +		struct device *dev, struct device_attribute *devattr,
> +		char *buf)
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct lm90_data *data = lm90_update_device(dev);
> +	struct lm90_data *data = dev_get_drvdata(dev);
>   	int bitnr = attr->index;
>
> +	lm90_update(data);
> +
>   	return sprintf(buf, "%d\n", (data->alarms >> bitnr) & 1);
>   }
>
> -static ssize_t show_update_interval(struct device *dev,
> -				    struct device_attribute *attr, char *buf)
> +static ssize_t lm90_sysfs_show_update_interval(
> +		struct device *dev, struct device_attribute *attr,
> +		char *buf)
>   {
>   	struct lm90_data *data = dev_get_drvdata(dev);
>
>   	return sprintf(buf, "%u\n", data->update_interval);
>   }
>
> -static ssize_t set_update_interval(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t count)
> +static ssize_t lm90_sysfs_set_update_interval(
> +		struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
>   {
>   	struct lm90_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	unsigned long val;
>   	int err;
>
> @@ -987,49 +1061,55 @@ static ssize_t set_update_interval(struct device *dev,
>   	if (err)
>   		return err;
>
> -	mutex_lock(&data->update_lock);
> -	lm90_set_convrate(client, data, clamp_val(val, 0, 100000));
> -	mutex_unlock(&data->update_lock);
> +	lm90_set_convrate(data, clamp_val(val, 0, 100000));
>
>   	return count;
>   }
>
> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
> -	0, LOCAL_TEMP);
> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
> -	0, REMOTE_TEMP);
> -static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, LOCAL_LOW);
> -static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 0, REMOTE_LOW);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, LOCAL_HIGH);
> -static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 1, REMOTE_HIGH);
> -static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, LOCAL_CRIT);
> -static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, REMOTE_CRIT);
> -static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
> -	set_temphyst, LOCAL_CRIT);
> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
> -	REMOTE_CRIT);
> -static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 2, REMOTE_OFFSET);
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> +	lm90_sysfs_show_temp11, NULL, LOCAL_TEMP);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO,
> +	lm90_sysfs_show_temp11, NULL, REMOTE_TEMP);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_LOW);
> +static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_LOW);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_HIGH);
> +static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_HIGH);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_CRIT);
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE_CRIT);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temphyst, lm90_sysfs_set_temphyst, LOCAL_CRIT);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO,
> +	lm90_sysfs_show_temphyst, NULL, REMOTE_CRIT);
> +static SENSOR_DEVICE_ATTR(temp2_offset, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE_OFFSET);
>
>   /* Individual alarm files */
> -static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
> -static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
> -static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL, 2);
> -static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO, show_alarm, NULL, 3);
> -static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO, show_alarm, NULL, 4);
> -static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 5);
> -static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 6);
>   /* Raw alarm file for compatibility */
> -static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
> +static DEVICE_ATTR(alarms, S_IRUGO,
> +	lm90_sysfs_show_alarms, NULL);
>
> -static DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR, show_update_interval,
> -		   set_update_interval);
> +static DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
> +	lm90_sysfs_show_update_interval, lm90_sysfs_set_update_interval);
>
>   static struct attribute *lm90_attributes[] = {
>   	&sensor_dev_attr_temp1_input.dev_attr.attr,
> @@ -1071,14 +1151,14 @@ static const struct attribute_group lm90_temp2_offset_group = {
>   /*
>    * Additional attributes for devices with emergency sensors
>    */
> -static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, LOCAL_EMERG);
> -static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, REMOTE_EMERG);
> -static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, LOCAL_EMERG);
> -static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, REMOTE_EMERG);
> +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, LOCAL_EMERG);
> +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE_EMERG);
> +static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO,
> +	lm90_sysfs_show_temphyst, NULL, LOCAL_EMERG);
> +static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO,
> +	lm90_sysfs_show_temphyst, NULL, REMOTE_EMERG);
>
>   static struct attribute *lm90_emergency_attributes[] = {
>   	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
> @@ -1092,8 +1172,10 @@ static const struct attribute_group lm90_emergency_group = {
>   	.attrs = lm90_emergency_attributes,
>   };
>
> -static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO, show_alarm, NULL, 15);
> -static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO, show_alarm, NULL, 13);
> +static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 15);
> +static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 13);
>
>   static struct attribute *lm90_emergency_alarm_attributes[] = {
>   	&sensor_dev_attr_temp1_emergency_alarm.dev_attr.attr,
> @@ -1108,26 +1190,31 @@ 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, REMOTE2_TEMP);
> -static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 3, REMOTE2_LOW);
> -static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 4, REMOTE2_HIGH);
> -static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, REMOTE2_CRIT);
> -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
> -	REMOTE2_CRIT);
> -static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> -	set_temp8, REMOTE2_EMERG);
> -static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
> -			  NULL, 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);
> -static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
> -static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
> -static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO, show_alarm, NULL, 14);
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO,
> +	lm90_sysfs_show_temp11, NULL, REMOTE2_TEMP);
> +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE2_LOW);
> +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp11, lm90_sysfs_set_temp11, REMOTE2_HIGH);
> +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE2_CRIT);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO,
> +	lm90_sysfs_show_temphyst, NULL, REMOTE2_CRIT);
> +static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_temp8, lm90_sysfs_set_temp8, REMOTE2_EMERG);
> +static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO,
> +	lm90_sysfs_show_temphyst, NULL, REMOTE2_EMERG);
> +
> +static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 12);
> +static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO,
> +	lm90_sysfs_show_alarm, NULL, 14);
>
>   static struct attribute *lm90_temp3_attributes[] = {
>   	&sensor_dev_attr_temp3_input.dev_attr.attr,
> @@ -1151,15 +1238,15 @@ static const struct attribute_group lm90_temp3_group = {
>   };
>
>   /* pec used for ADM1032 only */
> -static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
> -			char *buf)
> +static ssize_t lm90_sysfs_show_pec(struct device *dev,
> +		struct device_attribute *dummy, char *buf)
>   {
>   	struct i2c_client *client = to_i2c_client(dev);
>   	return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
>   }
>
> -static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
> -		       const char *buf, size_t count)
> +static ssize_t lm90_sysfs_set_pec(struct device *dev,
> +		struct device_attribute *dummy, const char *buf, size_t count)
>   {
>   	struct i2c_client *client = to_i2c_client(dev);
>   	long val;
> @@ -1183,7 +1270,8 @@ static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
>   	return count;
>   }
>
> -static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
> +static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO,
> +	lm90_sysfs_show_pec, lm90_sysfs_set_pec);
>
>   /*
>    * Real code
> @@ -1413,8 +1501,9 @@ static void lm90_restore_conf(struct i2c_client *client, struct lm90_data *data)
>   				  data->config_orig);
>   }
>
> -static void lm90_init_client(struct i2c_client *client, struct lm90_data *data)
> +static void lm90_init_client(struct lm90_data *data)
>   {
> +	struct i2c_client *client = data->client;
>   	u8 config, convrate;
>
>   	if (lm90_read_reg(client, LM90_REG_R_CONVRATE, &convrate) < 0) {
> @@ -1426,7 +1515,7 @@ static void lm90_init_client(struct i2c_client *client, struct lm90_data *data)
>   	/*
>   	 * Start the conversions.
>   	 */
> -	lm90_set_convrate(client, data, 500);	/* 500ms; 2Hz conversion rate */
> +	lm90_set_convrate_locked(data, 500);  /* 500ms / 2Hz  */
>   	if (lm90_read_reg(client, LM90_REG_R_CONFIG1, &config) < 0) {
>   		dev_warn(&client->dev, "Initialization failed!\n");
>   		return;
> @@ -1557,7 +1646,7 @@ static int lm90_probe(struct i2c_client *client,
>   	data->max_convrate = lm90_params[data->kind].max_convrate;
>
>   	/* Initialize the LM90 chip */
> -	lm90_init_client(client, data);
> +	lm90_init_client(data);
>
>   	/* Register sysfs hooks */
>   	data->groups[groups++] = &lm90_group;
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] lm90: separate register accessors from sysfs

[Stéphan Kochen]

Guenter,

Thanks for the taking the time to review!

You are absolutely right. I will split this up, more carefully check
coding style and simply do away with unnecessary changes.

Regarding:

On Fri, Jan 22, 2016 at 06:42:26AM -0800, Guenter Roeck wrote:
> On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> >+#define SELECT_REGS(_reg, _channel) { \
> >+	high = LM90_REG_W_##_reg##H; \
> >+	low = LM90_REG_W_##_reg##L; \
> >+	channel = _channel; \
> >+}
> 
> We have been trying to get rid of function defines such as this one.
> Most of the time it increases code size, and reduces readability.
> Plus, it increases the amount of time we have to spend reviewing
> the code to ensure it is correct.
> 
> >+	switch (index) {
> >+	case REMOTE_LOW:    SELECT_REGS(REMOTE_LOW,  0); break;
> >+	case REMOTE_HIGH:   SELECT_REGS(REMOTE_HIGH, 0); break;
> >+	case REMOTE_OFFSET: SELECT_REGS(REMOTE_OFFS, 0); break;
> >+	case REMOTE2_LOW:   SELECT_REGS(REMOTE_LOW,  1); break;
> >+	case REMOTE2_HIGH:  SELECT_REGS(REMOTE_HIGH, 1); break;
> >+	default: return;
> 
> ... and, in this case, introduces checkpatch errors.

Should I simply unfold the macro here? Another option would be an array
of structs like in set_temp8, but it'd have some gaps here. Or is some
other construct preferred?

I guess this is where I snuck in another change (which I will separate),
to replace SENSOR_DEVICE_ATTR_2 with regular ATTR. The setter args of
_ATTR_2 were using literal numbers to point into an array defined in the
setter function. (Maybe to avoid exactly the kind of thing I built
here.)

Would it be an idea to combine temp8 and temp11 variants, and let the
getters and setters figure out what kind of register access to do based
on register index?

Thanks again,

-- 
Stéphan Kochen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [lm-sensors] [PATCH 1/3] lm90: separate register accessors from sysfs

[Jean Delvare]

Hi Stéphan and Guenter,

On Fri, 22 Jan 2016 23:00:57 +0100, Stéphan Kochen wrote:
> Guenter,
> 
> Thanks for the taking the time to review!
> 
> You are absolutely right. I will split this up, more carefully check
> coding style and simply do away with unnecessary changes.

Yes, thanks Guenter for the initial review.

> Regarding:
> 
> On Fri, Jan 22, 2016 at 06:42:26AM -0800, Guenter Roeck wrote:
> > On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> > >+#define SELECT_REGS(_reg, _channel) { \
> > >+	high = LM90_REG_W_##_reg##H; \
> > >+	low = LM90_REG_W_##_reg##L; \
> > >+	channel = _channel; \
> > >+}
> > 
> > We have been trying to get rid of function defines such as this one.
> > Most of the time it increases code size, and reduces readability.
> > Plus, it increases the amount of time we have to spend reviewing
> > the code to ensure it is correct.
> > 
> > >+	switch (index) {
> > >+	case REMOTE_LOW:    SELECT_REGS(REMOTE_LOW,  0); break;
> > >+	case REMOTE_HIGH:   SELECT_REGS(REMOTE_HIGH, 0); break;
> > >+	case REMOTE_OFFSET: SELECT_REGS(REMOTE_OFFS, 0); break;
> > >+	case REMOTE2_LOW:   SELECT_REGS(REMOTE_LOW,  1); break;
> > >+	case REMOTE2_HIGH:  SELECT_REGS(REMOTE_HIGH, 1); break;
> > >+	default: return;
> > 
> > ... and, in this case, introduces checkpatch errors.
> 
> Should I simply unfold the macro here? Another option would be an array
> of structs like in set_temp8, but it'd have some gaps here. Or is some
> other construct preferred?

Anything will be better than macro-generated functions.

> I guess this is where I snuck in another change (which I will separate),
> to replace SENSOR_DEVICE_ATTR_2 with regular ATTR. The setter args of
> _ATTR_2 were using literal numbers to point into an array defined in the
> setter function. (Maybe to avoid exactly the kind of thing I built
> here.)
> 
> Would it be an idea to combine temp8 and temp11 variants, and let the
> getters and setters figure out what kind of register access to do based
> on register index?

Can't answer that as long as I don't know which problem you are trying
to solve. Hopefully this will become clearer once the changes are
cleanly separated into incremental steps.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] lm90: initialize parameters from devicetree

[Stéphan Kochen]

Allow a device tree to set initial temperature sensor parameters.

Userspace can still override actual values through sysfs.

Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
---
 Documentation/devicetree/bindings/hwmon/lm90.txt | 40 +++++++++++++++++
 drivers/hwmon/lm90.c                             | 55 ++++++++++++++++++++++--
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
index e863248..045e94b 100644
--- a/Documentation/devicetree/bindings/hwmon/lm90.txt
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -33,6 +33,38 @@ Optional properties:
               LM90 "-ALERT" pin output.
               See interrupt-controller/interrupts.txt for the format.
 
+- update-interval: Interval at which temperatures are sampled,
+  Type: unsigned   in milliseconds.
+  Size: one cell
+
+- local-low:      Valid temperature range for the chip internal sensor,
+  local-high:     outside which the alert will be set. Values are in
+  local-critical: millicelcius.
+  Type: signed
+  Size: one cell
+
+- remote-low:      Valid temperature range for the external sensor,
+  remote-high:     outside which the alert will be set. Values are in
+  remote-critical: millicelciius.
+  Type: signed
+  Size: one cell
+
+- remote-offset:   Where available, an external sensor temperature offset.
+  Type: signed
+  Size: one cell
+
+- local-emergency:  On max6659, max6695 and max6696, a configurable
+  remote-emergency: 3rd upper bound on temperature.
+  Type: signed
+  Size: one cell
+
+- remote2-low:      On max6695 and max6696, a second external sensor.
+  remote2-high:
+  remote2-critical:
+  remote2-emergency:
+  Type: signed
+  Size: one cell
+
 Example LM90 node:
 
 temp-sensor {
@@ -41,4 +73,12 @@ temp-sensor {
 	vcc-supply = <&palmas_ldo6_reg>;
 	interrupt-parent = <&gpio>;
 	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
+	update-interval = <500>;
+	local-low = <5000>;
+	local-high = <80000>;
+	local-critical = <90000>;
+	remote-low = <5000>;
+	remote-high = <80000>;
+	remote-critical = <90000>;
+	remote-offset = <(-62125)>;
 }
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 88daf72..8ae8791 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -93,6 +93,7 @@
 #include <linux/hwmon.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
@@ -1504,8 +1505,16 @@ static void lm90_restore_conf(struct i2c_client *client, struct lm90_data *data)
 static void lm90_init_client(struct lm90_data *data)
 {
 	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 	u8 config, convrate;
+	u32 ms;
+#ifdef CONFIG_OF
+	s32 temp;
+#endif
 
+	/*
+	 * Save old conversion rate.
+	 */
 	if (lm90_read_reg(client, LM90_REG_R_CONVRATE, &convrate) < 0) {
 		dev_warn(&client->dev, "Failed to read convrate register!\n");
 		convrate = LM90_DEF_CONVRATE_RVAL;
@@ -1515,14 +1524,24 @@ static void lm90_init_client(struct lm90_data *data)
 	/*
 	 * Start the conversions.
 	 */
-	lm90_set_convrate_locked(data, 500);  /* 500ms / 2Hz  */
+#ifdef CONFIG_OF
+	if (of_property_read_u32(dev->of_node, "update-interval", &ms) < 0)
+#endif
+		ms = 500;  /* default rate: 2Hz */
+	lm90_set_convrate_locked(data, ms);
+
+	/*
+	 * Save old config
+	 */
 	if (lm90_read_reg(client, LM90_REG_R_CONFIG1, &config) < 0) {
-		dev_warn(&client->dev, "Initialization failed!\n");
+		dev_warn(dev, "Initialization failed!\n");
 		return;
 	}
 	data->config_orig = config;
 
-	/* Check Temperature Range Select */
+	/*
+	 * Check Temperature Range Select
+	 */
 	if (data->kind == adt7461 || data->kind == tmp451) {
 		if (config & 0x04)
 			data->flags |= LM90_FLAG_ADT7461_EXT;
@@ -1545,6 +1564,36 @@ static void lm90_init_client(struct lm90_data *data)
 	config &= 0xBF;	/* run */
 	if (config != data->config_orig) /* Only write if changed */
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
+
+#ifdef CONFIG_OF
+	/*
+	 * Set initial values from devicetree
+	 */
+#define INIT_REG(_property, _index, _bits) { \
+	if (of_property_read_s32(dev->of_node, _property, &temp) == 0) \
+		lm90_set_temp##_bits##_locked(data, _index, temp); \
+}
+	INIT_REG("local-low", LOCAL_LOW, 8);
+	INIT_REG("local-high", LOCAL_HIGH, 8);
+	INIT_REG("local-critical", LOCAL_CRIT, 8);
+	INIT_REG("remote-low", REMOTE_LOW, 11);
+	INIT_REG("remote-high", REMOTE_HIGH, 11);
+	INIT_REG("remote-critical", REMOTE_CRIT, 8);
+	if (data->flags & LM90_HAVE_OFFSET) {
+		INIT_REG("remote-offset", REMOTE_OFFSET, 11);
+	}
+	if (data->flags & LM90_HAVE_EMERGENCY) {
+		INIT_REG("local-emergency", LOCAL_EMERG, 8);
+		INIT_REG("remote-emergency", REMOTE_EMERG, 8);
+	}
+	if (data->flags & LM90_HAVE_TEMP3) {
+		INIT_REG("remote2-low", REMOTE2_LOW, 11);
+		INIT_REG("remote2-high", REMOTE2_HIGH, 11);
+		INIT_REG("remote2-critical", REMOTE2_CRIT, 8);
+		INIT_REG("remote2-emergency", REMOTE2_EMERG, 8);
+	}
+#undef INIT_REG
+#endif
 }
 
 static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] lm90: initialize parameters from devicetree

[Guenter Roeck]

On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> Allow a device tree to set initial temperature sensor parameters.
>
> Userspace can still override actual values through sysfs.
>
> Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
> ---
>   Documentation/devicetree/bindings/hwmon/lm90.txt | 40 +++++++++++++++++
>   drivers/hwmon/lm90.c                             | 55 ++++++++++++++++++++++--
>   2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
> index e863248..045e94b 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm90.txt
> +++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
> @@ -33,6 +33,38 @@ Optional properties:
>                 LM90 "-ALERT" pin output.
>                 See interrupt-controller/interrupts.txt for the format.
>
> +- update-interval: Interval at which temperatures are sampled,
> +  Type: unsigned   in milliseconds.
> +  Size: one cell
> +
> +- local-low:      Valid temperature range for the chip internal sensor,
> +  local-high:     outside which the alert will be set. Values are in
> +  local-critical: millicelcius.
> +  Type: signed
> +  Size: one cell
> +
> +- remote-low:      Valid temperature range for the external sensor,
> +  remote-high:     outside which the alert will be set. Values are in
> +  remote-critical: millicelciius.
> +  Type: signed
> +  Size: one cell
> +
> +- remote-offset:   Where available, an external sensor temperature offset.
> +  Type: signed
> +  Size: one cell
> +
> +- local-emergency:  On max6659, max6695 and max6696, a configurable
> +  remote-emergency: 3rd upper bound on temperature.
> +  Type: signed
> +  Size: one cell
> +
> +- remote2-low:      On max6695 and max6696, a second external sensor.
> +  remote2-high:
> +  remote2-critical:
> +  remote2-emergency:
> +  Type: signed
> +  Size: one cell
> +

This very much smells like configuration, not hardware description.

Having said that, the thermal subsystem does something similar. This raises even more
questions, though. Since patch 3 of the series introduces registration with the thermal
subsystem, it seems to me that the thermal subsystem should provide any limits used
to program the chips, and that there should be a mechanism for the thermal subsystem
to interact with the driver to both set the limits and to be informed if a limit
is exceeded.

Also, _if_ such a set of properties is introduced and accepted by the devicetree
reviewers, it should probably be a set of properties which applies to _all_ hardware
monitoring drivers, not just to one. Even if support is not implemented immediately
in the hwmon core, the properties should be usable in a generic way, and not
reflect sysfs attribute names used by the hwmon subsystem.

Thanks,
Guenter

>   Example LM90 node:
>
>   temp-sensor {
> @@ -41,4 +73,12 @@ temp-sensor {
>   	vcc-supply = <&palmas_ldo6_reg>;
>   	interrupt-parent = <&gpio>;
>   	interrupts = <TEGRA_GPIO(O, 4) IRQ_TYPE_LEVEL_LOW>;
> +	update-interval = <500>;
> +	local-low = <5000>;
> +	local-high = <80000>;
> +	local-critical = <90000>;
> +	remote-low = <5000>;
> +	remote-high = <80000>;
> +	remote-critical = <90000>;
> +	remote-offset = <(-62125)>;
>   }
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 88daf72..8ae8791 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -93,6 +93,7 @@
>   #include <linux/hwmon.h>
>   #include <linux/err.h>
>   #include <linux/mutex.h>
> +#include <linux/of.h>
>   #include <linux/sysfs.h>
>   #include <linux/interrupt.h>
>   #include <linux/regulator/consumer.h>
> @@ -1504,8 +1505,16 @@ static void lm90_restore_conf(struct i2c_client *client, struct lm90_data *data)
>   static void lm90_init_client(struct lm90_data *data)
>   {
>   	struct i2c_client *client = data->client;
> +	struct device *dev = &client->dev;
>   	u8 config, convrate;
> +	u32 ms;
> +#ifdef CONFIG_OF
> +	s32 temp;
> +#endif
>
> +	/*
> +	 * Save old conversion rate.
> +	 */
>   	if (lm90_read_reg(client, LM90_REG_R_CONVRATE, &convrate) < 0) {
>   		dev_warn(&client->dev, "Failed to read convrate register!\n");
>   		convrate = LM90_DEF_CONVRATE_RVAL;
> @@ -1515,14 +1524,24 @@ static void lm90_init_client(struct lm90_data *data)
>   	/*
>   	 * Start the conversions.
>   	 */
> -	lm90_set_convrate_locked(data, 500);  /* 500ms / 2Hz  */
> +#ifdef CONFIG_OF
> +	if (of_property_read_u32(dev->of_node, "update-interval", &ms) < 0)
> +#endif
> +		ms = 500;  /* default rate: 2Hz */
> +	lm90_set_convrate_locked(data, ms);
> +
> +	/*
> +	 * Save old config
> +	 */
>   	if (lm90_read_reg(client, LM90_REG_R_CONFIG1, &config) < 0) {
> -		dev_warn(&client->dev, "Initialization failed!\n");
> +		dev_warn(dev, "Initialization failed!\n");
>   		return;
>   	}
>   	data->config_orig = config;
>
> -	/* Check Temperature Range Select */
> +	/*
> +	 * Check Temperature Range Select
> +	 */
>   	if (data->kind == adt7461 || data->kind == tmp451) {
>   		if (config & 0x04)
>   			data->flags |= LM90_FLAG_ADT7461_EXT;
> @@ -1545,6 +1564,36 @@ static void lm90_init_client(struct lm90_data *data)
>   	config &= 0xBF;	/* run */
>   	if (config != data->config_orig) /* Only write if changed */
>   		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> +
> +#ifdef CONFIG_OF
> +	/*
> +	 * Set initial values from devicetree
> +	 */
> +#define INIT_REG(_property, _index, _bits) { \
> +	if (of_property_read_s32(dev->of_node, _property, &temp) == 0) \
> +		lm90_set_temp##_bits##_locked(data, _index, temp); \
> +}
> +	INIT_REG("local-low", LOCAL_LOW, 8);
> +	INIT_REG("local-high", LOCAL_HIGH, 8);
> +	INIT_REG("local-critical", LOCAL_CRIT, 8);
> +	INIT_REG("remote-low", REMOTE_LOW, 11);
> +	INIT_REG("remote-high", REMOTE_HIGH, 11);
> +	INIT_REG("remote-critical", REMOTE_CRIT, 8);
> +	if (data->flags & LM90_HAVE_OFFSET) {
> +		INIT_REG("remote-offset", REMOTE_OFFSET, 11);
> +	}
> +	if (data->flags & LM90_HAVE_EMERGENCY) {
> +		INIT_REG("local-emergency", LOCAL_EMERG, 8);
> +		INIT_REG("remote-emergency", REMOTE_EMERG, 8);
> +	}
> +	if (data->flags & LM90_HAVE_TEMP3) {
> +		INIT_REG("remote2-low", REMOTE2_LOW, 11);
> +		INIT_REG("remote2-high", REMOTE2_HIGH, 11);
> +		INIT_REG("remote2-critical", REMOTE2_CRIT, 8);
> +		INIT_REG("remote2-emergency", REMOTE2_EMERG, 8);
> +	}
> +#undef INIT_REG
> +#endif
>   }
>
>   static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] lm90: initialize parameters from devicetree

[Stéphan Kochen]

On Fri, Jan 22, 2016 at 06:53:44AM -0800, Guenter Roeck wrote:
> On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> >Allow a device tree to set initial temperature sensor parameters.
> 
> This very much smells like configuration, not hardware description.
> 
> Having said that, the thermal subsystem does something similar. This raises even more
> questions, though. Since patch 3 of the series introduces registration with the thermal
> subsystem, it seems to me that the thermal subsystem should provide any limits used
> to program the chips, and that there should be a mechanism for the thermal subsystem
> to interact with the driver to both set the limits and to be informed if a limit
> is exceeded.
> 
> Also, _if_ such a set of properties is introduced and accepted by the devicetree
> reviewers, it should probably be a set of properties which applies to _all_ hardware
> monitoring drivers, not just to one. Even if support is not implemented immediately
> in the hwmon core, the properties should be usable in a generic way, and not
> reflect sysfs attribute names used by the hwmon subsystem.

The original kernel for Ouya is a 3.1 kernel from nVidia's old
Linux4Tegra branches. This kernel had its own Tegra-specific thermal
zone support which seems to do more or less that: the thermal zone
system actually uses the sensor alert interrupts to detect when it needs
to act, and reconfigures alert bounds as the temperature slides to
another range it wants to monitor.

Thermal zones in current master only have the ability to check sensor
values at an interval.

I'd agree that update-interval and the low/high bounds could be
controlled by the thermal zone system. I'm less certain what to do with
the critical/emergency bounds, and the remote-offset.

Thing about Ouya is that, even with the thermal zone currently working,
the alert bounds set when the system comes up seem to be crippling the
system with interrupts shortly after the sensor is activated. So they
have to be set to something sane. Hence why I added both.

I'm not sure if you're expecting me to do the work of extending thermal
zones. I guess I'd need to figure out how a sensor would notify a
thermal zone of an alert, and implement that functionality while staying
backwards compatible.

Thanks again,

-- 
Stéphan Kochen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] lm90: initialize parameters from devicetree

[Guenter Roeck]

On 01/22/2016 02:32 PM, Stéphan Kochen wrote:
> On Fri, Jan 22, 2016 at 06:53:44AM -0800, Guenter Roeck wrote:
>> On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
>>> Allow a device tree to set initial temperature sensor parameters.
>>
>> This very much smells like configuration, not hardware description.
>>
>> Having said that, the thermal subsystem does something similar. This raises even more
>> questions, though. Since patch 3 of the series introduces registration with the thermal
>> subsystem, it seems to me that the thermal subsystem should provide any limits used
>> to program the chips, and that there should be a mechanism for the thermal subsystem
>> to interact with the driver to both set the limits and to be informed if a limit
>> is exceeded.
>>
>> Also, _if_ such a set of properties is introduced and accepted by the devicetree
>> reviewers, it should probably be a set of properties which applies to _all_ hardware
>> monitoring drivers, not just to one. Even if support is not implemented immediately
>> in the hwmon core, the properties should be usable in a generic way, and not
>> reflect sysfs attribute names used by the hwmon subsystem.
>
> The original kernel for Ouya is a 3.1 kernel from nVidia's old
> Linux4Tegra branches. This kernel had its own Tegra-specific thermal
> zone support which seems to do more or less that: the thermal zone
> system actually uses the sensor alert interrupts to detect when it needs
> to act, and reconfigures alert bounds as the temperature slides to
> another range it wants to monitor.
>
> Thermal zones in current master only have the ability to check sensor
> values at an interval.
>
> I'd agree that update-interval and the low/high bounds could be
> controlled by the thermal zone system. I'm less certain what to do with
> the critical/emergency bounds, and the remote-offset.
>
> Thing about Ouya is that, even with the thermal zone currently working,
> the alert bounds set when the system comes up seem to be crippling the
> system with interrupts shortly after the sensor is activated. So they
> have to be set to something sane. Hence why I added both.
>
> I'm not sure if you're expecting me to do the work of extending thermal
> zones. I guess I'd need to figure out how a sensor would notify a
> thermal zone of an alert, and implement that functionality while staying
> backwards compatible.
>
The thermal subsystem supports setting trip points, which I would think
is what you are looking for here. Question is if an how you can use the
information from the thermal subsystem (and thus the thermal zone configuration)
to set the various limits in the lm90 driver. This should hopefully be sufficient
to fix your immediate problem. For handling alerts, I guess we'll have to wait for
thermal subsystem improvements (unless of course you volunteer to do that work.

Copying the the linux-pm mailing list and thermal maintainers for additional advise.

Thanks,
Guenter

Interrupt driven thermal OF sensors (was: [PATCH 2/3] lm90: initialize parameters from devicetree)

[Stéphan Kochen]

On Fri, Jan 22, 2016 at 06:53:50PM -0800, Guenter Roeck wrote:
> The thermal subsystem supports setting trip points, which I would think
> is what you are looking for here. Question is if an how you can use the
> information from the thermal subsystem (and thus the thermal zone configuration)
> to set the various limits in the lm90 driver. This should hopefully be sufficient
> to fix your immediate problem. For handling alerts, I guess we'll have to wait for
> thermal subsystem improvements (unless of course you volunteer to do that work.

I may take a shot at this. So in short, the goal is to have a device
tree thermal zone communicate trip points to the sensor driver, and use
interrupts to act on trips.

(This indirectly solves my problem of my sensor having weird initial
values. Perhaps we also want a solution for this case if the thermal
subsystem is disabled, but for me there'd be no need.)

Here's what I see:

 - The thermal core layer already supports interrupt driven systems.
   Support is missing from thermal OF, the layer between thermal core
   and the sensor driver implementing device tree support.

 - Thermal OF registers a device in thermal core for each zone defined
   in the device tree.

 - In theory, a thermal zone in the device tree can have multiple
   sensors, and multiple zones can refer to the same sensor, but the
   current implementation only supports 1-on-1 relations.

 - There are already exports thermal_zone_device_update and
   thermal_notify_framework in thermal core, which allow external code
   to trigger an update.

 - Updates happen by explicit calls to such exports, or by an optional
   and configurable interval in thermal core.

What I think we want:

 - Any additions should be optional extensions implemented by sensor
   drivers. Polling should keep on working for all sensor drivers
   already supporting thermal OF, with no interface changes.

 - For interrupt-capable sensor drivers, the thermal OF device should
   keep the sensor driver updated with the current nearest trip
   temperature and hysteresis. (Don't burden drivers with a full list of
   trip points.)

 - In the case of LM90, this would set the low and high alert
   temperatures. LM90 can have additional alerts (critical, emergency),
   but a sensor driver registered with thermal OF should disable any
   additional alerts.

 - Similarly, a sensor driver should disable alerts when there is no
   current trip temperature or hysteresis. (E.g., we're below the lowest
   trip point.)

Implementation-wise:

 - The struct thermal_zone_of_device_ops needs an additional function to
   set the current sensor trip temperature and hysteresis. Presence of
   this function indicates the sensor driver has interrupt support.

 - The thermal OF device will call this function every time the
   temperature slides across trip points. (Or when trip points are
   altered.)

 - The thermal OF device should ignore the polling delay (set it to 0)
   if its sensor has interrupt support. (In this case, we can also make
   polling-delay optional. It's currently required in the device tree.)

 - On interrupt, the sensor driver should call
   thermal_zone_device_update with its thermal_zone_device, as returned
   by thermal_zone_of_sensor_register.

My only concern is that I don't understand kernel contexts and interrupt
handling well enough. It looks like at least its up to the sensor driver
to ensure calls into the thermal subsystem have long left the hardware
interrupt context, which I think should be sufficient.

Hoping all of this sounds about right!

-- 
Stéphan Kochen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Interrupt driven thermal OF sensors

[Guenter Roeck]

On 01/27/2016 01:18 PM, Stéphan Kochen wrote:
> On Fri, Jan 22, 2016 at 06:53:50PM -0800, Guenter Roeck wrote:
>> The thermal subsystem supports setting trip points, which I would think
>> is what you are looking for here. Question is if an how you can use the
>> information from the thermal subsystem (and thus the thermal zone configuration)
>> to set the various limits in the lm90 driver. This should hopefully be sufficient
>> to fix your immediate problem. For handling alerts, I guess we'll have to wait for
>> thermal subsystem improvements (unless of course you volunteer to do that work.
>
> I may take a shot at this. So in short, the goal is to have a device
> tree thermal zone communicate trip points to the sensor driver, and use
> interrupts to act on trips.
>
> (This indirectly solves my problem of my sensor having weird initial
> values. Perhaps we also want a solution for this case if the thermal
> subsystem is disabled, but for me there'd be no need.)
>
> Here's what I see:
>
>   - The thermal core layer already supports interrupt driven systems.
>     Support is missing from thermal OF, the layer between thermal core
>     and the sensor driver implementing device tree support.
>
>   - Thermal OF registers a device in thermal core for each zone defined
>     in the device tree.
>
>   - In theory, a thermal zone in the device tree can have multiple
>     sensors, and multiple zones can refer to the same sensor, but the
>     current implementation only supports 1-on-1 relations.
>
>   - There are already exports thermal_zone_device_update and
>     thermal_notify_framework in thermal core, which allow external code
>     to trigger an update.
>
>   - Updates happen by explicit calls to such exports, or by an optional
>     and configurable interval in thermal core.
>
> What I think we want:
>
>   - Any additions should be optional extensions implemented by sensor
>     drivers. Polling should keep on working for all sensor drivers
>     already supporting thermal OF, with no interface changes.
>
>   - For interrupt-capable sensor drivers, the thermal OF device should
>     keep the sensor driver updated with the current nearest trip
>     temperature and hysteresis. (Don't burden drivers with a full list of
>     trip points.)
>
>   - In the case of LM90, this would set the low and high alert
>     temperatures. LM90 can have additional alerts (critical, emergency),
>     but a sensor driver registered with thermal OF should disable any
>     additional alerts.
>
... and thus disable any external chip signals associated with those limits.

>   - Similarly, a sensor driver should disable alerts when there is no
>     current trip temperature or hysteresis. (E.g., we're below the lowest
>     trip point.)
>

The idea with most if not all temperature sensors is that multiple trip points
would be configured as multiple limits in the chip. Often those limits,
when reached, are tied to pins to cause a specific action (eg to cause
an interrupt, turn on a fan, or shut down the hardware). In effect, you
suggest to re-define this mechanism and, for all practical purposes,
use just one of the limits provided by the chip(s).

Personally I don't think that would be a good idea, because it would have
impact on hardware design. It would effectively limit the use of the thermal
subsystem with temperature sensor chips to hardware designs which take
the thermal subsystem's expectations and assumptions into account.
At the same time, it would for all practical purposes mandate the use
of the thermal subsystem on such systems, because the hardware would depend
on the thermal subsystem's implementation to control the temperature
in the system.

While it may be feasible in some situations to have the thermal subsystem
dynamically set free-moving limits, there are many other situations where
those limits are tied to hardware responses, and the limits need to be static.

Maybe this is just a different world view. The thermal subsystem may see the
world assuming that it always has an unlimited number of trip points available,
and the chip it supports only support lower and upper boundaries (or trip points),
which can be set and changed as needed. This is somewhat different to the
traditional world view, implemented not only in many temperature sensors,
but also in fan controller or Super-IO chips, where a set of temperatures
is programmed into the chip only once. I hope that it is possible to support
both mechanisms.

Thanks,
Guenter

> Implementation-wise:
>
>   - The struct thermal_zone_of_device_ops needs an additional function to
>     set the current sensor trip temperature and hysteresis. Presence of
>     this function indicates the sensor driver has interrupt support.
>
>   - The thermal OF device will call this function every time the
>     temperature slides across trip points. (Or when trip points are
>     altered.)
>
>   - The thermal OF device should ignore the polling delay (set it to 0)
>     if its sensor has interrupt support. (In this case, we can also make
>     polling-delay optional. It's currently required in the device tree.)
>
>   - On interrupt, the sensor driver should call
>     thermal_zone_device_update with its thermal_zone_device, as returned
>     by thermal_zone_of_sensor_register.
>
> My only concern is that I don't understand kernel contexts and interrupt
> handling well enough. It looks like at least its up to the sensor driver
> to ensure calls into the thermal subsystem have long left the hardware
> interrupt context, which I think should be sufficient.
>
> Hoping all of this sounds about right!
>

Re: Interrupt driven thermal OF sensors

[Stéphan Kochen]

On Wed, Jan 27, 2016 at 09:05:08PM -0800, Guenter Roeck wrote:
> ... and thus disable any external chip signals associated with those limits.
> 
> The idea with most if not all temperature sensors is that multiple trip points
> would be configured as multiple limits in the chip. Often those limits,
> when reached, are tied to pins to cause a specific action (eg to cause
> an interrupt, turn on a fan, or shut down the hardware). In effect, you
> suggest to re-define this mechanism and, for all practical purposes,
> use just one of the limits provided by the chip(s).
> 
> Personally I don't think that would be a good idea, because it would have
> impact on hardware design. It would effectively limit the use of the thermal
> subsystem with temperature sensor chips to hardware designs which take
> the thermal subsystem's expectations and assumptions into account.
> At the same time, it would for all practical purposes mandate the use
> of the thermal subsystem on such systems, because the hardware would depend
> on the thermal subsystem's implementation to control the temperature
> in the system.

Ah, whoops. That makes a lot of sense. And I think I even saw the
poweroff in action once, when I misconfigured the temperature offset
register.

> While it may be feasible in some situations to have the thermal subsystem
> dynamically set free-moving limits, there are many other situations where
> those limits are tied to hardware responses, and the limits need to be static.
> 
> Maybe this is just a different world view. The thermal subsystem may see the
> world assuming that it always has an unlimited number of trip points available,
> and the chip it supports only support lower and upper boundaries (or trip points),
> which can be set and changed as needed. This is somewhat different to the
> traditional world view, implemented not only in many temperature sensors,
> but also in fan controller or Super-IO chips, where a set of temperatures
> is programmed into the chip only once. I hope that it is possible to support
> both mechanisms.

That would be nice.

For starters, I think it should be possible for a driver to enable
thermal subsystem stuff only if an of_node is present. (Though I haven't
checked what that's set to with `status = "disabled"` in the device
tree. Hopefully NULL.)

So, one option is to, instead of thermal OF telling the driver what the
limits are, we simply have thermal OF only notify the driver of trip
changes, and the driver does all the work of inspecting defined trips.

In the LM90 case, it can set low/high and critical alerts nicely based
on trip types (critical or not). But, for example, MAX6696 has an
additional 'emergency' alert. Should that just be left untouched?

Either way, this means hardcoding thermal trip to sensor alert mapping
in the driver. A more elaborate solution would be to define that mapping
in the device tree, and have thermal OF handle it, instructing the
driver to set specific alerts. (And then the device tree author may
choose to leave certain sensor alerts alone.)

-- 
Stéphan Kochen

[PATCH 3/3] lm90: register with thermal zones

[Stéphan Kochen]

Any lm90-driven node in the device tree can now be referenced in thermal
zones. One thermal zone device is registered for each sensor present.

Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
---
 Documentation/devicetree/bindings/hwmon/lm90.txt |  8 ++++
 drivers/hwmon/lm90.c                             | 54 ++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
index 045e94b..da9aae7 100644
--- a/Documentation/devicetree/bindings/hwmon/lm90.txt
+++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
@@ -28,6 +28,13 @@ Required node properties:
 
 - vcc-supply: vcc regulator for the supply voltage.
 
+- #thermal-sensor-cells: Should be 1. See ../../thermal/thermal.txt for a
+                         description of this property. Use the following
+                         values to refer to a specific sensor:
+                         0: chip internal sensor
+                         1: external sensor
+                         2: second external sensor, if present
+
 Optional properties:
 - interrupts: Contains a single interrupt specifier which describes the
               LM90 "-ALERT" pin output.
@@ -81,4 +88,5 @@ temp-sensor {
 	remote-high = <80000>;
 	remote-critical = <90000>;
 	remote-offset = <(-62125)>;
+	#thermal-sensor-cells = <1>;
 }
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8ae8791..4577916 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -95,6 +95,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/sysfs.h>
+#include <linux/thermal.h>
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
 
@@ -207,6 +208,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define MAX6696_STATUS2_R2OT2	(1 << 6) /* remote2 emergency limit tripped */
 #define MAX6696_STATUS2_LOT2	(1 << 7) /* local emergency limit tripped */
 
+#define LM90_NUM_SENSORS(d) (((d)->flags & LM90_HAVE_TEMP3) ? 3 : 2)
+
 /*
  * Driver data (common to all clients)
  */
@@ -365,6 +368,13 @@ enum lm90_temp11_reg_index {
  * Client data (each client gets its own)
  */
 
+struct lm90_sensor {
+	struct lm90_data *data;
+	int index;
+
+	struct thermal_zone_device *tz;
+};
+
 struct lm90_data {
 	struct i2c_client *client;
 	struct device *hwmon_dev;
@@ -390,6 +400,10 @@ struct lm90_data {
 	s16 temp11[TEMP11_REG_NUM];
 	u8 temp_hyst;
 	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
+
+#ifdef CONFIG_THERMAL_OF
+	struct lm90_sensor sensors[3];
+#endif
 };
 
 /*
@@ -1275,6 +1289,23 @@ static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO,
 	lm90_sysfs_show_pec, lm90_sysfs_set_pec);
 
 /*
+ * Thermal zone code
+ */
+
+#ifdef CONFIG_THERMAL_OF
+static int lm90_of_get_temp(void *data, int *out_temp)
+{
+	struct lm90_sensor *sensor = data;
+	*out_temp = lm90_get_temp11(sensor->data, sensor->index);
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops lm90_of_thermal_ops = {
+	.get_temp = lm90_of_get_temp,
+};
+#endif
+
+/*
  * Real code
  */
 
@@ -1653,6 +1684,7 @@ static int lm90_probe(struct i2c_client *client,
 	struct regulator *regulator;
 	int groups = 0;
 	int err;
+	unsigned int i;
 
 	regulator = devm_regulator_get(dev, "vcc");
 	if (IS_ERR(regulator))
@@ -1737,6 +1769,20 @@ static int lm90_probe(struct i2c_client *client,
 		}
 	}
 
+#ifdef CONFIG_THERMAL_OF
+	data->sensors[0].index = LOCAL_TEMP;
+	data->sensors[1].index = REMOTE_TEMP;
+	data->sensors[2].index = REMOTE2_TEMP;
+	for (i = 0; i < LM90_NUM_SENSORS(data); i++) {
+		data->sensors[i].data = data;
+		data->sensors[i].tz = thermal_zone_of_sensor_register(
+			dev, i, &data->sensors[i],
+			&lm90_of_thermal_ops);
+		if (IS_ERR(data->sensors[i].tz))
+			data->sensors[i].tz = NULL;
+	}
+#endif
+
 	return 0;
 
 exit_unregister:
@@ -1753,6 +1799,14 @@ exit_restore:
 static int lm90_remove(struct i2c_client *client)
 {
 	struct lm90_data *data = i2c_get_clientdata(client);
+#ifdef CONFIG_THERMAL_OF
+	unsigned int i;
+
+	for (i = 0; i < LM90_NUM_SENSORS(data); i++) {
+		thermal_zone_of_sensor_unregister(&client->dev,
+						  data->sensors[i].tz);
+	}
+#endif
 
 	hwmon_device_unregister(data->hwmon_dev);
 	device_remove_file(&client->dev, &dev_attr_pec);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] lm90: register with thermal zones

[Rob Herring]

On Thu, Jan 21, 2016 at 08:34:37PM +0100, Stéphan Kochen wrote:
> Any lm90-driven node in the device tree can now be referenced in thermal
> zones. One thermal zone device is registered for each sensor present.
> 
> Signed-off-by: Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/hwmon/lm90.txt |  8 ++++
>  drivers/hwmon/lm90.c                             | 54 ++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/lm90.txt b/Documentation/devicetree/bindings/hwmon/lm90.txt
> index 045e94b..da9aae7 100644
> --- a/Documentation/devicetree/bindings/hwmon/lm90.txt
> +++ b/Documentation/devicetree/bindings/hwmon/lm90.txt
> @@ -28,6 +28,13 @@ Required node properties:
>  
>  - vcc-supply: vcc regulator for the supply voltage.
>  
> +- #thermal-sensor-cells: Should be 1. See ../../thermal/thermal.txt for a
> +                         description of this property. Use the following
> +                         values to refer to a specific sensor:
> +                         0: chip internal sensor
> +                         1: external sensor
> +                         2: second external sensor, if present

From my read of an LM90 datasheet, there is only an onchip sensor and a 
single external one.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] lm90: register with thermal zones

[Stéphan Kochen]

On Fri, Jan 22, 2016 at 04:45:42PM -0600, Rob Herring wrote:
> On Thu, Jan 21, 2016 at 08:34:37PM +0100, Stéphan Kochen wrote:
> > +- #thermal-sensor-cells: Should be 1. See ../../thermal/thermal.txt for a
> > +                         description of this property. Use the following
> > +                         values to refer to a specific sensor:
> > +                         0: chip internal sensor
> > +                         1: external sensor
> > +                         2: second external sensor, if present
> 
> From my read of an LM90 datasheet, there is only an onchip sensor and a 
> single external one.

MAX6695 and MAX6696 have two external sensors, and also use the lm90
driver. (The LM90_HAVE_TEMP3 flag in the code.)

-- 
Stéphan Kochen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] lm90: register with thermal zones

[Rob Herring]

On Fri, Jan 22, 2016 at 5:02 PM, Stéphan Kochen <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org> wrote:
> On Fri, Jan 22, 2016 at 04:45:42PM -0600, Rob Herring wrote:
>> On Thu, Jan 21, 2016 at 08:34:37PM +0100, Stéphan Kochen wrote:
>> > +- #thermal-sensor-cells: Should be 1. See ../../thermal/thermal.txt for a
>> > +                         description of this property. Use the following
>> > +                         values to refer to a specific sensor:
>> > +                         0: chip internal sensor
>> > +                         1: external sensor
>> > +                         2: second external sensor, if present
>>
>> From my read of an LM90 datasheet, there is only an onchip sensor and a
>> single external one.
>
> MAX6695 and MAX6696 have two external sensors, and also use the lm90
> driver. (The LM90_HAVE_TEMP3 flag in the code.)

Okay.

This change alone seems fine, but I agree with Guenter's comments. I
think having the data in DT is fine, but it should be something
common.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html