[lm-sensors] [PATCH] AMD K8 digital sensor

[Jean Delvare]

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