AMD K8 digital sensor - take2
PATCH 1/1 AMD K8 digital sensor - take2
Hello Jean,
This patch adds support for the temperature sensor(s) found in AMD K8 CPUs.
All fixed & tested on quad CPU - dual dual core ;), please apply.
regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: k8temp2.patch
Type: text/x-patch
Size: 10581 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060822/660aa2a4/attachment.bin
PATCH 1/1 AMD K8 digital sensor - take2
Hi Rudolf,
Quoted 7 lines · Rudolf Marek
Hello Jean,
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>All fixed & tested on quad CPU - dual dual core ;), please apply.
This line has me thinking...
+ u32 temp[2][2]; /* core, place */
AMD and Intel are both coming out with quad-core processors soon. Some could be backward-compatible with existing dual-core sockets. In addition, some servers have four sockets (quad processor, or 8-core, etc.).
Maybe this array should be a little bigger? It will save you some trouble later. If not, it's okay to ignore this message...
HTH,
David
PATCH 1/1 AMD K8 digital sensor - take2
In reply to Rudolf MarekHi David,
Quoted 8 lines
This line has me thinking...
+ u32 temp[2][2]; /* core, place */AMD and Intel are both coming out with quad-core processors soon. Some could be backward-compatible with existing dual-core sockets. In addition, some servers have four sockets (quad processor, or 8-core, etc.).
Four sockets are ok. It will appear as 4 devices, so 4 instances of this driver ;)
Quoted 2 lines
Maybe this array should be a little bigger? It will save you some trouble later. If not, it's okay to ignore this message...
Well I dont know how to switch between more than two cores anyway. The driver should handle 4temps/physical CPU.
regards
Rudolf
PATCH 1/1 AMD K8 digital sensor - take2
In reply to Rudolf MarekRudolf,
Quoted 5 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>All fixed & tested on quad CPU - dual dual core ;), please apply.
Sorry I still have some comments on the code...
Quoted 1 line
+#define
TEMP_FROM_REG(val)((((val >> 16) &0xFF) - 49) * 1000)
Missing parentheses around "val".
Review
Quoted 4 lines
(quoted diff) +4
-0+#define REG_TEMP 0xe4 +#define REG_TEMP_T 0xe6 +#define PLACE 0x40 +#define CORE 0x04
The names you chose for the register names are almost similar, it's confusing. In fact it's so confusing that I thought your code below was broken. Please find better names. Either make it clear which is the temperature value register and which is the core/sensor selection register, or don't define REG_TEMP_T at all.
While you're there it would be nice to properly align all the values. And maybe find a common prefix to PLACE and CORE to make it clear they refer to the sensor selection register (SEL_PLACE and SEL_CORE, maybe?)
Quoted 14 lines
(quoted diff) +14
-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 sensorsp; /* sensor presence bits - CORE & PLACE */ + u32 temp[2][2]; /* core, place */ +}; + +static struct k8temp_data *k8temp_update_device(struct device *dev)
Review
Quoted 10 lines
(quoted diff) +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)) { + dev_dbg(&pdev->dev, "Updating k8temp data.\n");
You can use "dev" directly. BTW I really wonder what is the point of this debug message. I know we have it in many other hardware monitoring drivers, but is it really useful? We pretty well know when the data is updated.
Review
Quoted 5 lines
(quoted diff) +5
-0+ + pci_read_config_byte(pdev, REG_TEMP, &tmp); + tmp &= ~(PLACE | CORE); /* Select sensor 0, core0 */ + pci_write_config_byte(pdev, REG_TEMP, tmp); + pci_read_config_dword(pdev, REG_TEMP, &data->temp[0][0]);
I found it very confusing to see you read the sensor selection and the temperature value from the same register, until I realized there was a byte read and a dword read and you are then using some masking/shifting to extract the part you need.
Why don't you make a _word_ read from REG_TEMP_T (0xe6) instead? This would be much clearer. At any rate, defining REG_TEMP_T and using it only during the initialization step is very confusing.
Quoted 202 lines
(quoted diff) +202
-0+ + if (data->sensorsp & PLACE) { + tmp |= PLACE; /* Select sensor 1, core0 */ + pci_write_config_byte(pdev, REG_TEMP, tmp); + pci_read_config_dword(pdev, REG_TEMP, + &data->temp[0][1]); + } + + if (data->sensorsp & CORE) { + tmp &= ~PLACE; /* Select sensor 0, core1 */ + tmp |= CORE; + pci_write_config_byte(pdev, REG_TEMP, tmp); + pci_read_config_dword(pdev, REG_TEMP, + &data->temp[1][0]); + + if (data->sensorsp & PLACE) { + tmp |= PLACE; /* Select sensor 1, core1 */ + pci_write_config_byte(pdev, REG_TEMP, tmp); + pci_read_config_dword(pdev, REG_TEMP, + &data->temp[1][1]); + } + } + + data->last_updated = jiffies; + data->valid = 1; + } + + mutex_unlock(&data->update_lock); + return data; +} + +/* + * 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 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 scfg, temp; + 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; + } + + if (!(data = kzalloc(sizeof(struct k8temp_data), GFP_KERNEL))) { + err = -ENOMEM; + goto exit; + } + + pci_read_config_byte(pdev, REG_TEMP, &scfg); + scfg &= ~(PLACE | CORE); /* Select sensor 0, core0 */ + pci_write_config_byte(pdev, REG_TEMP, scfg); + pci_read_config_byte(pdev, REG_TEMP, &scfg); + + if (scfg & (PLACE | CORE)) { + dev_err(&pdev->dev, "Configuration bit(s) stuck at 1!\n"); + err = -ENODEV; + goto exit_free; + } + + scfg |= (PLACE | CORE); + pci_write_config_byte(pdev, REG_TEMP, scfg); + + /* now we know if we can change core and/or sensor */ + pci_read_config_byte(pdev, REG_TEMP, &data->sensorsp); + + if (data->sensorsp & PLACE) { + scfg &= ~CORE; /* Select sensor 1, core0 */ + pci_write_config_byte(pdev, REG_TEMP, scfg); + pci_read_config_byte(pdev, REG_TEMP_T, &temp); + scfg |= CORE; /* prepare for next selection */ + if (!temp) /* if temp is 0 -49C is not likely */ + data->sensorsp &= ~PLACE; + } + + if (data->sensorsp & CORE) { + scfg &= ~PLACE; /* Select sensor 0, core1 */ + pci_write_config_byte(pdev, REG_TEMP, scfg); + pci_read_config_byte(pdev, REG_TEMP_T, &temp); + if (!temp) /* if temp is 0 -49C is not likely */ + data->sensorsp &= ~CORE; + } + + data->name = "k8temp"; + mutex_init(&data->update_lock); + dev_set_drvdata(&pdev->dev, data); + + /* Register sysfs hooks */ + err = device_create_file(&pdev->dev, + &sensor_dev_attr_temp1_input.dev_attr); + if (err) + goto exit_remove; + + /* sensor can be changed and reports something */ + if (data->sensorsp & PLACE) { + err = device_create_file(&pdev->dev, + &sensor_dev_attr_temp2_input.dev_attr); + if (err) + goto exit_remove; + } + + /* core can be changed and reports something */ + if (data->sensorsp & CORE) { + err = device_create_file(&pdev->dev, + &sensor_dev_attr_temp3_input.dev_attr); + if (err) + goto exit_remove; + if (data->sensorsp & PLACE) + err = device_create_file(&pdev->dev, + &sensor_dev_attr_temp4_input. + dev_attr); + if (err) + goto exit_remove; + } + + err = device_create_file(&pdev->dev, &dev_attr_name); + if (err) + goto exit_remove; + + data->class_dev = hwmon_device_register(&pdev->dev); + + if (IS_ERR(data->class_dev)) { + err = PTR_ERR(data->class_dev); + goto exit_remove; + } + + return 0; + +exit_remove: + 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); +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); +} +
Review
Quoted 10 lines
(quoted diff) +10
-0+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);
Deprecated, please use pci_register_driver instead. I was going to fix it myself, but since we'll need one more iteration anyway...
Quoted 14 lines
(quoted diff) +13
-0+} + +static void __exit k8temp_exit(void) +{ + pci_unregister_driver(&k8temp_driver); +} + +MODULE_AUTHOR("Rudolf Marek <r.marek at sh.cvut.cz>"); +MODULE_DESCRIPTION("AMD K8 core temperature monitor"); +MODULE_LICENSE("GPL"); + +module_init(k8temp_init) +module_exit(k8temp_exit)
Signature · 1 line
Jean Delvare