This is a driver for the on-die digital temperature sensor of
VIA's recent CPU models.
Signed-off-by: Harald Welte <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("VIA CPU temperature monitor");
+MODULE_LICENSE("GPL");
+
+module_init(via_cputemp_init)
+module_exit(via_cputemp_exit)
--
1.6.3.1
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
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)
On Wed, 10 Jun 2009 17:40:31 +0000 (UTC)
Tomaz Mertelj <[email protected]> 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.
On Thu, 11 Jun 2009 15:32:55 -0700, Andrew Morton wrote:
> On Wed, 10 Jun 2009 17:40:31 +0000 (UTC)
> Tomaz Mertelj <[email protected]> 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
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 <[email protected]> http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison
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 <[email protected]> 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
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 <[email protected]> http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison
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
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
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
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
>
>
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> + *
>> + * 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 <[email protected]>");
>> +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
>