Messages in this thread | | | Date | Thu, 13 Aug 2009 11:42:17 -0700 | Subject | Re: [lm-sensors] [PATCH] hwmon: Add driver for VIA CPU core temperature | From | 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 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |