linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: LM95245 driver
@ 2011-06-21  9:52 Alexander Stein
  2011-06-22 19:15 ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-21  9:52 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck; +Cc: lm-sensors, linux-kernel, Alexander Stein

An hwmon driver for the National Semiconductor LM95245 dual temperature
sensors chip.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/hwmon/Kconfig   |    9 +
 drivers/hwmon/Makefile  |    1 +
 drivers/hwmon/lm95245.c |  388 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 398 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lm95245.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 16db83c..4c2400b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -702,6 +702,15 @@ config SENSORS_LM95241
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm95241.
 
+config SENSORS_LM95245
+	tristate "National Semiconductor LM95245 sensor chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for LM95245 sensor chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lm95245.
+
 config SENSORS_MAX1111
 	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
 	depends on SPI_MASTER
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 28061cf..5598712 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
+obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4245)	+= ltc4245.o
diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
new file mode 100644
index 0000000..21a6215
--- /dev/null
+++ b/drivers/hwmon/lm95245.c
@@ -0,0 +1,388 @@
+/*
+ * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * The LM95245 is a sensor chip made by National Semiconductors.
+ * It reports up to two temperatures (its own plus an external one).
+ * Complete datasheet can be obtained from National's website at:
+ *   http://www.national.com/ds.cgi/LM/LM95245.pdf
+ *
+ * This driver is based on lm95241.c
+ *
+ * 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 "lm95245"
+
+static const unsigned short normal_i2c[] = {
+	0x18, 0x19, 0x4c, 0x4d, I2C_CLIENT_END };
+
+/* LM95245 registers */
+/* general registers */
+#define LM95245_REG_RW_CONFIG1		0x03
+#define LM95245_REG_RW_CONVERS_RATE	0x04
+#define LM95245_REG_W_ONE_SHOT		0x0F
+
+/* diode configuration */
+#define LM95245_REG_RW_CONFIG2		0xBF
+#define LM95245_REG_RW_REMOTE_OFFH	0x11
+#define LM95245_REG_RW_REMOTE_OFFL	0x12
+
+/* status registers */
+#define LM95245_REG_R_STATUS1		0x02
+#define LM95245_REG_R_STATUS2		0x33
+
+/* limit registers */
+#define LM95245_REG_RW_REMOTE_OS_LIMIT		0x07
+#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT	0x20
+#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT	0x19
+#define LM95245_REG_RW_COMMON_HYSTERESIS	0x21
+
+/* temperature signed */
+#define LM95245_REG_R_LOCAL_TEMPH_S	0x00
+#define LM95245_REG_R_LOCAL_TEMPL_S	0x30
+#define LM95245_REG_R_REMOTE_TEMPH_S	0x01
+#define LM95245_REG_R_REMOTE_TEMPL_S	0x10
+/* temperature unsigned */
+#define LM95245_REG_R_REMOTE_TEMPH_U	0x31
+#define LM95245_REG_R_REMOTE_TEMPL_U	0x32
+
+/* id registers */
+#define LM95245_REG_R_MAN_ID		0xFE
+#define LM95245_REG_R_CHIP_ID		0xFF
+
+/* LM95245 specific bitfields */
+#define CFG_STOP		0x40
+#define CFG_REMOTE_TCRIT_MASK	0x10
+#define CFG_REMOTE_OS_MASK	0x08
+#define CFG_LOCAL_TCRIT_MASK	0x04
+#define CFG_LOCAL_OS_MASK	0x02
+
+#define CFG2_OS_A0		0x40
+#define CFG2_DIODE_FAULT_OS	0x20
+#define CFG2_DIODE_FAULT_TCRIT	0x10
+#define CFG2_REMOTE_TT		0x08
+#define CFG2_REMOTE_FILTER_DIS	0x00
+#define CFG2_REMOTE_FILTER_EN	0x06
+
+/* conversation rate in ms */
+#define RATE_CR0063 0x00
+#define RATE_CR0364 0x01
+#define RATE_CR1000 0x02
+#define RATE_CR2500 0x03
+
+#define MANUFACTURER_ID 0x01
+#define DEFAULT_REVISION 0xB3
+
+static const u8 lm95245_reg_address[] = {
+	LM95245_REG_R_LOCAL_TEMPH_S,
+	LM95245_REG_R_LOCAL_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_S,
+	LM95245_REG_R_REMOTE_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_U,
+	LM95245_REG_R_REMOTE_TEMPL_U
+};
+
+/* Client data (each client gets its own) */
+struct lm95245_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned long last_updated, interval;	/* in jiffies */
+	char valid;		/* zero until following fields are valid */
+	/* registers values */
+	u8 temp[ARRAY_SIZE(lm95245_reg_address)];
+	u8 config1, config2, rate;
+};
+
+/* Conversions */
+static int TempFromRegUnsigned(u8 val_h, u8 val_l)
+{
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+static int TempFromRegSigned(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return val_h - 0x100;
+	return TempFromRegUnsigned(val_h, val_l);
+}
+
+static struct lm95245_data *lm95245_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + data->interval) ||
+	    !data->valid) {
+		int i;
+
+		dev_dbg(&client->dev, "Updating lm95245 data.\n");
+		for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
+			data->temp[i]
+			  = i2c_smbus_read_byte_data(client,
+						     lm95245_reg_address[i]);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+/* Sysfs stuff */
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int temp;
+	int index = to_sensor_dev_attr(attr)->index;
+
+	/*
+	 * Index 0 (Local temp) is always signed
+	 * Index 2 (Remote temp) has both signed and unsigned data
+	 * use signed calculation for remote if signed bit is set
+	 */
+	if (index == 0 || data->temp[index] & 0x80)
+		temp = TempFromRegSigned(data->temp[index],
+			    data->temp[index + 1]);
+	else
+		temp = TempFromRegUnsigned(data->temp[index + 2],
+			    data->temp[index + 3]);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
+}
+
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	return snprintf(buf, PAGE_SIZE - 1,
+		data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	if (val == 1)
+		data->config2 |= CFG2_REMOTE_TT;
+	else
+		data->config2 &= ~CFG2_REMOTE_TT;
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
+				  data->config2);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%lu\n", 1000 * data->interval
+			/ HZ);
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	data->interval = val * HZ / 1000;
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
+		set_type, 0);
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
+		set_interval);
+
+static struct attribute *lm95245_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_type.dev_attr.attr,
+	&dev_attr_update_interval.attr,
+	NULL
+};
+
+static const struct attribute_group lm95245_group = {
+	.attrs = lm95245_attributes,
+};
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int lm95245_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, LM95245_REG_R_MAN_ID)
+	     == MANUFACTURER_ID)
+	    && (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
+		>= DEFAULT_REVISION)) {
+		name = DEVNAME;
+	} else {
+		dev_dbg(&adapter->dev, "LM95245 detection failed at 0x%02x\n",
+			address);
+		return -ENODEV;
+	}
+
+	/* Fill the i2c board info */
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+	return 0;
+}
+
+static void lm95245_init_client(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	data->interval = HZ;	/* 1 sec default */
+	data->valid = 0;
+	data->config1 = 0;
+	data->config2 = CFG2_DIODE_FAULT_TCRIT | CFG2_REMOTE_TT | CFG2_REMOTE_FILTER_EN;
+	data->rate = RATE_CR0063;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1, data->config1);
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2, data->config2);
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, data->rate);
+}
+
+static int lm95245_probe(struct i2c_client *new_client,
+			 const struct i2c_device_id *id)
+{
+	struct lm95245_data *data;
+	int err;
+
+	data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(new_client, data);
+	mutex_init(&data->update_lock);
+
+	/* Initialize the LM95245 chip */
+	lm95245_init_client(new_client);
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int lm95245_remove(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &lm95245_group);
+
+	kfree(data);
+	return 0;
+}
+
+/* Driver data (common to all clients) */
+static const struct i2c_device_id lm95245_id[] = {
+	{ DEVNAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm95245_id);
+
+static struct i2c_driver lm95245_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= DEVNAME,
+	},
+	.probe		= lm95245_probe,
+	.remove		= lm95245_remove,
+	.id_table	= lm95245_id,
+	.detect		= lm95245_detect,
+	.address_list	= normal_i2c,
+};
+
+static int __init sensors_lm95245_init(void)
+{
+	return i2c_add_driver(&lm95245_driver);
+}
+
+static void __exit sensors_lm95245_exit(void)
+{
+	i2c_del_driver(&lm95245_driver);
+}
+
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_DESCRIPTION("LM95245 sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_lm95245_init);
+module_exit(sensors_lm95245_exit);
-- 
1.7.3.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-21  9:52 [PATCH] hwmon: LM95245 driver Alexander Stein
@ 2011-06-22 19:15 ` Guenter Roeck
  2011-06-23  9:14   ` Alexander Stein
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2011-06-22 19:15 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Tue, 2011-06-21 at 05:52 -0400, Alexander Stein wrote:
> An hwmon driver for the National Semiconductor LM95245 dual temperature
> sensors chip.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Overall pretty well written, only I am a bit at loss why you try to deal
with the unsigned temperature registers. That adds a bit of complexity
to the driver. Given that the signed temperature value range is up to
127 degrees C, and that the chip is only rated up to 125 degrees C, the
added complexity doesn't seem to be worth it.

Can you elaborate ?

Other comments:

For the interval attribute, idea would be to write the value into the
conversion rate register. Your code does not match the interval with the
rate programmed into the chip (which is the idea), nor does it update
the rate if the interval is changed.

Chip address 0x29 is missing.

It would be nice if the driver would support limit, hysteresis, alarm,
and fault attributes.

Thanks,
Guenter



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-22 19:15 ` Guenter Roeck
@ 2011-06-23  9:14   ` Alexander Stein
  2011-06-23  9:33     ` Jean Delvare
  2011-06-23 14:47     ` Guenter Roeck
  0 siblings, 2 replies; 15+ messages in thread
From: Alexander Stein @ 2011-06-23  9:14 UTC (permalink / raw)
  To: guenter.roeck; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Wednesday 22 June 2011 21:15:49 Guenter Roeck wrote:
> On Tue, 2011-06-21 at 05:52 -0400, Alexander Stein wrote:
> > An hwmon driver for the National Semiconductor LM95245 dual temperature
> > sensors chip.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> 
> Overall pretty well written, only I am a bit at loss why you try to deal
> with the unsigned temperature registers. That adds a bit of complexity
> to the driver. Given that the signed temperature value range is up to
> 127 degrees C, and that the chip is only rated up to 125 degrees C, the
> added complexity doesn't seem to be worth it.
> 
> Can you elaborate ?

Well, AFAIK the 125 degrees C limit is for the chip itself. I didn't found 
anything about a remote diode limit. Also the remote TCRIT and OS limit range 
up to 255 degrees C.
Another point is the optional offset register (not implemented, see below). I 
could not found much about it, but I guess this is immediately added to the 
remote temperature register.

> Other comments:
> 
> For the interval attribute, idea would be to write the value into the
> conversion rate register. Your code does not match the interval with the
> rate programmed into the chip (which is the idea), nor does it update
> the rate if the interval is changed.

Well, I noticed that. But I went the way lm95241 does. I'm also unsure which 
interval to choose, if user specify a unsupported interval. Choose the next 
small or the next greater one? Maybe you can give me a hint here.

> Chip address 0x29 is missing.

Nice catch.

> It would be nice if the driver would support limit, hysteresis, alarm,
> and fault attributes.

Let's see if I find time for that.
Thanks for the review though.

Regards,
Alexander

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23  9:14   ` Alexander Stein
@ 2011-06-23  9:33     ` Jean Delvare
  2011-06-23 10:50       ` Alexander Stein
  2011-06-23 14:47     ` Guenter Roeck
  1 sibling, 1 reply; 15+ messages in thread
From: Jean Delvare @ 2011-06-23  9:33 UTC (permalink / raw)
  To: Alexander Stein; +Cc: guenter.roeck, lm-sensors, linux-kernel

On Thu, 23 Jun 2011 11:14:52 +0200, Alexander Stein wrote:
> Well, I noticed that. But I went the way lm95241 does. I'm also unsure which 
> interval to choose, if user specify a unsupported interval. Choose the next 
> small or the next greater one? Maybe you can give me a hint here.

BTW, is the LM95245 so incompatible that support couldn't be added to
the lm95241 driver? If both devices are compatible to a reasonable
degree, a single driver would be preferred.

-- 
Jean Delvare

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23  9:33     ` Jean Delvare
@ 2011-06-23 10:50       ` Alexander Stein
  2011-06-23 14:26         ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-23 10:50 UTC (permalink / raw)
  To: Jean Delvare; +Cc: guenter.roeck, lm-sensors, linux-kernel

On Thursday 23 June 2011 11:33:16 Jean Delvare wrote:
> On Thu, 23 Jun 2011 11:14:52 +0200, Alexander Stein wrote:
> > Well, I noticed that. But I went the way lm95241 does. I'm also unsure
> > which interval to choose, if user specify a unsupported interval. Choose
> > the next small or the next greater one? Maybe you can give me a hint
> > here.
> 
> BTW, is the LM95245 so incompatible that support couldn't be added to
> the lm95241 driver? If both devices are compatible to a reasonable
> degree, a single driver would be preferred.

Well, the register mapping is completly different. Also some configuration 
bits are merged and moved into other registers than in lm95241. I doubt there 
is much more compatibility than i2c transfers and temperature format (despite 
unsigned/unsigned).

Regards,
Alexander

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23 10:50       ` Alexander Stein
@ 2011-06-23 14:26         ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-06-23 14:26 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Thu, Jun 23, 2011 at 06:50:37AM -0400, Alexander Stein wrote:
> On Thursday 23 June 2011 11:33:16 Jean Delvare wrote:
> > On Thu, 23 Jun 2011 11:14:52 +0200, Alexander Stein wrote:
> > > Well, I noticed that. But I went the way lm95241 does. I'm also unsure
> > > which interval to choose, if user specify a unsupported interval. Choose
> > > the next small or the next greater one? Maybe you can give me a hint
> > > here.
> > 
> > BTW, is the LM95245 so incompatible that support couldn't be added to
> > the lm95241 driver? If both devices are compatible to a reasonable
> > degree, a single driver would be preferred.
> 
> Well, the register mapping is completly different. Also some configuration 
> bits are merged and moved into other registers than in lm95241. I doubt there 
> is much more compatibility than i2c transfers and temperature format (despite 
> unsigned/unsigned).
> 
Same conclusion here. Odd but true.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23  9:14   ` Alexander Stein
  2011-06-23  9:33     ` Jean Delvare
@ 2011-06-23 14:47     ` Guenter Roeck
  2011-06-23 15:14       ` Alexander Stein
  1 sibling, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2011-06-23 14:47 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Thu, Jun 23, 2011 at 05:14:52AM -0400, Alexander Stein wrote:
> On Wednesday 22 June 2011 21:15:49 Guenter Roeck wrote:
> > On Tue, 2011-06-21 at 05:52 -0400, Alexander Stein wrote:
> > > An hwmon driver for the National Semiconductor LM95245 dual temperature
> > > sensors chip.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > 
> > Overall pretty well written, only I am a bit at loss why you try to deal
> > with the unsigned temperature registers. That adds a bit of complexity
> > to the driver. Given that the signed temperature value range is up to
> > 127 degrees C, and that the chip is only rated up to 125 degrees C, the
> > added complexity doesn't seem to be worth it.
> > 
> > Can you elaborate ?
> 
> Well, AFAIK the 125 degrees C limit is for the chip itself. I didn't found 
> anything about a remote diode limit. Also the remote TCRIT and OS limit range 
> up to 255 degrees C.

Specified accuracy range for the external diode is up to 125 degrees C.

Jean, do you have an opinion here ? I just don't think the added complexity
is worth the theoretical gain in functionality.

> Another point is the optional offset register (not implemented, see below). I 
> could not found much about it, but I guess this is immediately added to the 
> remote temperature register.
> 
You could set it to some value and see what happens.

> > Other comments:
> > 
> > For the interval attribute, idea would be to write the value into the
> > conversion rate register. Your code does not match the interval with the
> > rate programmed into the chip (which is the idea), nor does it update
> > the rate if the interval is changed.
> 
> Well, I noticed that. But I went the way lm95241 does. I'm also unsure which 
> interval to choose, if user specify a unsupported interval. Choose the next 
> small or the next greater one? Maybe you can give me a hint here.
> 
Two bad or wrong implementations don't make it a good one. If the value can be
configured into the lm95241, the code should do it.

The lm90 driver tries to find the closest match, which would be one way to go
and is what I usually do. Another possibility would be to select the next larger
valid interval.

> > Chip address 0x29 is missing.
> 
> Nice catch.
> 
> > It would be nice if the driver would support limit, hysteresis, alarm,
> > and fault attributes.
> 
> Let's see if I find time for that.
> Thanks for the review though.
> 
Um ... that wasn't a review. Just browsing through the code ;).

Another thing I noticed: You are re-configuring the chip during initialization.
This is even though the chip may already have been configured through ROMMON and/or BIOS,
and specifically overrides the external sensor type. That is not a good idea; even
though it may make sense in your application, it may screw up chip configuration
for other users of the chip.

If your BIOS/Firmware does not configure the chip, and you really have to do it
in the driver, one option would be to provide configuration data through optional
platform data.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23 14:47     ` Guenter Roeck
@ 2011-06-23 15:14       ` Alexander Stein
  2011-06-23 15:35         ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-23 15:14 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Thursday 23 June 2011 16:47:55 Guenter Roeck wrote:
> > Another point is the optional offset register (not implemented, see
> > below). I could not found much about it, but I guess this is immediately
> > added to the remote temperature register.
> 
> You could set it to some value and see what happens.

I might look into that, when I implement the offset feature.

> > > Other comments:
> > > 
> > > For the interval attribute, idea would be to write the value into the
> > > conversion rate register. Your code does not match the interval with
> > > the rate programmed into the chip (which is the idea), nor does it
> > > update the rate if the interval is changed.
> > 
> > Well, I noticed that. But I went the way lm95241 does. I'm also unsure
> > which interval to choose, if user specify a unsupported interval. Choose
> > the next small or the next greater one? Maybe you can give me a hint
> > here.
> 
> Two bad or wrong implementations don't make it a good one. If the value can
> be configured into the lm95241, the code should do it.
> 
> The lm90 driver tries to find the closest match, which would be one way to
> go and is what I usually do. Another possibility would be to select the
> next larger valid interval.

I think the closest match might be the best. As there is fixed set of 
possibilities, what do think about printing the possiblities upon read and 
marking the used one by an asterisk?

> Another thing I noticed: You are re-configuring the chip during
> initialization. This is even though the chip may already have been
> configured through ROMMON and/or BIOS, and specifically overrides the
> external sensor type. That is not a good idea; even though it may make
> sense in your application, it may screw up chip configuration for other
> users of the chip.
>
> If your BIOS/Firmware does not configure the chip, and you really have to
> do it in the driver, one option would be to provide configuration data
> through optional platform data.

Ehm, where do I add platform data on x86? It gets detected by device probing. 
For boards like arm, sure no problem there.

Reagrds,
Alexander

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hwmon: LM95245 driver
  2011-06-23 15:14       ` Alexander Stein
@ 2011-06-23 15:35         ` Guenter Roeck
  2011-06-27 15:49           ` [PATCH v2] " Alexander Stein
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2011-06-23 15:35 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Thu, Jun 23, 2011 at 11:14:19AM -0400, Alexander Stein wrote:
> On Thursday 23 June 2011 16:47:55 Guenter Roeck wrote:
> > > Another point is the optional offset register (not implemented, see
> > > below). I could not found much about it, but I guess this is immediately
> > > added to the remote temperature register.
> > 
> > You could set it to some value and see what happens.
> 
> I might look into that, when I implement the offset feature.
> 
> > > > Other comments:
> > > > 
> > > > For the interval attribute, idea would be to write the value into the
> > > > conversion rate register. Your code does not match the interval with
> > > > the rate programmed into the chip (which is the idea), nor does it
> > > > update the rate if the interval is changed.
> > > 
> > > Well, I noticed that. But I went the way lm95241 does. I'm also unsure
> > > which interval to choose, if user specify a unsupported interval. Choose
> > > the next small or the next greater one? Maybe you can give me a hint
> > > here.
> > 
> > Two bad or wrong implementations don't make it a good one. If the value can
> > be configured into the lm95241, the code should do it.
> > 
> > The lm90 driver tries to find the closest match, which would be one way to
> > go and is what I usually do. Another possibility would be to select the
> > next larger valid interval.
> 
> I think the closest match might be the best. As there is fixed set of 
> possibilities, what do think about printing the possiblities upon read and 
> marking the used one by an asterisk?
> 
Please stick with the ABI. libsensors won't be able to decode it if you make it
fancy, and it would violate the one-object-per-entry rule for sysfs attributes.

> > Another thing I noticed: You are re-configuring the chip during
> > initialization. This is even though the chip may already have been
> > configured through ROMMON and/or BIOS, and specifically overrides the
> > external sensor type. That is not a good idea; even though it may make
> > sense in your application, it may screw up chip configuration for other
> > users of the chip.
> >
> > If your BIOS/Firmware does not configure the chip, and you really have to
> > do it in the driver, one option would be to provide configuration data
> > through optional platform data.
> 
> Ehm, where do I add platform data on x86? It gets detected by device probing. 
> For boards like arm, sure no problem there.
> 
Good point ;).

Question is if you really need to reconfigure the chip. On X86 that should really have been
done by the BIOS. Only reasonable option here would really be to enable the chip
if it is disabled by default, and leave the rest to userspace based initialization
(ie use sensors3.conf to set desired values).

For most of the configured values (such as the update interval) it doesn't matter much
if you overwrite it when starting the driver. I am mostly concerned about the diode type,
which is the one relevant piece of configuration which should be known to the firmware
and should already be configured to the correct value when the driver is started. Even if
that is not the case with your board, it might and should be for other boards.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] hwmon: LM95245 driver
  2011-06-23 15:35         ` Guenter Roeck
@ 2011-06-27 15:49           ` Alexander Stein
  2011-06-27 20:21             ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-27 15:49 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, lm-sensors, linux-kernel, Alexander Stein

An hwmon driver for the National Semiconductor LM95245 dual temperature
sensors chip.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* The diode mode isn't overwritten upon probe
* Added crit, crit_hyst, alarm, fault
* Added i2c address 0x29

I didn't change the signed/unsigned part. I also didn't include offset support.
Strangely enough the alarmfor remote (temp2) is set everytime I tested it,
where temp is at +47.4 C and crit is 110 C with hyst at 10 C. I don't have any
explanation for that.

 drivers/hwmon/Kconfig   |    9 +
 drivers/hwmon/Makefile  |    1 +
 drivers/hwmon/lm95245.c |  532 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 542 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lm95245.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 16db83c..4c2400b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -702,6 +702,15 @@ config SENSORS_LM95241
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm95241.
 
+config SENSORS_LM95245
+	tristate "National Semiconductor LM95245 sensor chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for LM95245 sensor chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lm95245.
+
 config SENSORS_MAX1111
 	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
 	depends on SPI_MASTER
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 28061cf..5598712 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
+obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4245)	+= ltc4245.o
diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
new file mode 100644
index 0000000..1d93086
--- /dev/null
+++ b/drivers/hwmon/lm95245.c
@@ -0,0 +1,532 @@
+/*
+ * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * The LM95245 is a sensor chip made by National Semiconductors.
+ * It reports up to two temperatures (its own plus an external one).
+ * Complete datasheet can be obtained from National's website at:
+ *   http://www.national.com/ds.cgi/LM/LM95245.pdf
+ *
+ * This driver is based on lm95241.c
+ *
+ * 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 "lm95245"
+
+static const unsigned short normal_i2c[] = {
+	0x18, 0x19, 0x29, 0x4c, 0x4d, I2C_CLIENT_END };
+
+/* LM95245 registers */
+/* general registers */
+#define LM95245_REG_RW_CONFIG1		0x03
+#define LM95245_REG_RW_CONVERS_RATE	0x04
+#define LM95245_REG_W_ONE_SHOT		0x0F
+
+/* diode configuration */
+#define LM95245_REG_RW_CONFIG2		0xBF
+#define LM95245_REG_RW_REMOTE_OFFH	0x11
+#define LM95245_REG_RW_REMOTE_OFFL	0x12
+
+/* status registers */
+#define LM95245_REG_R_STATUS1		0x02
+#define LM95245_REG_R_STATUS2		0x33
+
+/* limit registers */
+#define LM95245_REG_RW_REMOTE_OS_LIMIT		0x07
+#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT	0x20
+#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT	0x19
+#define LM95245_REG_RW_COMMON_HYSTERESIS	0x21
+
+/* temperature signed */
+#define LM95245_REG_R_LOCAL_TEMPH_S	0x00
+#define LM95245_REG_R_LOCAL_TEMPL_S	0x30
+#define LM95245_REG_R_REMOTE_TEMPH_S	0x01
+#define LM95245_REG_R_REMOTE_TEMPL_S	0x10
+/* temperature unsigned */
+#define LM95245_REG_R_REMOTE_TEMPH_U	0x31
+#define LM95245_REG_R_REMOTE_TEMPL_U	0x32
+
+/* id registers */
+#define LM95245_REG_R_MAN_ID		0xFE
+#define LM95245_REG_R_CHIP_ID		0xFF
+
+/* LM95245 specific bitfields */
+#define CFG_STOP		0x40
+#define CFG_REMOTE_TCRIT_MASK	0x10
+#define CFG_REMOTE_OS_MASK	0x08
+#define CFG_LOCAL_TCRIT_MASK	0x04
+#define CFG_LOCAL_OS_MASK	0x02
+
+#define CFG2_OS_A0		0x40
+#define CFG2_DIODE_FAULT_OS	0x20
+#define CFG2_DIODE_FAULT_TCRIT	0x10
+#define CFG2_REMOTE_TT		0x08
+#define CFG2_REMOTE_FILTER_DIS	0x00
+#define CFG2_REMOTE_FILTER_EN	0x06
+
+/* conversation rate in ms */
+#define RATE_CR0063 0x00
+#define RATE_CR0364 0x01
+#define RATE_CR1000 0x02
+#define RATE_CR2500 0x03
+
+#define STATUS1_BUSY		0x80
+#define STATUS1_ROS		0x10
+#define STATUS1_DIODE_FAULT	0x04
+#define STATUS1_RTCRIT		0x02
+#define STATUS1_LOC		0x01
+
+#define MANUFACTURER_ID 0x01
+#define DEFAULT_REVISION 0xB3
+
+static const u8 lm95245_reg_address[] = {
+	LM95245_REG_R_LOCAL_TEMPH_S,
+	LM95245_REG_R_LOCAL_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_S,
+	LM95245_REG_R_REMOTE_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_U,
+	LM95245_REG_R_REMOTE_TEMPL_U,
+	LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT,
+	LM95245_REG_RW_REMOTE_TCRIT_LIMIT,
+	LM95245_REG_RW_COMMON_HYSTERESIS,
+	LM95245_REG_R_STATUS1,
+};
+
+/* Client data (each client gets its own) */
+struct lm95245_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned long last_updated;	/* in jiffies */
+	unsigned long interval;	/* in msecs */
+	char valid;		/* zero until following fields are valid */
+	/* registers values */
+	u8 temp[ARRAY_SIZE(lm95245_reg_address)];
+	u8 config1, config2;
+};
+
+/* Conversions */
+static int TempFromRegUnsigned(u8 val_h, u8 val_l)
+{
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+static int TempFromRegSigned(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return val_h - 0x100;
+	return TempFromRegUnsigned(val_h, val_l);
+}
+
+static struct lm95245_data *lm95245_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + msecs_to_jiffies(data->interval)) ||
+	    !data->valid) {
+		int i;
+
+		dev_dbg(&client->dev, "Updating lm95245 data.\n");
+		for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
+			data->temp[i]
+			  = i2c_smbus_read_byte_data(client,
+						     lm95245_reg_address[i]);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static unsigned long lm95245_set_conversation_rate(struct i2c_client *client, unsigned long interval)
+{
+	int rate;
+
+	if (interval < 63) {
+		interval = 63;
+		rate = RATE_CR0063;
+	} else if (interval < 364) {
+		interval = 364;
+		rate = RATE_CR0364;
+	} else if (interval < 1000) {
+		interval = 1000;
+		rate = RATE_CR1000;
+	} else {
+		interval = 2500;
+		rate = RATE_CR2500;
+	}
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, rate);
+
+	return interval;
+}
+
+/* Sysfs stuff */
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int temp;
+	int index = to_sensor_dev_attr(attr)->index;
+
+	/*
+	 * Index 0 (Local temp) is always signed
+	 * Index 2 (Remote temp) has both signed and unsigned data
+	 * use signed calculation for remote if signed bit is set
+	 */
+	if (index == 0 || data->temp[index] & 0x80)
+		temp = TempFromRegSigned(data->temp[index],
+			    data->temp[index + 1]);
+	else
+		temp = TempFromRegUnsigned(data->temp[index + 2],
+			    data->temp[index + 3]);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
+}
+
+static ssize_t show_crit(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+	int crit;
+
+	crit = TempFromRegUnsigned(data->temp[index], 0);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", crit);
+}
+static ssize_t set_crit(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	int index = to_sensor_dev_attr(attr)->index;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, (index == 6 ? 127 : 256));
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	if (index == 6)
+		/* local crit */
+		i2c_smbus_write_byte_data(client,
+			LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT, val);
+	else if (index == 7)
+		/* remote crit */
+		i2c_smbus_write_byte_data(client,
+			LM95245_REG_RW_REMOTE_TCRIT_LIMIT, val);
+	else
+		return -EINVAL;
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, 31);
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	/* shared crit hysteresis */
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS, val);
+
+	mutex_unlock(&data->update_lock);
+
+	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 lm95245_data *data = i2c_get_clientdata(client);
+
+	return snprintf(buf, PAGE_SIZE - 1,
+		data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
+}
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	if (val == 1)
+		data->config2 |= CFG2_REMOTE_TT;
+	else
+		data->config2 &= ~CFG2_REMOTE_TT;
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
+				  data->config2);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+	bool alarm;
+
+	mutex_lock(&data->update_lock);
+
+	alarm = lm95245_reg_address[9] & index;
+
+	mutex_unlock(&data->update_lock);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", !!alarm);
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%lu\n", data->interval);
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	data->interval = lm95245_set_conversation_rate(client, val);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_crit, set_crit, 6);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_crit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, STATUS1_LOC);
+
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_crit, set_crit, 7);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_crit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
+		set_type, 0);
+static SENSOR_DEVICE_ATTR(temp2_alarm, S_IRUGO, show_alarm, NULL, STATUS1_RTCRIT);
+static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL, STATUS1_DIODE_FAULT);
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
+		set_interval);
+
+static struct attribute *lm95245_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_type.dev_attr.attr,
+	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_fault.dev_attr.attr,
+	&dev_attr_update_interval.attr,
+	NULL
+};
+
+static const struct attribute_group lm95245_group = {
+	.attrs = lm95245_attributes,
+};
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int lm95245_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, LM95245_REG_R_MAN_ID)
+	     == MANUFACTURER_ID)
+	    && (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
+		>= DEFAULT_REVISION)) {
+		name = DEVNAME;
+	} else {
+		dev_dbg(&adapter->dev, "LM95245 detection failed at 0x%02x\n",
+			address);
+		return -ENODEV;
+	}
+
+	/* Fill the i2c board info */
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+	return 0;
+}
+
+static void lm95245_init_client(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	data->interval = 1000;	/* 1 sec default */
+	data->valid = 0;
+	data->config1 = 0;
+
+	data->config2 = i2c_smbus_read_byte_data(client, data->config2);
+
+	/* Do not set diode model in ervery case */
+	data->config2 |= CFG2_DIODE_FAULT_TCRIT | CFG2_REMOTE_FILTER_EN;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1, data->config1);
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2, data->config2);
+	lm95245_set_conversation_rate(client, data->interval);
+}
+
+static int lm95245_probe(struct i2c_client *new_client,
+			 const struct i2c_device_id *id)
+{
+	struct lm95245_data *data;
+	int err;
+
+	data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(new_client, data);
+	mutex_init(&data->update_lock);
+
+	/* Initialize the LM95245 chip */
+	lm95245_init_client(new_client);
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int lm95245_remove(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &lm95245_group);
+
+	kfree(data);
+	return 0;
+}
+
+/* Driver data (common to all clients) */
+static const struct i2c_device_id lm95245_id[] = {
+	{ DEVNAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm95245_id);
+
+static struct i2c_driver lm95245_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= DEVNAME,
+	},
+	.probe		= lm95245_probe,
+	.remove		= lm95245_remove,
+	.id_table	= lm95245_id,
+	.detect		= lm95245_detect,
+	.address_list	= normal_i2c,
+};
+
+static int __init sensors_lm95245_init(void)
+{
+	return i2c_add_driver(&lm95245_driver);
+}
+
+static void __exit sensors_lm95245_exit(void)
+{
+	i2c_del_driver(&lm95245_driver);
+}
+
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_DESCRIPTION("LM95245 sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_lm95245_init);
+module_exit(sensors_lm95245_exit);
-- 
1.7.3.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] hwmon: LM95245 driver
  2011-06-27 15:49           ` [PATCH v2] " Alexander Stein
@ 2011-06-27 20:21             ` Guenter Roeck
  2011-06-28  7:07               ` [PATCH v3] " Alexander Stein
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2011-06-27 20:21 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

Hi Alexander,

On Mon, Jun 27, 2011 at 11:49:09AM -0400, Alexander Stein wrote:
> An hwmon driver for the National Semiconductor LM95245 dual temperature
> sensors chip.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Now for a real review ...

Please make sure the driver does not generate any checkpatch errors or warnings.

> ---
> Changes in v2:
> * The diode mode isn't overwritten upon probe
> * Added crit, crit_hyst, alarm, fault
> * Added i2c address 0x29
> 
> I didn't change the signed/unsigned part. I also didn't include offset support.
> Strangely enough the alarmfor remote (temp2) is set everytime I tested it,
> where temp is at +47.4 C and crit is 110 C with hyst at 10 C. I don't have any
> explanation for that.
> 
Bug in your code - see below.

>  drivers/hwmon/Kconfig   |    9 +
>  drivers/hwmon/Makefile  |    1 +
>  drivers/hwmon/lm95245.c |  532 +++++++++++++++++++++++++++++++++++++++++++++++

Please provide Documentation/hwmon/lm95245.

>  3 files changed, 542 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/lm95245.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 16db83c..4c2400b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -702,6 +702,15 @@ config SENSORS_LM95241
>           This driver can also be built as a module.  If so, the module
>           will be called lm95241.
> 
> +config SENSORS_LM95245
> +       tristate "National Semiconductor LM95245 sensor chip"
> +       depends on I2C

		&& EXPERIMENTAL

> +       help
> +         If you say yes here you get support for LM95245 sensor chip.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called lm95245.
> +
>  config SENSORS_MAX1111
>         tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
>         depends on SPI_MASTER
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 28061cf..5598712 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)    += lm90.o
>  obj-$(CONFIG_SENSORS_LM92)     += lm92.o
>  obj-$(CONFIG_SENSORS_LM93)     += lm93.o
>  obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
> +obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>  obj-$(CONFIG_SENSORS_LTC4215)  += ltc4215.o
>  obj-$(CONFIG_SENSORS_LTC4245)  += ltc4245.o
> diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
> new file mode 100644
> index 0000000..1d93086
> --- /dev/null
> +++ b/drivers/hwmon/lm95245.c
> @@ -0,0 +1,532 @@
> +/*
> + * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
> + *
> + * The LM95245 is a sensor chip made by National Semiconductors.
> + * It reports up to two temperatures (its own plus an external one).
> + * Complete datasheet can be obtained from National's website at:
> + *   http://www.national.com/ds.cgi/LM/LM95245.pdf
> + *
> + * This driver is based on lm95241.c
> + *
> + * 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 "lm95245"
> +
> +static const unsigned short normal_i2c[] = {
> +       0x18, 0x19, 0x29, 0x4c, 0x4d, I2C_CLIENT_END };
> +
> +/* LM95245 registers */
> +/* general registers */
> +#define LM95245_REG_RW_CONFIG1         0x03
> +#define LM95245_REG_RW_CONVERS_RATE    0x04
> +#define LM95245_REG_W_ONE_SHOT         0x0F
> +
> +/* diode configuration */
> +#define LM95245_REG_RW_CONFIG2         0xBF
> +#define LM95245_REG_RW_REMOTE_OFFH     0x11
> +#define LM95245_REG_RW_REMOTE_OFFL     0x12
> +
> +/* status registers */
> +#define LM95245_REG_R_STATUS1          0x02
> +#define LM95245_REG_R_STATUS2          0x33
> +
> +/* limit registers */
> +#define LM95245_REG_RW_REMOTE_OS_LIMIT         0x07
> +#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT    0x20
> +#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT      0x19
> +#define LM95245_REG_RW_COMMON_HYSTERESIS       0x21
> +
> +/* temperature signed */
> +#define LM95245_REG_R_LOCAL_TEMPH_S    0x00
> +#define LM95245_REG_R_LOCAL_TEMPL_S    0x30
> +#define LM95245_REG_R_REMOTE_TEMPH_S   0x01
> +#define LM95245_REG_R_REMOTE_TEMPL_S   0x10
> +/* temperature unsigned */
> +#define LM95245_REG_R_REMOTE_TEMPH_U   0x31
> +#define LM95245_REG_R_REMOTE_TEMPL_U   0x32
> +
> +/* id registers */
> +#define LM95245_REG_R_MAN_ID           0xFE
> +#define LM95245_REG_R_CHIP_ID          0xFF
> +
> +/* LM95245 specific bitfields */
> +#define CFG_STOP               0x40
> +#define CFG_REMOTE_TCRIT_MASK  0x10
> +#define CFG_REMOTE_OS_MASK     0x08
> +#define CFG_LOCAL_TCRIT_MASK   0x04
> +#define CFG_LOCAL_OS_MASK      0x02
> +
> +#define CFG2_OS_A0             0x40
> +#define CFG2_DIODE_FAULT_OS    0x20
> +#define CFG2_DIODE_FAULT_TCRIT 0x10
> +#define CFG2_REMOTE_TT         0x08
> +#define CFG2_REMOTE_FILTER_DIS 0x00
> +#define CFG2_REMOTE_FILTER_EN  0x06
> +
> +/* conversation rate in ms */
> +#define RATE_CR0063 0x00
> +#define RATE_CR0364 0x01
> +#define RATE_CR1000 0x02
> +#define RATE_CR2500 0x03
> +
Please use tabs for alignment.

> +#define STATUS1_BUSY           0x80
> +#define STATUS1_ROS            0x10

Unless you plan to use the above OS and BUSY related defines, might as well drop it.

Would it make sense to support the "OS" limits, possibly as "max" limits ?

I don't really understand the logic in the datasheet, though,
since the OS limits seem to be intended for "shutdown", yet
the "critical" limit is higher by default.

> +#define STATUS1_DIODE_FAULT    0x04
> +#define STATUS1_RTCRIT         0x02
> +#define STATUS1_LOC            0x01
> +
> +#define MANUFACTURER_ID 0x01
> +#define DEFAULT_REVISION 0xB3
> +
Please use tabs for alignment.

> +static const u8 lm95245_reg_address[] = {
> +       LM95245_REG_R_LOCAL_TEMPH_S,
> +       LM95245_REG_R_LOCAL_TEMPL_S,
> +       LM95245_REG_R_REMOTE_TEMPH_S,
> +       LM95245_REG_R_REMOTE_TEMPL_S,
> +       LM95245_REG_R_REMOTE_TEMPH_U,
> +       LM95245_REG_R_REMOTE_TEMPL_U,
> +       LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT,
> +       LM95245_REG_RW_REMOTE_TCRIT_LIMIT,
> +       LM95245_REG_RW_COMMON_HYSTERESIS,
> +       LM95245_REG_R_STATUS1,
> +};
> +
> +/* Client data (each client gets its own) */
> +struct lm95245_data {
> +       struct device *hwmon_dev;
> +       struct mutex update_lock;
> +       unsigned long last_updated;     /* in jiffies */
> +       unsigned long interval; /* in msecs */
> +       char valid;             /* zero until following fields are valid */
> +       /* registers values */
> +       u8 temp[ARRAY_SIZE(lm95245_reg_address)];

A better name for the array might be regs[] or similar, since you use it
for values other than temperatures.

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

Should this be (val_h - 0x100) * 1000 ?

> +       return TempFromRegUnsigned(val_h, val_l);
> +}
> +
> +static struct lm95245_data *lm95245_update_device(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       mutex_lock(&data->update_lock);
> +
> +       if (time_after(jiffies, data->last_updated + msecs_to_jiffies(data->interval)) ||
> +           !data->valid) {
> +               int i;
> +
> +               dev_dbg(&client->dev, "Updating lm95245 data.\n");
> +               for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
> +                       data->temp[i]
> +                         = i2c_smbus_read_byte_data(client,
> +                                                    lm95245_reg_address[i]);
> +               data->last_updated = jiffies;
> +               data->valid = 1;
> +       }
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return data;
> +}
> +
> +static unsigned long lm95245_set_conversation_rate(struct i2c_client *client, unsigned long interval)
> +{

Longer than 80 columns (for which you get a checkpatch warning),
and maybe consider renaming the function to lm95245_set_conversion_rate ?

> +       int rate;
> +
> +       if (interval < 63) {
> +               interval = 63;
> +               rate = RATE_CR0063;
> +       } else if (interval < 364) {
> +               interval = 364;
> +               rate = RATE_CR0364;
> +       } else if (interval < 1000) {
> +               interval = 1000;
> +               rate = RATE_CR1000;
> +       } else {
> +               interval = 2500;
> +               rate = RATE_CR2500;
> +       }
> +
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, rate);
> +
> +       return interval;
> +}
> +
> +/* Sysfs stuff */
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +                         char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int temp;
> +       int index = to_sensor_dev_attr(attr)->index;
> +
> +       /*
> +        * Index 0 (Local temp) is always signed
> +        * Index 2 (Remote temp) has both signed and unsigned data
> +        * use signed calculation for remote if signed bit is set
> +        */
> +       if (index == 0 || data->temp[index] & 0x80)
> +               temp = TempFromRegSigned(data->temp[index],
> +                           data->temp[index + 1]);
> +       else
> +               temp = TempFromRegUnsigned(data->temp[index + 2],
> +                           data->temp[index + 3]);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
> +}
> +
> +static ssize_t show_crit(struct device *dev, struct device_attribute *attr,
> +                        char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int index = to_sensor_dev_attr(attr)->index;
> +       int crit;
> +
> +       crit = TempFromRegUnsigned(data->temp[index], 0);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n", crit);

	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
			data->temp[index] * 1000);

would be less complex and accomplish the same.

> +}

Empty line between functions, please.

> +static ssize_t set_crit(struct device *dev, struct device_attribute *attr,
> +                       const char *buf, size_t count)
> +{

set_limit and show_limit might be better names here, since you use the functions for
more than "crit" temperatures.

> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +       int index = to_sensor_dev_attr(attr)->index;
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       val /= 1000;
> +
> +       val = SENSORS_LIMIT(val, 0, (index == 6 ? 127 : 256));
> +
	256 -> 255

> +       mutex_lock(&data->update_lock);
> +
> +       data->valid = 0;
> +
> +       if (index == 6)
> +               /* local crit */
> +               i2c_smbus_write_byte_data(client,
> +                       LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT, val);
> +       else if (index == 7)
> +               /* remote crit */
> +               i2c_smbus_write_byte_data(client,
> +                       LM95245_REG_RW_REMOTE_TCRIT_LIMIT, val);
> +       else
> +               return -EINVAL;
> +
You can simplify this code to
        i2c_smbus_write_byte_data(client, lm95245_reg_address[index], val);

> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}

Empty line between functions, please.

> +static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
> +                       const char *buf, size_t count)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       val /= 1000;
> +
> +       val = SENSORS_LIMIT(val, 0, 31);
> +
> +       mutex_lock(&data->update_lock);
> +
> +       data->valid = 0;
> +
> +       /* shared crit hysteresis */
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS, val);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       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 lm95245_data *data = i2c_get_clientdata(client);
> +
> +       return snprintf(buf, PAGE_SIZE - 1,
> +               data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
> +}

Empty line between functions, please.

> +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 lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +       if (val != 1 && val != 2)
> +               return -EINVAL;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       if (val == 1)
> +               data->config2 |= CFG2_REMOTE_TT;
> +       else
> +               data->config2 &= ~CFG2_REMOTE_TT;
> +
> +       data->valid = 0;
> +
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
> +                                 data->config2);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}
> +
> +static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
> +                        char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int index = to_sensor_dev_attr(attr)->index;
> +       bool alarm;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       alarm = lm95245_reg_address[9] & index;
> +

That may explain your problem with the always-on alarm... you are using
the register address, not the register value. You might want to try
        alarm = data->temp[9] & index;
for better results.

> +       mutex_unlock(&data->update_lock);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n", !!alarm);

Locks are not needed here. Also,

	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
                        !!(data->temp[9] & index));

would accomplish the same.

If you want to keep the alarm variable, you don't need !!alarm,
since the assignment to bool alarm already converts the numeric value to {0, 1}.

> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +                            char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%lu\n", data->interval);
> +}
> +
> +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 lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       data->interval = lm95245_set_conversation_rate(client, val);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_crit, set_crit, 6);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_crit,
> +               set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, STATUS1_LOC);
> +
This should be temp1_crit_alarm, since you use "crit" for the limits as well.

> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_crit, set_crit, 7);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_crit,
> +               set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
> +               set_type, 0);
> +static SENSOR_DEVICE_ATTR(temp2_alarm, S_IRUGO, show_alarm, NULL, STATUS1_RTCRIT);

temp2_crit_alarm

> +static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL, STATUS1_DIODE_FAULT);
> +
> +static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
> +               set_interval);
> +
> +static struct attribute *lm95245_attributes[] = {
> +       &sensor_dev_attr_temp1_input.dev_attr.attr,
> +       &sensor_dev_attr_temp1_crit.dev_attr.attr,
> +       &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +       &sensor_dev_attr_temp1_alarm.dev_attr.attr,
> +       &sensor_dev_attr_temp2_input.dev_attr.attr,
> +       &sensor_dev_attr_temp2_crit.dev_attr.attr,
> +       &sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
> +       &sensor_dev_attr_temp2_type.dev_attr.attr,
> +       &sensor_dev_attr_temp2_alarm.dev_attr.attr,
> +       &sensor_dev_attr_temp2_fault.dev_attr.attr,
> +       &dev_attr_update_interval.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group lm95245_group = {
> +       .attrs = lm95245_attributes,
> +};
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int lm95245_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, LM95245_REG_R_MAN_ID)
> +            == MANUFACTURER_ID)
> +           && (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
> +               >= DEFAULT_REVISION)) {

This should be ==. We'll also have to change that for the LM95241 driver,
which accepts the LM95245 since it uses >= as well. Also, there is an unnecessary
set of ().

> +               name = DEVNAME;
> +       } else {
> +               dev_dbg(&adapter->dev, "LM95245 detection failed at 0x%02x\n",
> +                       address);

There should be no message here, even debugging. Otherwise there is lot of
debugging noise for each supported address (and each chip failing to be detected
on it). Again something to change for the LM95241 driver.

Also,
        if (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_MAN_ID)
            != MANUFACTURER_ID
            || i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
            != DEFAULT_REVISION) {
                return -ENODEV;

        strlcpy(info->type, DEVNAME, I2C_NAME_SIZE);

would be much simpler.

> +               return -ENODEV;
> +       }
> +
> +       /* Fill the i2c board info */

This comment does not really provide any value.

> +       strlcpy(info->type, name, I2C_NAME_SIZE);
> +       return 0;
> +}
> +
> +static void lm95245_init_client(struct i2c_client *client)
> +{
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       data->interval = 1000;  /* 1 sec default */
> +       data->valid = 0;
> +       data->config1 = 0;
> +
config1 defaults to 0 after poweron, so if it is different this was
set on purpose by BIOS/ROMMON. I don't think you should override that,
except for clearing bit 6 to enable the chip if it was disabled.

> +       data->config2 = i2c_smbus_read_byte_data(client, data->config2);
> +
> +       /* Do not set diode model in ervery case */
> +       data->config2 |= CFG2_DIODE_FAULT_TCRIT | CFG2_REMOTE_FILTER_EN;
> +

Both bit masks are already set after chip reset, so if they are disabled it
was done on purpose by the BIOS/ROMMON. I don't think you should override that.

> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1, data->config1);
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2, data->config2);
> +       lm95245_set_conversation_rate(client, data->interval);
> +}
> +
> +static int lm95245_probe(struct i2c_client *new_client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct lm95245_data *data;
> +       int err;
> +
> +       data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
> +       if (!data) {
> +               err = -ENOMEM;
> +               goto exit;
> +       }
> +
> +       i2c_set_clientdata(new_client, data);
> +       mutex_init(&data->update_lock);
> +
> +       /* Initialize the LM95245 chip */
> +       lm95245_init_client(new_client);
> +
> +       /* Register sysfs hooks */
> +       err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
> +exit_free:
> +       kfree(data);
> +exit:
> +       return err;
> +}
> +
> +static int lm95245_remove(struct i2c_client *client)
> +{
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       hwmon_device_unregister(data->hwmon_dev);
> +       sysfs_remove_group(&client->dev.kobj, &lm95245_group);
> +
> +       kfree(data);
> +       return 0;
> +}
> +
> +/* Driver data (common to all clients) */
> +static const struct i2c_device_id lm95245_id[] = {
> +       { DEVNAME, 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm95245_id);
> +
> +static struct i2c_driver lm95245_driver = {
> +       .class          = I2C_CLASS_HWMON,
> +       .driver = {
> +               .name   = DEVNAME,
> +       },
> +       .probe          = lm95245_probe,
> +       .remove         = lm95245_remove,
> +       .id_table       = lm95245_id,
> +       .detect         = lm95245_detect,
> +       .address_list   = normal_i2c,
> +};
> +
> +static int __init sensors_lm95245_init(void)
> +{
> +       return i2c_add_driver(&lm95245_driver);
> +}
> +
> +static void __exit sensors_lm95245_exit(void)
> +{
> +       i2c_del_driver(&lm95245_driver);
> +}
> +
> +MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
> +MODULE_DESCRIPTION("LM95245 sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_lm95245_init);
> +module_exit(sensors_lm95245_exit);
> --
> 1.7.3.4
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] hwmon: LM95245 driver
  2011-06-27 20:21             ` Guenter Roeck
@ 2011-06-28  7:07               ` Alexander Stein
  2011-06-28 14:43                 ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-28  7:07 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, lm-sensors, linux-kernel, Alexander Stein

An hwmon driver for the National Semiconductor LM95245 dual temperature
sensors chip.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes since v2:
* Applied review notes from Guenter
* Added Documentation/hwmon/lm95245 (Dunno if is enough though)

Guenter,
I skipped adding OS limits. For one point I don't understand them exactly.
TCRIT and OS limit is indicated the same for local temperature and AFAICS
it uses the external pin A0/OS which have to be configured. I can't test this.

 Documentation/hwmon/lm95245 |   33 +++
 drivers/hwmon/Kconfig       |    9 +
 drivers/hwmon/Makefile      |    1 +
 drivers/hwmon/lm95245.c     |  517 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 560 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwmon/lm95245
 create mode 100644 drivers/hwmon/lm95245.c

diff --git a/Documentation/hwmon/lm95245 b/Documentation/hwmon/lm95245
new file mode 100644
index 0000000..7472c64
--- /dev/null
+++ b/Documentation/hwmon/lm95245
@@ -0,0 +1,33 @@
+Kernel driver lm95245
+==================
+
+Supported chips:
+  * National Semiconductor LM95245
+    Addresses scanned: I2C 0x18, 0x19, 0x29, 0x4c, 0x4d
+    Datasheet: Publicly available at the National Semiconductor website
+               http://www.national.com/mpf/LM/LM95245.html
+
+
+Author: Alexander Stein <alexander.stein@systec-electronic.com>
+
+Description
+-----------
+
+The LM95245 is an 11-bit digital temperature sensor with a 2-wire System
+Management Bus (SMBus) interface and TruTherm technology that can monitor
+the temperature of a remote diode as well as its own temperature.
+The LM95245 can be used to very accurately monitor the temperature of
+external devices such as microprocessors.
+
+All temperature values are given in millidegrees Celsius. Local temperature
+is given within a range of -127 to +127.875 degrees. Remote temperatures are
+given within a range of -127 to +255 degrees. Resolution depends on
+temperature input and range.
+
+Each sensor has its own critical limit, but the hysteresis is common to all
+two channels.
+
+The lm95245 driver can change its update interval to a fixed set of values.
+It will round up to the next selectable interval. See the datasheet for exact
+values. Reading them more often will do no harm, but will return
+'old' values.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 16db83c..ffabf1f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -702,6 +702,15 @@ config SENSORS_LM95241
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm95241.
 
+config SENSORS_LM95245
+	tristate "National Semiconductor LM95245 sensor chip"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for LM95245 sensor chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lm95245.
+
 config SENSORS_MAX1111
 	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
 	depends on SPI_MASTER
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 28061cf..5598712 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
+obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4245)	+= ltc4245.o
diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
new file mode 100644
index 0000000..014973d
--- /dev/null
+++ b/drivers/hwmon/lm95245.c
@@ -0,0 +1,517 @@
+/*
+ * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * The LM95245 is a sensor chip made by National Semiconductors.
+ * It reports up to two temperatures (its own plus an external one).
+ * Complete datasheet can be obtained from National's website at:
+ *   http://www.national.com/ds.cgi/LM/LM95245.pdf
+ *
+ * This driver is based on lm95241.c
+ *
+ * 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 "lm95245"
+
+static const unsigned short normal_i2c[] = {
+	0x18, 0x19, 0x29, 0x4c, 0x4d, I2C_CLIENT_END };
+
+/* LM95245 registers */
+/* general registers */
+#define LM95245_REG_RW_CONFIG1		0x03
+#define LM95245_REG_RW_CONVERS_RATE	0x04
+#define LM95245_REG_W_ONE_SHOT		0x0F
+
+/* diode configuration */
+#define LM95245_REG_RW_CONFIG2		0xBF
+#define LM95245_REG_RW_REMOTE_OFFH	0x11
+#define LM95245_REG_RW_REMOTE_OFFL	0x12
+
+/* status registers */
+#define LM95245_REG_R_STATUS1		0x02
+#define LM95245_REG_R_STATUS2		0x33
+
+/* limit registers */
+#define LM95245_REG_RW_REMOTE_OS_LIMIT		0x07
+#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT	0x20
+#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT	0x19
+#define LM95245_REG_RW_COMMON_HYSTERESIS	0x21
+
+/* temperature signed */
+#define LM95245_REG_R_LOCAL_TEMPH_S	0x00
+#define LM95245_REG_R_LOCAL_TEMPL_S	0x30
+#define LM95245_REG_R_REMOTE_TEMPH_S	0x01
+#define LM95245_REG_R_REMOTE_TEMPL_S	0x10
+/* temperature unsigned */
+#define LM95245_REG_R_REMOTE_TEMPH_U	0x31
+#define LM95245_REG_R_REMOTE_TEMPL_U	0x32
+
+/* id registers */
+#define LM95245_REG_R_MAN_ID		0xFE
+#define LM95245_REG_R_CHIP_ID		0xFF
+
+/* LM95245 specific bitfields */
+#define CFG_STOP		0x40
+#define CFG_REMOTE_TCRIT_MASK	0x10
+#define CFG_REMOTE_OS_MASK	0x08
+#define CFG_LOCAL_TCRIT_MASK	0x04
+#define CFG_LOCAL_OS_MASK	0x02
+
+#define CFG2_OS_A0		0x40
+#define CFG2_DIODE_FAULT_OS	0x20
+#define CFG2_DIODE_FAULT_TCRIT	0x10
+#define CFG2_REMOTE_TT		0x08
+#define CFG2_REMOTE_FILTER_DIS	0x00
+#define CFG2_REMOTE_FILTER_EN	0x06
+
+/* conversation rate in ms */
+#define RATE_CR0063	0x00
+#define RATE_CR0364	0x01
+#define RATE_CR1000	0x02
+#define RATE_CR2500	0x03
+
+#define STATUS1_DIODE_FAULT	0x04
+#define STATUS1_RTCRIT		0x02
+#define STATUS1_LOC		0x01
+
+#define MANUFACTURER_ID		0x01
+#define DEFAULT_REVISION	0xB3
+
+static const u8 lm95245_reg_address[] = {
+	LM95245_REG_R_LOCAL_TEMPH_S,
+	LM95245_REG_R_LOCAL_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_S,
+	LM95245_REG_R_REMOTE_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_U,
+	LM95245_REG_R_REMOTE_TEMPL_U,
+	LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT,
+	LM95245_REG_RW_REMOTE_TCRIT_LIMIT,
+	LM95245_REG_RW_COMMON_HYSTERESIS,
+	LM95245_REG_R_STATUS1,
+};
+
+/* Client data (each client gets its own) */
+struct lm95245_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned long last_updated;	/* in jiffies */
+	unsigned long interval;	/* in msecs */
+	char valid;		/* zero until following fields are valid */
+	/* registers values */
+	u8 regs[ARRAY_SIZE(lm95245_reg_address)];
+	u8 config1, config2;
+};
+
+/* Conversions */
+static int TempFromRegUnsigned(u8 val_h, u8 val_l)
+{
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+static int TempFromRegSigned(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return (val_h - 0x100) * 1000;
+	return TempFromRegUnsigned(val_h, val_l);
+}
+
+static struct lm95245_data *lm95245_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated
+		+ msecs_to_jiffies(data->interval)) || !data->valid) {
+		int i;
+
+		dev_dbg(&client->dev, "Updating lm95245 data.\n");
+		for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
+			data->regs[i]
+			  = i2c_smbus_read_byte_data(client,
+						     lm95245_reg_address[i]);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static unsigned long lm95245_set_conversion_rate(struct i2c_client *client,
+			unsigned long interval)
+{
+	int rate;
+
+	if (interval <= 63) {
+		interval = 63;
+		rate = RATE_CR0063;
+	} else if (interval <= 364) {
+		interval = 364;
+		rate = RATE_CR0364;
+	} else if (interval <= 1000) {
+		interval = 1000;
+		rate = RATE_CR1000;
+	} else {
+		interval = 2500;
+		rate = RATE_CR2500;
+	}
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, rate);
+
+	return interval;
+}
+
+/* Sysfs stuff */
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int temp;
+	int index = to_sensor_dev_attr(attr)->index;
+
+	/*
+	 * Index 0 (Local temp) is always signed
+	 * Index 2 (Remote temp) has both signed and unsigned data
+	 * use signed calculation for remote if signed bit is set
+	 */
+	if (index == 0 || data->regs[index] & 0x80)
+		temp = TempFromRegSigned(data->regs[index],
+			    data->regs[index + 1]);
+	else
+		temp = TempFromRegUnsigned(data->regs[index + 2],
+			    data->regs[index + 3]);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
+}
+
+static ssize_t show_limit(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+			data->regs[index] * 1000);
+}
+
+static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	int index = to_sensor_dev_attr(attr)->index;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, (index == 6 ? 127 : 255));
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, lm95245_reg_address[index], val);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, 31);
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	/* shared crit hysteresis */
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
+		val);
+
+	mutex_unlock(&data->update_lock);
+
+	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 lm95245_data *data = i2c_get_clientdata(client);
+
+	return snprintf(buf, PAGE_SIZE - 1,
+		data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	if (val == 1)
+		data->config2 |= CFG2_REMOTE_TT;
+	else
+		data->config2 &= ~CFG2_REMOTE_TT;
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
+				  data->config2);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+			!!(data->regs[9] & index));
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%lu\n", data->interval);
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	data->interval = lm95245_set_conversion_rate(client, val);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
+		set_limit, 6);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
+		STATUS1_LOC);
+
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
+		set_limit, 7);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
+		STATUS1_RTCRIT);
+static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
+		set_type, 0);
+static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL,
+		STATUS1_DIODE_FAULT);
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
+		set_interval);
+
+static struct attribute *lm95245_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_type.dev_attr.attr,
+	&sensor_dev_attr_temp2_fault.dev_attr.attr,
+	&dev_attr_update_interval.attr,
+	NULL
+};
+
+static const struct attribute_group lm95245_group = {
+	.attrs = lm95245_attributes,
+};
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int lm95245_detect(struct i2c_client *new_client,
+			  struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = new_client->adapter;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	if (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_MAN_ID)
+			!= MANUFACTURER_ID
+		|| i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
+			!= DEFAULT_REVISION)
+		return -ENODEV;
+
+	strlcpy(info->type, DEVNAME, I2C_NAME_SIZE);
+	return 0;
+}
+
+static void lm95245_init_client(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	data->interval = 1000;	/* 1 sec default */
+	data->valid = 0;
+
+	data->config1 = i2c_smbus_read_byte_data(client,
+		LM95245_REG_RW_CONFIG1);
+	data->config2 = i2c_smbus_read_byte_data(client,
+		LM95245_REG_RW_CONFIG2);
+
+	/* Clear the standby bit */
+	data->config1 &= ~CFG_STOP;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1,
+		data->config1);
+	lm95245_set_conversion_rate(client, data->interval);
+}
+
+static int lm95245_probe(struct i2c_client *new_client,
+			 const struct i2c_device_id *id)
+{
+	struct lm95245_data *data;
+	int err;
+
+	data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(new_client, data);
+	mutex_init(&data->update_lock);
+
+	/* Initialize the LM95245 chip */
+	lm95245_init_client(new_client);
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int lm95245_remove(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &lm95245_group);
+
+	kfree(data);
+	return 0;
+}
+
+/* Driver data (common to all clients) */
+static const struct i2c_device_id lm95245_id[] = {
+	{ DEVNAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm95245_id);
+
+static struct i2c_driver lm95245_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= DEVNAME,
+	},
+	.probe		= lm95245_probe,
+	.remove		= lm95245_remove,
+	.id_table	= lm95245_id,
+	.detect		= lm95245_detect,
+	.address_list	= normal_i2c,
+};
+
+static int __init sensors_lm95245_init(void)
+{
+	return i2c_add_driver(&lm95245_driver);
+}
+
+static void __exit sensors_lm95245_exit(void)
+{
+	i2c_del_driver(&lm95245_driver);
+}
+
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_DESCRIPTION("LM95245 sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_lm95245_init);
+module_exit(sensors_lm95245_exit);
-- 
1.7.3.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] hwmon: LM95245 driver
  2011-06-28  7:07               ` [PATCH v3] " Alexander Stein
@ 2011-06-28 14:43                 ` Guenter Roeck
  2011-06-28 15:11                   ` [PATCH v4] " Alexander Stein
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2011-06-28 14:43 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Tue, Jun 28, 2011 at 03:07:09AM -0400, Alexander Stein wrote:
> An hwmon driver for the National Semiconductor LM95245 dual temperature
> sensors chip.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> Changes since v2:
> * Applied review notes from Guenter
> * Added Documentation/hwmon/lm95245 (Dunno if is enough though)
> 
> Guenter,
> I skipped adding OS limits. For one point I don't understand them exactly.
> TCRIT and OS limit is indicated the same for local temperature and AFAICS
> it uses the external pin A0/OS which have to be configured. I can't test this.
> 
Hi Alexander,

no problem. I requested samples, so I may add that myself later on if I can get it
to work and if I think it makes sense.

Only nitpicks this time around.

>  Documentation/hwmon/lm95245 |   33 +++
>  drivers/hwmon/Kconfig       |    9 +
>  drivers/hwmon/Makefile      |    1 +
>  drivers/hwmon/lm95245.c     |  517 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 560 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/hwmon/lm95245
>  create mode 100644 drivers/hwmon/lm95245.c
> 
> diff --git a/Documentation/hwmon/lm95245 b/Documentation/hwmon/lm95245
> new file mode 100644
> index 0000000..7472c64
> --- /dev/null
> +++ b/Documentation/hwmon/lm95245
> @@ -0,0 +1,33 @@
> +Kernel driver lm95245
> +==================
> +
> +Supported chips:
> +  * National Semiconductor LM95245
> +    Addresses scanned: I2C 0x18, 0x19, 0x29, 0x4c, 0x4d
> +    Datasheet: Publicly available at the National Semiconductor website
> +               http://www.national.com/mpf/LM/LM95245.html
> +
> +
> +Author: Alexander Stein <alexander.stein@systec-electronic.com>
> +
> +Description
> +-----------
> +
> +The LM95245 is an 11-bit digital temperature sensor with a 2-wire System
> +Management Bus (SMBus) interface and TruTherm technology that can monitor
> +the temperature of a remote diode as well as its own temperature.
> +The LM95245 can be used to very accurately monitor the temperature of
> +external devices such as microprocessors.
> +
> +All temperature values are given in millidegrees Celsius. Local temperature
> +is given within a range of -127 to +127.875 degrees. Remote temperatures are
> +given within a range of -127 to +255 degrees. Resolution depends on
> +temperature input and range.
> +
> +Each sensor has its own critical limit, but the hysteresis is common to all
> +two channels.
> +
> +The lm95245 driver can change its update interval to a fixed set of values.
> +It will round up to the next selectable interval. See the datasheet for exact
> +values. Reading them more often will do no harm, but will return

s/them/sensor values/

> +'old' values.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 16db83c..ffabf1f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -702,6 +702,15 @@ config SENSORS_LM95241
>           This driver can also be built as a module.  If so, the module
>           will be called lm95241.
> 
> +config SENSORS_LM95245
> +       tristate "National Semiconductor LM95245 sensor chip"
> +       depends on I2C && EXPERIMENTAL
> +       help
> +         If you say yes here you get support for LM95245 sensor chip.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called lm95245.
> +
>  config SENSORS_MAX1111
>         tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
>         depends on SPI_MASTER
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 28061cf..5598712 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)    += lm90.o
>  obj-$(CONFIG_SENSORS_LM92)     += lm92.o
>  obj-$(CONFIG_SENSORS_LM93)     += lm93.o
>  obj-$(CONFIG_SENSORS_LM95241)  += lm95241.o
> +obj-$(CONFIG_SENSORS_LM95245)  += lm95245.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>  obj-$(CONFIG_SENSORS_LTC4215)  += ltc4215.o
>  obj-$(CONFIG_SENSORS_LTC4245)  += ltc4245.o
> diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
> new file mode 100644
> index 0000000..014973d
> --- /dev/null
> +++ b/drivers/hwmon/lm95245.c
> @@ -0,0 +1,517 @@
> +/*
> + * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
> + *
> + * The LM95245 is a sensor chip made by National Semiconductors.
> + * It reports up to two temperatures (its own plus an external one).
> + * Complete datasheet can be obtained from National's website at:
> + *   http://www.national.com/ds.cgi/LM/LM95245.pdf
> + *
> + * This driver is based on lm95241.c
> + *
> + * 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 "lm95245"
> +
> +static const unsigned short normal_i2c[] = {
> +       0x18, 0x19, 0x29, 0x4c, 0x4d, I2C_CLIENT_END };
> +
> +/* LM95245 registers */
> +/* general registers */
> +#define LM95245_REG_RW_CONFIG1         0x03
> +#define LM95245_REG_RW_CONVERS_RATE    0x04
> +#define LM95245_REG_W_ONE_SHOT         0x0F
> +
> +/* diode configuration */
> +#define LM95245_REG_RW_CONFIG2         0xBF
> +#define LM95245_REG_RW_REMOTE_OFFH     0x11
> +#define LM95245_REG_RW_REMOTE_OFFL     0x12
> +
> +/* status registers */
> +#define LM95245_REG_R_STATUS1          0x02
> +#define LM95245_REG_R_STATUS2          0x33
> +
> +/* limit registers */
> +#define LM95245_REG_RW_REMOTE_OS_LIMIT         0x07
> +#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT    0x20
> +#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT      0x19
> +#define LM95245_REG_RW_COMMON_HYSTERESIS       0x21
> +
> +/* temperature signed */
> +#define LM95245_REG_R_LOCAL_TEMPH_S    0x00
> +#define LM95245_REG_R_LOCAL_TEMPL_S    0x30
> +#define LM95245_REG_R_REMOTE_TEMPH_S   0x01
> +#define LM95245_REG_R_REMOTE_TEMPL_S   0x10
> +/* temperature unsigned */
> +#define LM95245_REG_R_REMOTE_TEMPH_U   0x31
> +#define LM95245_REG_R_REMOTE_TEMPL_U   0x32
> +
> +/* id registers */
> +#define LM95245_REG_R_MAN_ID           0xFE
> +#define LM95245_REG_R_CHIP_ID          0xFF
> +
> +/* LM95245 specific bitfields */
> +#define CFG_STOP               0x40
> +#define CFG_REMOTE_TCRIT_MASK  0x10
> +#define CFG_REMOTE_OS_MASK     0x08
> +#define CFG_LOCAL_TCRIT_MASK   0x04
> +#define CFG_LOCAL_OS_MASK      0x02
> +
> +#define CFG2_OS_A0             0x40
> +#define CFG2_DIODE_FAULT_OS    0x20
> +#define CFG2_DIODE_FAULT_TCRIT 0x10
> +#define CFG2_REMOTE_TT         0x08
> +#define CFG2_REMOTE_FILTER_DIS 0x00
> +#define CFG2_REMOTE_FILTER_EN  0x06
> +
> +/* conversation rate in ms */
> +#define RATE_CR0063    0x00
> +#define RATE_CR0364    0x01
> +#define RATE_CR1000    0x02
> +#define RATE_CR2500    0x03
> +
> +#define STATUS1_DIODE_FAULT    0x04
> +#define STATUS1_RTCRIT         0x02
> +#define STATUS1_LOC            0x01
> +
> +#define MANUFACTURER_ID                0x01
> +#define DEFAULT_REVISION       0xB3
> +
> +static const u8 lm95245_reg_address[] = {
> +       LM95245_REG_R_LOCAL_TEMPH_S,
> +       LM95245_REG_R_LOCAL_TEMPL_S,
> +       LM95245_REG_R_REMOTE_TEMPH_S,
> +       LM95245_REG_R_REMOTE_TEMPL_S,
> +       LM95245_REG_R_REMOTE_TEMPH_U,
> +       LM95245_REG_R_REMOTE_TEMPL_U,
> +       LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT,
> +       LM95245_REG_RW_REMOTE_TCRIT_LIMIT,
> +       LM95245_REG_RW_COMMON_HYSTERESIS,
> +       LM95245_REG_R_STATUS1,
> +};
> +
> +/* Client data (each client gets its own) */
> +struct lm95245_data {
> +       struct device *hwmon_dev;
> +       struct mutex update_lock;
> +       unsigned long last_updated;     /* in jiffies */
> +       unsigned long interval; /* in msecs */
> +       char valid;             /* zero until following fields are valid */

Just a side note ... char doesn't buy you anything here, and will only make the code
more complex, at least for some architectures. You'd be better off using int
(or bool if you want to be fancy).

> +       /* registers values */
> +       u8 regs[ARRAY_SIZE(lm95245_reg_address)];
> +       u8 config1, config2;
> +};
> +
> +/* Conversions */
> +static int TempFromRegUnsigned(u8 val_h, u8 val_l)
> +{
> +       return val_h * 1000 + val_l * 1000 / 256;
> +}
> +
> +static int TempFromRegSigned(u8 val_h, u8 val_l)
> +{
> +       if (val_h & 0x80)
> +               return (val_h - 0x100) * 1000;
> +       return TempFromRegUnsigned(val_h, val_l);
> +}
> +
I keep forgetting this ... the above function names violate CodingStyle, chapter 4
("mixed-case names are frowned upon").

> +static struct lm95245_data *lm95245_update_device(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       mutex_lock(&data->update_lock);
> +
> +       if (time_after(jiffies, data->last_updated
> +               + msecs_to_jiffies(data->interval)) || !data->valid) {
> +               int i;
> +
> +               dev_dbg(&client->dev, "Updating lm95245 data.\n");
> +               for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
> +                       data->regs[i]
> +                         = i2c_smbus_read_byte_data(client,
> +                                                    lm95245_reg_address[i]);
> +               data->last_updated = jiffies;
> +               data->valid = 1;
> +       }
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return data;
> +}
> +
> +static unsigned long lm95245_set_conversion_rate(struct i2c_client *client,
> +                       unsigned long interval)
> +{
> +       int rate;
> +
> +       if (interval <= 63) {
> +               interval = 63;
> +               rate = RATE_CR0063;
> +       } else if (interval <= 364) {
> +               interval = 364;
> +               rate = RATE_CR0364;
> +       } else if (interval <= 1000) {
> +               interval = 1000;
> +               rate = RATE_CR1000;
> +       } else {
> +               interval = 2500;
> +               rate = RATE_CR2500;
> +       }
> +
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, rate);
> +
> +       return interval;
> +}
> +
> +/* Sysfs stuff */
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +                         char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int temp;
> +       int index = to_sensor_dev_attr(attr)->index;
> +
> +       /*
> +        * Index 0 (Local temp) is always signed
> +        * Index 2 (Remote temp) has both signed and unsigned data
> +        * use signed calculation for remote if signed bit is set
> +        */
> +       if (index == 0 || data->regs[index] & 0x80)
> +               temp = TempFromRegSigned(data->regs[index],
> +                           data->regs[index + 1]);
> +       else
> +               temp = TempFromRegUnsigned(data->regs[index + 2],
> +                           data->regs[index + 3]);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
> +}
> +
> +static ssize_t show_limit(struct device *dev, struct device_attribute *attr,
> +                        char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int index = to_sensor_dev_attr(attr)->index;
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> +                       data->regs[index] * 1000);
> +}
> +
> +static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
> +                       const char *buf, size_t count)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +       int index = to_sensor_dev_attr(attr)->index;
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       val /= 1000;
> +
> +       val = SENSORS_LIMIT(val, 0, (index == 6 ? 127 : 255));
> +
> +       mutex_lock(&data->update_lock);
> +
> +       data->valid = 0;
> +
> +       i2c_smbus_write_byte_data(client, lm95245_reg_address[index], val);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}
> +
> +static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
> +                       const char *buf, size_t count)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       val /= 1000;
> +
> +       val = SENSORS_LIMIT(val, 0, 31);
> +
> +       mutex_lock(&data->update_lock);
> +
> +       data->valid = 0;
> +
> +       /* shared crit hysteresis */
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
> +               val);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       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 lm95245_data *data = i2c_get_clientdata(client);
> +
> +       return snprintf(buf, PAGE_SIZE - 1,
> +               data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
> +}
> +
> +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 lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +       if (val != 1 && val != 2)
> +               return -EINVAL;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       if (val == 1)
> +               data->config2 |= CFG2_REMOTE_TT;
> +       else
> +               data->config2 &= ~CFG2_REMOTE_TT;
> +
> +       data->valid = 0;
> +
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
> +                                 data->config2);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}
> +
> +static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
> +                        char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +       int index = to_sensor_dev_attr(attr)->index;
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> +                       !!(data->regs[9] & index));
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +                            char *buf)
> +{
> +       struct lm95245_data *data = lm95245_update_device(dev);
> +
> +       return snprintf(buf, PAGE_SIZE - 1, "%lu\n", data->interval);
> +}
> +
> +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 lm95245_data *data = i2c_get_clientdata(client);
> +       unsigned long val;
> +
> +       if (strict_strtoul(buf, 10, &val) < 0)
> +               return -EINVAL;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       data->interval = lm95245_set_conversion_rate(client, val);
> +
> +       mutex_unlock(&data->update_lock);
> +
> +       return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
> +               set_limit, 6);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
> +               set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
> +               STATUS1_LOC);
> +
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
> +               set_limit, 7);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
> +               set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
> +               STATUS1_RTCRIT);
> +static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
> +               set_type, 0);
> +static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL,
> +               STATUS1_DIODE_FAULT);
> +
> +static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
> +               set_interval);
> +
> +static struct attribute *lm95245_attributes[] = {
> +       &sensor_dev_attr_temp1_input.dev_attr.attr,
> +       &sensor_dev_attr_temp1_crit.dev_attr.attr,
> +       &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +       &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> +       &sensor_dev_attr_temp2_input.dev_attr.attr,
> +       &sensor_dev_attr_temp2_crit.dev_attr.attr,
> +       &sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
> +       &sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
> +       &sensor_dev_attr_temp2_type.dev_attr.attr,
> +       &sensor_dev_attr_temp2_fault.dev_attr.attr,
> +       &dev_attr_update_interval.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group lm95245_group = {
> +       .attrs = lm95245_attributes,
> +};
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int lm95245_detect(struct i2c_client *new_client,
> +                         struct i2c_board_info *info)
> +{
> +       struct i2c_adapter *adapter = new_client->adapter;
> +
> +       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +               return -ENODEV;
> +
> +       if (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_MAN_ID)
> +                       != MANUFACTURER_ID
> +               || i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
> +                       != DEFAULT_REVISION)
> +               return -ENODEV;
> +
> +       strlcpy(info->type, DEVNAME, I2C_NAME_SIZE);
> +       return 0;
> +}
> +
> +static void lm95245_init_client(struct i2c_client *client)
> +{
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       data->interval = 1000;  /* 1 sec default */

Same as config1 applies here - the default interval is 1s, so any change must have been 
on purpose. As such, it might make sense to read the conversion interval register and set
data->interval accordingly.

> +       data->valid = 0;
> +
> +       data->config1 = i2c_smbus_read_byte_data(client,
> +               LM95245_REG_RW_CONFIG1);
> +       data->config2 = i2c_smbus_read_byte_data(client,
> +               LM95245_REG_RW_CONFIG2);
> +
> +       /* Clear the standby bit */
> +       data->config1 &= ~CFG_STOP;
> +
> +       i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1,
> +               data->config1);

This write is only necessary if the configuration actually changed. Since I2C writes
are a bit slow, it makes sense to skip them if unnecessary. Something like

	if (data->config1 & CFG_STOP) {
		data->config1 &= ~CFG_STOP;
		i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1,
					  data->config1);
	}

would be a bit more efficient.

> +       lm95245_set_conversion_rate(client, data->interval);

you don't need to call this function unless the interval actually changed.
In other words, you don't need it if you read the conversion interval above.

> +}
> +
> +static int lm95245_probe(struct i2c_client *new_client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct lm95245_data *data;
> +       int err;
> +
> +       data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
> +       if (!data) {
> +               err = -ENOMEM;
> +               goto exit;
> +       }
> +
> +       i2c_set_clientdata(new_client, data);
> +       mutex_init(&data->update_lock);
> +
> +       /* Initialize the LM95245 chip */
> +       lm95245_init_client(new_client);
> +
> +       /* Register sysfs hooks */
> +       err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
> +exit_free:
> +       kfree(data);
> +exit:
> +       return err;
> +}
> +
> +static int lm95245_remove(struct i2c_client *client)
> +{
> +       struct lm95245_data *data = i2c_get_clientdata(client);
> +
> +       hwmon_device_unregister(data->hwmon_dev);
> +       sysfs_remove_group(&client->dev.kobj, &lm95245_group);
> +
> +       kfree(data);
> +       return 0;
> +}
> +
> +/* Driver data (common to all clients) */
> +static const struct i2c_device_id lm95245_id[] = {
> +       { DEVNAME, 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm95245_id);
> +
> +static struct i2c_driver lm95245_driver = {
> +       .class          = I2C_CLASS_HWMON,
> +       .driver = {
> +               .name   = DEVNAME,
> +       },
> +       .probe          = lm95245_probe,
> +       .remove         = lm95245_remove,
> +       .id_table       = lm95245_id,
> +       .detect         = lm95245_detect,
> +       .address_list   = normal_i2c,
> +};
> +
> +static int __init sensors_lm95245_init(void)
> +{
> +       return i2c_add_driver(&lm95245_driver);
> +}
> +
> +static void __exit sensors_lm95245_exit(void)
> +{
> +       i2c_del_driver(&lm95245_driver);
> +}
> +
> +MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
> +MODULE_DESCRIPTION("LM95245 sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_lm95245_init);
> +module_exit(sensors_lm95245_exit);
> --
> 1.7.3.4
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4] hwmon: LM95245 driver
  2011-06-28 14:43                 ` Guenter Roeck
@ 2011-06-28 15:11                   ` Alexander Stein
  2011-06-30  9:06                     ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Stein @ 2011-06-28 15:11 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, lm-sensors, linux-kernel, Alexander Stein

An hwmon driver for the National Semiconductor LM95245 dual temperature
sensors chip.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v4:
* Applied more of Guenter's suggestions

 Documentation/hwmon/lm95245 |   33 +++
 drivers/hwmon/Kconfig       |    9 +
 drivers/hwmon/Makefile      |    1 +
 drivers/hwmon/lm95245.c     |  543 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 586 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwmon/lm95245
 create mode 100644 drivers/hwmon/lm95245.c

diff --git a/Documentation/hwmon/lm95245 b/Documentation/hwmon/lm95245
new file mode 100644
index 0000000..cbd8aea
--- /dev/null
+++ b/Documentation/hwmon/lm95245
@@ -0,0 +1,33 @@
+Kernel driver lm95245
+==================
+
+Supported chips:
+  * National Semiconductor LM95245
+    Addresses scanned: I2C 0x18, 0x19, 0x29, 0x4c, 0x4d
+    Datasheet: Publicly available at the National Semiconductor website
+               http://www.national.com/mpf/LM/LM95245.html
+
+
+Author: Alexander Stein <alexander.stein@systec-electronic.com>
+
+Description
+-----------
+
+The LM95245 is an 11-bit digital temperature sensor with a 2-wire System
+Management Bus (SMBus) interface and TruTherm technology that can monitor
+the temperature of a remote diode as well as its own temperature.
+The LM95245 can be used to very accurately monitor the temperature of
+external devices such as microprocessors.
+
+All temperature values are given in millidegrees Celsius. Local temperature
+is given within a range of -127 to +127.875 degrees. Remote temperatures are
+given within a range of -127 to +255 degrees. Resolution depends on
+temperature input and range.
+
+Each sensor has its own critical limit, but the hysteresis is common to all
+two channels.
+
+The lm95245 driver can change its update interval to a fixed set of values.
+It will round up to the next selectable interval. See the datasheet for exact
+values. Reading sensor values more often will do no harm, but will return
+'old' values.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 16db83c..ffabf1f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -702,6 +702,15 @@ config SENSORS_LM95241
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm95241.
 
+config SENSORS_LM95245
+	tristate "National Semiconductor LM95245 sensor chip"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for LM95245 sensor chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lm95245.
+
 config SENSORS_MAX1111
 	tristate "Maxim MAX1111 Multichannel, Serial 8-bit ADC chip"
 	depends on SPI_MASTER
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 28061cf..5598712 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
+obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4245)	+= ltc4245.o
diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
new file mode 100644
index 0000000..dce9e68
--- /dev/null
+++ b/drivers/hwmon/lm95245.c
@@ -0,0 +1,543 @@
+/*
+ * Copyright (C) 2011 Alexander Stein <alexander.stein@systec-electronic.com>
+ *
+ * The LM95245 is a sensor chip made by National Semiconductors.
+ * It reports up to two temperatures (its own plus an external one).
+ * Complete datasheet can be obtained from National's website at:
+ *   http://www.national.com/ds.cgi/LM/LM95245.pdf
+ *
+ * This driver is based on lm95241.c
+ *
+ * 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 "lm95245"
+
+static const unsigned short normal_i2c[] = {
+	0x18, 0x19, 0x29, 0x4c, 0x4d, I2C_CLIENT_END };
+
+/* LM95245 registers */
+/* general registers */
+#define LM95245_REG_RW_CONFIG1		0x03
+#define LM95245_REG_RW_CONVERS_RATE	0x04
+#define LM95245_REG_W_ONE_SHOT		0x0F
+
+/* diode configuration */
+#define LM95245_REG_RW_CONFIG2		0xBF
+#define LM95245_REG_RW_REMOTE_OFFH	0x11
+#define LM95245_REG_RW_REMOTE_OFFL	0x12
+
+/* status registers */
+#define LM95245_REG_R_STATUS1		0x02
+#define LM95245_REG_R_STATUS2		0x33
+
+/* limit registers */
+#define LM95245_REG_RW_REMOTE_OS_LIMIT		0x07
+#define LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT	0x20
+#define LM95245_REG_RW_REMOTE_TCRIT_LIMIT	0x19
+#define LM95245_REG_RW_COMMON_HYSTERESIS	0x21
+
+/* temperature signed */
+#define LM95245_REG_R_LOCAL_TEMPH_S	0x00
+#define LM95245_REG_R_LOCAL_TEMPL_S	0x30
+#define LM95245_REG_R_REMOTE_TEMPH_S	0x01
+#define LM95245_REG_R_REMOTE_TEMPL_S	0x10
+/* temperature unsigned */
+#define LM95245_REG_R_REMOTE_TEMPH_U	0x31
+#define LM95245_REG_R_REMOTE_TEMPL_U	0x32
+
+/* id registers */
+#define LM95245_REG_R_MAN_ID		0xFE
+#define LM95245_REG_R_CHIP_ID		0xFF
+
+/* LM95245 specific bitfields */
+#define CFG_STOP		0x40
+#define CFG_REMOTE_TCRIT_MASK	0x10
+#define CFG_REMOTE_OS_MASK	0x08
+#define CFG_LOCAL_TCRIT_MASK	0x04
+#define CFG_LOCAL_OS_MASK	0x02
+
+#define CFG2_OS_A0		0x40
+#define CFG2_DIODE_FAULT_OS	0x20
+#define CFG2_DIODE_FAULT_TCRIT	0x10
+#define CFG2_REMOTE_TT		0x08
+#define CFG2_REMOTE_FILTER_DIS	0x00
+#define CFG2_REMOTE_FILTER_EN	0x06
+
+/* conversation rate in ms */
+#define RATE_CR0063	0x00
+#define RATE_CR0364	0x01
+#define RATE_CR1000	0x02
+#define RATE_CR2500	0x03
+
+#define STATUS1_DIODE_FAULT	0x04
+#define STATUS1_RTCRIT		0x02
+#define STATUS1_LOC		0x01
+
+#define MANUFACTURER_ID		0x01
+#define DEFAULT_REVISION	0xB3
+
+static const u8 lm95245_reg_address[] = {
+	LM95245_REG_R_LOCAL_TEMPH_S,
+	LM95245_REG_R_LOCAL_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_S,
+	LM95245_REG_R_REMOTE_TEMPL_S,
+	LM95245_REG_R_REMOTE_TEMPH_U,
+	LM95245_REG_R_REMOTE_TEMPL_U,
+	LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT,
+	LM95245_REG_RW_REMOTE_TCRIT_LIMIT,
+	LM95245_REG_RW_COMMON_HYSTERESIS,
+	LM95245_REG_R_STATUS1,
+};
+
+/* Client data (each client gets its own) */
+struct lm95245_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned long last_updated;	/* in jiffies */
+	unsigned long interval;	/* in msecs */
+	bool valid;		/* zero until following fields are valid */
+	/* registers values */
+	u8 regs[ARRAY_SIZE(lm95245_reg_address)];
+	u8 config1, config2;
+};
+
+/* Conversions */
+static int temp_from_reg_unsigned(u8 val_h, u8 val_l)
+{
+	return val_h * 1000 + val_l * 1000 / 256;
+}
+
+static int temp_from_reg_signed(u8 val_h, u8 val_l)
+{
+	if (val_h & 0x80)
+		return (val_h - 0x100) * 1000;
+	return temp_from_reg_unsigned(val_h, val_l);
+}
+
+static struct lm95245_data *lm95245_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated
+		+ msecs_to_jiffies(data->interval)) || !data->valid) {
+		int i;
+
+		dev_dbg(&client->dev, "Updating lm95245 data.\n");
+		for (i = 0; i < ARRAY_SIZE(lm95245_reg_address); i++)
+			data->regs[i]
+			  = i2c_smbus_read_byte_data(client,
+						     lm95245_reg_address[i]);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static unsigned long lm95245_read_conversion_rate(struct i2c_client *client)
+{
+	int rate;
+	unsigned long interval;
+
+	rate = i2c_smbus_read_byte_data(client, LM95245_REG_RW_CONVERS_RATE);
+
+	switch (rate) {
+	case RATE_CR0063:
+		interval = 63;
+		break;
+	case RATE_CR0364:
+		interval = 364;
+		break;
+	case RATE_CR1000:
+		interval = 1000;
+		break;
+	case RATE_CR2500:
+	default:
+		interval = 2500;
+		break;
+	}
+
+	return interval;
+}
+
+static unsigned long lm95245_set_conversion_rate(struct i2c_client *client,
+			unsigned long interval)
+{
+	int rate;
+
+	if (interval <= 63) {
+		interval = 63;
+		rate = RATE_CR0063;
+	} else if (interval <= 364) {
+		interval = 364;
+		rate = RATE_CR0364;
+	} else if (interval <= 1000) {
+		interval = 1000;
+		rate = RATE_CR1000;
+	} else {
+		interval = 2500;
+		rate = RATE_CR2500;
+	}
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONVERS_RATE, rate);
+
+	return interval;
+}
+
+/* Sysfs stuff */
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int temp;
+	int index = to_sensor_dev_attr(attr)->index;
+
+	/*
+	 * Index 0 (Local temp) is always signed
+	 * Index 2 (Remote temp) has both signed and unsigned data
+	 * use signed calculation for remote if signed bit is set
+	 */
+	if (index == 0 || data->regs[index] & 0x80)
+		temp = temp_from_reg_signed(data->regs[index],
+			    data->regs[index + 1]);
+	else
+		temp = temp_from_reg_unsigned(data->regs[index + 2],
+			    data->regs[index + 3]);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", temp);
+}
+
+static ssize_t show_limit(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+			data->regs[index] * 1000);
+}
+
+static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	int index = to_sensor_dev_attr(attr)->index;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, (index == 6 ? 127 : 255));
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, lm95245_reg_address[index], val);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	val /= 1000;
+
+	val = SENSORS_LIMIT(val, 0, 31);
+
+	mutex_lock(&data->update_lock);
+
+	data->valid = 0;
+
+	/* shared crit hysteresis */
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
+		val);
+
+	mutex_unlock(&data->update_lock);
+
+	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 lm95245_data *data = i2c_get_clientdata(client);
+
+	return snprintf(buf, PAGE_SIZE - 1,
+		data->config2 & CFG2_REMOTE_TT ? "1\n" : "2\n");
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	if (val == 1)
+		data->config2 |= CFG2_REMOTE_TT;
+	else
+		data->config2 &= ~CFG2_REMOTE_TT;
+
+	data->valid = 0;
+
+	i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG2,
+				  data->config2);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+			!!(data->regs[9] & index));
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%lu\n", data->interval);
+}
+
+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 lm95245_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	data->interval = lm95245_set_conversion_rate(client, val);
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
+		set_limit, 6);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
+		STATUS1_LOC);
+
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
+		set_limit, 7);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
+		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
+		STATUS1_RTCRIT);
+static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
+		set_type, 0);
+static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_alarm, NULL,
+		STATUS1_DIODE_FAULT);
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_interval,
+		set_interval);
+
+static struct attribute *lm95245_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_type.dev_attr.attr,
+	&sensor_dev_attr_temp2_fault.dev_attr.attr,
+	&dev_attr_update_interval.attr,
+	NULL
+};
+
+static const struct attribute_group lm95245_group = {
+	.attrs = lm95245_attributes,
+};
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int lm95245_detect(struct i2c_client *new_client,
+			  struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = new_client->adapter;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	if (i2c_smbus_read_byte_data(new_client, LM95245_REG_R_MAN_ID)
+			!= MANUFACTURER_ID
+		|| i2c_smbus_read_byte_data(new_client, LM95245_REG_R_CHIP_ID)
+			!= DEFAULT_REVISION)
+		return -ENODEV;
+
+	strlcpy(info->type, DEVNAME, I2C_NAME_SIZE);
+	return 0;
+}
+
+static void lm95245_init_client(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	data->valid = 0;
+	data->interval = lm95245_read_conversion_rate(client);
+
+	data->config1 = i2c_smbus_read_byte_data(client,
+		LM95245_REG_RW_CONFIG1);
+	data->config2 = i2c_smbus_read_byte_data(client,
+		LM95245_REG_RW_CONFIG2);
+
+	if (data->config1 & CFG_STOP) {
+		/* Clear the standby bit */
+		data->config1 &= ~CFG_STOP;
+		i2c_smbus_write_byte_data(client, LM95245_REG_RW_CONFIG1,
+			data->config1);
+	}
+}
+
+static int lm95245_probe(struct i2c_client *new_client,
+			 const struct i2c_device_id *id)
+{
+	struct lm95245_data *data;
+	int err;
+
+	data = kzalloc(sizeof(struct lm95245_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(new_client, data);
+	mutex_init(&data->update_lock);
+
+	/* Initialize the LM95245 chip */
+	lm95245_init_client(new_client);
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&new_client->dev.kobj, &lm95245_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, &lm95245_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int lm95245_remove(struct i2c_client *client)
+{
+	struct lm95245_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &lm95245_group);
+
+	kfree(data);
+	return 0;
+}
+
+/* Driver data (common to all clients) */
+static const struct i2c_device_id lm95245_id[] = {
+	{ DEVNAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm95245_id);
+
+static struct i2c_driver lm95245_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= DEVNAME,
+	},
+	.probe		= lm95245_probe,
+	.remove		= lm95245_remove,
+	.id_table	= lm95245_id,
+	.detect		= lm95245_detect,
+	.address_list	= normal_i2c,
+};
+
+static int __init sensors_lm95245_init(void)
+{
+	return i2c_add_driver(&lm95245_driver);
+}
+
+static void __exit sensors_lm95245_exit(void)
+{
+	i2c_del_driver(&lm95245_driver);
+}
+
+MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>");
+MODULE_DESCRIPTION("LM95245 sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_lm95245_init);
+module_exit(sensors_lm95245_exit);
-- 
1.7.3.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] hwmon: LM95245 driver
  2011-06-28 15:11                   ` [PATCH v4] " Alexander Stein
@ 2011-06-30  9:06                     ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-06-30  9:06 UTC (permalink / raw)
  To: Alexander Stein; +Cc: Jean Delvare, lm-sensors, linux-kernel

On Tue, Jun 28, 2011 at 11:11:23AM -0400, Alexander Stein wrote:
> An hwmon driver for the National Semiconductor LM95245 dual temperature
> sensors chip.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Thanks, applied.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-06-30  9:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-21  9:52 [PATCH] hwmon: LM95245 driver Alexander Stein
2011-06-22 19:15 ` Guenter Roeck
2011-06-23  9:14   ` Alexander Stein
2011-06-23  9:33     ` Jean Delvare
2011-06-23 10:50       ` Alexander Stein
2011-06-23 14:26         ` Guenter Roeck
2011-06-23 14:47     ` Guenter Roeck
2011-06-23 15:14       ` Alexander Stein
2011-06-23 15:35         ` Guenter Roeck
2011-06-27 15:49           ` [PATCH v2] " Alexander Stein
2011-06-27 20:21             ` Guenter Roeck
2011-06-28  7:07               ` [PATCH v3] " Alexander Stein
2011-06-28 14:43                 ` Guenter Roeck
2011-06-28 15:11                   ` [PATCH v4] " Alexander Stein
2011-06-30  9:06                     ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).