[PATCH] hwmon: Add driver for VIA CPU core temperature

[PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

[lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

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

Re: [PATCH] hwmon: Add driver for VIA CPU core temperature

[Tomaz Mertelj]

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

Re: [PATCH] hwmon: Add driver for VIA CPU core temperature

[Tomaz Mertelj]

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)

Re: [PATCH] hwmon: Add driver for VIA CPU core temperature

[Andrew Morton]

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.

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Andrew Morton]

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.


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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Jean Delvare]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Jean Delvare]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Tomaz Mertelj]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU

[Harald Welte]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Michael S. Zick]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Michael S. Zick]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU

[Harald Welte]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Michael S. Zick]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Michael S. Zick]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Michael S. Zick]

[-- 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);
}

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Michael S. Zick]

[-- 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);
}


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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Michael S. Zick]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Michael S. Zick]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Michael S. Zick]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Michael S. Zick]

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



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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Jean Delvare]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Jean Delvare]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Juerg Haefliger]

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
>

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Juerg Haefliger]

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
>

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Juerg Haefliger]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Juerg Haefliger]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Harald Welte]

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

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Juerg Haefliger]

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
>

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Juerg Haefliger]

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
>

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Harald Welte]

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Harald Welte]

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

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

[lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature

[Jean Delvare]

From: Harald Welte <HaraldWelte@viatech.com>
Subject: hwmon: Add driver for VIA CPU core temperature

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

[JD: Misc clean-ups.]

Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
Cc: Juerg Haefliger <juergh@gmail.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
Harald, Juerg, all: this is the driver I intend to push to Linus for
kernel 2.6.33. It is based on Harald's driver, to which I applied
almost all the fixes I had suggested in my review back in June. I do
not own a VIA-based system myself so I couldn't test it. I would
appreciate if someone could test it and report, just to make sure I
didn't accidentally break the driver with my clean-ups. Thanks.

I didn't pick Justin's modified driver because its indentation didn't
match what the kernel wants, and I didn't want to waste my time
re-indenting it.

Still missing in this driver are:
* Warnings that should be printed for CPU models with known errata.
* Support for Vcore (or is it VID?) reporting.
Both can be added on top of the current driver, using incremental
patches.

 drivers/hwmon/Kconfig       |    8 
 drivers/hwmon/Makefile      |    1 
 drivers/hwmon/via-cputemp.c |  356 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 365 insertions(+)
 create mode 100644 drivers/hwmon/via-cputemp.c

--- linux-2.6.33-rc0.orig/drivers/hwmon/Kconfig	2009-12-10 18:57:39.000000000 +0100
+++ linux-2.6.33-rc0/drivers/hwmon/Kconfig	2009-12-10 18:57:44.000000000 +0100
@@ -822,6 +822,14 @@ config SENSORS_TMP421
 	  This driver can also be built as a module.  If so, the module
 	  will be called tmp421.
 
+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
--- linux-2.6.33-rc0.orig/drivers/hwmon/Makefile	2009-12-10 18:57:39.000000000 +0100
+++ linux-2.6.33-rc0/drivers/hwmon/Makefile	2009-12-10 18:57:44.000000000 +0100
@@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc4
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.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
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33-rc0/drivers/hwmon/via-cputemp.c	2009-12-10 19:54:27.000000000 +0100
@@ -0,0 +1,356 @@
+/*
+ * 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"
+
+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;
+
+	return sprintf(buf, "%lu\n", ((unsigned long)eax & 0xffffff) * 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;
+
+	data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL);
+	if (!data) {
+		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);
+
+	err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group);
+	if (err)
+		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_remove;
+	}
+
+	return 0;
+
+exit_remove:
+	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
+exit_free:
+	platform_set_drvdata(pdev, NULL);
+	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;
+	struct pdev_entry *p, *n;
+
+	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
+		printk(KERN_DEBUG DRVNAME ": Not a VIA CPU\n");
+		err = -ENODEV;
+		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 0x%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)


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Juerg Haefliger]

Hi Jean,

> This is a driver for the on-die digital temperature sensor of
> VIA's recent CPU models.
>
> [JD: Misc clean-ups.]
>
> Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
> Cc: Juerg Haefliger <juergh@gmail.com>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
> Harald, Juerg, all: this is the driver I intend to push to Linus for
> kernel 2.6.33. It is based on Harald's driver, to which I applied
> almost all the fixes I had suggested in my review back in June. I do
> not own a VIA-based system myself so I couldn't test it. I would
> appreciate if someone could test it and report, just to make sure I
> didn't accidentally break the driver with my clean-ups. Thanks.

I can't test it. I don't have a C7 CPU, only a C3.

...juerg



> I didn't pick Justin's modified driver because its indentation didn't
> match what the kernel wants, and I didn't want to waste my time
> re-indenting it.
>
> Still missing in this driver are:
> * Warnings that should be printed for CPU models with known errata.
> * Support for Vcore (or is it VID?) reporting.
> Both can be added on top of the current driver, using incremental
> patches.
>
>  drivers/hwmon/Kconfig       |    8
>  drivers/hwmon/Makefile      |    1
>  drivers/hwmon/via-cputemp.c |  356 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 365 insertions(+)
>  create mode 100644 drivers/hwmon/via-cputemp.c
>
> --- linux-2.6.33-rc0.orig/drivers/hwmon/Kconfig 2009-12-10 18:57:39.000000000 +0100
> +++ linux-2.6.33-rc0/drivers/hwmon/Kconfig      2009-12-10 18:57:44.000000000 +0100
> @@ -822,6 +822,14 @@ config SENSORS_TMP421
>          This driver can also be built as a module.  If so, the module
>          will be called tmp421.
>
> +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
> --- linux-2.6.33-rc0.orig/drivers/hwmon/Makefile        2009-12-10 18:57:39.000000000 +0100
> +++ linux-2.6.33-rc0/drivers/hwmon/Makefile     2009-12-10 18:57:44.000000000 +0100
> @@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc4
>  obj-$(CONFIG_SENSORS_THMC50)   += thmc50.o
>  obj-$(CONFIG_SENSORS_TMP401)   += tmp401.o
>  obj-$(CONFIG_SENSORS_TMP421)   += tmp421.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
> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.33-rc0/drivers/hwmon/via-cputemp.c        2009-12-10 19:54:27.000000000 +0100
> @@ -0,0 +1,356 @@
> +/*
> + * 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"
> +
> +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;
> +
> +       return sprintf(buf, "%lu\n", ((unsigned long)eax & 0xffffff) * 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;
> +
> +       data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL);
> +       if (!data) {
> +               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);
> +
> +       err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group);
> +       if (err)
> +               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_remove;
> +       }
> +
> +       return 0;
> +
> +exit_remove:
> +       sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
> +exit_free:
> +       platform_set_drvdata(pdev, NULL);
> +       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;
> +       struct pdev_entry *p, *n;
> +
> +       if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
> +               printk(KERN_DEBUG DRVNAME ": Not a VIA CPU\n");
> +               err = -ENODEV;
> +               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 0x%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)
>
>
> --
> Jean Delvare
>

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Harald Welte]

On Thu, Dec 10, 2009 at 08:39:04PM +0100, Jean Delvare wrote:

> almost all the fixes I had suggested in my review back in June. I do
> not own a VIA-based system myself so I couldn't test it. I would
> appreciate if someone could test it and report, just to make sure I
> didn't accidentally break the driver with my clean-ups. Thanks.

I will test it on a C7-M and Nano based system ASAP, which should be within the
next two days.
-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
======================================
VIA Free and Open Source Software Liaison

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Adam Nielsen]

> I would
> appreciate if someone could test it and report, just to make sure I
> didn't accidentally break the driver with my clean-ups. Thanks.

I've been using it for the last few hours on an EPIA M700 (1GHz C7) and it 
seems to work fine.

I still don't get any output through "sensors" though:

$ sensors
via-cputemp-isa-0000
Adapter: ISA adapter

I'm guessing because on line 118 the name is set to "via-cputemp" with a dash, 
instead of DRVNAME.

sysfs and GKrellM seem to report accurate temperatures though.

Cheers,
Adam.

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Jean Delvare]

On Sat, 12 Dec 2009 20:48:17 +1000, Adam Nielsen wrote:
> > I would
> > appreciate if someone could test it and report, just to make sure I
> > didn't accidentally break the driver with my clean-ups. Thanks.
> 
> I've been using it for the last few hours on an EPIA M700 (1GHz C7) and it 
> seems to work fine.
> 
> I still don't get any output through "sensors" though:
> 
> $ sensors
> via-cputemp-isa-0000
> Adapter: ISA adapter
> 
> I'm guessing because on line 118 the name is set to "via-cputemp" with a dash, 
> instead of DRVNAME.

Oops, thanks for reporting. I thought I had fixed it, but obviously
not. This is done now.

But that wouldn't be the reason for your problem. More likely, you're
running an old version of lm-sensors (2.x). You need 3.x.

> sysfs and GKrellM seem to report accurate temperatures though.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Andre Prendel]

On Thu, Dec 10, 2009 at 08:39:04PM +0100, Jean Delvare wrote:
> From: Harald Welte <HaraldWelte@viatech.com>
> Subject: hwmon: Add driver for VIA CPU core temperature
> 
> This is a driver for the on-die digital temperature sensor of
> VIA's recent CPU models.
> 
> [JD: Misc clean-ups.]
> 
> Signed-off-by: Harald Welte <HaraldWelte@viatech.com>
> Cc: Juerg Haefliger <juergh@gmail.com>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
> Harald, Juerg, all:

Hi Jean,

this is the driver I intend to push to Linus for
> kernel 2.6.33. It is based on Harald's driver, to which I applied
> almost all the fixes I had suggested in my review back in June. I do
> not own a VIA-based system myself so I couldn't test it. I would
> appreciate if someone could test it and report, just to make sure I
> didn't accidentally break the driver with my clean-ups. Thanks.
> 
> I didn't pick Justin's modified driver because its indentation didn't
> match what the kernel wants, and I didn't want to waste my time
> re-indenting it.
> 
> Still missing in this driver are:
> * Warnings that should be printed for CPU models with known errata.
> * Support for Vcore (or is it VID?) reporting.
> Both can be added on top of the current driver, using incremental
> patches.
> 
>  drivers/hwmon/Kconfig       |    8 
>  drivers/hwmon/Makefile      |    1 
>  drivers/hwmon/via-cputemp.c |  356 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 365 insertions(+)
>  create mode 100644 drivers/hwmon/via-cputemp.c
> 
> --- linux-2.6.33-rc0.orig/drivers/hwmon/Kconfig	2009-12-10 18:57:39.000000000 +0100
> +++ linux-2.6.33-rc0/drivers/hwmon/Kconfig	2009-12-10 18:57:44.000000000 +0100
> @@ -822,6 +822,14 @@ config SENSORS_TMP421
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tmp421.
>  
> +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

There's a typo (all are all) since Harald's first version.

> +	  of the VIA C7 and Nano.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on PCI
> --- linux-2.6.33-rc0.orig/drivers/hwmon/Makefile	2009-12-10 18:57:39.000000000 +0100
> +++ linux-2.6.33-rc0/drivers/hwmon/Makefile	2009-12-10 18:57:44.000000000 +0100
> @@ -88,6 +88,7 @@ obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc4
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
>  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.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
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.33-rc0/drivers/hwmon/via-cputemp.c	2009-12-10 19:54:27.000000000 +0100
> @@ -0,0 +1,356 @@
> +/*
> + * 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"
> +
> +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;
> +
> +	return sprintf(buf, "%lu\n", ((unsigned long)eax & 0xffffff) * 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;
> +
> +	data = kzalloc(sizeof(struct via_cputemp_data), GFP_KERNEL);
> +	if (!data) {
> +		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);
> +
> +	err = sysfs_create_group(&pdev->dev.kobj, &via_cputemp_group);
> +	if (err)
> +		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_remove;
> +	}
> +
> +	return 0;
> +
> +exit_remove:
> +	sysfs_remove_group(&pdev->dev.kobj, &via_cputemp_group);
> +exit_free:
> +	platform_set_drvdata(pdev, NULL);
> +	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;
> +	struct pdev_entry *p, *n;
> +
> +	if (cpu_data(0).x86_vendor != X86_VENDOR_CENTAUR) {
> +		printk(KERN_DEBUG DRVNAME ": Not a VIA CPU\n");
> +		err = -ENODEV;
> +		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 0x%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)
> 
> 
> -- 
> Jean Delvare

Regards,
Andre

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

Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core

[Jean Delvare]

On Tue, 15 Dec 2009 21:27:32 +0100, Andre Prendel wrote:
> On Thu, Dec 10, 2009 at 08:39:04PM +0100, Jean Delvare wrote:
> > --- linux-2.6.33-rc0.orig/drivers/hwmon/Kconfig	2009-12-10 18:57:39.000000000 +0100
> > +++ linux-2.6.33-rc0/drivers/hwmon/Kconfig	2009-12-10 18:57:44.000000000 +0100
> > @@ -822,6 +822,14 @@ config SENSORS_TMP421
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called tmp421.
> >  
> > +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
> 
> There's a typo (all are all) since Harald's first version.

Fixed, thanks.

-- 
Jean Delvare

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