[lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input values

[lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input values

[Jean Delvare]

This clears the following build-time warnings I was seeing:

drivers/hwmon/lm95241.c: In function "set_interval":
drivers/hwmon/lm95241.c:132:15: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_max2":
drivers/hwmon/lm95241.c:278:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_max1":
drivers/hwmon/lm95241.c:277:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_min2":
drivers/hwmon/lm95241.c:249:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_min1":
drivers/hwmon/lm95241.c:248:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_type2":
drivers/hwmon/lm95241.c:220:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result
drivers/hwmon/lm95241.c: In function "set_type1":
drivers/hwmon/lm95241.c:219:1: warning: ignoring return value of "strict_strtol", declared with attribute warn_unused_result

This also fixes a small race in set_interval() as a side effect: by
working with a temporary local variable we prevent data->interval from
being accessed at a time it contains the interval value in the wrong
unit.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Davide Rizzo <elpa.rizzo@gmail.com>
---
The sysfs interface part of this driver would deserve a complete
rewrite, macro-generated functions are simply too ugly. But I don't
have the time to do this right now.

 drivers/hwmon/lm95241.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

--- linux-2.6.37-rc1.orig/drivers/hwmon/lm95241.c	2010-10-21 10:01:12.000000000 +0200
+++ linux-2.6.37-rc1/drivers/hwmon/lm95241.c	2010-11-05 16:25:00.000000000 +0100
@@ -128,9 +128,12 @@ static ssize_t set_interval(struct devic
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm95241_data *data = i2c_get_clientdata(client);
+	unsigned long val;
 
-	strict_strtol(buf, 10, &data->interval);
-	data->interval = data->interval * HZ / 1000;
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	data->interval = val * HZ / 1000;
 
 	return count;
 }
@@ -188,7 +191,9 @@ static ssize_t set_type##flag(struct dev
 	struct lm95241_data *data = i2c_get_clientdata(client); \
 \
 	long val; \
-	strict_strtol(buf, 10, &val); \
+\
+	if (strict_strtol(buf, 10, &val) < 0) \
+		return -EINVAL; \
 \
 	if ((val = 1) || (val = 2)) { \
 \
@@ -227,7 +232,9 @@ static ssize_t set_min##flag(struct devi
 	struct lm95241_data *data = i2c_get_clientdata(client); \
 \
 	long val; \
-	strict_strtol(buf, 10, &val); \
+\
+	if (strict_strtol(buf, 10, &val) < 0) \
+		return -EINVAL;\
 \
 	mutex_lock(&data->update_lock); \
 \
@@ -256,7 +263,9 @@ static ssize_t set_max##flag(struct devi
 	struct lm95241_data *data = i2c_get_clientdata(client); \
 \
 	long val; \
-	strict_strtol(buf, 10, &val); \
+\
+	if (strict_strtol(buf, 10, &val) < 0) \
+		return -EINVAL; \
 \
 	mutex_lock(&data->update_lock); \
 \


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Davide Rizzo]

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

2010/11/5 Jean Delvare <khali@linux-fr.org>

> This clears the following build-time warnings I was seeing:
>
> drivers/hwmon/lm95241.c: In function "set_interval":
> drivers/hwmon/lm95241.c:132:15: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_max2":
> drivers/hwmon/lm95241.c:278:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_max1":
> drivers/hwmon/lm95241.c:277:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_min2":
> drivers/hwmon/lm95241.c:249:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_min1":
> drivers/hwmon/lm95241.c:248:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_type2":
> drivers/hwmon/lm95241.c:220:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
> drivers/hwmon/lm95241.c: In function "set_type1":
> drivers/hwmon/lm95241.c:219:1: warning: ignoring return value of
> "strict_strtol", declared with attribute warn_unused_result
>
> This also fixes a small race in set_interval() as a side effect: by
> working with a temporary local variable we prevent data->interval from
> being accessed at a time it contains the interval value in the wrong
> unit.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Davide Rizzo <elpa.rizzo@gmail.com>
> ---
> The sysfs interface part of this driver would deserve a complete
> rewrite, macro-generated functions are simply too ugly. But I don't
> have the time to do this right now.
>
>
> Jean Delvare
>

I already rewrote drivers/hwmon/lm95241.c without macro-generated functions,
I posted it to the list a long time ago but nobody cared it.
Here it is again (including your corrections).
Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com>
------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 2865 bytes --]

[-- Attachment #2: lm95241.c --]
[-- Type: text/x-csrc, Size: 14254 bytes --]

/*
 * drivers/hwmon/lm95241.c
 *
 * Copyright (C) 2009 Davide Rizzo <elpa.rizzo@gmail.com>
 *
 * The LM95241 is a sensor chip made by National Semiconductors.
 * It reports up to three temperatures (its own plus up to two external ones).
 * Complete datasheet can be obtained from National's website at:
 *   http://www.national.com/ds.cgi/LM/LM95241.pdf
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */

#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/jiffies.h>
#include <linux/i2c.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/sysfs.h>

#define DEVNAME "lm95241"

static const unsigned short normal_i2c[] = {
	0x19, 0x2a, 0x2b, I2C_CLIENT_END};

/* LM95241 registers */
#define LM95241_REG_R_MAN_ID		0xFE
#define LM95241_REG_R_CHIP_ID		0xFF
#define LM95241_REG_R_STATUS		0x02
#define LM95241_REG_RW_CONFIG		0x03
#define LM95241_REG_RW_REM_FILTER	0x06
#define LM95241_REG_RW_TRUTHERM		0x07
#define LM95241_REG_W_ONE_SHOT  	0x0F
#define LM95241_REG_R_LOCAL_TEMPH	0x10
#define LM95241_REG_R_REMOTE1_TEMPH	0x11
#define LM95241_REG_R_REMOTE2_TEMPH	0x12
#define LM95241_REG_R_LOCAL_TEMPL	0x20
#define LM95241_REG_R_REMOTE1_TEMPL	0x21
#define LM95241_REG_R_REMOTE2_TEMPL	0x22
#define LM95241_REG_RW_REMOTE_MODEL	0x30

/* LM95241 specific bitfields */
#define CFG_STOP 0x40
#define CFG_CR0076 0x00
#define CFG_CR0182 0x10
#define CFG_CR1000 0x20
#define CFG_CR2700 0x30
#define R1MS_SHIFT 0
#define R2MS_SHIFT 2
#define R1MS_MASK (0x01 << (R1MS_SHIFT))
#define R2MS_MASK (0x01 << (R2MS_SHIFT))
#define R1DF_SHIFT 1
#define R2DF_SHIFT 2
#define R1DF_MASK (0x01 << (R1DF_SHIFT))
#define R2DF_MASK (0x01 << (R2DF_SHIFT))
#define R1FE_MASK 0x01
#define R2FE_MASK 0x05
#define TT1_SHIFT 0
#define TT2_SHIFT 4
#define TT_OFF 0
#define TT_ON 1
#define TT_MASK 7
#define MANUFACTURER_ID 0x01
#define DEFAULT_REVISION 0xA4

/* Conversions and various macros */
#define TEMP_FROM_REG(val_h, val_l) (((val_h) & 0x80 ? (val_h) - 0x100 : \
    (val_h)) * 1000 + (val_l) * 1000 / 256)

/* Functions declaration */
static void lm95241_init_client(struct i2c_client *client);
static struct lm95241_data *lm95241_update_device(struct device *dev);

/* Client data (each client gets its own) */
struct lm95241_data {
	struct device *hwmon_dev;
	struct mutex update_lock;
	unsigned long last_updated, rate; /* in jiffies */
	char valid; /* zero until following fields are valid */
	/* registers values */
	u8 local_h, local_l; /* local */
	u8 remote1_h, remote1_l; /* remote1 */
	u8 remote2_h, remote2_l; /* remote2 */
	u8 config, model, trutherm;
};

static ssize_t show_input(struct device *dev, struct device_attribute *attr,
	char *buf);
static ssize_t show_rate(struct device *dev, struct device_attribute *attr,
	char *buf);
static ssize_t set_rate(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count);
static ssize_t show_type(struct device *dev, struct device_attribute *attr,
	char *buf);
static ssize_t show_min(struct device *dev, struct device_attribute *attr,
	char *buf);
static ssize_t show_max(struct device *dev, struct device_attribute *attr,
	char *buf);
static ssize_t set_type(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count);
static ssize_t set_min(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count);
static ssize_t set_max(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count);

static DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL);
static DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL);
static DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL);
static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type, set_type);
static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min);
static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min);
static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max);
static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max);
static DEVICE_ATTR(rate, S_IWUSR | S_IRUGO, show_rate, set_rate);

static struct attribute *lm95241_attributes[] = {
	&dev_attr_temp1_input.attr,
	&dev_attr_temp2_input.attr,
	&dev_attr_temp3_input.attr,
	&dev_attr_temp2_type.attr,
	&dev_attr_temp3_type.attr,
	&dev_attr_temp2_min.attr,
	&dev_attr_temp3_min.attr,
	&dev_attr_temp2_max.attr,
	&dev_attr_temp3_max.attr,
	&dev_attr_rate.attr,
	NULL
};

static const struct attribute_group lm95241_group = {
	.attrs = lm95241_attributes,
};

/* Conversions */
static int TempFromReg(u8 val_h, u8 val_l)
{
	if (val_h & 0x80)
		return val_h - 0x100;
	return val_h * 1000 + val_l * 1000 / 256;
}

/* Sysfs stuff */
static ssize_t show_input(struct device *dev, struct device_attribute *attr,
	char *buf)
{
	struct lm95241_data *data = lm95241_update_device(dev);
	int value;

	if (attr == &dev_attr_temp1_input)
		value = TempFromReg(data->local_h, data->local_l);
	else if (attr == &dev_attr_temp2_input)
		value = TempFromReg(data->remote1_h, data->remote1_l);
	else
		value = TempFromReg(data->remote2_h, data->remote2_l);
	snprintf(buf, PAGE_SIZE - 1, "%d\n", value);
	return strlen(buf);
}

static ssize_t show_rate(struct device *dev, struct device_attribute *attr,
	char *buf)
{
	struct lm95241_data *data = lm95241_update_device(dev);

	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->rate / HZ);
	return strlen(buf);
}

static ssize_t set_rate(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);
	unsigned long val;

	if (strict_strtol(buf, 10, &val) < 0)
		return -EINVAL;
	
	data->rate = val * HZ / 1000;

	return count;
}

static ssize_t show_type(struct device *dev, struct device_attribute *attr,
	char *buf)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);

	if (attr == &dev_attr_temp2_type)
		snprintf(buf, PAGE_SIZE - 1,
			data->model & R1MS_MASK ? "1\n" : "2\n");
	else
		snprintf(buf, PAGE_SIZE - 1,
			data->model & R2MS_MASK ? "1\n" : "2\n");
	return strlen(buf);
}

static ssize_t show_min(struct device *dev, struct device_attribute *attr,
	char *buf)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);

	if (attr == &dev_attr_temp2_min)
		snprintf(buf, PAGE_SIZE - 1,
			data->config & R1DF_MASK ? "-127000\n" : "0\n");
	else
		snprintf(buf, PAGE_SIZE - 1,
			data->config & R2DF_MASK ? "-127000\n" : "0\n");
	return strlen(buf);
}

static ssize_t show_max(struct device *dev, struct device_attribute *attr,
	char *buf)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);

	if (attr == &dev_attr_temp2_max)
		snprintf(buf, PAGE_SIZE - 1,
			data->config & R1DF_MASK ? "127000\n" : "255000\n");
	else
		snprintf(buf, PAGE_SIZE - 1,
			data->config & R2DF_MASK ? "127000\n" : "255000\n");
	return strlen(buf);
}

static ssize_t set_type(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);
	unsigned long val;

	if (strict_strtoul(buf, 10, &val) < 0)
		return -EINVAL;
	
	if ((val == 1) || (val == 2)) {
		int shift;
		u8 mask;
		if (attr == &dev_attr_temp2_type) {
			shift = TT1_SHIFT;
			mask = R1MS_MASK;
		} else {
			shift = TT2_SHIFT;
			mask = R2MS_MASK;
		}

		mutex_lock(&data->update_lock);

		data->trutherm &= ~(TT_MASK << shift);
		if (val == 1) {
			data->model |= mask;
			data->trutherm |= (TT_ON << shift);
		} else {
			data->model &= ~mask;
			data->trutherm |= (TT_OFF << shift);
		}

		data->valid = 0;

		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
					  data->model);
		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
					  data->trutherm);

		mutex_unlock(&data->update_lock);

	}
	return count;
}

static ssize_t set_min(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);
	long val;
	u8 mask;

	if (strict_strtol(buf, 10, &val) < 0)
		return -EINVAL;
	
	if (attr == &dev_attr_temp2_min)
		mask = R1DF_MASK;
	else
		mask = R2DF_MASK;

	mutex_lock(&data->update_lock);

	if (val < 0)
		data->config |= mask;
	else
		data->config &= mask;
	data->valid = 0;

	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);

	mutex_unlock(&data->update_lock);

	return count;
}

static ssize_t set_max(struct device *dev, struct device_attribute *attr,
	const char *buf, size_t count)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);
	long val;
	u8 mask;

	if (strict_strtol(buf, 10, &val) < 0)
		return -EINVAL;

	if (attr == &dev_attr_temp2_max)
		mask = R1DF_MASK;
	else
		mask = R2DF_MASK;

	mutex_lock(&data->update_lock);

	if (val <= 127000)
		data->config |= R1DF_MASK;
	else
		data->config &= ~R2DF_MASK;
	data->valid = 0;

	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);

	mutex_unlock(&data->update_lock);

	return count;
}

/* Return 0 if detection is successful, -ENODEV otherwise */
static int lm95241_detect(struct i2c_client *new_client,
			  struct i2c_board_info *info)
{
	struct i2c_adapter *adapter = new_client->adapter;
	int address = new_client->addr;
	const char *name;

	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
		return -ENODEV;

	if ((i2c_smbus_read_byte_data(new_client, LM95241_REG_R_MAN_ID)
	     == MANUFACTURER_ID)
	 && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
	     >= DEFAULT_REVISION)) {
		name = DEVNAME;
	} else {
		dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
			address);
		return -ENODEV;
	}

	/* Fill the i2c board info */
	strlcpy(info->type, name, I2C_NAME_SIZE);
	return 0;
}

static int lm95241_probe(struct i2c_client *new_client,
			 const struct i2c_device_id *id)
{
	struct lm95241_data *data;
	int err;

	data = kzalloc(sizeof(struct lm95241_data), GFP_KERNEL);
	if (!data) {
		err = -ENOMEM;
		goto exit;
	}

	i2c_set_clientdata(new_client, data);
	mutex_init(&data->update_lock);

	/* Initialize the LM95241 chip */
	lm95241_init_client(new_client);

	/* Register sysfs hooks */
	err = sysfs_create_group(&new_client->dev.kobj, &lm95241_group);
	if (err)
		goto exit_free;

	data->hwmon_dev = hwmon_device_register(&new_client->dev);
	if (IS_ERR(data->hwmon_dev)) {
		err = PTR_ERR(data->hwmon_dev);
		goto exit_remove_files;
	}

	return 0;

exit_remove_files:
	sysfs_remove_group(&new_client->dev.kobj, &lm95241_group);
exit_free:
	kfree(data);
exit:
	return err;
}

static void lm95241_init_client(struct i2c_client *client)
{
	struct lm95241_data *data = i2c_get_clientdata(client);

	data->rate = HZ;    /* 1 sec default */
	data->valid = 0;
	data->config = CFG_CR0076;
	data->model = 0;
	data->trutherm = (TT_OFF << TT1_SHIFT) | (TT_OFF << TT2_SHIFT);

	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG,
				  data->config);
	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REM_FILTER,
				  R1FE_MASK | R2FE_MASK);
	i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
				  data->trutherm);
	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
				  data->model);
}

static int lm95241_remove(struct i2c_client *client)
{
	struct lm95241_data *data = i2c_get_clientdata(client);

	hwmon_device_unregister(data->hwmon_dev);
	sysfs_remove_group(&client->dev.kobj, &lm95241_group);

	i2c_set_clientdata(client, NULL);
	kfree(data);
	return 0;
}

static struct lm95241_data *lm95241_update_device(struct device *dev)
{
	struct i2c_client *client = to_i2c_client(dev);
	struct lm95241_data *data = i2c_get_clientdata(client);

	mutex_lock(&data->update_lock);

	if (time_after(jiffies, data->last_updated + data->rate) ||
	    !data->valid) {
		dev_dbg(&client->dev, "Updating lm95241 data.\n");
		data->local_h =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_LOCAL_TEMPH);
		data->local_l =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_LOCAL_TEMPL);
		data->remote1_h =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_REMOTE1_TEMPH);
		data->remote1_l =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_REMOTE1_TEMPL);
		data->remote2_h =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_REMOTE2_TEMPH);
		data->remote2_l =
			i2c_smbus_read_byte_data(client,
						 LM95241_REG_R_REMOTE2_TEMPL);
		data->last_updated = jiffies;
		data->valid = 1;
	}

	mutex_unlock(&data->update_lock);

	return data;
}

/* Driver data (common to all clients) */
static const struct i2c_device_id lm95241_id[] = {
	{ DEVNAME, 0 },
	{ }
};
MODULE_DEVICE_TABLE(i2c, lm95241_id);

static struct i2c_driver lm95241_driver = {
	.class		= I2C_CLASS_HWMON,
	.driver = {
		.name   = DEVNAME,
	},
	.probe		= lm95241_probe,
	.remove		= lm95241_remove,
	.id_table	= lm95241_id,
	.detect		= lm95241_detect,
	.address_list	= normal_i2c,
};

static int __init sensors_lm95241_init(void)
{
	return i2c_add_driver(&lm95241_driver);
}

static void __exit sensors_lm95241_exit(void)
{
	i2c_del_driver(&lm95241_driver);
}

MODULE_AUTHOR("Davide Rizzo <elpa.rizzo@gmail.com>");
MODULE_DESCRIPTION("LM95241 sensor driver");
MODULE_LICENSE("GPL");

module_init(sensors_lm95241_init);
module_exit(sensors_lm95241_exit);


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

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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Guenter Roeck]

On Sun, Nov 07, 2010 at 04:11:52AM -0500, Davide Rizzo wrote:
[ ... ]

> I already rewrote drivers/hwmon/lm95241.c without macro-generated functions, I posted it to the list a long time ago but nobody cared it.
> Here it is again (including your corrections).
> Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com<mailto:elpa.rizzo@gmail.com>>
> ------------------------------------------------
> 
Would be great if you could provide a patch on top of Jean's version.
There were other changes in the driver since the initial version which are
undone by the complete file, plus it has some unnecessary formatting changes
and whitespace errors.

Guenter

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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Davide Rizzo]

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

Here it is as requested, against linux-2.6.37-rc1 and including Jeans' last
modify.

2010/11/7 Guenter Roeck <guenter.roeck@ericsson.com>

> On Sun, Nov 07, 2010 at 04:11:52AM -0500, Davide Rizzo wrote:
> [ ... ]
>
> > I already rewrote drivers/hwmon/lm95241.c without macro-generated
> functions, I posted it to the list a long time ago but nobody cared it.
> > Here it is again (including your corrections).
> > Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com<mailto:
> elpa.rizzo@gmail.com>>
> > ------------------------------------------------
> >
> Would be great if you could provide a patch on top of Jean's version.
> There were other changes in the driver since the initial version which are
> undone by the complete file, plus it has some unnecessary formatting
> changes
> and whitespace errors.
>
> Guenter
>



-- 
All difficult problems have a simple solution. That is wrong. [A. Einstein]

[-- Attachment #1.2: Type: text/html, Size: 1384 bytes --]

[-- Attachment #2: lm95241.c.patch --]
[-- Type: text/x-patch, Size: 14509 bytes --]

Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com>
---
Rewritten without macro-generated functions

--- linux-2.6.37-rc1/drivers/hwmon/lm95241.c	2010-11-01 12:54:12.000000000 +0100
+++ linux-2.6.37-rc1.elpa/drivers/hwmon/lm95241.c	2010-11-08 12:58:01.124914446 +0100
@@ -1,13 +1,11 @@
 /*
- * lm95241.c - Part of lm_sensors, Linux kernel modules for hardware
- *             monitoring
- * Copyright (C) 2008 Davide Rizzo <elpa-rizzo@gmail.com>
+ * drivers/hwmon/lm95241.c
  *
- * Based on the max1619 driver. The LM95241 is a sensor chip made by National
- *   Semiconductors.
- * It reports up to three temperatures (its own plus up to
- * two external ones). Complete datasheet can be
- * obtained from National's website at:
+ * Copyright (C) 2009 Davide Rizzo <elpa.rizzo@gmail.com>
+ *
+ * The LM95241 is a sensor chip made by National Semiconductors.
+ * It reports up to three temperatures (its own plus up to two external ones).
+ * Complete datasheet can be obtained from National's website at:
  *   http://www.national.com/ds.cgi/LM/LM95241.pdf
  *
  * This program is free software; you can redistribute it and/or modify
@@ -36,6 +34,8 @@
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
 
+#define DEVNAME "lm95241"
+
 static const unsigned short normal_i2c[] = {
 	0x19, 0x2a, 0x2b, I2C_CLIENT_END};
 
@@ -100,194 +100,36 @@ struct lm95241_data {
 	u8 config, model, trutherm;
 };
 
-/* Sysfs stuff */
-#define show_temp(value) \
-static ssize_t show_##value(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct lm95241_data *data = lm95241_update_device(dev); \
-	snprintf(buf, PAGE_SIZE - 1, "%d\n", \
-		TEMP_FROM_REG(data->value##_h, data->value##_l)); \
-	return strlen(buf); \
-}
-show_temp(local);
-show_temp(remote1);
-show_temp(remote2);
-
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+	char *buf);
 static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct lm95241_data *data = lm95241_update_device(dev);
-
-	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
-	return strlen(buf);
-}
-
+	char *buf);
 static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
-			const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct lm95241_data *data = i2c_get_clientdata(client);
-
-	strict_strtol(buf, 10, &data->interval);
-	data->interval = data->interval * HZ / 1000;
-
-	return count;
-}
-
-#define show_type(flag) \
-static ssize_t show_type##flag(struct device *dev, \
-				   struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->model & R##flag##MS_MASK ? "1\n" : "2\n"); \
-	return strlen(buf); \
-}
-show_type(1);
-show_type(2);
-
-#define show_min(flag) \
-static ssize_t show_min##flag(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->config & R##flag##DF_MASK ?	\
-		"-127000\n" : "0\n"); \
-	return strlen(buf); \
-}
-show_min(1);
-show_min(2);
-
-#define show_max(flag) \
-static ssize_t show_max##flag(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->config & R##flag##DF_MASK ? \
-		"127000\n" : "255000\n"); \
-	return strlen(buf); \
-}
-show_max(1);
-show_max(2);
-
-#define set_type(flag) \
-static ssize_t set_type##flag(struct device *dev, \
-				  struct device_attribute *attr, \
-				  const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	if ((val == 1) || (val == 2)) { \
-\
-		mutex_lock(&data->update_lock); \
-\
-		data->trutherm &= ~(TT_MASK << TT##flag##_SHIFT); \
-		if (val == 1) { \
-			data->model |= R##flag##MS_MASK; \
-			data->trutherm |= (TT_ON << TT##flag##_SHIFT); \
-		} \
-		else { \
-			data->model &= ~R##flag##MS_MASK; \
-			data->trutherm |= (TT_OFF << TT##flag##_SHIFT); \
-		} \
-\
-		data->valid = 0; \
-\
-		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL, \
-					  data->model); \
-		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM, \
-					  data->trutherm); \
-\
-		mutex_unlock(&data->update_lock); \
-\
-	} \
-	return count; \
-}
-set_type(1);
-set_type(2);
-
-#define set_min(flag) \
-static ssize_t set_min##flag(struct device *dev, \
-	struct device_attribute *devattr, const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	mutex_lock(&data->update_lock); \
-\
-	if (val < 0) \
-		data->config |= R##flag##DF_MASK; \
-	else \
-		data->config &= ~R##flag##DF_MASK; \
-\
-	data->valid = 0; \
-\
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
-		data->config); \
-\
-	mutex_unlock(&data->update_lock); \
-\
-	return count; \
-}
-set_min(1);
-set_min(2);
-
-#define set_max(flag) \
-static ssize_t set_max##flag(struct device *dev, \
-	struct device_attribute *devattr, const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	mutex_lock(&data->update_lock); \
-\
-	if (val <= 127000) \
-		data->config |= R##flag##DF_MASK; \
-	else \
-		data->config &= ~R##flag##DF_MASK; \
-\
-	data->valid = 0; \
-\
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
-		data->config); \
-\
-	mutex_unlock(&data->update_lock); \
-\
-	return count; \
-}
-set_max(1);
-set_max(2);
-
-static DEVICE_ATTR(temp1_input, S_IRUGO, show_local, NULL);
-static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote1, NULL);
-static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote2, NULL);
-static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type1, set_type1);
-static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type2, set_type2);
-static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min1, set_min1);
-static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min2, set_min2);
-static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max1, set_max1);
-static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max2, set_max2);
+	const char *buf, size_t count);
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
+	char *buf);
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+	char *buf);
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+	char *buf);
+static ssize_t set_type(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count);
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count);
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count);
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL);
+static DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL);
+static DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL);
+static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
+static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type, set_type);
+static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min);
+static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min);
+static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max);
+static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max);
 static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
-		   set_interval);
+	set_interval);
 
 static struct attribute *lm95241_attributes[] = {
 	&dev_attr_temp1_input.attr,
@@ -307,6 +149,207 @@ static const struct attribute_group lm95
 	.attrs = lm95241_attributes,
 };
 
+/* Conversions */
+static int TempFromReg(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return val_h - 0x100;
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+/* Sysfs stuff */
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	struct lm95241_data *data = lm95241_update_device(dev);
+	int value;
+
+	if (attr == &dev_attr_temp1_input)
+		value = TempFromReg(data->local_h, data->local_l);
+	else if (attr == &dev_attr_temp2_input)
+		value = TempFromReg(data->remote1_h, data->remote1_l);
+	else
+		value = TempFromReg(data->remote2_h, data->remote2_l);
+	snprintf(buf, PAGE_SIZE - 1, "%d\n", value);
+	return strlen(buf);
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	struct lm95241_data *data = lm95241_update_device(dev);
+
+	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
+	return strlen(buf);
+}
+
+static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	data->interval = val * HZ / 1000;
+
+	return count;
+}
+
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_type)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->model & R1MS_MASK ? "1\n" : "2\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->model & R2MS_MASK ? "1\n" : "2\n");
+	return strlen(buf);
+}
+
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_min)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R1DF_MASK ? "-127000\n" : "0\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R2DF_MASK ? "-127000\n" : "0\n");
+	return strlen(buf);
+}
+
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+	char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_max)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R1DF_MASK ? "127000\n" : "255000\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R2DF_MASK ? "127000\n" : "255000\n");
+	return strlen(buf);
+}
+
+static ssize_t set_type(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if ((val == 1) || (val == 2)) {
+		int shift;
+		u8 mask;
+		if (attr == &dev_attr_temp2_type) {
+			shift = TT1_SHIFT;
+			mask = R1MS_MASK;
+		} else {
+			shift = TT2_SHIFT;
+			mask = R2MS_MASK;
+		}
+
+		mutex_lock(&data->update_lock);
+
+		data->trutherm &= ~(TT_MASK << shift);
+		if (val == 1) {
+			data->model |= mask;
+			data->trutherm |= (TT_ON << shift);
+		} else {
+			data->model &= ~mask;
+			data->trutherm |= (TT_OFF << shift);
+		}
+
+		data->valid = 0;
+
+		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
+					  data->model);
+		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
+					  data->trutherm);
+
+		mutex_unlock(&data->update_lock);
+
+	}
+	return count;
+}
+
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	long val;
+	u8 mask;
+
+	if (strict_strtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (attr == &dev_attr_temp2_min)
+		mask = R1DF_MASK;
+	else
+		mask = R2DF_MASK;
+
+	mutex_lock(&data->update_lock);
+
+	if (val < 0)
+		data->config |= mask;
+	else
+		data->config &= mask;
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	long val;
+	u8 mask;
+
+	if (strict_strtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (attr == &dev_attr_temp2_max)
+		mask = R1DF_MASK;
+	else
+		mask = R2DF_MASK;
+
+	mutex_lock(&data->update_lock);
+
+	if (val <= 127000)
+		data->config |= R1DF_MASK;
+	else
+		data->config &= ~R2DF_MASK;
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int lm95241_detect(struct i2c_client *new_client,
 			  struct i2c_board_info *info)
@@ -322,7 +365,7 @@ static int lm95241_detect(struct i2c_cli
 	     == MANUFACTURER_ID)
 	 && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
 	     >= DEFAULT_REVISION)) {
-		name = "lm95241";
+		name = DEVNAME;
 	} else {
 		dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
 			address);
@@ -443,7 +486,7 @@ static struct lm95241_data *lm95241_upda
 
 /* Driver data (common to all clients) */
 static const struct i2c_device_id lm95241_id[] = {
-	{ "lm95241", 0 },
+	{ DEVNAME, 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, lm95241_id);
@@ -451,7 +494,7 @@ MODULE_DEVICE_TABLE(i2c, lm95241_id);
 static struct i2c_driver lm95241_driver = {
 	.class		= I2C_CLASS_HWMON,
 	.driver = {
-		.name   = "lm95241",
+		.name   = DEVNAME,
 	},
 	.probe		= lm95241_probe,
 	.remove		= lm95241_remove,
@@ -470,9 +513,10 @@ static void __exit sensors_lm95241_exit(
 	i2c_del_driver(&lm95241_driver);
 }
 
-MODULE_AUTHOR("Davide Rizzo <elpa-rizzo@gmail.com>");
+MODULE_AUTHOR("Davide Rizzo <elpa.rizzo@gmail.com>");
 MODULE_DESCRIPTION("LM95241 sensor driver");
 MODULE_LICENSE("GPL");
 
 module_init(sensors_lm95241_init);
 module_exit(sensors_lm95241_exit);
+

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

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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Guenter Roeck]

On Mon, Nov 08, 2010 at 07:06:00AM -0500, Davide Rizzo wrote:
> Here it is as requested, against linux-2.6.37-rc1 and including Jeans' last modify.
> 
Davide,

Please check Documentation/SubmittingPatches when submitting patches.

Couple of comments below.

> 2010/11/7 Guenter Roeck <guenter.roeck@ericsson.com<mailto:guenter.roeck@ericsson.com>>
> On Sun, Nov 07, 2010 at 04:11:52AM -0500, Davide Rizzo wrote:
> [ ... ]
> 
> > I already rewrote drivers/hwmon/lm95241.c without macro-generated functions, I posted it to the list a long time ago but nobody cared it.
> > Here it is again (including your corrections).
> > Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com<mailto:elpa.rizzo@gmail.com><mailto:elpa.rizzo@gmail.com<mailto:elpa.rizzo@gmail.com>>>
> > ------------------------------------------------
> >
> Would be great if you could provide a patch on top of Jean's version.
> There were other changes in the driver since the initial version which are
> undone by the complete file, plus it has some unnecessary formatting changes
> and whitespace errors.
> 
> Guenter
> 
> 
> 
> --
> All difficult problems have a simple solution. That is wrong. [A. Einstein]

> Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com>
> ---
> Rewritten without macro-generated functions
> 
> --- linux-2.6.37-rc1/drivers/hwmon/lm95241.c	2010-11-01 12:54:12.000000000 +0100
> +++ linux-2.6.37-rc1.elpa/drivers/hwmon/lm95241.c	2010-11-08 12:58:01.124914446 +0100
> @@ -1,13 +1,11 @@
>  /*
> - * lm95241.c - Part of lm_sensors, Linux kernel modules for hardware
> - *             monitoring
> - * Copyright (C) 2008 Davide Rizzo <elpa-rizzo@gmail.com>
> + * drivers/hwmon/lm95241.c

This information is redundant and does not provide any value.

>   *
> - * Based on the max1619 driver. The LM95241 is a sensor chip made by National
> - *   Semiconductors.

It is no longer based on the max1619 driver ?

> - * It reports up to three temperatures (its own plus up to
> - * two external ones). Complete datasheet can be
> - * obtained from National's website at:
> + * Copyright (C) 2009 Davide Rizzo <elpa.rizzo@gmail.com>
> + *
Should probably be 2010 or 2008, 2010.

> + * The LM95241 is a sensor chip made by National Semiconductors.
> + * It reports up to three temperatures (its own plus up to two external ones).
> + * Complete datasheet can be obtained from National's website at:
>   *   http://www.national.com/ds.cgi/LM/LM95241.pdf
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -36,6 +34,8 @@
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
>  
> +#define DEVNAME "lm95241"
> +
>  static const unsigned short normal_i2c[] = {
>  	0x19, 0x2a, 0x2b, I2C_CLIENT_END};
>  
> @@ -100,194 +100,36 @@ struct lm95241_data {
>  	u8 config, model, trutherm;
>  };
>  
> -/* Sysfs stuff */
> -#define show_temp(value) \
> -static ssize_t show_##value(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct lm95241_data *data = lm95241_update_device(dev); \
> -	snprintf(buf, PAGE_SIZE - 1, "%d\n", \
> -		TEMP_FROM_REG(data->value##_h, data->value##_l)); \
> -	return strlen(buf); \
> -}
> -show_temp(local);
> -show_temp(remote1);
> -show_temp(remote2);
> -
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +	char *buf);
>  static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct lm95241_data *data = lm95241_update_device(dev);
> -
> -	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
> -	return strlen(buf);
> -}
> -
> +	char *buf);

Good idea to fix those alignments. However, please consider using 

static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
			     char *buf)

ie align with '('. Otherwise there are still two sets of alignments in the code
which doesn't really make it easier to read.

Also,  Idon't think the functions should be moved. As a result, you have to declare 
all functions, since they are used before being defined, plus the diffs get larger
than needed. More on that below.

>  static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> -			const char *buf, size_t count)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct lm95241_data *data = i2c_get_clientdata(client);
> -
> -	strict_strtol(buf, 10, &data->interval);
> -	data->interval = data->interval * HZ / 1000;
> -
> -	return count;
> -}
> -
> -#define show_type(flag) \
> -static ssize_t show_type##flag(struct device *dev, \
> -				   struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->model & R##flag##MS_MASK ? "1\n" : "2\n"); \
> -	return strlen(buf); \
> -}
> -show_type(1);
> -show_type(2);
> -
> -#define show_min(flag) \
> -static ssize_t show_min##flag(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->config & R##flag##DF_MASK ?	\
> -		"-127000\n" : "0\n"); \
> -	return strlen(buf); \
> -}
> -show_min(1);
> -show_min(2);
> -
> -#define show_max(flag) \
> -static ssize_t show_max##flag(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->config & R##flag##DF_MASK ? \
> -		"127000\n" : "255000\n"); \
> -	return strlen(buf); \
> -}
> -show_max(1);
> -show_max(2);
> -
> -#define set_type(flag) \
> -static ssize_t set_type##flag(struct device *dev, \
> -				  struct device_attribute *attr, \
> -				  const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	if ((val = 1) || (val = 2)) { \
> -\
> -		mutex_lock(&data->update_lock); \
> -\
> -		data->trutherm &= ~(TT_MASK << TT##flag##_SHIFT); \
> -		if (val = 1) { \
> -			data->model |= R##flag##MS_MASK; \
> -			data->trutherm |= (TT_ON << TT##flag##_SHIFT); \
> -		} \
> -		else { \
> -			data->model &= ~R##flag##MS_MASK; \
> -			data->trutherm |= (TT_OFF << TT##flag##_SHIFT); \
> -		} \
> -\
> -		data->valid = 0; \
> -\
> -		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL, \
> -					  data->model); \
> -		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM, \
> -					  data->trutherm); \
> -\
> -		mutex_unlock(&data->update_lock); \
> -\
> -	} \
> -	return count; \
> -}
> -set_type(1);
> -set_type(2);
> -
> -#define set_min(flag) \
> -static ssize_t set_min##flag(struct device *dev, \
> -	struct device_attribute *devattr, const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	mutex_lock(&data->update_lock); \
> -\
> -	if (val < 0) \
> -		data->config |= R##flag##DF_MASK; \
> -	else \
> -		data->config &= ~R##flag##DF_MASK; \
> -\
> -	data->valid = 0; \
> -\
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
> -		data->config); \
> -\
> -	mutex_unlock(&data->update_lock); \
> -\
> -	return count; \
> -}
> -set_min(1);
> -set_min(2);
> -
> -#define set_max(flag) \
> -static ssize_t set_max##flag(struct device *dev, \
> -	struct device_attribute *devattr, const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	mutex_lock(&data->update_lock); \
> -\
> -	if (val <= 127000) \
> -		data->config |= R##flag##DF_MASK; \
> -	else \
> -		data->config &= ~R##flag##DF_MASK; \
> -\
> -	data->valid = 0; \
> -\
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
> -		data->config); \
> -\
> -	mutex_unlock(&data->update_lock); \
> -\
> -	return count; \
> -}
> -set_max(1);
> -set_max(2);
> -
> -static DEVICE_ATTR(temp1_input, S_IRUGO, show_local, NULL);
> -static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote1, NULL);
> -static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote2, NULL);
> -static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type1, set_type1);
> -static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type2, set_type2);
> -static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min1, set_min1);
> -static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min2, set_min2);
> -static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max1, set_max1);
> -static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max2, set_max2);
> +	const char *buf, size_t count);
> +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> +	char *buf);
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +	char *buf);
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +	char *buf);
> +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count);
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count);
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count);
> +

It would be better to move DEVICE_ATTR, lm95241_attributes, and lm95241_group
after the actual functions. Then you don't need the static declarations above.

> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL);
> +static DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL);
> +static DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL);
> +static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
> +static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type, set_type);
> +static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min);
> +static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min);
> +static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max);
> +static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max);
>  static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
> -		   set_interval);
> +	set_interval);
>  
>  static struct attribute *lm95241_attributes[] = {
>  	&dev_attr_temp1_input.attr,
> @@ -307,6 +149,207 @@ static const struct attribute_group lm95
>  	.attrs = lm95241_attributes,
>  };
>  
> +/* Conversions */
> +static int TempFromReg(u8 val_h, u8 val_l)
> +{
> +	if (val_h & 0x80)
> +		return val_h - 0x100;
> +	return val_h * 1000 + val_l * 1000 / 256;
> +}
> +
> +/* Sysfs stuff */
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct lm95241_data *data = lm95241_update_device(dev);
> +	int value;
> +
> +	if (attr = &dev_attr_temp1_input)
> +		value = TempFromReg(data->local_h, data->local_l);
> +	else if (attr = &dev_attr_temp2_input)
> +		value = TempFromReg(data->remote1_h, data->remote1_l);
> +	else
> +		value = TempFromReg(data->remote2_h, data->remote2_l);
> +	snprintf(buf, PAGE_SIZE - 1, "%d\n", value);
> +	return strlen(buf);
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct lm95241_data *data = lm95241_update_device(dev);
> +
> +	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
> +	return strlen(buf);
> +}
> +
> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	unsigned long val;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	data->interval = val * HZ / 1000;
> +
This accepts negative intervals and converts it to some large positive number.
You might want to use strict_strtoul() instead.

> +	return count;
> +}
> +
> +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_type)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->model & R1MS_MASK ? "1\n" : "2\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->model & R2MS_MASK ? "1\n" : "2\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_min)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R1DF_MASK ? "-127000\n" : "0\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R2DF_MASK ? "-127000\n" : "0\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_max)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R1DF_MASK ? "127000\n" : "255000\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R2DF_MASK ? "127000\n" : "255000\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	unsigned long val;
> +
> +	if (strict_strtoul(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if ((val = 1) || (val = 2)) {

Those extra () are really unnecessary.

> +		int shift;
> +		u8 mask;
> +		if (attr = &dev_attr_temp2_type) {
> +			shift = TT1_SHIFT;
> +			mask = R1MS_MASK;
> +		} else {
> +			shift = TT2_SHIFT;
> +			mask = R2MS_MASK;
> +		}
> +
> +		mutex_lock(&data->update_lock);
> +
> +		data->trutherm &= ~(TT_MASK << shift);
> +		if (val = 1) {
> +			data->model |= mask;
> +			data->trutherm |= (TT_ON << shift);
> +		} else {
> +			data->model &= ~mask;
> +			data->trutherm |= (TT_OFF << shift);
> +		}
> +
> +		data->valid = 0;
> +
> +		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
> +					  data->model);
> +		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
> +					  data->trutherm);
> +
> +		mutex_unlock(&data->update_lock);
> +
> +	}
> +	return count;
> +}
> +
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	long val;
> +	u8 mask;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if (attr = &dev_attr_temp2_min)
> +		mask = R1DF_MASK;
> +	else
> +		mask = R2DF_MASK;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (val < 0)
> +		data->config |= mask;
> +	else
> +		data->config &= mask;
> +	data->valid = 0;
> +
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +	const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	long val;
> +	u8 mask;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if (attr = &dev_attr_temp2_max)
> +		mask = R1DF_MASK;
> +	else
> +		mask = R2DF_MASK;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (val <= 127000)
> +		data->config |= R1DF_MASK;
> +	else
> +		data->config &= ~R2DF_MASK;
> +	data->valid = 0;
> +
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
>  /* Return 0 if detection is successful, -ENODEV otherwise */
>  static int lm95241_detect(struct i2c_client *new_client,
>  			  struct i2c_board_info *info)
> @@ -322,7 +365,7 @@ static int lm95241_detect(struct i2c_cli
>  	     = MANUFACTURER_ID)
>  	 && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
>  	     >= DEFAULT_REVISION)) {
> -		name = "lm95241";
> +		name = DEVNAME;
>  	} else {
>  		dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
>  			address);
> @@ -443,7 +486,7 @@ static struct lm95241_data *lm95241_upda
>  
>  /* Driver data (common to all clients) */
>  static const struct i2c_device_id lm95241_id[] = {
> -	{ "lm95241", 0 },
> +	{ DEVNAME, 0 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, lm95241_id);
> @@ -451,7 +494,7 @@ MODULE_DEVICE_TABLE(i2c, lm95241_id);
>  static struct i2c_driver lm95241_driver = {
>  	.class		= I2C_CLASS_HWMON,
>  	.driver = {
> -		.name   = "lm95241",
> +		.name   = DEVNAME,
>  	},
>  	.probe		= lm95241_probe,
>  	.remove		= lm95241_remove,
> @@ -470,9 +513,10 @@ static void __exit sensors_lm95241_exit(
>  	i2c_del_driver(&lm95241_driver);
>  }
>  
> -MODULE_AUTHOR("Davide Rizzo <elpa-rizzo@gmail.com>");
> +MODULE_AUTHOR("Davide Rizzo <elpa.rizzo@gmail.com>");
>  MODULE_DESCRIPTION("LM95241 sensor driver");
>  MODULE_LICENSE("GPL");
>  
>  module_init(sensors_lm95241_init);
>  module_exit(sensors_lm95241_exit);
> +


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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Davide Rizzo]

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

Here it is again, including your corrections.
Please note that most of the function declarations cannot be omitted.

-- 
All difficult problems have a simple solution. That is wrong. [A. Einstein]

[-- Attachment #1.2: Type: text/html, Size: 226 bytes --]

[-- Attachment #2: lm95241.c.patch --]
[-- Type: text/x-patch, Size: 17993 bytes --]

Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com>
---
Rewritten without macro-generated functions

--- linux-2.6.37-rc1/drivers/hwmon/lm95241.c	2010-11-01 12:54:12.000000000 +0100
+++ linux-2.6.37-rc1.elpa/drivers/hwmon/lm95241.c	2010-11-08 19:16:35.873755269 +0100
@@ -1,13 +1,9 @@
 /*
- * lm95241.c - Part of lm_sensors, Linux kernel modules for hardware
- *             monitoring
- * Copyright (C) 2008 Davide Rizzo <elpa-rizzo@gmail.com>
+ * Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
  *
- * Based on the max1619 driver. The LM95241 is a sensor chip made by National
- *   Semiconductors.
- * It reports up to three temperatures (its own plus up to
- * two external ones). Complete datasheet can be
- * obtained from National's website at:
+ * The LM95241 is a sensor chip made by National Semiconductors.
+ * It reports up to three temperatures (its own plus up to two external ones).
+ * Complete datasheet can be obtained from National's website at:
  *   http://www.national.com/ds.cgi/LM/LM95241.pdf
  *
  * This program is free software; you can redistribute it and/or modify
@@ -36,6 +32,8 @@
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
 
+#define DEVNAME "lm95241"
+
 static const unsigned short normal_i2c[] = {
 	0x19, 0x2a, 0x2b, I2C_CLIENT_END};
 
@@ -83,10 +81,6 @@ static const unsigned short normal_i2c[]
 #define TEMP_FROM_REG(val_h, val_l) (((val_h) & 0x80 ? (val_h) - 0x100 : \
     (val_h)) * 1000 + (val_l) * 1000 / 256)
 
-/* Functions declaration */
-static void lm95241_init_client(struct i2c_client *client);
-static struct lm95241_data *lm95241_update_device(struct device *dev);
-
 /* Client data (each client gets its own) */
 struct lm95241_data {
 	struct device *hwmon_dev;
@@ -100,23 +94,254 @@ struct lm95241_data {
 	u8 config, model, trutherm;
 };
 
+/* Conversions */
+static int TempFromReg(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return val_h - 0x100;
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+static struct lm95241_data *lm95241_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + data->interval) ||
+	    !data->valid) {
+		dev_dbg(&client->dev, "Updating lm95241 data.\n");
+		data->local_h =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_LOCAL_TEMPH);
+		data->local_l =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_LOCAL_TEMPL);
+		data->remote1_h =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_REMOTE1_TEMPH);
+		data->remote1_l =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_REMOTE1_TEMPL);
+		data->remote2_h =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_REMOTE2_TEMPH);
+		data->remote2_l =
+			i2c_smbus_read_byte_data(client,
+						 LM95241_REG_R_REMOTE2_TEMPL);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf);
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL);
+static DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL);
+static DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL);
+
 /* Sysfs stuff */
-#define show_temp(value) \
-static ssize_t show_##value(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct lm95241_data *data = lm95241_update_device(dev); \
-	snprintf(buf, PAGE_SIZE - 1, "%d\n", \
-		TEMP_FROM_REG(data->value##_h, data->value##_l)); \
-	return strlen(buf); \
-}
-show_temp(local);
-show_temp(remote1);
-show_temp(remote2);
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct lm95241_data *data = lm95241_update_device(dev);
+	int value;
 
-static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+	if (attr == &dev_attr_temp1_input)
+		value = TempFromReg(data->local_h, data->local_l);
+	else if (attr == &dev_attr_temp2_input)
+		value = TempFromReg(data->remote1_h, data->remote1_l);
+	else
+		value = TempFromReg(data->remote2_h, data->remote2_l);
+	snprintf(buf, PAGE_SIZE - 1, "%d\n", value);
+	return strlen(buf);
+}
+
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
+			 char *buf);
+static ssize_t set_type(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count);
+
+static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
+static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type, set_type);
+
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_type)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->model & R1MS_MASK ? "1\n" : "2\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->model & R2MS_MASK ? "1\n" : "2\n");
+	return strlen(buf);
+}
+
+static ssize_t set_type(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (val == 1 || val == 2) {
+		int shift;
+		u8 mask;
+		if (attr == &dev_attr_temp2_type) {
+			shift = TT1_SHIFT;
+			mask = R1MS_MASK;
+		} else {
+			shift = TT2_SHIFT;
+			mask = R2MS_MASK;
+		}
+
+		mutex_lock(&data->update_lock);
+
+		data->trutherm &= ~(TT_MASK << shift);
+		if (val == 1) {
+			data->model |= mask;
+			data->trutherm |= (TT_ON << shift);
+		} else {
+			data->model &= ~mask;
+			data->trutherm |= (TT_OFF << shift);
+		}
+
+		data->valid = 0;
+
+		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
+					  data->model);
+		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
+					  data->trutherm);
+
+		mutex_unlock(&data->update_lock);
+
+	}
+	return count;
+}
+
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+			char *buf);
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count);
+
+static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min);
+static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min);
+
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_min)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R1DF_MASK ? "-127000\n" : "0\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R2DF_MASK ? "-127000\n" : "0\n");
+	return strlen(buf);
+}
+
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	long val;
+	u8 mask;
+
+	if (strict_strtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (attr == &dev_attr_temp2_min)
+		mask = R1DF_MASK;
+	else
+		mask = R2DF_MASK;
+
+	mutex_lock(&data->update_lock);
+
+	if (val < 0)
+		data->config |= mask;
+	else
+		data->config &= mask;
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+			char *buf);
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count);
+
+static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max);
+static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max);
+
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	if (attr == &dev_attr_temp2_max)
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R1DF_MASK ? "127000\n" : "255000\n");
+	else
+		snprintf(buf, PAGE_SIZE - 1,
+			data->config & R2DF_MASK ? "127000\n" : "255000\n");
+	return strlen(buf);
+}
+
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95241_data *data = i2c_get_clientdata(client);
+	long val;
+	u8 mask;
+
+	if (strict_strtol(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	if (attr == &dev_attr_temp2_max)
+		mask = R1DF_MASK;
+	else
+		mask = R2DF_MASK;
+
+	mutex_lock(&data->update_lock);
+
+	if (val <= 127000)
+		data->config |= R1DF_MASK;
+	else
+		data->config &= ~R2DF_MASK;
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
 	struct lm95241_data *data = lm95241_update_device(dev);
 
 	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
@@ -124,168 +349,20 @@ static ssize_t show_interval(struct devi
 }
 
 static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
-			const char *buf, size_t count)
+			    const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm95241_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
 
-	strict_strtol(buf, 10, &data->interval);
-	data->interval = data->interval * HZ / 1000;
+	data->interval = val * HZ / 1000;
 
 	return count;
 }
 
-#define show_type(flag) \
-static ssize_t show_type##flag(struct device *dev, \
-				   struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->model & R##flag##MS_MASK ? "1\n" : "2\n"); \
-	return strlen(buf); \
-}
-show_type(1);
-show_type(2);
-
-#define show_min(flag) \
-static ssize_t show_min##flag(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->config & R##flag##DF_MASK ?	\
-		"-127000\n" : "0\n"); \
-	return strlen(buf); \
-}
-show_min(1);
-show_min(2);
-
-#define show_max(flag) \
-static ssize_t show_max##flag(struct device *dev, \
-    struct device_attribute *attr, char *buf) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	snprintf(buf, PAGE_SIZE - 1, \
-		data->config & R##flag##DF_MASK ? \
-		"127000\n" : "255000\n"); \
-	return strlen(buf); \
-}
-show_max(1);
-show_max(2);
-
-#define set_type(flag) \
-static ssize_t set_type##flag(struct device *dev, \
-				  struct device_attribute *attr, \
-				  const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	if ((val == 1) || (val == 2)) { \
-\
-		mutex_lock(&data->update_lock); \
-\
-		data->trutherm &= ~(TT_MASK << TT##flag##_SHIFT); \
-		if (val == 1) { \
-			data->model |= R##flag##MS_MASK; \
-			data->trutherm |= (TT_ON << TT##flag##_SHIFT); \
-		} \
-		else { \
-			data->model &= ~R##flag##MS_MASK; \
-			data->trutherm |= (TT_OFF << TT##flag##_SHIFT); \
-		} \
-\
-		data->valid = 0; \
-\
-		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL, \
-					  data->model); \
-		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM, \
-					  data->trutherm); \
-\
-		mutex_unlock(&data->update_lock); \
-\
-	} \
-	return count; \
-}
-set_type(1);
-set_type(2);
-
-#define set_min(flag) \
-static ssize_t set_min##flag(struct device *dev, \
-	struct device_attribute *devattr, const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	mutex_lock(&data->update_lock); \
-\
-	if (val < 0) \
-		data->config |= R##flag##DF_MASK; \
-	else \
-		data->config &= ~R##flag##DF_MASK; \
-\
-	data->valid = 0; \
-\
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
-		data->config); \
-\
-	mutex_unlock(&data->update_lock); \
-\
-	return count; \
-}
-set_min(1);
-set_min(2);
-
-#define set_max(flag) \
-static ssize_t set_max##flag(struct device *dev, \
-	struct device_attribute *devattr, const char *buf, size_t count) \
-{ \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct lm95241_data *data = i2c_get_clientdata(client); \
-\
-	long val; \
-	strict_strtol(buf, 10, &val); \
-\
-	mutex_lock(&data->update_lock); \
-\
-	if (val <= 127000) \
-		data->config |= R##flag##DF_MASK; \
-	else \
-		data->config &= ~R##flag##DF_MASK; \
-\
-	data->valid = 0; \
-\
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
-		data->config); \
-\
-	mutex_unlock(&data->update_lock); \
-\
-	return count; \
-}
-set_max(1);
-set_max(2);
-
-static DEVICE_ATTR(temp1_input, S_IRUGO, show_local, NULL);
-static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote1, NULL);
-static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote2, NULL);
-static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type1, set_type1);
-static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type2, set_type2);
-static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min1, set_min1);
-static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min2, set_min2);
-static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max1, set_max1);
-static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max2, set_max2);
 static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
 		   set_interval);
 
@@ -322,7 +399,7 @@ static int lm95241_detect(struct i2c_cli
 	     == MANUFACTURER_ID)
 	 && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
 	     >= DEFAULT_REVISION)) {
-		name = "lm95241";
+		name = DEVNAME;
 	} else {
 		dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
 			address);
@@ -334,6 +411,26 @@ static int lm95241_detect(struct i2c_cli
 	return 0;
 }
 
+static void lm95241_init_client(struct i2c_client *client)
+{
+	struct lm95241_data *data = i2c_get_clientdata(client);
+
+	data->interval = HZ;    /* 1 sec default */
+	data->valid = 0;
+	data->config = CFG_CR0076;
+	data->model = 0;
+	data->trutherm = (TT_OFF << TT1_SHIFT) | (TT_OFF << TT2_SHIFT);
+
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG,
+				  data->config);
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REM_FILTER,
+				  R1FE_MASK | R2FE_MASK);
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
+				  data->trutherm);
+	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
+				  data->model);
+}
+
 static int lm95241_probe(struct i2c_client *new_client,
 			 const struct i2c_device_id *id)
 {
@@ -373,26 +470,6 @@ exit:
 	return err;
 }
 
-static void lm95241_init_client(struct i2c_client *client)
-{
-	struct lm95241_data *data = i2c_get_clientdata(client);
-
-	data->interval = HZ;    /* 1 sec default */
-	data->valid = 0;
-	data->config = CFG_CR0076;
-	data->model = 0;
-	data->trutherm = (TT_OFF << TT1_SHIFT) | (TT_OFF << TT2_SHIFT);
-
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG,
-				  data->config);
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REM_FILTER,
-				  R1FE_MASK | R2FE_MASK);
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
-				  data->trutherm);
-	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
-				  data->model);
-}
-
 static int lm95241_remove(struct i2c_client *client)
 {
 	struct lm95241_data *data = i2c_get_clientdata(client);
@@ -404,46 +481,9 @@ static int lm95241_remove(struct i2c_cli
 	return 0;
 }
 
-static struct lm95241_data *lm95241_update_device(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct lm95241_data *data = i2c_get_clientdata(client);
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + data->interval) ||
-	    !data->valid) {
-		dev_dbg(&client->dev, "Updating lm95241 data.\n");
-		data->local_h =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_LOCAL_TEMPH);
-		data->local_l =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_LOCAL_TEMPL);
-		data->remote1_h =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_REMOTE1_TEMPH);
-		data->remote1_l =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_REMOTE1_TEMPL);
-		data->remote2_h =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_REMOTE2_TEMPH);
-		data->remote2_l =
-			i2c_smbus_read_byte_data(client,
-						 LM95241_REG_R_REMOTE2_TEMPL);
-		data->last_updated = jiffies;
-		data->valid = 1;
-	}
-
-	mutex_unlock(&data->update_lock);
-
-	return data;
-}
-
 /* Driver data (common to all clients) */
 static const struct i2c_device_id lm95241_id[] = {
-	{ "lm95241", 0 },
+	{ DEVNAME, 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, lm95241_id);
@@ -451,7 +491,7 @@ MODULE_DEVICE_TABLE(i2c, lm95241_id);
 static struct i2c_driver lm95241_driver = {
 	.class		= I2C_CLASS_HWMON,
 	.driver = {
-		.name   = "lm95241",
+		.name   = DEVNAME,
 	},
 	.probe		= lm95241_probe,
 	.remove		= lm95241_remove,
@@ -470,9 +510,10 @@ static void __exit sensors_lm95241_exit(
 	i2c_del_driver(&lm95241_driver);
 }
 
-MODULE_AUTHOR("Davide Rizzo <elpa-rizzo@gmail.com>");
+MODULE_AUTHOR("Davide Rizzo <elpa.rizzo@gmail.com>");
 MODULE_DESCRIPTION("LM95241 sensor driver");
 MODULE_LICENSE("GPL");
 
 module_init(sensors_lm95241_init);
 module_exit(sensors_lm95241_exit);
+

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

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

Re: [lm-sensors] [PATCH] hwmon: (lm95241) Check validity of input

[Guenter Roeck]

On Mon, Nov 08, 2010 at 01:22:38PM -0500, Davide Rizzo wrote:
> Here it is again, including your corrections.
> Please note that most of the function declarations cannot be omitted.
> 
Part of the reasons for suggesting to look at the SubmittingPatches
file were rules #2, #7, and #11/#15. Specifically, not to use attachments
and to use an appropriate subject. It isn't really a good idea to attach 
this exchange to a different subject.

Anyway, this is getting worse, not better. I dislike the notion of using 
the attribute address to distinguish sensors (which is causing the requirement
for forward declarations). I prefer the more common method of using SENSOR_DEVICE_ATTR 
and passing the register offset or some other means of distinguishing sensors 
to the function. For example, one could use an array instead of separate variable 
names to identify the sensors in lm95241_data, and then simply index the sensors
using SENSOR_DEVICE_ATTR.

But maybe that is just my personal view. Jean, any comments from your side ?

Guenter

> --
> All difficult problems have a simple solution. That is wrong. [A. Einstein]

> Signed-off-by: Davide Rizzo <elpa.rizzo@gmail.com>
> ---
> Rewritten without macro-generated functions
> 
> --- linux-2.6.37-rc1/drivers/hwmon/lm95241.c	2010-11-01 12:54:12.000000000 +0100
> +++ linux-2.6.37-rc1.elpa/drivers/hwmon/lm95241.c	2010-11-08 19:16:35.873755269 +0100
> @@ -1,13 +1,9 @@
>  /*
> - * lm95241.c - Part of lm_sensors, Linux kernel modules for hardware
> - *             monitoring
> - * Copyright (C) 2008 Davide Rizzo <elpa-rizzo@gmail.com>
> + * Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
>   *
> - * Based on the max1619 driver. The LM95241 is a sensor chip made by National
> - *   Semiconductors.
> - * It reports up to three temperatures (its own plus up to
> - * two external ones). Complete datasheet can be
> - * obtained from National's website at:
> + * The LM95241 is a sensor chip made by National Semiconductors.
> + * It reports up to three temperatures (its own plus up to two external ones).
> + * Complete datasheet can be obtained from National's website at:
>   *   http://www.national.com/ds.cgi/LM/LM95241.pdf
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -36,6 +32,8 @@
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
>  
> +#define DEVNAME "lm95241"
> +
>  static const unsigned short normal_i2c[] = {
>  	0x19, 0x2a, 0x2b, I2C_CLIENT_END};
>  
> @@ -83,10 +81,6 @@ static const unsigned short normal_i2c[]
>  #define TEMP_FROM_REG(val_h, val_l) (((val_h) & 0x80 ? (val_h) - 0x100 : \
>      (val_h)) * 1000 + (val_l) * 1000 / 256)
>  
> -/* Functions declaration */
> -static void lm95241_init_client(struct i2c_client *client);
> -static struct lm95241_data *lm95241_update_device(struct device *dev);
> -
>  /* Client data (each client gets its own) */
>  struct lm95241_data {
>  	struct device *hwmon_dev;
> @@ -100,23 +94,254 @@ struct lm95241_data {
>  	u8 config, model, trutherm;
>  };
>  
> +/* Conversions */
> +static int TempFromReg(u8 val_h, u8 val_l)
> +{
> +	if (val_h & 0x80)
> +		return val_h - 0x100;
> +	return val_h * 1000 + val_l * 1000 / 256;
> +}
> +
> +static struct lm95241_data *lm95241_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (time_after(jiffies, data->last_updated + data->interval) ||
> +	    !data->valid) {
> +		dev_dbg(&client->dev, "Updating lm95241 data.\n");
> +		data->local_h > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_LOCAL_TEMPH);
> +		data->local_l > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_LOCAL_TEMPL);
> +		data->remote1_h > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_REMOTE1_TEMPH);
> +		data->remote1_l > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_REMOTE1_TEMPL);
> +		data->remote2_h > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_REMOTE2_TEMPH);
> +		data->remote2_l > +			i2c_smbus_read_byte_data(client,
> +						 LM95241_REG_R_REMOTE2_TEMPL);
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +			  char *buf);
> +
> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL);
> +static DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL);
> +static DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL);
> +
>  /* Sysfs stuff */
> -#define show_temp(value) \
> -static ssize_t show_##value(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct lm95241_data *data = lm95241_update_device(dev); \
> -	snprintf(buf, PAGE_SIZE - 1, "%d\n", \
> -		TEMP_FROM_REG(data->value##_h, data->value##_l)); \
> -	return strlen(buf); \
> -}
> -show_temp(local);
> -show_temp(remote1);
> -show_temp(remote2);
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct lm95241_data *data = lm95241_update_device(dev);
> +	int value;
>  
> -static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +	if (attr = &dev_attr_temp1_input)
> +		value = TempFromReg(data->local_h, data->local_l);
> +	else if (attr = &dev_attr_temp2_input)
> +		value = TempFromReg(data->remote1_h, data->remote1_l);
> +	else
> +		value = TempFromReg(data->remote2_h, data->remote2_l);
> +	snprintf(buf, PAGE_SIZE - 1, "%d\n", value);
> +	return strlen(buf);
> +}
> +
> +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> +			 char *buf);
> +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count);
> +
> +static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
> +static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type, set_type);
> +
> +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_type)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->model & R1MS_MASK ? "1\n" : "2\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->model & R2MS_MASK ? "1\n" : "2\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	unsigned long val;
> +
> +	if (strict_strtoul(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if (val = 1 || val = 2) {
> +		int shift;
> +		u8 mask;
> +		if (attr = &dev_attr_temp2_type) {
> +			shift = TT1_SHIFT;
> +			mask = R1MS_MASK;
> +		} else {
> +			shift = TT2_SHIFT;
> +			mask = R2MS_MASK;
> +		}
> +
> +		mutex_lock(&data->update_lock);
> +
> +		data->trutherm &= ~(TT_MASK << shift);
> +		if (val = 1) {
> +			data->model |= mask;
> +			data->trutherm |= (TT_ON << shift);
> +		} else {
> +			data->model &= ~mask;
> +			data->trutherm |= (TT_OFF << shift);
> +		}
> +
> +		data->valid = 0;
> +
> +		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
> +					  data->model);
> +		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
> +					  data->trutherm);
> +
> +		mutex_unlock(&data->update_lock);
> +
> +	}
> +	return count;
> +}
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +			char *buf);
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count);
> +
> +static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min);
> +static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min);
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_min)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R1DF_MASK ? "-127000\n" : "0\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R2DF_MASK ? "-127000\n" : "0\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	long val;
> +	u8 mask;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if (attr = &dev_attr_temp2_min)
> +		mask = R1DF_MASK;
> +	else
> +		mask = R2DF_MASK;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (val < 0)
> +		data->config |= mask;
> +	else
> +		data->config &= mask;
> +	data->valid = 0;
> +
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +			char *buf);
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count);
> +
> +static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max);
> +static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max);
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	if (attr = &dev_attr_temp2_max)
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R1DF_MASK ? "127000\n" : "255000\n");
> +	else
> +		snprintf(buf, PAGE_SIZE - 1,
> +			data->config & R2DF_MASK ? "127000\n" : "255000\n");
> +	return strlen(buf);
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +	long val;
> +	u8 mask;
> +
> +	if (strict_strtol(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	if (attr = &dev_attr_temp2_max)
> +		mask = R1DF_MASK;
> +	else
> +		mask = R2DF_MASK;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (val <= 127000)
> +		data->config |= R1DF_MASK;
> +	else
> +		data->config &= ~R2DF_MASK;
> +	data->valid = 0;
> +
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
>  	struct lm95241_data *data = lm95241_update_device(dev);
>  
>  	snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval / HZ);
> @@ -124,168 +349,20 @@ static ssize_t show_interval(struct devi
>  }
>  
>  static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> -			const char *buf, size_t count)
> +			    const char *buf, size_t count)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm95241_data *data = i2c_get_clientdata(client);
> +	unsigned long val;
> +
> +	if (strict_strtoul(buf, 10, &val) < 0)
> +		return -EINVAL;
>  
> -	strict_strtol(buf, 10, &data->interval);
> -	data->interval = data->interval * HZ / 1000;
> +	data->interval = val * HZ / 1000;
>  
>  	return count;
>  }
>  
> -#define show_type(flag) \
> -static ssize_t show_type##flag(struct device *dev, \
> -				   struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->model & R##flag##MS_MASK ? "1\n" : "2\n"); \
> -	return strlen(buf); \
> -}
> -show_type(1);
> -show_type(2);
> -
> -#define show_min(flag) \
> -static ssize_t show_min##flag(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->config & R##flag##DF_MASK ?	\
> -		"-127000\n" : "0\n"); \
> -	return strlen(buf); \
> -}
> -show_min(1);
> -show_min(2);
> -
> -#define show_max(flag) \
> -static ssize_t show_max##flag(struct device *dev, \
> -    struct device_attribute *attr, char *buf) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	snprintf(buf, PAGE_SIZE - 1, \
> -		data->config & R##flag##DF_MASK ? \
> -		"127000\n" : "255000\n"); \
> -	return strlen(buf); \
> -}
> -show_max(1);
> -show_max(2);
> -
> -#define set_type(flag) \
> -static ssize_t set_type##flag(struct device *dev, \
> -				  struct device_attribute *attr, \
> -				  const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	if ((val = 1) || (val = 2)) { \
> -\
> -		mutex_lock(&data->update_lock); \
> -\
> -		data->trutherm &= ~(TT_MASK << TT##flag##_SHIFT); \
> -		if (val = 1) { \
> -			data->model |= R##flag##MS_MASK; \
> -			data->trutherm |= (TT_ON << TT##flag##_SHIFT); \
> -		} \
> -		else { \
> -			data->model &= ~R##flag##MS_MASK; \
> -			data->trutherm |= (TT_OFF << TT##flag##_SHIFT); \
> -		} \
> -\
> -		data->valid = 0; \
> -\
> -		i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL, \
> -					  data->model); \
> -		i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM, \
> -					  data->trutherm); \
> -\
> -		mutex_unlock(&data->update_lock); \
> -\
> -	} \
> -	return count; \
> -}
> -set_type(1);
> -set_type(2);
> -
> -#define set_min(flag) \
> -static ssize_t set_min##flag(struct device *dev, \
> -	struct device_attribute *devattr, const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	mutex_lock(&data->update_lock); \
> -\
> -	if (val < 0) \
> -		data->config |= R##flag##DF_MASK; \
> -	else \
> -		data->config &= ~R##flag##DF_MASK; \
> -\
> -	data->valid = 0; \
> -\
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
> -		data->config); \
> -\
> -	mutex_unlock(&data->update_lock); \
> -\
> -	return count; \
> -}
> -set_min(1);
> -set_min(2);
> -
> -#define set_max(flag) \
> -static ssize_t set_max##flag(struct device *dev, \
> -	struct device_attribute *devattr, const char *buf, size_t count) \
> -{ \
> -	struct i2c_client *client = to_i2c_client(dev); \
> -	struct lm95241_data *data = i2c_get_clientdata(client); \
> -\
> -	long val; \
> -	strict_strtol(buf, 10, &val); \
> -\
> -	mutex_lock(&data->update_lock); \
> -\
> -	if (val <= 127000) \
> -		data->config |= R##flag##DF_MASK; \
> -	else \
> -		data->config &= ~R##flag##DF_MASK; \
> -\
> -	data->valid = 0; \
> -\
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG, \
> -		data->config); \
> -\
> -	mutex_unlock(&data->update_lock); \
> -\
> -	return count; \
> -}
> -set_max(1);
> -set_max(2);
> -
> -static DEVICE_ATTR(temp1_input, S_IRUGO, show_local, NULL);
> -static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote1, NULL);
> -static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote2, NULL);
> -static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type1, set_type1);
> -static DEVICE_ATTR(temp3_type, S_IWUSR | S_IRUGO, show_type2, set_type2);
> -static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min1, set_min1);
> -static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min2, set_min2);
> -static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max1, set_max1);
> -static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max2, set_max2);
>  static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
>  		   set_interval);
>  
> @@ -322,7 +399,7 @@ static int lm95241_detect(struct i2c_cli
>  	     = MANUFACTURER_ID)
>  	 && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
>  	     >= DEFAULT_REVISION)) {
> -		name = "lm95241";
> +		name = DEVNAME;
>  	} else {
>  		dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
>  			address);
> @@ -334,6 +411,26 @@ static int lm95241_detect(struct i2c_cli
>  	return 0;
>  }
>  
> +static void lm95241_init_client(struct i2c_client *client)
> +{
> +	struct lm95241_data *data = i2c_get_clientdata(client);
> +
> +	data->interval = HZ;    /* 1 sec default */
> +	data->valid = 0;
> +	data->config = CFG_CR0076;
> +	data->model = 0;
> +	data->trutherm = (TT_OFF << TT1_SHIFT) | (TT_OFF << TT2_SHIFT);
> +
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG,
> +				  data->config);
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REM_FILTER,
> +				  R1FE_MASK | R2FE_MASK);
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
> +				  data->trutherm);
> +	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
> +				  data->model);
> +}
> +
>  static int lm95241_probe(struct i2c_client *new_client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -373,26 +470,6 @@ exit:
>  	return err;
>  }
>  
> -static void lm95241_init_client(struct i2c_client *client)
> -{
> -	struct lm95241_data *data = i2c_get_clientdata(client);
> -
> -	data->interval = HZ;    /* 1 sec default */
> -	data->valid = 0;
> -	data->config = CFG_CR0076;
> -	data->model = 0;
> -	data->trutherm = (TT_OFF << TT1_SHIFT) | (TT_OFF << TT2_SHIFT);
> -
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_CONFIG,
> -				  data->config);
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REM_FILTER,
> -				  R1FE_MASK | R2FE_MASK);
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_TRUTHERM,
> -				  data->trutherm);
> -	i2c_smbus_write_byte_data(client, LM95241_REG_RW_REMOTE_MODEL,
> -				  data->model);
> -}
> -
>  static int lm95241_remove(struct i2c_client *client)
>  {
>  	struct lm95241_data *data = i2c_get_clientdata(client);
> @@ -404,46 +481,9 @@ static int lm95241_remove(struct i2c_cli
>  	return 0;
>  }
>  
> -static struct lm95241_data *lm95241_update_device(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct lm95241_data *data = i2c_get_clientdata(client);
> -
> -	mutex_lock(&data->update_lock);
> -
> -	if (time_after(jiffies, data->last_updated + data->interval) ||
> -	    !data->valid) {
> -		dev_dbg(&client->dev, "Updating lm95241 data.\n");
> -		data->local_h > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_LOCAL_TEMPH);
> -		data->local_l > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_LOCAL_TEMPL);
> -		data->remote1_h > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_REMOTE1_TEMPH);
> -		data->remote1_l > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_REMOTE1_TEMPL);
> -		data->remote2_h > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_REMOTE2_TEMPH);
> -		data->remote2_l > -			i2c_smbus_read_byte_data(client,
> -						 LM95241_REG_R_REMOTE2_TEMPL);
> -		data->last_updated = jiffies;
> -		data->valid = 1;
> -	}
> -
> -	mutex_unlock(&data->update_lock);
> -
> -	return data;
> -}
> -
>  /* Driver data (common to all clients) */
>  static const struct i2c_device_id lm95241_id[] = {
> -	{ "lm95241", 0 },
> +	{ DEVNAME, 0 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, lm95241_id);
> @@ -451,7 +491,7 @@ MODULE_DEVICE_TABLE(i2c, lm95241_id);
>  static struct i2c_driver lm95241_driver = {
>  	.class		= I2C_CLASS_HWMON,
>  	.driver = {
> -		.name   = "lm95241",
> +		.name   = DEVNAME,
>  	},
>  	.probe		= lm95241_probe,
>  	.remove		= lm95241_remove,
> @@ -470,9 +510,10 @@ static void __exit sensors_lm95241_exit(
>  	i2c_del_driver(&lm95241_driver);
>  }
>  
> -MODULE_AUTHOR("Davide Rizzo <elpa-rizzo@gmail.com>");
> +MODULE_AUTHOR("Davide Rizzo <elpa.rizzo@gmail.com>");
>  MODULE_DESCRIPTION("LM95241 sensor driver");
>  MODULE_LICENSE("GPL");
>  
>  module_init(sensors_lm95241_init);
>  module_exit(sensors_lm95241_exit);
> +


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