AMD K8 digital sensor
PATCH 1/1 AMD K8 digital sensor
Hello all,
This patch adds support for the temperature sensor(s) found in AMD K8 CPUs.
Info for Jean:
This patch has only one bigger change compared to older version - the access width to register is now whole 32bits. This is neccessary for future enhancements because addtional reserved bits might be used for temperature, gaining more precision. Trust me ;)
The dependency is set to X86 because of usage of cpuid funcs. No need to check for specific procesor because it is in fact PCI driver. The latest version works on dual core - dual procesor AMD server and it works fine on my single core Opteron too.
I'm leaving for next week and something -> on thursday. I would like to get this patch into 2.6.19, so if you have some minor comments please fix the patch as you like.
And info for users:
The userspace patch is still in here
http://assembler.cz/download/amd_digital_temp.tar.gz
the patch itself is for 2.6.18rc-2 but should work on any post 2.6.15 kernel. If you need standalone version, use that one in the archive.
I will check into the SVN the userspace code once Jean or Mark says it is OK ;)
Thanks,
Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: k8temp.patch
Type: text/x-patch
Size: 9921 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060726/bfb27574/attachment.bin
PATCH 1/1 AMD K8 digital sensor
Quoted 8 lines · Rudolf Marek
Hello all,
And info for users:
The userspace patch is still in here
http://assembler.cz/download/amd_digital_temp.tar.gz
the patch itself is for 2.6.18rc-2 but should work on any post 2.6.15 kernel. If you need standalone version, use that one in the archive.
I was thinking about doing things differently with regards to userspace. As I already posted to the list I'm working on a driver for the uGuru2. The driver is done (waiting for feedback from testers) but I still need todo userspace. Since the uGuru2 driver is going to be 2.6 only and since 2.6 has a clear API/ABI between userspace and the kernel for hwmon chips I was thinking about adding code to libsensors for generic 2.6 support. The idea is to write a piece of code which will come into action only if libsensors doesn't have a chip definition for a found chip and libsensors is running on a 2.6 kernel. This piece of code would then fill a dynamicly allocated structure describing the unknown chip, using the same structure as for known chips.
Does this sound like a plan? With this in place we no longer need to write support for every new 2.6 hwmon driver, actually if this goes in I would like to see explicit support for the uGuru (1) be removed, since this generic code should do just as good of a job.
Does this sound like a plan (for 2.6 only drivers) ? Or are you planning on porting this driver to 2.4?
Regards,
Hans
PATCH 1/1 AMD K8 digital sensor
In reply to Rudolf MarekHi Hans,
Quoted 11 lines
I was thinking about doing things differently with regards to userspace. As I already posted to the list I'm working on a driver for the uGuru2. The driver is done (waiting for feedback from testers) but I still need todo userspace. Since the uGuru2 driver is going to be 2.6 only and since 2.6 has a clear API/ABI between userspace and the kernel for hwmon chips I was thinking about adding code to libsensors for generic 2.6 support. The idea is to write a piece of code which will come into action only if libsensors doesn't have a chip definition for a found chip and libsensors is running on a 2.6 kernel. This piece of code would then fill a dynamicly allocated structure describing the unknown chip, using the same structure as for known chips.
Yes this is in long term plan.
Quoted 4 lines
Does this sound like a plan With this in place we no longer need to write support for every new 2.6 hwmon driver, actually if this goes in I would like to see explicit support for the uGuru (1) be removed, since this generic code should do just as good of a job.
Yes.
There are still questions if to rewrite the libsensors and allow LGPL or change libsensors to what you propose.
Quoted 2 lines
Does this sound like a plan (for 2.6 only drivers) ? Or are you planning on porting this driver to 2.4?
No plan for 2.4.
I'm leaving now for two weeks so please be patient with further answers from me.
Regards
Rudolf
PATCH 1/1 AMD K8 digital sensor
In reply to Rudolf MarekHi Rudolf,
Quoted 10 lines
This patch adds support for the temperature sensor(s) found in AMD K8 CPUs.
Signed-off-by Rudolf Marek <r.marek at sh.cvut.cz>Info for Jean:
This patch has only one bigger change compared to older version - the access width to register is now whole 32bits. This is neccessary for future enhancements because addtional reserved bits might be used for temperature, gaining more precision. Trust me ;)
Your code would need further changes to support that anyway. I'm fine with 32-bit reads anyway.
Quoted 8 lines
The dependency is set to X86 because of usage of cpuid funcs. No need to check for specific procesor because it is in fact PCI driver. The latest version works on dual core - dual procesor AMD server and it works fine on my single core Opteron too.
I'm leaving for next week and something -> on thursday. I would like to get this patch into 2.6.19, so if you have some minor comments please fix the patch as you like.
Sorry for the late answer, here comes my review.
drivers/hwmon/k8temp.c line 3
Quoted 7 lines
diff -uprN
linux-2.6.18-rc2/drivers/hwmon/k8temp.clinux-2.6.18-rc2-patched/drivers/hwmon/k8temp.cdrivers/hwmon/k8temp.c +3
-0@@ -0,0 +1,278 @@ +/* +* k8temp.c - Part of lm_sensors, Linux kernel modules for hardware +* monitoring
No, this is part of the Linux kernel.
drivers/hwmon/k8temp.c line 6
Quoted 3 lines
drivers/hwmon/k8temp.c +3
-0+* Copyright (C) 2006 Rudolf Marek <r.marek at sh.cvut.cz> +* +* Inspired from the w83785 and amd756 driver.
Typo: drivers.
Quoted 25 lines
drivers/hwmon/k8temp.c +25
-0+* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 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/pci.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h>
drivers/hwmon/k8temp.c line 41
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+#include <linux/err.h> +#include <linux/mutex.h> + +#define TEMP_FROM_REG (val) ((((val >> 16) & 0xFF) - 49) * 1000) + +/* +* Functions declaration +*/ + +static struct k8temp_data *k8temp_update_device (struct device *dev);
You can easily reorder the functions not to need this forward declaration.
drivers/hwmon/k8temp.c line 51
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+ +struct k8temp_data { +struct class_device *class_dev; +struct mutex update_lock; +const char *name; +char valid;/* zero until following fields are valid */ +unsigned long last_updated;/* in jiffies */ + +/* registers values */ +u8 sensors;/* bit6 place can be changed, bit2 core */
I don't understand this comment.
drivers/hwmon/k8temp.c line 55
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+u32 temp[2 ][2 ];/* core, place */ +}; + +static struct k8temp_data *k8temp_update_device (struct device *dev);
Duplicate forward declaration.
Quoted 35 lines
drivers/hwmon/k8temp.c +35
-0+ +/* +* Sysfs stuff +*/ + +static ssize_t show_name (struct device *dev,struct device_attribute + *devattr,char *buf) +{ +struct k8temp_data *data =dev_get_drvdata (dev); + +return sprintf (buf,"%s\n" , data->name); +} + + +static ssize_t show_temp (struct device *dev, +struct device_attribute *devattr,char *buf) +{ + struct sensor_device_attribute_2 *attr > + to_sensor_dev_attr_2(devattr); +int core = attr->nr; +int place = attr->index; +struct k8temp_data *data =k8temp_update_device (dev); + +return sprintf (buf,"%d\n" , +TEMP_FROM_REG (data->temp[core][place])); +} + +/* core, place */ + +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp, NULL, 0, 0); +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, 0, 1); +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, 1, 0); +static SENSOR_DEVICE_ATTR_2(temp4_input, S_IRUGO, show_temp, NULL, 1, 1); +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); + +static struct k8temp_data *k8temp_update_device (struct device *dev)
drivers/hwmon/k8temp.c line 100
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+{ +struct k8temp_data *data =dev_get_drvdata (dev); +struct pci_dev *pdev =to_pci_dev (dev); +u8 tmp; + +mutex_lock (&data->update_lock); + +if (!data->valid + ||time_after (jiffies, data->last_updated +HZ *2 )) { +
No blank line here.
What's the rationale for limiting the updates to every other second? For such a simple driver, and PCI accesses, which I guess are fast, I would think every second is OK.
Quoted 18 lines
drivers/hwmon/k8temp.c +18
-0+dev_dbg (&pdev->dev,"Updating k8temp data.\n" ); + +pci_read_config_byte (pdev,0xe4 , &tmp); + tmp &= ~0x44 ;/* Select sensor 0, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , &data->temp[0 ][0 ]); + +if (data->sensors &0x40 ) { + tmp |=0x40 ;/* Select sensor 1, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , + &data->temp[0 ][1 ]); + } + +if (data->sensors &0x4 ) { + tmp &= ~0x40 ;/* Select sensor 0, core1 */ + tmp |=0x4 ; +pci_write_config_byte (pdev,0xe4 , tmp);
drivers/hwmon/k8temp.c line 128
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+pci_read_config_dword (pdev,0xe4 , + &data->temp[1 ][0 ]); + +if (data->sensors &0x40 ) { + tmp |=0x40 ;/* Select sensor 1, core1 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , + &data->temp[1 ][1 ]); + } + }
Maybe you could have defines for registers and bits, to make the code a bit clearer.
Quoted 9 lines
drivers/hwmon/k8temp.c +9
-0+ + data->last_updated = jiffies; + data->valid =1 ; + } + +mutex_unlock (&data->update_lock); +return data; +} +
drivers/hwmon/k8temp.c line 147
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+static struct pci_device_id k8temp_ids[] = { + {PCI_DEVICE (PCI_VENDOR_ID_AMD ,PCI_DEVICE_ID_AMD_K8_NB_MISC ) }, + {0 }, +}; + +static int __devinit k8temp_probe (struct pci_dev *pdev, +const struct pci_device_id *id) +{ +int err; +u8 tmp, temp;
"tmp" is a poor name a too similar to "temp" so it's confusing. Please find a better name.
drivers/hwmon/k8temp.c line 155
Quoted 8 lines
drivers/hwmon/k8temp.c +8
-0+struct k8temp_data *data; +u32 cpuid =cpuid_eax (1 ); + +/* this feature should be available since SH-C0 core */ +if ((cpuid =0xf40 ) || (cpuid =0xf50 ) || (cpuid =0xf51 )) { + err = -ENODEV ; + goto exit; + }
Isn't this test a bit weak? Are you sure these are the only unsupported K8 out there? What about (cpuid <= 0xf51) instead? Or even better (cpuid < {first supported})?
Quoted 24 lines
drivers/hwmon/k8temp.c +24
-0+ +if (!(data =kzalloc (sizeof (struct k8temp_data ),GFP_KERNEL ))) { + err = -ENOMEM ; + goto exit; + } + +pci_read_config_byte (pdev,0xe4 , &tmp); + tmp &= ~0x44 ;/* Select sensor 0, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe4 , &tmp); + +if (tmp &0x44 ) { +dev_err (&pdev->dev,"Configuration bits stuck at 1!\n" ); + err = -ENODEV ; + goto exit_free; + } + + tmp |=0x44 ; +pci_write_config_byte (pdev,0xe4 , tmp); + +/* now we know if we can change core and/or sensor */ +pci_read_config_byte (pdev,0xe4 , &data->sensors); + +if (data->sensors &0x40 ) {
drivers/hwmon/k8temp.c line 189
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+ tmp &= ~0x4 ;/* Select sensor 1, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe6 , &temp); +if (!temp)/* if temp is 0 -49C is not likely */ + data->sensors &= ~0x40 ; + } + +if (data->sensors &0x4 ) { + tmp &= ~0x40 ;/* Select sensor 0, core1 */ + tmp |=0x4 ;
This one belongs to the previous block.
drivers/hwmon/k8temp.c line 202
Quoted 13 lines
drivers/hwmon/k8temp.c +13
-0+pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe6 , &temp); +if (!temp)/* if temp is 0 -49C is not likely */ + data->sensors &= ~0x4 ; + } + + data->name ="k8temp" ; +mutex_init (&data->update_lock); +dev_set_drvdata (&pdev->dev, data); + +/* Register sysfs hooks */ + data->class_dev =hwmon_device_register (&pdev->dev); +
No blank line here.
Quoted 15 lines
drivers/hwmon/k8temp.c +15
-0+if (IS_ERR (data->class_dev)) { + err =PTR_ERR (data->class_dev); + goto exit_free; + } + +device_create_file (&pdev->dev, + &sensor_dev_attr_temp1_input.dev_attr); + +/* sensor can be changed and reports something */ +if (data->sensors &0x40 ) +device_create_file (&pdev->dev, + &sensor_dev_attr_temp2_input.dev_attr); + +/* core can be changed and reports something */ +if (data->sensors &0x4 ) {
drivers/hwmon/k8temp.c line 227
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+device_create_file (&pdev->dev, + &sensor_dev_attr_temp3_input.dev_attr); + +if (data->sensors &0x40 ) +device_create_file (&pdev->dev, + &sensor_dev_attr_temp4_input. + dev_attr); + } + +device_create_file (&pdev->dev, &dev_attr_name);
According to the new laws you must check the return value of device_create_file. Do not forget to remove the already created files on error.
You should also create the files first, before registering with the hwmon class.
Quoted 36 lines
drivers/hwmon/k8temp.c +36
-0+return 0 ; + +exit_free: +dev_set_drvdata (&pdev->dev,NULL ); +kfree (data); +exit: +return err; +} + +static void __devexit k8temp_remove(struct pci_dev *pdev) +{ +struct k8temp_data *data =dev_get_drvdata (&pdev->dev); + +hwmon_device_unregister (data->class_dev); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp1_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp2_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp3_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp4_input.dev_attr); +device_remove_file (&pdev->dev, &dev_attr_name); +dev_set_drvdata (&pdev->dev,NULL ); +kfree (data); +} + +static struct pci_driver k8temp_driver = { + .name ="k8temp" , + .id_table = k8temp_ids, + .probe = k8temp_probe, + .remove =__devexit_p (k8temp_remove), +}; + +static int __init k8temp_init (void ) +{
drivers/hwmon/k8temp.c line 273
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+return pci_module_init (&k8temp_driver); +} + +static void __exit k8temp_exit(void) +{ +pci_unregister_driver (&k8temp_driver); +} + +MODULE_AUTHOR ("Rudolf Marek <r.marek at sh.cvut.cz>" ); +MODULE_DESCRIPTION ("k8 core temperature monitor" );
AMD K8.
Quoted 8 lines
drivers/hwmon/k8temp.c +4
-0+MODULE_LICENSE ("GPL" ); + +module_init (k8temp_init) +module_exit (k8temp_exit)diff -uprN linux-2.6.18-rc2/drivers/hwmon/Kconfig linux-2.6.18-rc2-patched/drivers/hwmon/Kconfig
drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 99
Quoted 6 lines
drivers/hwmon/Kconfig +3
-0@@ -94,6 +94,16 @@ config SENSORS_ADM9240 This driver can also be built as a module. If so, the module will be called adm9240. +config SENSORS_K8TEMP + tristate "AMD K8 processor sensor" + depends on HWMON && X86 && EXPERIMENTAL
Depends on PCI as well.
drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 106
Quoted 7 lines
drivers/hwmon/Kconfig +7
-0+ help + If you say yes here you get support for the temperature + sensor(s) inside your AMD K8 CPU. + + This driver can also be built as a module. If so, the module + will be called k8temp. +
Looks quite good, and it works very well for me too (single core, single sensor). Please fix the remaining issues and we can merge that driver.
Can we have a documentation file for that driver?
Thanks,
Signature · 1 line
Jean Delvare
PATCH 1/1 AMD K8 digital sensor
In reply to Rudolf MarekQuoted 3 lines
Your code would need further changes to support that anyway. I'm fine with 32-bit reads anyway.
Yep But sometimes you dont like such things ;)
Quoted 10 lines
Quoted 8 lines
The dependency is set to X86 because of usage of cpuid funcs. No need to check for specific procesor because it is in fact PCI driver. The latest version works on dual core - dual procesor AMD server and it works fine on my single core Opteron too.
I'm leaving for next week and something -> on thursday. I would like to get this patch into 2.6.19, so if you have some minor comments please fix the patch as you like.
Sorry for the late answer, here comes my review.
Better now then never ;)
Quoted 10 lines
drivers/hwmon/k8temp.c line 3
Quoted 8 lines
diff -uprN
linux-2.6.18-rc2/drivers/hwmon/k8temp.clinux-2.6.18-rc2-patched/drivers/hwmon/k8temp.cdrivers/hwmon/k8temp.c +3
-0@@ -0,0 +1,278 @@ +/* +* k8temp.c - Part of lm_sensors, Linux kernel modules for hardware +* monitoring No, this is part of the Linux kernel.
Well this was cut & paste maybe we should change it?
Quoted 5 lines
drivers/hwmon/k8temp.c line 6
Quoted 3 lines
drivers/hwmon/k8temp.c +3
-0+* Copyright (C) 2006 Rudolf Marek <r.marek at sh.cvut.cz> +* +* Inspired from the w83785 and amd756 driver. Typo: drivers.
Ok
Quoted 10 lines
Quoted 10 lines
drivers/hwmon/k8temp.c +9
-0+* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Quoted 8 lines
drivers/hwmon/k8temp.c line 23
Quoted 8 lines
drivers/hwmon/k8temp.c +8
-0+* 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. +*/ +
Please note that this is actually new FSF address. Which I got correctly ;)
Quoted 12 lines
Quoted 12 lines
drivers/hwmon/k8temp.c +12
-0+#include <linux/module.h> +#include <linux/delay.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/pci.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> + +#define TEMP_FROM_REG (val) ((((val >> 16) & 0xFF) - 49) * 1000)
Quoted 9 lines
drivers/hwmon/k8temp.c line 41
Quoted 6 lines
drivers/hwmon/k8temp.c +6
-0+ +/* +* Functions declaration +*/ + +static struct k8temp_data *k8temp_update_device (struct device *dev);You can easily reorder the functions not to need this forward declaration.
Hm ok I will look into it.
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+ +struct k8temp_data { +struct class_device *class_dev; +struct mutex update_lock;
Quoted 8 lines
drivers/hwmon/k8temp.c line 51
Quoted 6 lines
drivers/hwmon/k8temp.c +6
-0+const char *name; +char valid;/* zero until following fields are valid */ +unsigned long last_updated;/* in jiffies */ + +/* registers values */ +u8 sensors;/* bit6 place can be changed, bit2 core */ I don't understand this comment.
This is an autodetection variable that detects if the bit for different core may be change or to the diffent sensor in the core
Quoted 7 lines
drivers/hwmon/k8temp.c line 55
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+u32 temp[2 ][2 ];/* core, place */ +}; + +static struct k8temp_data *k8temp_update_device (struct device *dev);Duplicate forward declaration.
Ooop this is really a bug ;)
Quoted 48 lines
Quoted 35 lines
drivers/hwmon/k8temp.c +35
-0+ +/* +* Sysfs stuff +*/ + +static ssize_t show_name (struct device *dev,struct device_attribute + *devattr,char *buf) +{ +struct k8temp_data *data =dev_get_drvdata (dev); + +return sprintf (buf,"%s\n" , data->name); +} + + +static ssize_t show_temp (struct device *dev, +struct device_attribute *devattr,char *buf) +{ + struct sensor_device_attribute_2 *attr >> + to_sensor_dev_attr_2(devattr); +int core = attr->nr; +int place = attr->index; +struct k8temp_data *data =k8temp_update_device (dev); + +return sprintf (buf,"%d\n" , +TEMP_FROM_REG (data->temp[core][place])); +} + +/* core, place */ + +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp, NULL, 0, 0); +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, 0, 1); +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, 1, 0); +static SENSOR_DEVICE_ATTR_2(temp4_input, S_IRUGO, show_temp, NULL, 1, 1); +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); + +static struct k8temp_data *k8temp_update_device (struct device *dev)drivers/hwmon/k8temp.c line 100
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+{ +struct k8temp_data *data =dev_get_drvdata (dev); +struct pci_dev *pdev =to_pci_dev (dev); +u8 tmp; + +mutex_lock (&data->update_lock); + +if (!data->valid + ||time_after (jiffies, data->last_updated +HZ *2 )) { +No blank line here.
Quoted 3 lines
What's the rationale for limiting the updates to every other second? For such a simple driver, and PCI accesses, which I guess are fast, I would think every second is OK.
I think I left it for 2 seconds because others do. I will change it to 1 sec.
Quoted 24 lines
Quoted 24 lines
drivers/hwmon/k8temp.c +23
-0+dev_dbg (&pdev->dev,"Updating k8temp data.\n" ); + +pci_read_config_byte (pdev,0xe4 , &tmp); + tmp &= ~0x44 ;/* Select sensor 0, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , &data->temp[0 ][0 ]); + +if (data->sensors &0x40 ) { + tmp |=0x40 ;/* Select sensor 1, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , + &data->temp[0 ][1 ]); + } + +if (data->sensors &0x4 ) { + tmp &= ~0x40 ;/* Select sensor 0, core1 */ + tmp |=0x4 ; +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , + &data->temp[1 ][0 ]); + +if (data->sensors &0x40 ) { + tmp |=0x40 ;/* Select sensor 1, core1 */
Quoted 8 lines
drivers/hwmon/k8temp.c line 128
Quoted 5 lines
drivers/hwmon/k8temp.c +5
-0+pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_dword (pdev,0xe4 , + &data->temp[1 ][1 ]); + } + }Maybe you could have defines for registers and bits, to make the code a bit clearer.
Well there are only 0xe4 0x40 and 0x4 I will see if it looks better with symbolic names.
Quoted 15 lines
Quoted 15 lines
drivers/hwmon/k8temp.c +14
-0+ + data->last_updated = jiffies; + data->valid =1 ; + } + +mutex_unlock (&data->update_lock); +return data; +} + +static struct pci_device_id k8temp_ids[] = { + {PCI_DEVICE (PCI_VENDOR_ID_AMD ,PCI_DEVICE_ID_AMD_K8_NB_MISC ) }, + {0 }, +}; +
Quoted 8 lines
drivers/hwmon/k8temp.c line 147
Quoted 5 lines
drivers/hwmon/k8temp.c +5
-0+static int __devinit k8temp_probe (struct pci_dev *pdev, +const struct pci_device_id *id) +{ +int err; +u8 tmp, temp;"tmp" is a poor name a too similar to "temp" so it's confusing. Please find a better name.
OK
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+struct k8temp_data *data; +u32 cpuid =cpuid_eax (1 ); + +/* this feature should be available since SH-C0 core */
Quoted 8 lines
drivers/hwmon/k8temp.c line 155
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+if ((cpuid =0xf40 ) || (cpuid =0xf50 ) || (cpuid =0xf51 )) { + err = -ENODEV ; + goto exit; + }Isn't this test a bit weak? Are you sure these are the only unsupported K8 out there? What about (cpuid <=
0xf51) instead? Or even better (cpuid < {first supported})?
Hmm revisions guides are not easy to get. It is generally belived that this CPUID ids do not have it.
Quoted 28 lines
Quoted 28 lines
drivers/hwmon/k8temp.c +28
-0+ +if (!(data =kzalloc (sizeof (struct k8temp_data ),GFP_KERNEL ))) { + err = -ENOMEM ; + goto exit; + } + +pci_read_config_byte (pdev,0xe4 , &tmp); + tmp &= ~0x44 ;/* Select sensor 0, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe4 , &tmp); + +if (tmp &0x44 ) { +dev_err (&pdev->dev,"Configuration bits stuck at 1!\n" ); + err = -ENODEV ; + goto exit_free; + } + + tmp |=0x44 ; +pci_write_config_byte (pdev,0xe4 , tmp); + +/* now we know if we can change core and/or sensor */ +pci_read_config_byte (pdev,0xe4 , &data->sensors); + +if (data->sensors &0x40 ) { + tmp &= ~0x4 ;/* Select sensor 1, core0 */ +pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe6 , &temp); +if (!temp)/* if temp is 0 -49C is not likely */
Quoted 8 lines
drivers/hwmon/k8temp.c line 189
Quoted 6 lines
drivers/hwmon/k8temp.c +6
-0+ data->sensors &= ~0x40 ; + } + +if (data->sensors &0x4 ) { + tmp &= ~0x40 ;/* Select sensor 0, core1 */ + tmp |=0x4 ;This one belongs to the previous block.
Hm what? Sorry I have parse error.
Quoted 17 lines
Quoted 4 lines
drivers/hwmon/k8temp.c +3
-0+pci_write_config_byte (pdev,0xe4 , tmp); +pci_read_config_byte (pdev,0xe6 , &temp); +if (!temp)/* if temp is 0 -49C is not likely */ drivers/hwmon/k8temp.c line 202
Quoted 10 lines
drivers/hwmon/k8temp.c +10
-0+ data->sensors &= ~0x4 ; + } + + data->name ="k8temp" ; +mutex_init (&data->update_lock); +dev_set_drvdata (&pdev->dev, data); + +/* Register sysfs hooks */ + data->class_dev =hwmon_device_register (&pdev->dev); +No blank line here.
ok
Quoted 21 lines
Quoted 21 lines
drivers/hwmon/k8temp.c +21
-0+if (IS_ERR (data->class_dev)) { + err =PTR_ERR (data->class_dev); + goto exit_free; + } + +device_create_file (&pdev->dev, + &sensor_dev_attr_temp1_input.dev_attr); + +/* sensor can be changed and reports something */ +if (data->sensors &0x40 ) +device_create_file (&pdev->dev, + &sensor_dev_attr_temp2_input.dev_attr); + +/* core can be changed and reports something */ + if (data->sensors & 0x4) { + device_create_file(&pdev->dev, + &sensor_dev_attr_temp3_input.dev_attr); + + if (data->sensors & 0x40) + device_create_file(&pdev->dev, + &sensor_dev_attr_temp4_input.
Quoted 8 lines
drivers/hwmon/k8temp.c line 227
Quoted 4 lines
drivers/hwmon/k8temp.c +4
-0+ dev_attr); + } + +device_create_file (&pdev->dev, &dev_attr_name);According to the new laws you must check the return value of
device_create_file. Do not forget to remove the already created files on error.
ok
Quoted 2 lines
You should also create the files first, before registering with the hwmon class.
ok
Quoted 40 lines
Quoted 40 lines
drivers/hwmon/k8temp.c +40
-0+return 0 ; + +exit_free: +dev_set_drvdata (&pdev->dev,NULL ); +kfree (data); +exit: +return err; +} + +static void __devexit k8temp_remove(struct pci_dev *pdev) +{ +struct k8temp_data *data =dev_get_drvdata (&pdev->dev); + +hwmon_device_unregister (data->class_dev); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp1_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp2_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp3_input.dev_attr); +device_remove_file (&pdev->dev, + &sensor_dev_attr_temp4_input.dev_attr); +device_remove_file (&pdev->dev, &dev_attr_name); +dev_set_drvdata (&pdev->dev,NULL ); +kfree (data); +} + +static struct pci_driver k8temp_driver = { + .name ="k8temp" , + .id_table = k8temp_ids, + .probe = k8temp_probe, + .remove =__devexit_p (k8temp_remove), +}; + +static int __init k8temp_init (void ) +{ +return pci_module_init (&k8temp_driver); +} + +static void __exit k8temp_exit(void)
Quoted 9 lines
drivers/hwmon/k8temp.c line 273
Quoted 6 lines
drivers/hwmon/k8temp.c +6
-0+{ +pci_unregister_driver (&k8temp_driver); +} + +MODULE_AUTHOR ("Rudolf Marek <r.marek at sh.cvut.cz>" ); +MODULE_DESCRIPTION ("k8 core temperature monitor" );AMD K8.
Good catch
Quoted 11 lines
Quoted 11 lines
drivers/hwmon/k8temp.c +4
-0+MODULE_LICENSE ("GPL" ); + +module_init (k8temp_init) +module_exit (k8temp_exit)diff -uprN linux-2.6.18-rc2/drivers/hwmon/Kconfig linux-2.6.18-rc2-patched/drivers/hwmon/Kconfig
drivers/hwmon/Kconfig +0
-0@@ -94,6 +94,16 @@ config SENSORS_ADM9240 This driver can also be built as a module. If so, the module will be called adm9240.
Quoted 6 lines
drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 99
Quoted 3 lines
drivers/hwmon/Kconfig +3
-0+config SENSORS_K8TEMP + tristate "AMD K8 processor sensor" + depends on HWMON && X86 && EXPERIMENTALDepends on PCI as well.
Yes true! How did I forget?
Quoted 12 lines
drivers/hwmon/Kconfig config SENSORS_ADM9240 · line 106
Quoted 7 lines
drivers/hwmon/Kconfig +7
-0+ help + If you say yes here you get support for the temperature + sensor(s) inside your AMD K8 CPU. + + This driver can also be built as a module. If so, the module + will be called k8temp. +Looks quite good, and it works very well for me too (single core, single sensor). Please fix the remaining issues and we can merge that driver.
Quoted 1 line
Can we have a documentation file for that driver?
Yes If I find the time ;) But I think so.
Quoted 1 line
Thanks,
Many thanks too. It is funny that even if I tried hard not to screw stuff in the driver there is always something.
Regards
Rudolf
PATCH 1/1 AMD K8 digital sensor
In reply to Rudolf MarekQuoted 11 lines
Quoted 9 lines
drivers/hwmon/k8temp.c line 3
Quoted 7 lines
diff -uprN
linux-2.6.18-rc2/drivers/hwmon/k8temp.clinux-2.6.18-rc2-patched/drivers/hwmon/k8temp.cdrivers/hwmon/k8temp.c +3
-0@@ -0,0 +1,278 @@ +/* +* k8temp.c - Part of lm_sensors, Linux kernel modules for hardware +* monitoring No, this is part of the Linux kernel.
Well this was cut & paste maybe we should change it?
This makes some sense for drivers which were ported from the lm_sensors project, so I don't mind leaving it for these. But we shouldn't include that sentence in new drivers.
Quoted 2 lines
Many thanks too. It is funny that even if I tried hard not to screw stuff in the driver there is always something.
Heh, no worry. That's the whole point of reviewing patches, and also the reason why I would like my own patches to get some review too.
Thanks,
Signature · 1 line
Jean Delvare