* [lm-sensors] [PATCH] AMD K8 digital sensor
2006-07-25 22:08 [lm-sensors] [PATCH] AMD K8 digital sensor Rudolf Marek
@ 2006-07-26 14:36 ` Hans de Goede
2006-07-26 14:44 ` Rudolf Marek
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2006-07-26 14:36 UTC (permalink / raw)
To: lm-sensors
Rudolf Marek wrote:
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] AMD K8 digital sensor
2006-07-25 22:08 [lm-sensors] [PATCH] AMD K8 digital sensor Rudolf Marek
2006-07-26 14:36 ` Hans de Goede
@ 2006-07-26 14:44 ` Rudolf Marek
2006-08-22 8:44 ` Jean Delvare
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Rudolf Marek @ 2006-07-26 14:44 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
> 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.
> 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.
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] AMD K8 digital sensor
2006-07-25 22:08 [lm-sensors] [PATCH] AMD K8 digital sensor Rudolf Marek
2006-07-26 14:36 ` Hans de Goede
2006-07-26 14:44 ` Rudolf Marek
@ 2006-08-22 8:44 ` Jean Delvare
2006-08-22 18:23 ` Rudolf Marek
2006-08-23 7:10 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2006-08-22 8:44 UTC (permalink / raw)
To: lm-sensors
Hi Rudolf,
> 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.
> 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.
> diff -uprN linux-2.6.18-rc2/drivers/hwmon/k8temp.c linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c
> --- linux-2.6.18-rc2/drivers/hwmon/k8temp.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c 2006-07-25 23:45:00.274085500 +0200
> @@ -0,0 +1,278 @@
> +/*
> + * k8temp.c - Part of lm_sensors, Linux kernel modules for hardware
> + * monitoring
No, this is part of the Linux kernel.
> + * Copyright (C) 2006 Rudolf Marek <r.marek at sh.cvut.cz>
> + *
> + * Inspired from the w83785 and amd756 driver.
Typo: drivers.
> + *
> + * 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>
> +#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.
> +
> +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.
> + u32 temp[2][2]; /* core, place */
> +};
> +
> +static struct k8temp_data *k8temp_update_device(struct device *dev);
Duplicate forward declaration.
> +
> +/*
> + * 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)
> +{
> + 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.
> + 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 */
> + 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.
> +
> + 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 },
> +};
> +
> +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.
> + 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})?
> +
> + 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 */
> + data->sensors &= ~0x40;
> + }
> +
> + if (data->sensors & 0x4) {
> + tmp &= ~0x40; /* Select sensor 0, core1 */
> + tmp |= 0x4;
This one belongs to the previous block.
> + 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.
> + 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.
> + 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.
> + 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)
> +{
> + pci_unregister_driver(&k8temp_driver);
> +}
> +
> +MODULE_AUTHOR("Rudolf Marek <r.marek at sh.cvut.cz>");
> +MODULE_DESCRIPTION("k8 core temperature monitor");
AMD K8.
> +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
> --- linux-2.6.18-rc2/drivers/hwmon/Kconfig 2006-07-15 23:53:08.000000000 +0200
> +++ linux-2.6.18-rc2-patched/drivers/hwmon/Kconfig 2006-07-25 23:18:59.008512500 +0200
> @@ -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.
> + 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,
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] AMD K8 digital sensor
2006-07-25 22:08 [lm-sensors] [PATCH] AMD K8 digital sensor Rudolf Marek
` (2 preceding siblings ...)
2006-08-22 8:44 ` Jean Delvare
@ 2006-08-22 18:23 ` Rudolf Marek
2006-08-23 7:10 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Rudolf Marek @ 2006-08-22 18:23 UTC (permalink / raw)
To: lm-sensors
>
> 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 ;)
>> 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 ;)
>
>> diff -uprN linux-2.6.18-rc2/drivers/hwmon/k8temp.c linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c
>> --- linux-2.6.18-rc2/drivers/hwmon/k8temp.c 1970-01-01 01:00:00.000000000 +0100
>> +++ linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c 2006-07-25 23:45:00.274085500 +0200
>> @@ -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?
>> + * Copyright (C) 2006 Rudolf Marek <r.marek at sh.cvut.cz>
>> + *
>> + * Inspired from the w83785 and amd756 driver.
>
> Typo: drivers.
Ok
>
>> + *
>> + * 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.
>> + */
>> +
Please note that this is actually new FSF address. Which I got correctly ;)
>> +#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)
>> +
>> +/*
>> + * 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.
>> +
>> +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.
This is an autodetection variable that detects if the bit for different core may
be change or to the diffent sensor in the core
>> + 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 ;)
>> +
>> +/*
>> + * 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)
>> +{
>> + 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.
I think I left it for 2 seconds because others do. I will change it to 1 sec.
>
>> + 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 */
>> + 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.
>
>> +
>> + 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 },
>> +};
>> +
>> +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
>> + 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})?
Hmm revisions guides are not easy to get. It is generally belived that this
CPUID ids do not have it.
>> +
>> + 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 */
>> + 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.
>
>> + 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.
>
ok
>> + 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.
>> + 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
> You should also create the files first, before registering with the
> hwmon class.
ok
>> + 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)
>> +{
>> + 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
>> +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
>> --- linux-2.6.18-rc2/drivers/hwmon/Kconfig 2006-07-15 23:53:08.000000000 +0200
>> +++ linux-2.6.18-rc2-patched/drivers/hwmon/Kconfig 2006-07-25 23:18:59.008512500 +0200
>> @@ -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.
>
Yes true! How did I forget?
>> + 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?
Yes If I find the time ;) But I think so.
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [lm-sensors] [PATCH] AMD K8 digital sensor
2006-07-25 22:08 [lm-sensors] [PATCH] AMD K8 digital sensor Rudolf Marek
` (3 preceding siblings ...)
2006-08-22 18:23 ` Rudolf Marek
@ 2006-08-23 7:10 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2006-08-23 7:10 UTC (permalink / raw)
To: lm-sensors
> >> diff -uprN linux-2.6.18-rc2/drivers/hwmon/k8temp.c linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c
> >> --- linux-2.6.18-rc2/drivers/hwmon/k8temp.c 1970-01-01 01:00:00.000000000 +0100
> >> +++ linux-2.6.18-rc2-patched/drivers/hwmon/k8temp.c 2006-07-25 23:45:00.274085500 +0200
> >> @@ -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.
> 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,
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread