linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: Add driver for VIA CPU core temperature
@ 2009-06-09  8:34 Harald Welte
  2009-06-10 17:40 ` Tomaz Mertelj
  2009-06-19 21:11 ` Jean Delvare
  0 siblings, 2 replies; 19+ messages in thread
From: Harald Welte @ 2009-06-09  8:34 UTC (permalink / raw)
  To: Linux Kernel Mailinglist; +Cc: lm-sensors

This is a driver for the on-die digital temperature sensor of
VIA's recent CPU models.

Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
---
 drivers/hwmon/Kconfig       |    8 +
 drivers/hwmon/Makefile      |    1 +
 drivers/hwmon/via-cputemp.c |  354 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/via-cputemp.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d73f5f4..e863833 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -787,6 +787,14 @@ config SENSORS_THMC50
 	  This driver can also be built as a module.  If so, the module
 	  will be called thmc50.
 
+config SENSORS_VIA_CPUTEMP
+	tristate "VIA CPU temperature sensor"
+	depends on X86
+	help
+	  If you say yes here you get support for the temperature
+	  sensor inside your CPU. Supported all are all known variants
+	  of the VIA C7 and Nano.
+
 config SENSORS_VIA686A
 	tristate "VIA686A"
 	depends on PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0ae2698..3edf7fa 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
+obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
new file mode 100644
index 0000000..1032355
--- /dev/null
+++ b/drivers/hwmon/via-cputemp.c
@@ -0,0 +1,354 @@
+/*
+ * via-cputemp.c - Driver for VIA CPU core temperature monitoring
+ * Copyright (C) 2009 VIA Technologies, Inc.
+ *
+ * based on existing coretemp.c, which is
+ *
+ * Copyright (C) 2007 Rudolf Marek <r.marek@assembler.cz>
+ *
+ * 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; version 2 of the License.
+ *
+ * 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., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/platform_device.h>
+#include <linux/cpu.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
+
+#define DRVNAME	"via-cputemp"
+
+typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;
+
+/*
+ * Functions declaration
+ */
+
+struct via_cputemp_data {
+	struct device *hwmon_dev;
+	const char *name;
+	u32 id;
+	u32 msr;
+};
+
+/*
+ * Sysfs stuff
+ */
+
+static ssize_t show_name(struct device *dev, struct device_attribute
+			  *devattr, char *buf)
+{
+	int ret;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct via_cputemp_data *data = dev_get_drvdata(dev);
+
+	if (attr->index == SHOW_NAME)
+		ret = sprintf(buf, "%s\n", data->name);
+	else	/* show label */
+		ret = sprintf(buf, "Core %d\n", data->id);
+	return ret;
+}
+
+static ssize_t show_temp(struct device *dev,
+			 struct device_attribute *devattr, char *buf)
+{
+	struct via_cputemp_data *data = dev_get_drvdata(dev);
+	u32 eax, edx;
+	int err;
+
+	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
+	if (err)
+		return -EAGAIN;
+
+	err = sprintf(buf, "%d\n", eax & 0xffffff);
+
+	return err;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
+			  SHOW_TEMP);
+static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
+static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
+
+static struct attribute *via_cputemp_attributes[] = {
+	&sensor_dev_attr_name.dev_attr.attr,
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group via_cputemp_group = {
+	.attrs = via_cputemp_attributes,
+};
+
+static int __devinit via_cputemp_probe(struct platform_device *pdev)
+{
+	struct via_cputemp_data *data;
+	struct cpuinfo_x86 *c = &cpu_data(pdev->id);
+	int err;
+	u32 eax, edx;
+
+	if (!(data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		dev_err(&pdev->dev, "Out of memory\n");
+		goto exit;
+	}
+
+	data->id = pdev->id;
+	data->name = "via-cputemp";
+
+	switch (c->x86_model) {
+	case 0xA:
+		/* C7 A */
+	case 0xD:
+		/* C7 D */
+		data->msr = 0x1169;
+		break;
+	case 0xF:
+		/* Nano */
+		data->msr = 0x1423;
+		break;
+	default:
+		err = -ENODEV;
+		goto exit_free;
+	}
+
+	/* test if we can access the TEMPERATURE MSR */
+	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Unable to access TEMPERATURE MSR, giving up\n");
+		goto exit_free;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	if ((err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group)))
+		goto exit_free;
+
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		dev_err(&pdev->dev, "Class registration failed (%d)\n",
+			err);
+		goto exit_class;
+	}
+
+	return 0;
+
+exit_class:
+	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int __devexit via_cputemp_remove(struct platform_device *pdev)
+{
+	struct via_cputemp_data *data = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
+	platform_set_drvdata(pdev, NULL);
+	kfree(data);
+	return 0;
+}
+
+static struct platform_driver via_cputemp_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = DRVNAME,
+	},
+	.probe = via_cputemp_probe,
+	.remove = __devexit_p(via_cputemp_remove),
+};
+
+struct pdev_entry {
+	struct list_head list;
+	struct platform_device *pdev;
+	unsigned int cpu;
+};
+
+static LIST_HEAD(pdev_list);
+static DEFINE_MUTEX(pdev_list_mutex);
+
+static int __cpuinit via_cputemp_device_add(unsigned int cpu)
+{
+	int err;
+	struct platform_device *pdev;
+	struct pdev_entry *pdev_entry;
+
+	pdev = platform_device_alloc(DRVNAME, cpu);
+	if (!pdev) {
+		err = -ENOMEM;
+		printk(KERN_ERR DRVNAME ": Device allocation failed\n");
+		goto exit;
+	}
+
+	pdev_entry = kzalloc(sizeof(struct pdev_entry), GFP_KERNEL);
+	if (!pdev_entry) {
+		err = -ENOMEM;
+		goto exit_device_put;
+	}
+
+	err = platform_device_add(pdev);
+	if (err) {
+		printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
+		       err);
+		goto exit_device_free;
+	}
+
+	pdev_entry->pdev = pdev;
+	pdev_entry->cpu = cpu;
+	mutex_lock(&pdev_list_mutex);
+	list_add_tail(&pdev_entry->list, &pdev_list);
+	mutex_unlock(&pdev_list_mutex);
+
+	return 0;
+
+exit_device_free:
+	kfree(pdev_entry);
+exit_device_put:
+	platform_device_put(pdev);
+exit:
+	return err;
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+static void via_cputemp_device_remove(unsigned int cpu)
+{
+	struct pdev_entry *p, *n;
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		if (p->cpu == cpu) {
+			platform_device_unregister(p->pdev);
+			list_del(&p->list);
+			kfree(p);
+		}
+	}
+	mutex_unlock(&pdev_list_mutex);
+}
+
+static int __cpuinit via_cputemp_cpu_callback(struct notifier_block *nfb,
+				 unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long) hcpu;
+
+	switch (action) {
+	case CPU_ONLINE:
+	case CPU_DOWN_FAILED:
+		via_cputemp_device_add(cpu);
+		break;
+	case CPU_DOWN_PREPARE:
+		via_cputemp_device_remove(cpu);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block via_cputemp_cpu_notifier __refdata = {
+	.notifier_call = via_cputemp_cpu_callback,
+};
+#endif				/* !CONFIG_HOTPLUG_CPU */
+
+static int __init via_cputemp_init(void)
+{
+	int i, err = -ENODEV;
+	struct pdev_entry *p, *n;
+
+	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
+		printk(KERN_DEBUG "not a VIA CPU\n");
+		goto exit;
+	}
+
+	err = platform_driver_register(&via_cputemp_driver);
+	if (err)
+		goto exit;
+
+	for_each_online_cpu(i) {
+		struct cpuinfo_x86 *c = &cpu_data(i);
+
+		if (c->x86 != 6)
+			continue;
+
+		if (c->x86_model < 0x0a)
+			continue;
+
+		if (c->x86_model > 0x0f) {
+			printk(KERN_WARNING DRVNAME ": Unknown CPU "
+				"model %x\n", c->x86_model);
+			continue;
+		}
+
+		err = via_cputemp_device_add(i);
+		if (err)
+			goto exit_devices_unreg;
+	}
+	if (list_empty(&pdev_list)) {
+		err = -ENODEV;
+		goto exit_driver_unreg;
+	}
+
+#ifdef CONFIG_HOTPLUG_CPU
+	register_hotcpu_notifier(&via_cputemp_cpu_notifier);
+#endif
+	return 0;
+
+exit_devices_unreg:
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		platform_device_unregister(p->pdev);
+		list_del(&p->list);
+		kfree(p);
+	}
+	mutex_unlock(&pdev_list_mutex);
+exit_driver_unreg:
+	platform_driver_unregister(&via_cputemp_driver);
+exit:
+	return err;
+}
+
+static void __exit via_cputemp_exit(void)
+{
+	struct pdev_entry *p, *n;
+#ifdef CONFIG_HOTPLUG_CPU
+	unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
+#endif
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		platform_device_unregister(p->pdev);
+		list_del(&p->list);
+		kfree(p);
+	}
+	mutex_unlock(&pdev_list_mutex);
+	platform_driver_unregister(&via_cputemp_driver);
+}
+
+MODULE_AUTHOR("Harald Welte <HaraldWelte@viatech.com>");
+MODULE_DESCRIPTION("VIA CPU temperature monitor");
+MODULE_LICENSE("GPL");
+
+module_init(via_cputemp_init)
+module_exit(via_cputemp_exit)
-- 
1.6.3.1

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

* Re: [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-09  8:34 [PATCH] hwmon: Add driver for VIA CPU core temperature Harald Welte
@ 2009-06-10 17:40 ` Tomaz Mertelj
  2009-06-11 22:02   ` Tomaz Mertelj
  2009-06-11 22:32   ` Andrew Morton
  2009-06-19 21:11 ` Jean Delvare
  1 sibling, 2 replies; 19+ messages in thread
From: Tomaz Mertelj @ 2009-06-10 17:40 UTC (permalink / raw)
  To: linux-kernel

Harald Welte <HaraldWelte <at> viatech.com> writes:

> 
> This is a driver for the on-die digital temperature sensor of
> VIA's recent CPU models.
> 
> Signed-off-by: Harald Welte <HaraldWelte <at> viatech.com>
> 

Harald,

I tested it on 2.6.28.3 kernel on VIA VB7002 motherboard and it does not work 
correctly.

Output form sensors:

via-cputemp-isa-0000
Adapter: ISA adapter
Core 0:       +0.0 C

On the other hand 
cat '/sys/class/hwmon/hwmon2/device/driver/via-cputemp.0/temp1_input' outputs 
numbers between 25-27. 

Tomaz Mertelj



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

* Re: [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-10 17:40 ` Tomaz Mertelj
@ 2009-06-11 22:02   ` Tomaz Mertelj
  2009-06-11 22:32   ` Andrew Morton
  1 sibling, 0 replies; 19+ messages in thread
From: Tomaz Mertelj @ 2009-06-11 22:02 UTC (permalink / raw)
  To: linux-kernel

Tomaz Mertelj <tomaz.mertelj <at> guest.arnes.si> writes:

> 
> Harald,
> 
> I tested it on 2.6.28.3 kernel on VIA VB7002 motherboard and it does not work 
> correctly.
> 


Multiplication of the output temperature by 1000 is necessary. 
Here is a modified 
patch.

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d73f5f4..e863833 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -787,6 +787,14 @@ config SENSORS_THMC50
 	  This driver can also be built as a module.  If so, the module
 	  will be called thmc50.

+config SENSORS_VIA_CPUTEMP
+	tristate "VIA CPU temperature sensor"
+	depends on X86
+	help
+	  If you say yes here you get support for the temperature
+	  sensor inside your CPU. Supported all are all known variants
+	  of the VIA C7 and Nano.
+
 config SENSORS_VIA686A
 	tristate "VIA686A"
 	depends on PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0ae2698..3edf7fa 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
+obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
new file mode 100644
index 0000000..1032355
--- /dev/null
+++ b/drivers/hwmon/via-cputemp.c
@@ -0,0 +1,354 @@
+/*
+ * via-cputemp.c - Driver for VIA CPU core temperature monitoring
+ * Copyright (C) 2009 VIA Technologies, Inc.
+ *
+ * based on existing coretemp.c, which is
+ *
+ * Copyright (C) 2007 Rudolf Marek <r.marek <at> assembler.cz>
+ *
+ * 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; version 2 of the License.
+ *
+ * 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., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/platform_device.h>
+#include <linux/cpu.h>
+#include <asm/msr.h>
+#include <asm/processor.h>
+
+#define DRVNAME	"via-cputemp"
+
+typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;
+
+/*
+ * Functions declaration
+ */
+
+struct via_cputemp_data {
+	struct device *hwmon_dev;
+	const char *name;
+	u32 id;
+	u32 msr;
+};
+
+/*
+ * Sysfs stuff
+ */
+
+static ssize_t show_name(struct device *dev, struct device_attribute
+			  *devattr, char *buf)
+{
+	int ret;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct via_cputemp_data *data = dev_get_drvdata(dev);
+
+	if (attr->index == SHOW_NAME)
+		ret = sprintf(buf, "%s\n", data->name);
+	else	/* show label */
+		ret = sprintf(buf, "Core %d\n", data->id);
+	return ret;
+}
+
+static ssize_t show_temp(struct device *dev,
+			 struct device_attribute *devattr, char *buf)
+{
+	struct via_cputemp_data *data = dev_get_drvdata(dev);
+	u32 eax, edx;
+	int err;
+
+	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
+	if (err)
+		return -EAGAIN;
+
+	err = sprintf(buf, "%d\n", (eax & 0xffffff)*1000);
+
+	return err;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
+			  SHOW_TEMP);
+static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
+static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
+
+static struct attribute *via_cputemp_attributes[] = {
+	&sensor_dev_attr_name.dev_attr.attr,
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group via_cputemp_group = {
+	.attrs = via_cputemp_attributes,
+};
+
+static int __devinit via_cputemp_probe(struct platform_device *pdev)
+{
+	struct via_cputemp_data *data;
+	struct cpuinfo_x86 *c = &cpu_data(pdev->id);
+	int err;
+	u32 eax, edx;
+
+	if (!(data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		dev_err(&pdev->dev, "Out of memory\n");
+		goto exit;
+	}
+
+	data->id = pdev->id;
+	data->name = "via-cputemp";
+
+	switch (c->x86_model) {
+	case 0xA:
+		/* C7 A */
+	case 0xD:
+		/* C7 D */
+		data->msr = 0x1169;
+		break;
+	case 0xF:
+		/* Nano */
+		data->msr = 0x1423;
+		break;
+	default:
+		err = -ENODEV;
+		goto exit_free;
+	}
+
+	/* test if we can access the TEMPERATURE MSR */
+	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Unable to access TEMPERATURE MSR, giving up\n");
+		goto exit_free;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	if ((err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group)))
+		goto exit_free;
+
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		dev_err(&pdev->dev, "Class registration failed (%d)\n",
+			err);
+		goto exit_class;
+	}
+
+	return 0;
+
+exit_class:
+	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int __devexit via_cputemp_remove(struct platform_device *pdev)
+{
+	struct via_cputemp_data *data = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
+	platform_set_drvdata(pdev, NULL);
+	kfree(data);
+	return 0;
+}
+
+static struct platform_driver via_cputemp_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = DRVNAME,
+	},
+	.probe = via_cputemp_probe,
+	.remove = __devexit_p(via_cputemp_remove),
+};
+
+struct pdev_entry {
+	struct list_head list;
+	struct platform_device *pdev;
+	unsigned int cpu;
+};
+
+static LIST_HEAD(pdev_list);
+static DEFINE_MUTEX(pdev_list_mutex);
+
+static int __cpuinit via_cputemp_device_add(unsigned int cpu)
+{
+	int err;
+	struct platform_device *pdev;
+	struct pdev_entry *pdev_entry;
+
+	pdev = platform_device_alloc(DRVNAME, cpu);
+	if (!pdev) {
+		err = -ENOMEM;
+		printk(KERN_ERR DRVNAME ": Device allocation failed\n");
+		goto exit;
+	}
+
+	pdev_entry = kzalloc(sizeof(struct pdev_entry), GFP_KERNEL);
+	if (!pdev_entry) {
+		err = -ENOMEM;
+		goto exit_device_put;
+	}
+
+	err = platform_device_add(pdev);
+	if (err) {
+		printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
+		       err);
+		goto exit_device_free;
+	}
+
+	pdev_entry->pdev = pdev;
+	pdev_entry->cpu = cpu;
+	mutex_lock(&pdev_list_mutex);
+	list_add_tail(&pdev_entry->list, &pdev_list);
+	mutex_unlock(&pdev_list_mutex);
+
+	return 0;
+
+exit_device_free:
+	kfree(pdev_entry);
+exit_device_put:
+	platform_device_put(pdev);
+exit:
+	return err;
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+static void via_cputemp_device_remove(unsigned int cpu)
+{
+	struct pdev_entry *p, *n;
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		if (p->cpu == cpu) {
+			platform_device_unregister(p->pdev);
+			list_del(&p->list);
+			kfree(p);
+		}
+	}
+	mutex_unlock(&pdev_list_mutex);
+}
+
+static int __cpuinit via_cputemp_cpu_callback(struct notifier_block *nfb,
+				 unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long) hcpu;
+
+	switch (action) {
+	case CPU_ONLINE:
+	case CPU_DOWN_FAILED:
+		via_cputemp_device_add(cpu);
+		break;
+	case CPU_DOWN_PREPARE:
+		via_cputemp_device_remove(cpu);
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block via_cputemp_cpu_notifier __refdata = {
+	.notifier_call = via_cputemp_cpu_callback,
+};
+#endif				/* !CONFIG_HOTPLUG_CPU */
+
+static int __init via_cputemp_init(void)
+{
+	int i, err = -ENODEV;
+	struct pdev_entry *p, *n;
+
+	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
+		printk(KERN_DEBUG "not a VIA CPU\n");
+		goto exit;
+	}
+
+	err = platform_driver_register(&via_cputemp_driver);
+	if (err)
+		goto exit;
+
+	for_each_online_cpu(i) {
+		struct cpuinfo_x86 *c = &cpu_data(i);
+
+		if (c->x86 != 6)
+			continue;
+
+		if (c->x86_model < 0x0a)
+			continue;
+
+		if (c->x86_model > 0x0f) {
+			printk(KERN_WARNING DRVNAME ": Unknown CPU "
+				"model %x\n", c->x86_model);
+			continue;
+		}
+
+		err = via_cputemp_device_add(i);
+		if (err)
+			goto exit_devices_unreg;
+	}
+	if (list_empty(&pdev_list)) {
+		err = -ENODEV;
+		goto exit_driver_unreg;
+	}
+
+#ifdef CONFIG_HOTPLUG_CPU
+	register_hotcpu_notifier(&via_cputemp_cpu_notifier);
+#endif
+	return 0;
+
+exit_devices_unreg:
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		platform_device_unregister(p->pdev);
+		list_del(&p->list);
+		kfree(p);
+	}
+	mutex_unlock(&pdev_list_mutex);
+exit_driver_unreg:
+	platform_driver_unregister(&via_cputemp_driver);
+exit:
+	return err;
+}
+
+static void __exit via_cputemp_exit(void)
+{
+	struct pdev_entry *p, *n;
+#ifdef CONFIG_HOTPLUG_CPU
+	unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
+#endif
+	mutex_lock(&pdev_list_mutex);
+	list_for_each_entry_safe(p, n, &pdev_list, list) {
+		platform_device_unregister(p->pdev);
+		list_del(&p->list);
+		kfree(p);
+	}
+	mutex_unlock(&pdev_list_mutex);
+	platform_driver_unregister(&via_cputemp_driver);
+}
+
+MODULE_AUTHOR("Harald Welte <HaraldWelte <at> viatech.com>");
+MODULE_DESCRIPTION("VIA CPU temperature monitor");
+MODULE_LICENSE("GPL");
+
+module_init(via_cputemp_init)
+module_exit(via_cputemp_exit)




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

* Re: [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-10 17:40 ` Tomaz Mertelj
  2009-06-11 22:02   ` Tomaz Mertelj
@ 2009-06-11 22:32   ` Andrew Morton
  2009-06-12  8:12     ` [lm-sensors] " Jean Delvare
  1 sibling, 1 reply; 19+ messages in thread
From: Andrew Morton @ 2009-06-11 22:32 UTC (permalink / raw)
  To: Tomaz Mertelj; +Cc: linux-kernel, lm-sensors, Harald Welte

On Wed, 10 Jun 2009 17:40:31 +0000 (UTC)
Tomaz Mertelj <tomaz.mertelj@guest.arnes.si> wrote:

> Harald Welte <HaraldWelte <at> viatech.com> writes:
> 
> > 
> > This is a driver for the on-die digital temperature sensor of
> > VIA's recent CPU models.
> > 
> > Signed-off-by: Harald Welte <HaraldWelte <at> viatech.com>
> > 
> 
> Harald,

You carefully removed Harald from Cc: so he probably didn't read your
email.

> I tested it on 2.6.28.3 kernel on VIA VB7002 motherboard and it does not work 
> correctly.
> 
> Output form sensors:
> 
> via-cputemp-isa-0000
> Adapter: ISA adapter
> Core 0:       +0.0 C
> 
> On the other hand 
> cat '/sys/class/hwmon/hwmon2/device/driver/via-cputemp.0/temp1_input' outputs 
> numbers between 25-27. 
> 

Thanks for testing it.

Harald, please cc myself on updates to this patch.

<looks quickly at the patch>

It has a few trivial coding-style glitches.  Please use checkpatch.

Please take a look at using hotcpu_notifier() rather than a bare
register_hotcpu_notifier().  Many ifdefs should fall away as a result.


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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-06-11 22:32   ` Andrew Morton
@ 2009-06-12  8:12     ` Jean Delvare
  2009-06-12  9:11       ` Tomaz Mertelj
                         ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Jean Delvare @ 2009-06-12  8:12 UTC (permalink / raw)
  To: Tomaz Mertelj, Harald Welte; +Cc: Andrew Morton, linux-kernel, lm-sensors

On Thu, 11 Jun 2009 15:32:55 -0700, Andrew Morton wrote:
> On Wed, 10 Jun 2009 17:40:31 +0000 (UTC)
> Tomaz Mertelj <tomaz.mertelj@guest.arnes.si> wrote:
> 
> > Harald Welte <HaraldWelte <at> viatech.com> writes:
> > 
> > > 
> > > This is a driver for the on-die digital temperature sensor of
> > > VIA's recent CPU models.
> > > 
> > > Signed-off-by: Harald Welte <HaraldWelte <at> viatech.com>
> > > 
> > 
> > Harald,
> 
> You carefully removed Harald from Cc: so he probably didn't read your
> email.
> 
> > I tested it on 2.6.28.3 kernel on VIA VB7002 motherboard and it does not work 
> > correctly.
> > 
> > Output form sensors:
> > 
> > via-cputemp-isa-0000

Harald, dashes in hwmon chip names are _prohibited_. Please change to
via_cputemp or similar.

> > Adapter: ISA adapter
> > Core 0:       +0.0 C
> > 
> > On the other hand 
> > cat '/sys/class/hwmon/hwmon2/device/driver/via-cputemp.0/temp1_input' outputs 
> > numbers between 25-27. 
> > 

Temperature values are supposed to be expressed in millidegrees C, not
degrees C as it seems to be doing (although 25 degrees C seems pretty
low for a CPU temperature?) The drivers needs to multiply values by
1000 before exporting them to sysfs. Then "sensors" will report the
correct temperature value.

-- 
Jean Delvare

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-06-12  8:12     ` [lm-sensors] " Jean Delvare
@ 2009-06-12  9:11       ` Tomaz Mertelj
  2009-06-12  9:27       ` Harald Welte
  2009-06-12 11:46       ` Michael S. Zick
  2 siblings, 0 replies; 19+ messages in thread
From: Tomaz Mertelj @ 2009-06-12  9:11 UTC (permalink / raw)
  To: linux-kernel

Jean Delvare <khali <at> linux-fr.org> writes:
>  
> > > On the other hand 
> > > cat '/sys/class/hwmon/hwmon2/device/driver/via-cputemp.0/temp1_input' 
outputs 
> > > numbers between 25-27. 
> > > 
> 
> Temperature values are supposed to be expressed in millidegrees C, not
> degrees C as it seems to be doing (although 25 degrees C seems pretty
> low for a CPU temperature?) The drivers needs to multiply values by
> 1000 before exporting them to sysfs. Then "sensors" will report the
> correct temperature value.
> 

It goes up to 48 degrees C when I run cpuburnP6. 

Tomaz Mertelj



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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12  8:12     ` [lm-sensors] " Jean Delvare
  2009-06-12  9:11       ` Tomaz Mertelj
@ 2009-06-12  9:27       ` Harald Welte
  2009-06-12 11:46       ` Michael S. Zick
  2 siblings, 0 replies; 19+ messages in thread
From: Harald Welte @ 2009-06-12  9:27 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

Hi Jean and Tomaz,

On Fri, Jun 12, 2009 at 10:12:00AM +0200, Jean Delvare wrote:
> > > Harald,
> > 
> > You carefully removed Harald from Cc: so he probably didn't read your
> > email.

That was indeed true, thanks for noticing this.

> > > via-cputemp-isa-0000
> 
> Harald, dashes in hwmon chip names are _prohibited_. Please change to
> via_cputemp or similar.

done.

> Temperature values are supposed to be expressed in millidegrees C, not
> degrees C as it seems to be doing 

> (although 25 degrees C seems pretty low for a CPU temperature?) 

It's really low, though the VIA Nano on my desk has 30 degrees C and doesn't
need a fan.  Only during kernel compiles it goes to something like 40
centigrade.

Some older C7 (Model A) had an errata where the temperature sensors had a
really high error on low temperatures.

There is also another errata with that C7 Model A, where if you underclock
the FSB of the CPU, the temperature reading would be wrong (but much too
high in that case).

Since there really is nothing we can do in software to fix this, I didn't
put any of this in the driver.  What I could do though is some bits to
add a printk() message once you load the driver on such a system.

> The drivers needs to multiply values by 1000 before exporting them to sysfs.
> Then "sensors" will report the correct temperature value.

done.  Please find an incremental patch right here:
=============
diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
index 1032355..2abe516 100644
--- a/drivers/hwmon/via-cputemp.c
+++ b/drivers/hwmon/via-cputemp.c
@@ -37,7 +37,7 @@
 #include <asm/msr.h>
 #include <asm/processor.h>
 
-#define DRVNAME	"via-cputemp"
+#define DRVNAME	"via_cputemp"
 
 typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;
 
@@ -81,7 +81,7 @@ static ssize_t show_temp(struct device *dev,
 	if (err)
 		return -EAGAIN;
 
-	err = sprintf(buf, "%d\n", eax & 0xffffff);
+	err = sprintf(buf, "%d\n", (eax & 0xffffff) * 1000);
 
 	return err;
 }
=============

I'll re-submit the full driver after adding the above-mentioned printk.

-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-06-12  8:12     ` [lm-sensors] " Jean Delvare
  2009-06-12  9:11       ` Tomaz Mertelj
  2009-06-12  9:27       ` Harald Welte
@ 2009-06-12 11:46       ` Michael S. Zick
  2009-06-12 12:54         ` Harald Welte
  2 siblings, 1 reply; 19+ messages in thread
From: Michael S. Zick @ 2009-06-12 11:46 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Tomaz Mertelj, Harald Welte, Andrew Morton, linux-kernel, lm-sensors

On Fri June 12 2009, Jean Delvare wrote:
> On Thu, 11 Jun 2009 15:32:55 -0700, Andrew Morton wrote:
> > On Wed, 10 Jun 2009 17:40:31 +0000 (UTC)
> > Tomaz Mertelj <tomaz.mertelj@guest.arnes.si> wrote:
> > 
> > > Harald Welte <HaraldWelte <at> viatech.com> writes:
> > > 
> > > > 
> > > > This is a driver for the on-die digital temperature sensor of
> > > > VIA's recent CPU models.
> > > > 
> > > > Signed-off-by: Harald Welte <HaraldWelte <at> viatech.com>
> > > > 
> > > 
> > > Harald,
> > 
> > You carefully removed Harald from Cc: so he probably didn't read your
> > email.
> > 
> > > I tested it on 2.6.28.3 kernel on VIA VB7002 motherboard and it does not work 
> > > correctly.
> > > 
> > > Output form sensors:
> > > 
> > > via-cputemp-isa-0000
> 
> Harald, dashes in hwmon chip names are _prohibited_. Please change to
> via_cputemp or similar.
> 
> > > Adapter: ISA adapter
> > > Core 0:       +0.0 C
> > > 
> > > On the other hand 
> > > cat '/sys/class/hwmon/hwmon2/device/driver/via-cputemp.0/temp1_input' outputs 
> > > numbers between 25-27. 
> > > 
> 
> Temperature values are supposed to be expressed in millidegrees C, not
> degrees C as it seems to be doing (although 25 degrees C seems pretty
> low for a CPU temperature?) The drivers needs to multiply values by
> 1000 before exporting them to sysfs. Then "sensors" will report the
> correct temperature value.
> 

Ah, 25 degrees C is room temperature - real hard for the junction temperature
to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.

Look for an "off by one" error in shifting or masking the value.

Mike

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12 11:46       ` Michael S. Zick
@ 2009-06-12 12:54         ` Harald Welte
  2009-06-12 13:09           ` Michael S. Zick
  2009-06-12 13:41           ` Michael S. Zick
  0 siblings, 2 replies; 19+ messages in thread
From: Harald Welte @ 2009-06-12 12:54 UTC (permalink / raw)
  To: Michael S. Zick
  Cc: Jean Delvare, Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

On Fri, Jun 12, 2009 at 06:46:45AM -0500, Michael S. Zick wrote:
> > Temperature values are supposed to be expressed in millidegrees C, not
> > degrees C as it seems to be doing (although 25 degrees C seems pretty
> > low for a CPU temperature?) The drivers needs to multiply values by
> > 1000 before exporting them to sysfs. Then "sensors" will report the
> > correct temperature value.
> > 
> 
> Ah, 25 degrees C is room temperature - real hard for the junction temperature
> to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.
> 
> Look for an "off by one" error in shifting or masking the value.

there is no shifting and the masking is 0xffffffff :)

it might be that the BIOS is doing something wrong when programming the
calibration MSR's at early botoup.  I would need the contents of MSR 
0x1160 ... 0x116C as well as 0x1152 and 0x1153 to be able to determine that.

-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12 12:54         ` Harald Welte
@ 2009-06-12 13:09           ` Michael S. Zick
  2009-06-12 13:41           ` Michael S. Zick
  1 sibling, 0 replies; 19+ messages in thread
From: Michael S. Zick @ 2009-06-12 13:09 UTC (permalink / raw)
  To: Harald Welte
  Cc: Jean Delvare, Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

On Fri June 12 2009, Harald Welte wrote:
> On Fri, Jun 12, 2009 at 06:46:45AM -0500, Michael S. Zick wrote:
> > > Temperature values are supposed to be expressed in millidegrees C, not
> > > degrees C as it seems to be doing (although 25 degrees C seems pretty
> > > low for a CPU temperature?) The drivers needs to multiply values by
> > > 1000 before exporting them to sysfs. Then "sensors" will report the
> > > correct temperature value.
> > > 
> > 
> > Ah, 25 degrees C is room temperature - real hard for the junction temperature
> > to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.
> > 
> > Look for an "off by one" error in shifting or masking the value.
> 
> there is no shifting and the masking is 0xffffffff :)
> 
> it might be that the BIOS is doing something wrong when programming the
> calibration MSR's at early botoup.  I would need the contents of MSR 
> 0x1160 ... 0x116C as well as 0x1152 and 0x1153 to be able to determine that.
> 

I was commenting on someone else's observation - -
Will build with your patch and poke at it a bit.

I should be able to read those registers from /dev/cpu/0/msr, correct?
Or is there a "safe" mask in the msr driver that prevents reading random
MSR registers?

Mike

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12 12:54         ` Harald Welte
  2009-06-12 13:09           ` Michael S. Zick
@ 2009-06-12 13:41           ` Michael S. Zick
  2009-06-12 13:47             ` Michael S. Zick
  2009-06-12 14:23             ` Michael S. Zick
  1 sibling, 2 replies; 19+ messages in thread
From: Michael S. Zick @ 2009-06-12 13:41 UTC (permalink / raw)
  To: Harald Welte
  Cc: Jean Delvare, Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

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

On Fri June 12 2009, Harald Welte wrote:
> On Fri, Jun 12, 2009 at 06:46:45AM -0500, Michael S. Zick wrote:
> > > Temperature values are supposed to be expressed in millidegrees C, not
> > > degrees C as it seems to be doing (although 25 degrees C seems pretty
> > > low for a CPU temperature?) The drivers needs to multiply values by
> > > 1000 before exporting them to sysfs. Then "sensors" will report the
> > > correct temperature value.
> > > 
> > 
> > Ah, 25 degrees C is room temperature - real hard for the junction temperature
> > to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.
> > 
> > Look for an "off by one" error in shifting or masking the value.
> 
> there is no shifting and the masking is 0xffffffff :)
> 
> it might be that the BIOS is doing something wrong when programming the
> calibration MSR's at early botoup.  I would need the contents of MSR 
> 0x1160 ... 0x116C as well as 0x1152 and 0x1153 to be able to determine that.
> 

root@cb01:~# for r in 0x1160 0x1161 0x1162 0x1163 0x1164 0x1165 0x1166 0x1167 0x1168 0x1169 0x116a 0x116b 0x116c 0x1152 0x1153 ; do ./rdmsr $r ; done
MSR register 0x1160 => 08:04:98:10:b7:ef:8f:f4
MSR register 0x1161 => 08:04:98:10:b7:fb:af:f4
MSR register 0x1162 => 08:04:98:10:b7:ec:ff:f4
MSR register 0x1163 => 08:04:98:10:b7:fa:cf:f4
MSR register 0x1164 => 08:04:98:10:b7:f4:cf:f4
MSR register 0x1165 => 08:04:98:10:b7:f3:2f:f4
MSR register 0x1166 => 08:04:98:10:b7:f1:7f:f4
MSR register 0x1167 => 08:04:98:10:b7:f2:0f:f4
MSR register 0x1168 => 08:04:98:10:b7:ef:0f:f4
MSR register 0x1169 => 08:04:98:10:b7:fc:8f:f4
MSR register 0x116a => 08:04:98:10:b7:ef:8f:f4
MSR register 0x116b => 08:04:98:10:b7:f8:bf:f4
MSR register 0x116c => 08:04:98:10:b7:f9:df:f4
MSR register 0x1152 => 08:04:98:10:b7:ed:5f:f4
MSR register 0x1153 => 08:04:98:10:b7:ed:cf:f4

The high 32b look strange in that output - might be
the utility I used (attached).

Mike


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

/* By Ron Minnich @ wiki.laptop.org */
/* No rights or license mentioned.  */

#define _LARGEFILE64_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>

int main (int argc, char *argv[])
{
    unsigned char buf[8];
    int fd_msr, i;
    unsigned long addr = 0;

    if (argc < 2) {
        printf("usage:rdmsr reg\n");
        exit(1);
    }
    addr = strtoul(argv[1], NULL, 0);

    fd_msr = open("/dev/cpu/0/msr", O_RDONLY);
    lseek64(fd_msr, (off64_t)addr, SEEK_SET);
    read(fd_msr, buf, 8);

    printf("MSR register 0x%lx => ", addr);
    for (i = 7; i > 0; i--)
        printf("%2.2x:", buf[i]);
    printf("%2.2x\n", buf[i]);

    return(0);
}


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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12 13:41           ` Michael S. Zick
@ 2009-06-12 13:47             ` Michael S. Zick
  2009-06-12 14:23             ` Michael S. Zick
  1 sibling, 0 replies; 19+ messages in thread
From: Michael S. Zick @ 2009-06-12 13:47 UTC (permalink / raw)
  To: Harald Welte
  Cc: Jean Delvare, Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

On Fri June 12 2009, Michael S. Zick wrote:
> On Fri June 12 2009, Harald Welte wrote:
> > On Fri, Jun 12, 2009 at 06:46:45AM -0500, Michael S. Zick wrote:
> > > > Temperature values are supposed to be expressed in millidegrees C, not
> > > > degrees C as it seems to be doing (although 25 degrees C seems pretty
> > > > low for a CPU temperature?) The drivers needs to multiply values by
> > > > 1000 before exporting them to sysfs. Then "sensors" will report the
> > > > correct temperature value.
> > > > 
> > > 
> > > Ah, 25 degrees C is room temperature - real hard for the junction temperature
> > > to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.
> > > 
> > > Look for an "off by one" error in shifting or masking the value.
> > 
> > there is no shifting and the masking is 0xffffffff :)
> > 
> > it might be that the BIOS is doing something wrong when programming the
> > calibration MSR's at early botoup.  I would need the contents of MSR 
> > 0x1160 ... 0x116C as well as 0x1152 and 0x1153 to be able to determine that.
> > 
> 
> root@cb01:~# for r in 0x1160 0x1161 0x1162 0x1163 0x1164 0x1165 0x1166 0x1167 0x1168 0x1169 0x116a 0x116b 0x116c 0x1152 0x1153 ; do ./rdmsr $r ; done
> MSR register 0x1160 => 08:04:98:10:b7:ef:8f:f4
> MSR register 0x1161 => 08:04:98:10:b7:fb:af:f4
> MSR register 0x1162 => 08:04:98:10:b7:ec:ff:f4
> MSR register 0x1163 => 08:04:98:10:b7:fa:cf:f4
> MSR register 0x1164 => 08:04:98:10:b7:f4:cf:f4
> MSR register 0x1165 => 08:04:98:10:b7:f3:2f:f4
> MSR register 0x1166 => 08:04:98:10:b7:f1:7f:f4
> MSR register 0x1167 => 08:04:98:10:b7:f2:0f:f4
> MSR register 0x1168 => 08:04:98:10:b7:ef:0f:f4
> MSR register 0x1169 => 08:04:98:10:b7:fc:8f:f4
> MSR register 0x116a => 08:04:98:10:b7:ef:8f:f4
> MSR register 0x116b => 08:04:98:10:b7:f8:bf:f4
> MSR register 0x116c => 08:04:98:10:b7:f9:df:f4
> MSR register 0x1152 => 08:04:98:10:b7:ed:5f:f4
> MSR register 0x1153 => 08:04:98:10:b7:ed:cf:f4
> 
> The high 32b look strange in that output - might be
> the utility I used (attached).
> 
> Mike
> 
> 

Hmm ... if the utility is correct and I read that
under a 16bit mask - that is 36,852 milliDegrees.
Sounds about right for this machine and conditions.

Mike

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-12 13:41           ` Michael S. Zick
  2009-06-12 13:47             ` Michael S. Zick
@ 2009-06-12 14:23             ` Michael S. Zick
  1 sibling, 0 replies; 19+ messages in thread
From: Michael S. Zick @ 2009-06-12 14:23 UTC (permalink / raw)
  To: Harald Welte
  Cc: Jean Delvare, Tomaz Mertelj, Andrew Morton, linux-kernel, lm-sensors

On Fri June 12 2009, Michael S. Zick wrote:
> On Fri June 12 2009, Harald Welte wrote:
> > On Fri, Jun 12, 2009 at 06:46:45AM -0500, Michael S. Zick wrote:
> > > > Temperature values are supposed to be expressed in millidegrees C, not
> > > > degrees C as it seems to be doing (although 25 degrees C seems pretty
> > > > low for a CPU temperature?) The drivers needs to multiply values by
> > > > 1000 before exporting them to sysfs. Then "sensors" will report the
> > > > correct temperature value.
> > > > 
> > > 
> > > Ah, 25 degrees C is room temperature - real hard for the junction temperature
> > > to be 25 degrees C with power applied; lacking an infinitely perfect heatsink.
> > > 
> > > Look for an "off by one" error in shifting or masking the value.
> > 
> > there is no shifting and the masking is 0xffffffff :)
> > 
> > it might be that the BIOS is doing something wrong when programming the
> > calibration MSR's at early botoup.  I would need the contents of MSR 
> > 0x1160 ... 0x116C as well as 0x1152 and 0x1153 to be able to determine that.
> > 
> 
> root@cb01:~# for r in 0x1160 0x1161 0x1162 0x1163 0x1164 0x1165 0x1166 0x1167 0x1168 0x1169 0x116a 0x116b 0x116c 0x1152 0x1153 ; do ./rdmsr $r ; done
> MSR register 0x1160 => 08:04:98:10:b7:ef:8f:f4
> MSR register 0x1161 => 08:04:98:10:b7:fb:af:f4
> MSR register 0x1162 => 08:04:98:10:b7:ec:ff:f4
> MSR register 0x1163 => 08:04:98:10:b7:fa:cf:f4
> MSR register 0x1164 => 08:04:98:10:b7:f4:cf:f4
> MSR register 0x1165 => 08:04:98:10:b7:f3:2f:f4
> MSR register 0x1166 => 08:04:98:10:b7:f1:7f:f4
> MSR register 0x1167 => 08:04:98:10:b7:f2:0f:f4
> MSR register 0x1168 => 08:04:98:10:b7:ef:0f:f4
> MSR register 0x1169 => 08:04:98:10:b7:fc:8f:f4
> MSR register 0x116a => 08:04:98:10:b7:ef:8f:f4
> MSR register 0x116b => 08:04:98:10:b7:f8:bf:f4
> MSR register 0x116c => 08:04:98:10:b7:f9:df:f4
> MSR register 0x1152 => 08:04:98:10:b7:ed:5f:f4
> MSR register 0x1153 => 08:04:98:10:b7:ed:cf:f4
> 
> The high 32b look strange in that output - might be
> the utility I used (attached).
> 

Yes - that utility had problems with casting the address;
I think I fixed that, this looks better:

root@cb01:~# for r in 0x1160 0x1161 0x1162 0x1163 0x1164 0x1165 0x1166 0x1167 0x1168 0x1169 0x116a 0x116b 0x116c 0x1152 0x1153 ; do ./rdmsrll $r ; done
MSR register 0x1160 => 08:04:83:94:bf:86:e1:d8
MSR register 0x1161 => 08:04:83:94:bf:ec:73:78
MSR register 0x1162 => 08:04:83:94:bf:bf:4b:58
MSR register 0x1163 => 08:04:83:94:bf:bc:82:28
MSR register 0x1164 => 08:04:83:94:bf:b1:1d:98
MSR register 0x1165 => 08:04:83:94:bf:cb:09:88
MSR register 0x1166 => 08:04:83:94:bf:d0:03:e8
MSR register 0x1167 => 08:04:83:94:bf:91:26:18
MSR register 0x1168 => 08:04:83:94:bf:99:26:38
MSR register 0x1169 => 08:04:83:94:bf:b3:f1:08
MSR register 0x116a => 08:04:83:94:bf:d5:42:88
MSR register 0x116b => 08:04:83:94:bf:f8:b6:28
MSR register 0x116c => 08:04:83:94:bf:f6:68:88
MSR register 0x1152 => 08:04:83:94:bf:d5:b4:f8
MSR register 0x1153 => 08:04:83:94:bf:bd:d5:28

Mike
> Mike
> 
> 



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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-06-09  8:34 [PATCH] hwmon: Add driver for VIA CPU core temperature Harald Welte
  2009-06-10 17:40 ` Tomaz Mertelj
@ 2009-06-19 21:11 ` Jean Delvare
  2009-06-27 18:34   ` Juerg Haefliger
  1 sibling, 1 reply; 19+ messages in thread
From: Jean Delvare @ 2009-06-19 21:11 UTC (permalink / raw)
  To: Harald Welte; +Cc: Linux Kernel Mailinglist, lm-sensors, Juerg Haefliger

Hi Harald,

On Tue, 9 Jun 2009 16:34:06 +0800, Harald Welte wrote:
> This is a driver for the on-die digital temperature sensor of
> VIA's recent CPU models.
> 
> Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
> ---
>  drivers/hwmon/Kconfig       |    8 +
>  drivers/hwmon/Makefile      |    1 +
>  drivers/hwmon/via-cputemp.c |  354 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 363 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/via-cputemp.c

Interesting. I'm adding Juerg Haefliger in Cc. A few months ago, Juerg
submitted a driver named "c7temp" doing basically the same, with
mitigated success. I seem to remember that Juerg's driver was also
displaying either the VID or Vcore for the CPU. We don't want to have
two drivers for the same hardware, so let's merge everything in one
driver and push this upstream.

Juerg, you were there first, so I'd like to hear you on this. Are there
differences between Harald'd driver and yours? Which one should I pick?

For now, here's a review of Harald's driver:

> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d73f5f4..e863833 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -787,6 +787,14 @@ config SENSORS_THMC50
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called thmc50.
>  
> +config SENSORS_VIA_CPUTEMP
> +	tristate "VIA CPU temperature sensor"
> +	depends on X86
> +	help
> +	  If you say yes here you get support for the temperature
> +	  sensor inside your CPU. Supported all are all known variants
> +	  of the VIA C7 and Nano.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on PCI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 0ae2698..3edf7fa 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> +obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
> new file mode 100644
> index 0000000..1032355
> --- /dev/null
> +++ b/drivers/hwmon/via-cputemp.c
> @@ -0,0 +1,354 @@
> +/*
> + * via-cputemp.c - Driver for VIA CPU core temperature monitoring
> + * Copyright (C) 2009 VIA Technologies, Inc.
> + *
> + * based on existing coretemp.c, which is
> + *
> + * Copyright (C) 2007 Rudolf Marek <r.marek@assembler.cz>
> + *
> + * 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; version 2 of the License.
> + *
> + * 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., 51 Franklin Street, Fifth Floor, Boston, MA
> + * 02110-1301 USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/list.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpu.h>
> +#include <asm/msr.h>
> +#include <asm/processor.h>
> +
> +#define DRVNAME	"via-cputemp"
> +
> +typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;

No typedef in kernel code please, an enum is enough.

> +
> +/*
> + * Functions declaration
> + */
> +
> +struct via_cputemp_data {
> +	struct device *hwmon_dev;
> +	const char *name;
> +	u32 id;
> +	u32 msr;
> +};
> +
> +/*
> + * Sysfs stuff
> + */
> +
> +static ssize_t show_name(struct device *dev, struct device_attribute
> +			  *devattr, char *buf)
> +{
> +	int ret;
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct via_cputemp_data *data = dev_get_drvdata(dev);
> +
> +	if (attr->index == SHOW_NAME)
> +		ret = sprintf(buf, "%s\n", data->name);
> +	else	/* show label */
> +		ret = sprintf(buf, "Core %d\n", data->id);
> +	return ret;
> +}
> +
> +static ssize_t show_temp(struct device *dev,
> +			 struct device_attribute *devattr, char *buf)
> +{
> +	struct via_cputemp_data *data = dev_get_drvdata(dev);
> +	u32 eax, edx;
> +	int err;
> +
> +	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
> +	if (err)
> +		return -EAGAIN;
> +
> +	err = sprintf(buf, "%d\n", eax & 0xffffff);
> +
> +	return err;

return sprintf()... would be clearer, as the returned value is never an
error in this case.

Also note that in theory the result could overflow once multiplied by
1000.

> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
> +			  SHOW_TEMP);
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
> +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
> +
> +static struct attribute *via_cputemp_attributes[] = {
> +	&sensor_dev_attr_name.dev_attr.attr,
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group via_cputemp_group = {
> +	.attrs = via_cputemp_attributes,
> +};
> +
> +static int __devinit via_cputemp_probe(struct platform_device *pdev)
> +{
> +	struct via_cputemp_data *data;
> +	struct cpuinfo_x86 *c = &cpu_data(pdev->id);
> +	int err;
> +	u32 eax, edx;
> +
> +	if (!(data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL))) {

ERROR: do not use assignment in if condition

> +		err = -ENOMEM;
> +		dev_err(&pdev->dev, "Out of memory\n");
> +		goto exit;
> +	}
> +
> +	data->id = pdev->id;

pdev->id is an unsigned int, so shouldn't data->id be too?

> +	data->name = "via-cputemp";

This must be made "via_cputemp", as dashes aren't accepted in hwmon
device names.

> +
> +	switch (c->x86_model) {
> +	case 0xA:
> +		/* C7 A */
> +	case 0xD:
> +		/* C7 D */
> +		data->msr = 0x1169;
> +		break;
> +	case 0xF:
> +		/* Nano */
> +		data->msr = 0x1423;
> +		break;
> +	default:
> +		err = -ENODEV;
> +		goto exit_free;
> +	}
> +
> +	/* test if we can access the TEMPERATURE MSR */
> +	err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
> +	if (err) {

In the runtime read function, you handle errors with -EAGAIN, which
means transient errors are expected. If such an error happens at
registration time, we could see random failures. That's confusing for
the user. I believe you should either skip the test at registration
(are there really CPU models where it always fails?) or try more than
once to rule out temporary failures.

> +		dev_err(&pdev->dev,
> +			"Unable to access TEMPERATURE MSR, giving up\n");
> +		goto exit_free;
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	if ((err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group)))

ERROR: do not use assignment in if condition

> +		goto exit_free;
> +
> +	data->hwmon_dev = hwmon_device_register(&pdev->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_dev);
> +		dev_err(&pdev->dev, "Class registration failed (%d)\n",
> +			err);
> +		goto exit_class;
> +	}
> +
> +	return 0;
> +
> +exit_class:
> +	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
> +exit_free:

A platform_set_drvdata(pdev, NULL) would be welcome here.

> +	kfree(data);

This is a little inconsistent, as the first label refers to the last
action that succeeded and the second label refers to the first cleanup
step to execute. Renaming exit_class to exit_remove would help.

> +exit:
> +	return err;
> +}
> +
> +static int __devexit via_cputemp_remove(struct platform_device *pdev)
> +{
> +	struct via_cputemp_data *data = platform_get_drvdata(pdev);
> +
> +	hwmon_device_unregister(data->hwmon_dev);
> +	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct platform_driver via_cputemp_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = DRVNAME,
> +	},
> +	.probe = via_cputemp_probe,
> +	.remove = __devexit_p(via_cputemp_remove),
> +};
> +
> +struct pdev_entry {
> +	struct list_head list;
> +	struct platform_device *pdev;
> +	unsigned int cpu;
> +};
> +
> +static LIST_HEAD(pdev_list);
> +static DEFINE_MUTEX(pdev_list_mutex);
> +
> +static int __cpuinit via_cputemp_device_add(unsigned int cpu)
> +{
> +	int err;
> +	struct platform_device *pdev;
> +	struct pdev_entry *pdev_entry;
> +
> +	pdev = platform_device_alloc(DRVNAME, cpu);
> +	if (!pdev) {
> +		err = -ENOMEM;
> +		printk(KERN_ERR DRVNAME ": Device allocation failed\n");
> +		goto exit;
> +	}
> +
> +	pdev_entry = kzalloc(sizeof(struct pdev_entry), GFP_KERNEL);
> +	if (!pdev_entry) {
> +		err = -ENOMEM;
> +		goto exit_device_put;
> +	}
> +
> +	err = platform_device_add(pdev);
> +	if (err) {
> +		printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
> +		       err);
> +		goto exit_device_free;
> +	}
> +
> +	pdev_entry->pdev = pdev;
> +	pdev_entry->cpu = cpu;
> +	mutex_lock(&pdev_list_mutex);
> +	list_add_tail(&pdev_entry->list, &pdev_list);
> +	mutex_unlock(&pdev_list_mutex);
> +
> +	return 0;
> +
> +exit_device_free:
> +	kfree(pdev_entry);
> +exit_device_put:
> +	platform_device_put(pdev);
> +exit:
> +	return err;
> +}
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +static void via_cputemp_device_remove(unsigned int cpu)
> +{
> +	struct pdev_entry *p, *n;
> +	mutex_lock(&pdev_list_mutex);
> +	list_for_each_entry_safe(p, n, &pdev_list, list) {
> +		if (p->cpu == cpu) {
> +			platform_device_unregister(p->pdev);
> +			list_del(&p->list);
> +			kfree(p);
> +		}
> +	}
> +	mutex_unlock(&pdev_list_mutex);
> +}
> +
> +static int __cpuinit via_cputemp_cpu_callback(struct notifier_block *nfb,
> +				 unsigned long action, void *hcpu)
> +{
> +	unsigned int cpu = (unsigned long) hcpu;
> +
> +	switch (action) {
> +	case CPU_ONLINE:
> +	case CPU_DOWN_FAILED:
> +		via_cputemp_device_add(cpu);
> +		break;
> +	case CPU_DOWN_PREPARE:
> +		via_cputemp_device_remove(cpu);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block via_cputemp_cpu_notifier __refdata = {
> +	.notifier_call = via_cputemp_cpu_callback,
> +};
> +#endif				/* !CONFIG_HOTPLUG_CPU */
> +
> +static int __init via_cputemp_init(void)
> +{
> +	int i, err = -ENODEV;

err would be better initialized when the error occurs, for consistency.

> +	struct pdev_entry *p, *n;
> +
> +	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
> +		printk(KERN_DEBUG "not a VIA CPU\n");

Missing DRV_NAME.

> +		goto exit;
> +	}
> +
> +	err = platform_driver_register(&via_cputemp_driver);
> +	if (err)
> +		goto exit;
> +
> +	for_each_online_cpu(i) {
> +		struct cpuinfo_x86 *c = &cpu_data(i);
> +
> +		if (c->x86 != 6)
> +			continue;
> +
> +		if (c->x86_model < 0x0a)
> +			continue;
> +
> +		if (c->x86_model > 0x0f) {
> +			printk(KERN_WARNING DRVNAME ": Unknown CPU "
> +				"model %x\n", c->x86_model);

Please use 0x%x for clarity.

> +			continue;
> +		}
> +
> +		err = via_cputemp_device_add(i);
> +		if (err)
> +			goto exit_devices_unreg;
> +	}
> +	if (list_empty(&pdev_list)) {
> +		err = -ENODEV;
> +		goto exit_driver_unreg;
> +	}
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +	register_hotcpu_notifier(&via_cputemp_cpu_notifier);
> +#endif
> +	return 0;
> +
> +exit_devices_unreg:
> +	mutex_lock(&pdev_list_mutex);
> +	list_for_each_entry_safe(p, n, &pdev_list, list) {
> +		platform_device_unregister(p->pdev);
> +		list_del(&p->list);
> +		kfree(p);
> +	}
> +	mutex_unlock(&pdev_list_mutex);
> +exit_driver_unreg:
> +	platform_driver_unregister(&via_cputemp_driver);
> +exit:
> +	return err;
> +}
> +
> +static void __exit via_cputemp_exit(void)
> +{
> +	struct pdev_entry *p, *n;
> +#ifdef CONFIG_HOTPLUG_CPU
> +	unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
> +#endif
> +	mutex_lock(&pdev_list_mutex);
> +	list_for_each_entry_safe(p, n, &pdev_list, list) {
> +		platform_device_unregister(p->pdev);
> +		list_del(&p->list);
> +		kfree(p);
> +	}
> +	mutex_unlock(&pdev_list_mutex);
> +	platform_driver_unregister(&via_cputemp_driver);
> +}
> +
> +MODULE_AUTHOR("Harald Welte <HaraldWelte@viatech.com>");
> +MODULE_DESCRIPTION("VIA CPU temperature monitor");
> +MODULE_LICENSE("GPL");
> +
> +module_init(via_cputemp_init)
> +module_exit(via_cputemp_exit)

The rest looks OK.

-- 
Jean Delvare

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-06-19 21:11 ` Jean Delvare
@ 2009-06-27 18:34   ` Juerg Haefliger
  2009-08-13 18:42     ` Juerg Haefliger
  0 siblings, 1 reply; 19+ messages in thread
From: Juerg Haefliger @ 2009-06-27 18:34 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Harald Welte, Linux Kernel Mailinglist, lm-sensors

Hi Jean,


> Hi Harald,
>
> On Tue, 9 Jun 2009 16:34:06 +0800, Harald Welte wrote:
>> This is a driver for the on-die digital temperature sensor of
>> VIA's recent CPU models.
>>
>> Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
>> ---
>>  drivers/hwmon/Kconfig       |    8 +
>>  drivers/hwmon/Makefile      |    1 +
>>  drivers/hwmon/via-cputemp.c |  354 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 363 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/hwmon/via-cputemp.c
>
> Interesting. I'm adding Juerg Haefliger in Cc. A few months ago, Juerg
> submitted a driver named "c7temp" doing basically the same, with
> mitigated success. I seem to remember that Juerg's driver was also
> displaying either the VID or Vcore for the CPU. We don't want to have
> two drivers for the same hardware, so let's merge everything in one
> driver and push this upstream.
>
> Juerg, you were there first, so I'd like to hear you on this. Are there
> differences between Harald'd driver and yours? Which one should I pick?

Personally I don't care which driver you pick as long as we get one
into mainline eventually. Just a few comments:

1) My driver uses the CPUID instruction to read the performance
registers that contain the temp and voltage data. Harald's driver
reads MSRs. I don't know if there are any benefits of using one method
over the other.

2) If we pick Harald's, it would be nice if his driver can also read
and export the CPU core voltage.

3) Quite a few testers of my driver reported 0 temp readings for some
C7 CPUs. I was never able to figure out why some CPUs return 0 temp
but I'm guessing it depends on the thermal monitor settings. I'd like
to understand what is going on and hope Harald can shed some light.

...juerg


> For now, here's a review of Harald's driver:
>
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index d73f5f4..e863833 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -787,6 +787,14 @@ config SENSORS_THMC50
>>         This driver can also be built as a module.  If so, the module
>>         will be called thmc50.
>>
>> +config SENSORS_VIA_CPUTEMP
>> +     tristate "VIA CPU temperature sensor"
>> +     depends on X86
>> +     help
>> +       If you say yes here you get support for the temperature
>> +       sensor inside your CPU. Supported all are all known variants
>> +       of the VIA C7 and Nano.
>> +
>>  config SENSORS_VIA686A
>>       tristate "VIA686A"
>>       depends on PCI
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 0ae2698..3edf7fa 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -82,6 +82,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>  obj-$(CONFIG_SENSORS_SMSC47M1)       += smsc47m1.o
>>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>>  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>> +obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>  obj-$(CONFIG_SENSORS_VIA686A)        += via686a.o
>>  obj-$(CONFIG_SENSORS_VT1211) += vt1211.o
>>  obj-$(CONFIG_SENSORS_VT8231) += vt8231.o
>> diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
>> new file mode 100644
>> index 0000000..1032355
>> --- /dev/null
>> +++ b/drivers/hwmon/via-cputemp.c
>> @@ -0,0 +1,354 @@
>> +/*
>> + * via-cputemp.c - Driver for VIA CPU core temperature monitoring
>> + * Copyright (C) 2009 VIA Technologies, Inc.
>> + *
>> + * based on existing coretemp.c, which is
>> + *
>> + * Copyright (C) 2007 Rudolf Marek <r.marek@assembler.cz>
>> + *
>> + * 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; version 2 of the License.
>> + *
>> + * 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., 51 Franklin Street, Fifth Floor, Boston, MA
>> + * 02110-1301 USA.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/list.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/cpu.h>
>> +#include <asm/msr.h>
>> +#include <asm/processor.h>
>> +
>> +#define DRVNAME      "via-cputemp"
>> +
>> +typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;
>
> No typedef in kernel code please, an enum is enough.
>
>> +
>> +/*
>> + * Functions declaration
>> + */
>> +
>> +struct via_cputemp_data {
>> +     struct device *hwmon_dev;
>> +     const char *name;
>> +     u32 id;
>> +     u32 msr;
>> +};
>> +
>> +/*
>> + * Sysfs stuff
>> + */
>> +
>> +static ssize_t show_name(struct device *dev, struct device_attribute
>> +                       *devattr, char *buf)
>> +{
>> +     int ret;
>> +     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +     struct via_cputemp_data *data = dev_get_drvdata(dev);
>> +
>> +     if (attr->index == SHOW_NAME)
>> +             ret = sprintf(buf, "%s\n", data->name);
>> +     else    /* show label */
>> +             ret = sprintf(buf, "Core %d\n", data->id);
>> +     return ret;
>> +}
>> +
>> +static ssize_t show_temp(struct device *dev,
>> +                      struct device_attribute *devattr, char *buf)
>> +{
>> +     struct via_cputemp_data *data = dev_get_drvdata(dev);
>> +     u32 eax, edx;
>> +     int err;
>> +
>> +     err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
>> +     if (err)
>> +             return -EAGAIN;
>> +
>> +     err = sprintf(buf, "%d\n", eax & 0xffffff);
>> +
>> +     return err;
>
> return sprintf()... would be clearer, as the returned value is never an
> error in this case.
>
> Also note that in theory the result could overflow once multiplied by
> 1000.
>
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
>> +                       SHOW_TEMP);
>> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
>> +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
>> +
>> +static struct attribute *via_cputemp_attributes[] = {
>> +     &sensor_dev_attr_name.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_label.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static const struct attribute_group via_cputemp_group = {
>> +     .attrs = via_cputemp_attributes,
>> +};
>> +
>> +static int __devinit via_cputemp_probe(struct platform_device *pdev)
>> +{
>> +     struct via_cputemp_data *data;
>> +     struct cpuinfo_x86 *c = &cpu_data(pdev->id);
>> +     int err;
>> +     u32 eax, edx;
>> +
>> +     if (!(data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL))) {
>
> ERROR: do not use assignment in if condition
>
>> +             err = -ENOMEM;
>> +             dev_err(&pdev->dev, "Out of memory\n");
>> +             goto exit;
>> +     }
>> +
>> +     data->id = pdev->id;
>
> pdev->id is an unsigned int, so shouldn't data->id be too?
>
>> +     data->name = "via-cputemp";
>
> This must be made "via_cputemp", as dashes aren't accepted in hwmon
> device names.
>
>> +
>> +     switch (c->x86_model) {
>> +     case 0xA:
>> +             /* C7 A */
>> +     case 0xD:
>> +             /* C7 D */
>> +             data->msr = 0x1169;
>> +             break;
>> +     case 0xF:
>> +             /* Nano */
>> +             data->msr = 0x1423;
>> +             break;
>> +     default:
>> +             err = -ENODEV;
>> +             goto exit_free;
>> +     }
>> +
>> +     /* test if we can access the TEMPERATURE MSR */
>> +     err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
>> +     if (err) {
>
> In the runtime read function, you handle errors with -EAGAIN, which
> means transient errors are expected. If such an error happens at
> registration time, we could see random failures. That's confusing for
> the user. I believe you should either skip the test at registration
> (are there really CPU models where it always fails?) or try more than
> once to rule out temporary failures.
>
>> +             dev_err(&pdev->dev,
>> +                     "Unable to access TEMPERATURE MSR, giving up\n");
>> +             goto exit_free;
>> +     }
>> +
>> +     platform_set_drvdata(pdev, data);
>> +
>> +     if ((err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group)))
>
> ERROR: do not use assignment in if condition
>
>> +             goto exit_free;
>> +
>> +     data->hwmon_dev = hwmon_device_register(&pdev->dev);
>> +     if (IS_ERR(data->hwmon_dev)) {
>> +             err = PTR_ERR(data->hwmon_dev);
>> +             dev_err(&pdev->dev, "Class registration failed (%d)\n",
>> +                     err);
>> +             goto exit_class;
>> +     }
>> +
>> +     return 0;
>> +
>> +exit_class:
>> +     sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
>> +exit_free:
>
> A platform_set_drvdata(pdev, NULL) would be welcome here.
>
>> +     kfree(data);
>
> This is a little inconsistent, as the first label refers to the last
> action that succeeded and the second label refers to the first cleanup
> step to execute. Renaming exit_class to exit_remove would help.
>
>> +exit:
>> +     return err;
>> +}
>> +
>> +static int __devexit via_cputemp_remove(struct platform_device *pdev)
>> +{
>> +     struct via_cputemp_data *data = platform_get_drvdata(pdev);
>> +
>> +     hwmon_device_unregister(data->hwmon_dev);
>> +     sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
>> +     platform_set_drvdata(pdev, NULL);
>> +     kfree(data);
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver via_cputemp_driver = {
>> +     .driver = {
>> +             .owner = THIS_MODULE,
>> +             .name = DRVNAME,
>> +     },
>> +     .probe = via_cputemp_probe,
>> +     .remove = __devexit_p(via_cputemp_remove),
>> +};
>> +
>> +struct pdev_entry {
>> +     struct list_head list;
>> +     struct platform_device *pdev;
>> +     unsigned int cpu;
>> +};
>> +
>> +static LIST_HEAD(pdev_list);
>> +static DEFINE_MUTEX(pdev_list_mutex);
>> +
>> +static int __cpuinit via_cputemp_device_add(unsigned int cpu)
>> +{
>> +     int err;
>> +     struct platform_device *pdev;
>> +     struct pdev_entry *pdev_entry;
>> +
>> +     pdev = platform_device_alloc(DRVNAME, cpu);
>> +     if (!pdev) {
>> +             err = -ENOMEM;
>> +             printk(KERN_ERR DRVNAME ": Device allocation failed\n");
>> +             goto exit;
>> +     }
>> +
>> +     pdev_entry = kzalloc(sizeof(struct pdev_entry), GFP_KERNEL);
>> +     if (!pdev_entry) {
>> +             err = -ENOMEM;
>> +             goto exit_device_put;
>> +     }
>> +
>> +     err = platform_device_add(pdev);
>> +     if (err) {
>> +             printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
>> +                    err);
>> +             goto exit_device_free;
>> +     }
>> +
>> +     pdev_entry->pdev = pdev;
>> +     pdev_entry->cpu = cpu;
>> +     mutex_lock(&pdev_list_mutex);
>> +     list_add_tail(&pdev_entry->list, &pdev_list);
>> +     mutex_unlock(&pdev_list_mutex);
>> +
>> +     return 0;
>> +
>> +exit_device_free:
>> +     kfree(pdev_entry);
>> +exit_device_put:
>> +     platform_device_put(pdev);
>> +exit:
>> +     return err;
>> +}
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +static void via_cputemp_device_remove(unsigned int cpu)
>> +{
>> +     struct pdev_entry *p, *n;
>> +     mutex_lock(&pdev_list_mutex);
>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>> +             if (p->cpu == cpu) {
>> +                     platform_device_unregister(p->pdev);
>> +                     list_del(&p->list);
>> +                     kfree(p);
>> +             }
>> +     }
>> +     mutex_unlock(&pdev_list_mutex);
>> +}
>> +
>> +static int __cpuinit via_cputemp_cpu_callback(struct notifier_block *nfb,
>> +                              unsigned long action, void *hcpu)
>> +{
>> +     unsigned int cpu = (unsigned long) hcpu;
>> +
>> +     switch (action) {
>> +     case CPU_ONLINE:
>> +     case CPU_DOWN_FAILED:
>> +             via_cputemp_device_add(cpu);
>> +             break;
>> +     case CPU_DOWN_PREPARE:
>> +             via_cputemp_device_remove(cpu);
>> +             break;
>> +     }
>> +     return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block via_cputemp_cpu_notifier __refdata = {
>> +     .notifier_call = via_cputemp_cpu_callback,
>> +};
>> +#endif                               /* !CONFIG_HOTPLUG_CPU */
>> +
>> +static int __init via_cputemp_init(void)
>> +{
>> +     int i, err = -ENODEV;
>
> err would be better initialized when the error occurs, for consistency.
>
>> +     struct pdev_entry *p, *n;
>> +
>> +     if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
>> +             printk(KERN_DEBUG "not a VIA CPU\n");
>
> Missing DRV_NAME.
>
>> +             goto exit;
>> +     }
>> +
>> +     err = platform_driver_register(&via_cputemp_driver);
>> +     if (err)
>> +             goto exit;
>> +
>> +     for_each_online_cpu(i) {
>> +             struct cpuinfo_x86 *c = &cpu_data(i);
>> +
>> +             if (c->x86 != 6)
>> +                     continue;
>> +
>> +             if (c->x86_model < 0x0a)
>> +                     continue;
>> +
>> +             if (c->x86_model > 0x0f) {
>> +                     printk(KERN_WARNING DRVNAME ": Unknown CPU "
>> +                             "model %x\n", c->x86_model);
>
> Please use 0x%x for clarity.
>
>> +                     continue;
>> +             }
>> +
>> +             err = via_cputemp_device_add(i);
>> +             if (err)
>> +                     goto exit_devices_unreg;
>> +     }
>> +     if (list_empty(&pdev_list)) {
>> +             err = -ENODEV;
>> +             goto exit_driver_unreg;
>> +     }
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +     register_hotcpu_notifier(&via_cputemp_cpu_notifier);
>> +#endif
>> +     return 0;
>> +
>> +exit_devices_unreg:
>> +     mutex_lock(&pdev_list_mutex);
>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>> +             platform_device_unregister(p->pdev);
>> +             list_del(&p->list);
>> +             kfree(p);
>> +     }
>> +     mutex_unlock(&pdev_list_mutex);
>> +exit_driver_unreg:
>> +     platform_driver_unregister(&via_cputemp_driver);
>> +exit:
>> +     return err;
>> +}
>> +
>> +static void __exit via_cputemp_exit(void)
>> +{
>> +     struct pdev_entry *p, *n;
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +     unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
>> +#endif
>> +     mutex_lock(&pdev_list_mutex);
>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>> +             platform_device_unregister(p->pdev);
>> +             list_del(&p->list);
>> +             kfree(p);
>> +     }
>> +     mutex_unlock(&pdev_list_mutex);
>> +     platform_driver_unregister(&via_cputemp_driver);
>> +}
>> +
>> +MODULE_AUTHOR("Harald Welte <HaraldWelte@viatech.com>");
>> +MODULE_DESCRIPTION("VIA CPU temperature monitor");
>> +MODULE_LICENSE("GPL");
>> +
>> +module_init(via_cputemp_init)
>> +module_exit(via_cputemp_exit)
>
> The rest looks OK.
>
> --
> Jean Delvare
>

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-06-27 18:34   ` Juerg Haefliger
@ 2009-08-13 18:42     ` Juerg Haefliger
  2009-08-19 14:32       ` Harald Welte
  0 siblings, 1 reply; 19+ messages in thread
From: Juerg Haefliger @ 2009-08-13 18:42 UTC (permalink / raw)
  To: Jean Delvare; +Cc: Harald Welte, Linux Kernel Mailinglist, lm-sensors

Jean,

Harald seems to be to busy to answer emails. Can we push my driver upstream?

...juerg


On Sat, Jun 27, 2009 at 11:34 AM, Juerg Haefliger<juergh@gmail.com> wrote:
> Hi Jean,
>
>
>> Hi Harald,
>>
>> On Tue, 9 Jun 2009 16:34:06 +0800, Harald Welte wrote:
>>> This is a driver for the on-die digital temperature sensor of
>>> VIA's recent CPU models.
>>>
>>> Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
>>> ---
>>>  drivers/hwmon/Kconfig       |    8 +
>>>  drivers/hwmon/Makefile      |    1 +
>>>  drivers/hwmon/via-cputemp.c |  354 +++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 363 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/hwmon/via-cputemp.c
>>
>> Interesting. I'm adding Juerg Haefliger in Cc. A few months ago, Juerg
>> submitted a driver named "c7temp" doing basically the same, with
>> mitigated success. I seem to remember that Juerg's driver was also
>> displaying either the VID or Vcore for the CPU. We don't want to have
>> two drivers for the same hardware, so let's merge everything in one
>> driver and push this upstream.
>>
>> Juerg, you were there first, so I'd like to hear you on this. Are there
>> differences between Harald'd driver and yours? Which one should I pick?
>
> Personally I don't care which driver you pick as long as we get one
> into mainline eventually. Just a few comments:
>
> 1) My driver uses the CPUID instruction to read the performance
> registers that contain the temp and voltage data. Harald's driver
> reads MSRs. I don't know if there are any benefits of using one method
> over the other.
>
> 2) If we pick Harald's, it would be nice if his driver can also read
> and export the CPU core voltage.
>
> 3) Quite a few testers of my driver reported 0 temp readings for some
> C7 CPUs. I was never able to figure out why some CPUs return 0 temp
> but I'm guessing it depends on the thermal monitor settings. I'd like
> to understand what is going on and hope Harald can shed some light.
>
> ...juerg
>
>
>> For now, here's a review of Harald's driver:
>>
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index d73f5f4..e863833 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -787,6 +787,14 @@ config SENSORS_THMC50
>>>         This driver can also be built as a module.  If so, the module
>>>         will be called thmc50.
>>>
>>> +config SENSORS_VIA_CPUTEMP
>>> +     tristate "VIA CPU temperature sensor"
>>> +     depends on X86
>>> +     help
>>> +       If you say yes here you get support for the temperature
>>> +       sensor inside your CPU. Supported all are all known variants
>>> +       of the VIA C7 and Nano.
>>> +
>>>  config SENSORS_VIA686A
>>>       tristate "VIA686A"
>>>       depends on PCI
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 0ae2698..3edf7fa 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -82,6 +82,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>>  obj-$(CONFIG_SENSORS_SMSC47M1)       += smsc47m1.o
>>>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>>>  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>>> +obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>>  obj-$(CONFIG_SENSORS_VIA686A)        += via686a.o
>>>  obj-$(CONFIG_SENSORS_VT1211) += vt1211.o
>>>  obj-$(CONFIG_SENSORS_VT8231) += vt8231.o
>>> diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
>>> new file mode 100644
>>> index 0000000..1032355
>>> --- /dev/null
>>> +++ b/drivers/hwmon/via-cputemp.c
>>> @@ -0,0 +1,354 @@
>>> +/*
>>> + * via-cputemp.c - Driver for VIA CPU core temperature monitoring
>>> + * Copyright (C) 2009 VIA Technologies, Inc.
>>> + *
>>> + * based on existing coretemp.c, which is
>>> + *
>>> + * Copyright (C) 2007 Rudolf Marek <r.marek@assembler.cz>
>>> + *
>>> + * 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; version 2 of the License.
>>> + *
>>> + * 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., 51 Franklin Street, Fifth Floor, Boston, MA
>>> + * 02110-1301 USA.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/init.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/err.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/list.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/cpu.h>
>>> +#include <asm/msr.h>
>>> +#include <asm/processor.h>
>>> +
>>> +#define DRVNAME      "via-cputemp"
>>> +
>>> +typedef enum { SHOW_TEMP, SHOW_LABEL, SHOW_NAME } SHOW;
>>
>> No typedef in kernel code please, an enum is enough.
>>
>>> +
>>> +/*
>>> + * Functions declaration
>>> + */
>>> +
>>> +struct via_cputemp_data {
>>> +     struct device *hwmon_dev;
>>> +     const char *name;
>>> +     u32 id;
>>> +     u32 msr;
>>> +};
>>> +
>>> +/*
>>> + * Sysfs stuff
>>> + */
>>> +
>>> +static ssize_t show_name(struct device *dev, struct device_attribute
>>> +                       *devattr, char *buf)
>>> +{
>>> +     int ret;
>>> +     struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +     struct via_cputemp_data *data = dev_get_drvdata(dev);
>>> +
>>> +     if (attr->index == SHOW_NAME)
>>> +             ret = sprintf(buf, "%s\n", data->name);
>>> +     else    /* show label */
>>> +             ret = sprintf(buf, "Core %d\n", data->id);
>>> +     return ret;
>>> +}
>>> +
>>> +static ssize_t show_temp(struct device *dev,
>>> +                      struct device_attribute *devattr, char *buf)
>>> +{
>>> +     struct via_cputemp_data *data = dev_get_drvdata(dev);
>>> +     u32 eax, edx;
>>> +     int err;
>>> +
>>> +     err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
>>> +     if (err)
>>> +             return -EAGAIN;
>>> +
>>> +     err = sprintf(buf, "%d\n", eax & 0xffffff);
>>> +
>>> +     return err;
>>
>> return sprintf()... would be clearer, as the returned value is never an
>> error in this case.
>>
>> Also note that in theory the result could overflow once multiplied by
>> 1000.
>>
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL,
>>> +                       SHOW_TEMP);
>>> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_name, NULL, SHOW_LABEL);
>>> +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, SHOW_NAME);
>>> +
>>> +static struct attribute *via_cputemp_attributes[] = {
>>> +     &sensor_dev_attr_name.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_label.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static const struct attribute_group via_cputemp_group = {
>>> +     .attrs = via_cputemp_attributes,
>>> +};
>>> +
>>> +static int __devinit via_cputemp_probe(struct platform_device *pdev)
>>> +{
>>> +     struct via_cputemp_data *data;
>>> +     struct cpuinfo_x86 *c = &cpu_data(pdev->id);
>>> +     int err;
>>> +     u32 eax, edx;
>>> +
>>> +     if (!(data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL))) {
>>
>> ERROR: do not use assignment in if condition
>>
>>> +             err = -ENOMEM;
>>> +             dev_err(&pdev->dev, "Out of memory\n");
>>> +             goto exit;
>>> +     }
>>> +
>>> +     data->id = pdev->id;
>>
>> pdev->id is an unsigned int, so shouldn't data->id be too?
>>
>>> +     data->name = "via-cputemp";
>>
>> This must be made "via_cputemp", as dashes aren't accepted in hwmon
>> device names.
>>
>>> +
>>> +     switch (c->x86_model) {
>>> +     case 0xA:
>>> +             /* C7 A */
>>> +     case 0xD:
>>> +             /* C7 D */
>>> +             data->msr = 0x1169;
>>> +             break;
>>> +     case 0xF:
>>> +             /* Nano */
>>> +             data->msr = 0x1423;
>>> +             break;
>>> +     default:
>>> +             err = -ENODEV;
>>> +             goto exit_free;
>>> +     }
>>> +
>>> +     /* test if we can access the TEMPERATURE MSR */
>>> +     err = rdmsr_safe_on_cpu(data->id, data->msr, &eax, &edx);
>>> +     if (err) {
>>
>> In the runtime read function, you handle errors with -EAGAIN, which
>> means transient errors are expected. If such an error happens at
>> registration time, we could see random failures. That's confusing for
>> the user. I believe you should either skip the test at registration
>> (are there really CPU models where it always fails?) or try more than
>> once to rule out temporary failures.
>>
>>> +             dev_err(&pdev->dev,
>>> +                     "Unable to access TEMPERATURE MSR, giving up\n");
>>> +             goto exit_free;
>>> +     }
>>> +
>>> +     platform_set_drvdata(pdev, data);
>>> +
>>> +     if ((err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group)))
>>
>> ERROR: do not use assignment in if condition
>>
>>> +             goto exit_free;
>>> +
>>> +     data->hwmon_dev = hwmon_device_register(&pdev->dev);
>>> +     if (IS_ERR(data->hwmon_dev)) {
>>> +             err = PTR_ERR(data->hwmon_dev);
>>> +             dev_err(&pdev->dev, "Class registration failed (%d)\n",
>>> +                     err);
>>> +             goto exit_class;
>>> +     }
>>> +
>>> +     return 0;
>>> +
>>> +exit_class:
>>> +     sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
>>> +exit_free:
>>
>> A platform_set_drvdata(pdev, NULL) would be welcome here.
>>
>>> +     kfree(data);
>>
>> This is a little inconsistent, as the first label refers to the last
>> action that succeeded and the second label refers to the first cleanup
>> step to execute. Renaming exit_class to exit_remove would help.
>>
>>> +exit:
>>> +     return err;
>>> +}
>>> +
>>> +static int __devexit via_cputemp_remove(struct platform_device *pdev)
>>> +{
>>> +     struct via_cputemp_data *data = platform_get_drvdata(pdev);
>>> +
>>> +     hwmon_device_unregister(data->hwmon_dev);
>>> +     sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
>>> +     platform_set_drvdata(pdev, NULL);
>>> +     kfree(data);
>>> +     return 0;
>>> +}
>>> +
>>> +static struct platform_driver via_cputemp_driver = {
>>> +     .driver = {
>>> +             .owner = THIS_MODULE,
>>> +             .name = DRVNAME,
>>> +     },
>>> +     .probe = via_cputemp_probe,
>>> +     .remove = __devexit_p(via_cputemp_remove),
>>> +};
>>> +
>>> +struct pdev_entry {
>>> +     struct list_head list;
>>> +     struct platform_device *pdev;
>>> +     unsigned int cpu;
>>> +};
>>> +
>>> +static LIST_HEAD(pdev_list);
>>> +static DEFINE_MUTEX(pdev_list_mutex);
>>> +
>>> +static int __cpuinit via_cputemp_device_add(unsigned int cpu)
>>> +{
>>> +     int err;
>>> +     struct platform_device *pdev;
>>> +     struct pdev_entry *pdev_entry;
>>> +
>>> +     pdev = platform_device_alloc(DRVNAME, cpu);
>>> +     if (!pdev) {
>>> +             err = -ENOMEM;
>>> +             printk(KERN_ERR DRVNAME ": Device allocation failed\n");
>>> +             goto exit;
>>> +     }
>>> +
>>> +     pdev_entry = kzalloc(sizeof(struct pdev_entry), GFP_KERNEL);
>>> +     if (!pdev_entry) {
>>> +             err = -ENOMEM;
>>> +             goto exit_device_put;
>>> +     }
>>> +
>>> +     err = platform_device_add(pdev);
>>> +     if (err) {
>>> +             printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
>>> +                    err);
>>> +             goto exit_device_free;
>>> +     }
>>> +
>>> +     pdev_entry->pdev = pdev;
>>> +     pdev_entry->cpu = cpu;
>>> +     mutex_lock(&pdev_list_mutex);
>>> +     list_add_tail(&pdev_entry->list, &pdev_list);
>>> +     mutex_unlock(&pdev_list_mutex);
>>> +
>>> +     return 0;
>>> +
>>> +exit_device_free:
>>> +     kfree(pdev_entry);
>>> +exit_device_put:
>>> +     platform_device_put(pdev);
>>> +exit:
>>> +     return err;
>>> +}
>>> +
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +static void via_cputemp_device_remove(unsigned int cpu)
>>> +{
>>> +     struct pdev_entry *p, *n;
>>> +     mutex_lock(&pdev_list_mutex);
>>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>>> +             if (p->cpu == cpu) {
>>> +                     platform_device_unregister(p->pdev);
>>> +                     list_del(&p->list);
>>> +                     kfree(p);
>>> +             }
>>> +     }
>>> +     mutex_unlock(&pdev_list_mutex);
>>> +}
>>> +
>>> +static int __cpuinit via_cputemp_cpu_callback(struct notifier_block *nfb,
>>> +                              unsigned long action, void *hcpu)
>>> +{
>>> +     unsigned int cpu = (unsigned long) hcpu;
>>> +
>>> +     switch (action) {
>>> +     case CPU_ONLINE:
>>> +     case CPU_DOWN_FAILED:
>>> +             via_cputemp_device_add(cpu);
>>> +             break;
>>> +     case CPU_DOWN_PREPARE:
>>> +             via_cputemp_device_remove(cpu);
>>> +             break;
>>> +     }
>>> +     return NOTIFY_OK;
>>> +}
>>> +
>>> +static struct notifier_block via_cputemp_cpu_notifier __refdata = {
>>> +     .notifier_call = via_cputemp_cpu_callback,
>>> +};
>>> +#endif                               /* !CONFIG_HOTPLUG_CPU */
>>> +
>>> +static int __init via_cputemp_init(void)
>>> +{
>>> +     int i, err = -ENODEV;
>>
>> err would be better initialized when the error occurs, for consistency.
>>
>>> +     struct pdev_entry *p, *n;
>>> +
>>> +     if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
>>> +             printk(KERN_DEBUG "not a VIA CPU\n");
>>
>> Missing DRV_NAME.
>>
>>> +             goto exit;
>>> +     }
>>> +
>>> +     err = platform_driver_register(&via_cputemp_driver);
>>> +     if (err)
>>> +             goto exit;
>>> +
>>> +     for_each_online_cpu(i) {
>>> +             struct cpuinfo_x86 *c = &cpu_data(i);
>>> +
>>> +             if (c->x86 != 6)
>>> +                     continue;
>>> +
>>> +             if (c->x86_model < 0x0a)
>>> +                     continue;
>>> +
>>> +             if (c->x86_model > 0x0f) {
>>> +                     printk(KERN_WARNING DRVNAME ": Unknown CPU "
>>> +                             "model %x\n", c->x86_model);
>>
>> Please use 0x%x for clarity.
>>
>>> +                     continue;
>>> +             }
>>> +
>>> +             err = via_cputemp_device_add(i);
>>> +             if (err)
>>> +                     goto exit_devices_unreg;
>>> +     }
>>> +     if (list_empty(&pdev_list)) {
>>> +             err = -ENODEV;
>>> +             goto exit_driver_unreg;
>>> +     }
>>> +
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +     register_hotcpu_notifier(&via_cputemp_cpu_notifier);
>>> +#endif
>>> +     return 0;
>>> +
>>> +exit_devices_unreg:
>>> +     mutex_lock(&pdev_list_mutex);
>>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>>> +             platform_device_unregister(p->pdev);
>>> +             list_del(&p->list);
>>> +             kfree(p);
>>> +     }
>>> +     mutex_unlock(&pdev_list_mutex);
>>> +exit_driver_unreg:
>>> +     platform_driver_unregister(&via_cputemp_driver);
>>> +exit:
>>> +     return err;
>>> +}
>>> +
>>> +static void __exit via_cputemp_exit(void)
>>> +{
>>> +     struct pdev_entry *p, *n;
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +     unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
>>> +#endif
>>> +     mutex_lock(&pdev_list_mutex);
>>> +     list_for_each_entry_safe(p, n, &pdev_list, list) {
>>> +             platform_device_unregister(p->pdev);
>>> +             list_del(&p->list);
>>> +             kfree(p);
>>> +     }
>>> +     mutex_unlock(&pdev_list_mutex);
>>> +     platform_driver_unregister(&via_cputemp_driver);
>>> +}
>>> +
>>> +MODULE_AUTHOR("Harald Welte <HaraldWelte@viatech.com>");
>>> +MODULE_DESCRIPTION("VIA CPU temperature monitor");
>>> +MODULE_LICENSE("GPL");
>>> +
>>> +module_init(via_cputemp_init)
>>> +module_exit(via_cputemp_exit)
>>
>> The rest looks OK.
>>
>> --
>> Jean Delvare
>>
>

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-08-13 18:42     ` Juerg Haefliger
@ 2009-08-19 14:32       ` Harald Welte
  2009-08-19 16:52         ` Juerg Haefliger
  0 siblings, 1 reply; 19+ messages in thread
From: Harald Welte @ 2009-08-19 14:32 UTC (permalink / raw)
  To: Juerg Haefliger; +Cc: Jean Delvare, Linux Kernel Mailinglist, lm-sensors

On Thu, Aug 13, 2009 at 11:42:17AM -0700, Juerg Haefliger wrote:
> Jean,
> 
> Harald seems to be to busy to answer emails. Can we push my driver upstream?

Well, I am not too busy anymore.  A terrible number of weeks just behind me.

Sorry for the delays.

I personally don't really care which driver is merged, just as long as one of
them ends up in mainline.
-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core  temperature
  2009-08-19 14:32       ` Harald Welte
@ 2009-08-19 16:52         ` Juerg Haefliger
  2009-12-09  7:17           ` Harald Welte
  0 siblings, 1 reply; 19+ messages in thread
From: Juerg Haefliger @ 2009-08-19 16:52 UTC (permalink / raw)
  To: Harald Welte; +Cc: Jean Delvare, Linux Kernel Mailinglist, lm-sensors

Hi Harald,


> On Thu, Aug 13, 2009 at 11:42:17AM -0700, Juerg Haefliger wrote:
>> Jean,
>>
>> Harald seems to be to busy to answer emails. Can we push my driver upstream?
>
> Well, I am not too busy anymore.  A terrible number of weeks just behind me.

Good to hear :-)


> Sorry for the delays.
>
> I personally don't really care which driver is merged, just as long as one of
> them ends up in mainline.


Do you have any answers to my previous questions:

1) My driver uses the CPUID instruction to read the performance
registers that contain the temp and voltage data. Harald's driver
reads MSRs. I don't know if there are any benefits of using one method
over the other.

2) If we pick Harald's, it would be nice if his driver can also read
and export the CPU core voltage.

3) Quite a few testers of my driver reported 0 temp readings for some
C7 CPUs. I was never able to figure out why some CPUs return 0 temp
but I'm guessing it depends on the thermal monitor settings. I'd like
to understand what is going on and hope Harald can shed some light.


...juerg


> - Harald Welte <HaraldWelte@viatech.com>            http://linux.via.com.tw/
> ============================================================================
> VIA Free and Open Source Software Liaison
>

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

* Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature
  2009-08-19 16:52         ` Juerg Haefliger
@ 2009-12-09  7:17           ` Harald Welte
  0 siblings, 0 replies; 19+ messages in thread
From: Harald Welte @ 2009-12-09  7:17 UTC (permalink / raw)
  To: Juerg Haefliger; +Cc: Jean Delvare, Linux Kernel Mailinglist, lm-sensors

Dear Juerg,

On Wed, Aug 19, 2009 at 09:52:25AM -0700, Juerg Haefliger wrote:

> Do you have any answers to my previous questions:
> 
> 1) My driver uses the CPUID instruction to read the performance
> registers that contain the temp and voltage data. Harald's driver
> reads MSRs. I don't know if there are any benefits of using one method
> over the other.

not that I am aware of, at least nowhere in the documentation.  

> 2) If we pick Harald's, it would be nice if his driver can also read
> and export the CPU core voltage.

I'm willing to add that, but I really don't need to push my driver. It doesn't
matter to me, I just want those features supported in mainline.

> 3) Quite a few testers of my driver reported 0 temp readings for some
> C7 CPUs. I was never able to figure out why some CPUs return 0 temp
> but I'm guessing it depends on the thermal monitor settings. I'd like
> to understand what is going on and hope Harald can shed some light.

Unfortunately I cannot.    I can contact some of the CPU hardware folks about
it, but then it would probably have more details on which particular CPU models
(or mainbaords) exposed that behavior, and what exactly was their CPU version
(as seen from cpuid).

-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison

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

end of thread, other threads:[~2009-12-09  8:08 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-09  8:34 [PATCH] hwmon: Add driver for VIA CPU core temperature Harald Welte
2009-06-10 17:40 ` Tomaz Mertelj
2009-06-11 22:02   ` Tomaz Mertelj
2009-06-11 22:32   ` Andrew Morton
2009-06-12  8:12     ` [lm-sensors] " Jean Delvare
2009-06-12  9:11       ` Tomaz Mertelj
2009-06-12  9:27       ` Harald Welte
2009-06-12 11:46       ` Michael S. Zick
2009-06-12 12:54         ` Harald Welte
2009-06-12 13:09           ` Michael S. Zick
2009-06-12 13:41           ` Michael S. Zick
2009-06-12 13:47             ` Michael S. Zick
2009-06-12 14:23             ` Michael S. Zick
2009-06-19 21:11 ` Jean Delvare
2009-06-27 18:34   ` Juerg Haefliger
2009-08-13 18:42     ` Juerg Haefliger
2009-08-19 14:32       ` Harald Welte
2009-08-19 16:52         ` Juerg Haefliger
2009-12-09  7:17           ` Harald Welte

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).