[lm-sensors] AMB FB DIMM driver

[lm-sensors] AMB FB DIMM driver

[Jeffrey Garrett]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Attached is a rough approximation of a driver for the Intel AMB FB-DIMM
temperature sensors...

There is a somewhat lonely, but relevant ticket at
http://www.lm-sensors.org/ticket/2143

The sensors exposed on the 5000P chipsets are located in a contiguous
range of memory spanning 128k.  Logically, this space is split into 2k
blocks, each of which representing a potential DIMM.  The temperature
of a DIMM in this PCIish space is at function 3, offset 0x85.  That is,
you just have to read [base addr] + 2048*[dimm idx] + 3*256 + 0x85.
The temperature is in 0.5 deg C.  So for example, 0x10 = 8C.

To find the base address of this contiguous area of memory, all you have
to do is look at your PCI configuration...  It's exposed at device 16,
function 0, offset 0x48.  This is the device 0x8086:0x25f0.

To find your sensors, one can scan the 128k area of memory for what
"looks" right.  The ticket suggested looking for a vendor id of 0x8086
(Intel) starting the particular 2k slot.  This works on the DSBF-D, but
I have a Tyan board which is less agreeable.  But on both boards, the
non-DIMM areas in that space have vendor id:device id equal to
0xffff:0xffff.  In other words, we can just look for anything not all
ones.  I haven't found anything on paper that justifies this, but it
works on the boards I have.  ;)

The attached driver does pretty much just this:  it's linked to the
0x8086:0x25f0 id, and it looks in the configuration space for the base
address, and scans for valid areas.  It then exposes this via sysfs.
I used k8temp as a template.

It is a bit rough right now.  It did work with a recent vintage 2.6.20
kernel, on at least two different boards.

Comments/criticisms/flames welcome.

Thanks,
Jeff Garrett
TeamHPC

[-- Attachment #2: ambtemp.c --]
[-- Type: text/x-csrc, Size: 9654 bytes --]

/*
 * ambtemp.c - Linux kernel module for hardware monitoring
 *
 * Copyright (C) 2007 Jeff Garrett <jgarrett@teamhpc.com>
 *
 * Inspired (stolen?) from the k8temp driver.
 *
 * 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>
#include <asm/bitops.h>


#define TEMP_FROM_REG(val)  ((((u16)(val)) * 500))
#define AMBASE_HIGH         0x48
#define AMB_EXTENT          0x20000
#define TEMP_OFFSET         ((3*256)+0x85)

struct ambtemp_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 */

	/* register values */
	void __iomem *base;
	u64 valid_sensors;	/* bitmap for which sensors are present */
	u8  temp[64];		/* all 64 possible sensors */
};

static struct ambtemp_data *ambtemp_update_device(struct device *dev)
{
	struct ambtemp_data *data = dev_get_drvdata(dev);
	int i;

	mutex_lock(&data->update_lock);

	if (!data->valid
		|| time_after(jiffies, data->last_updated + HZ)) {

		/* Read the sensors */
		for (i = 0; i < 64; i++)
			if (test_bit(i, &data->valid_sensors))
				data->temp[i] = readb(data->base + 2048*i + TEMP_OFFSET);

		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 ambtemp_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 *attr =
		to_sensor_dev_attr(devattr);
	int sensor = attr->index;
	struct ambtemp_data *data = ambtemp_update_device(dev);

	return sprintf(buf, "%d\n",
			   TEMP_FROM_REG(data->temp[sensor]));
}


/* There are a total of 4 channels, with 16 possible AMBs per channel */
static struct sensor_device_attribute temp_input[] = {
	SENSOR_ATTR(temp00_input, S_IRUGO, show_temp, NULL, 0),
	SENSOR_ATTR(temp01_input, S_IRUGO, show_temp, NULL, 1),
	SENSOR_ATTR(temp02_input, S_IRUGO, show_temp, NULL, 2),
	SENSOR_ATTR(temp03_input, S_IRUGO, show_temp, NULL, 3),
	SENSOR_ATTR(temp04_input, S_IRUGO, show_temp, NULL, 4),
	SENSOR_ATTR(temp05_input, S_IRUGO, show_temp, NULL, 5),
	SENSOR_ATTR(temp06_input, S_IRUGO, show_temp, NULL, 6),
	SENSOR_ATTR(temp07_input, S_IRUGO, show_temp, NULL, 7),
	SENSOR_ATTR(temp08_input, S_IRUGO, show_temp, NULL, 8),
	SENSOR_ATTR(temp09_input, S_IRUGO, show_temp, NULL, 9),
	SENSOR_ATTR(temp0a_input, S_IRUGO, show_temp, NULL, 10),
	SENSOR_ATTR(temp0b_input, S_IRUGO, show_temp, NULL, 11),
	SENSOR_ATTR(temp0c_input, S_IRUGO, show_temp, NULL, 12),
	SENSOR_ATTR(temp0d_input, S_IRUGO, show_temp, NULL, 13),
	SENSOR_ATTR(temp0e_input, S_IRUGO, show_temp, NULL, 14),
	SENSOR_ATTR(temp0f_input, S_IRUGO, show_temp, NULL, 15),
	SENSOR_ATTR(temp10_input, S_IRUGO, show_temp, NULL, 16),
	SENSOR_ATTR(temp11_input, S_IRUGO, show_temp, NULL, 17),
	SENSOR_ATTR(temp12_input, S_IRUGO, show_temp, NULL, 18),
	SENSOR_ATTR(temp13_input, S_IRUGO, show_temp, NULL, 19),
	SENSOR_ATTR(temp14_input, S_IRUGO, show_temp, NULL, 20),
	SENSOR_ATTR(temp15_input, S_IRUGO, show_temp, NULL, 21),
	SENSOR_ATTR(temp16_input, S_IRUGO, show_temp, NULL, 22),
	SENSOR_ATTR(temp17_input, S_IRUGO, show_temp, NULL, 23),
	SENSOR_ATTR(temp18_input, S_IRUGO, show_temp, NULL, 24),
	SENSOR_ATTR(temp19_input, S_IRUGO, show_temp, NULL, 25),
	SENSOR_ATTR(temp1a_input, S_IRUGO, show_temp, NULL, 26),
	SENSOR_ATTR(temp1b_input, S_IRUGO, show_temp, NULL, 27),
	SENSOR_ATTR(temp1c_input, S_IRUGO, show_temp, NULL, 28),
	SENSOR_ATTR(temp1d_input, S_IRUGO, show_temp, NULL, 29),
	SENSOR_ATTR(temp1e_input, S_IRUGO, show_temp, NULL, 30),
	SENSOR_ATTR(temp1f_input, S_IRUGO, show_temp, NULL, 31),
	SENSOR_ATTR(temp20_input, S_IRUGO, show_temp, NULL, 32),
	SENSOR_ATTR(temp21_input, S_IRUGO, show_temp, NULL, 33),
	SENSOR_ATTR(temp22_input, S_IRUGO, show_temp, NULL, 34),
	SENSOR_ATTR(temp23_input, S_IRUGO, show_temp, NULL, 35),
	SENSOR_ATTR(temp24_input, S_IRUGO, show_temp, NULL, 36),
	SENSOR_ATTR(temp25_input, S_IRUGO, show_temp, NULL, 37),
	SENSOR_ATTR(temp26_input, S_IRUGO, show_temp, NULL, 38),
	SENSOR_ATTR(temp27_input, S_IRUGO, show_temp, NULL, 39),
	SENSOR_ATTR(temp28_input, S_IRUGO, show_temp, NULL, 40),
	SENSOR_ATTR(temp29_input, S_IRUGO, show_temp, NULL, 41),
	SENSOR_ATTR(temp2a_input, S_IRUGO, show_temp, NULL, 42),
	SENSOR_ATTR(temp2b_input, S_IRUGO, show_temp, NULL, 43),
	SENSOR_ATTR(temp2c_input, S_IRUGO, show_temp, NULL, 44),
	SENSOR_ATTR(temp2d_input, S_IRUGO, show_temp, NULL, 45),
	SENSOR_ATTR(temp2e_input, S_IRUGO, show_temp, NULL, 46),
	SENSOR_ATTR(temp2f_input, S_IRUGO, show_temp, NULL, 47),
	SENSOR_ATTR(temp30_input, S_IRUGO, show_temp, NULL, 48),
	SENSOR_ATTR(temp31_input, S_IRUGO, show_temp, NULL, 49),
	SENSOR_ATTR(temp32_input, S_IRUGO, show_temp, NULL, 50),
	SENSOR_ATTR(temp33_input, S_IRUGO, show_temp, NULL, 51),
	SENSOR_ATTR(temp34_input, S_IRUGO, show_temp, NULL, 52),
	SENSOR_ATTR(temp35_input, S_IRUGO, show_temp, NULL, 53),
	SENSOR_ATTR(temp36_input, S_IRUGO, show_temp, NULL, 54),
	SENSOR_ATTR(temp37_input, S_IRUGO, show_temp, NULL, 55),
	SENSOR_ATTR(temp38_input, S_IRUGO, show_temp, NULL, 56),
	SENSOR_ATTR(temp39_input, S_IRUGO, show_temp, NULL, 57),
	SENSOR_ATTR(temp3a_input, S_IRUGO, show_temp, NULL, 58),
	SENSOR_ATTR(temp3b_input, S_IRUGO, show_temp, NULL, 59),
	SENSOR_ATTR(temp3c_input, S_IRUGO, show_temp, NULL, 60),
	SENSOR_ATTR(temp3d_input, S_IRUGO, show_temp, NULL, 61),
	SENSOR_ATTR(temp3e_input, S_IRUGO, show_temp, NULL, 62),
	SENSOR_ATTR(temp3f_input, S_IRUGO, show_temp, NULL, 63),
};

static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);

static struct pci_device_id ambtemp_ids[] = {
	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x25f0) },
	{ 0 },
};

MODULE_DEVICE_TABLE(pci, ambtemp_ids);

static int __devinit ambtemp_probe(struct pci_dev *pdev,
				  const struct pci_device_id *id)
{
	int err;
	int i;
	void *addr;
	unsigned long c;
	
	struct ambtemp_data *data;

	/* The Error Reporting Registers we piggy back on show up at
	 * 10.0, 10.1, and 10.2.  We only want the first function.  The
	 * AMBASE slot on the others is wrong.
	 */
	if (!(PCI_SLOT(pdev->devfn) == 0x10 && PCI_FUNC(pdev->devfn) == 0x00))
	{
		/* Go away */
		err = 0;
		goto exit;
	}

	if (!(data = kzalloc(sizeof(struct ambtemp_data), GFP_KERNEL))) {
		err = -ENOMEM;
		goto exit;
	}

	printk(KERN_INFO "ambtemp: Device found at %04x:%02x.%01x\n", 
		pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
	printk(KERN_INFO "ambtemp: Scanning for sensors...\n");

	/* Reserve the sensor range starting at AMBASE */
	pci_read_config_dword(pdev, AMBASE_HIGH, &addr);
	data->base = ioremap(addr, AMB_EXTENT);
	if (!data->base)
	{
		err = -EBUSY;
		goto exit;
	}

	for (i = 0; i < 64; i++)
	{
		c = readl(data->base + 2048*i);
		if (c != 0xffffffffl)
		{
			set_bit(i, &data->valid_sensors);
			printk(KERN_INFO "ambtemp: Sensor at offset 0x%02x\n", i);
		}
	}

	data->name = "ambtemp";
	mutex_init(&data->update_lock);
	dev_set_drvdata(&pdev->dev, data);

	/* Register sysfs hooks */
	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
		if (test_bit(i, &data->valid_sensors))
		{
			err = device_create_file(&pdev->dev, &temp_input[i].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:
	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);

	device_remove_file(&pdev->dev, &dev_attr_name);
	dev_set_drvdata(&pdev->dev, NULL);
	kfree(data);
exit:
	return err;
}

static void __devexit ambtemp_remove(struct pci_dev *pdev)
{
	int i;
	struct ambtemp_data *data = dev_get_drvdata(&pdev->dev);

	hwmon_device_unregister(data->class_dev);
	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);

	device_remove_file(&pdev->dev, &dev_attr_name);
	dev_set_drvdata(&pdev->dev, NULL);

	iounmap(data->base);
	kfree(data);
}

static struct pci_driver ambtemp_driver = {
	.name = "ambtemp",
	.id_table = ambtemp_ids,
	.probe = ambtemp_probe,
	.remove = __devexit_p(ambtemp_remove),
};

static int __init ambtemp_init(void)
{
	return pci_register_driver(&ambtemp_driver);
}

static void __exit ambtemp_exit(void)
{
	pci_unregister_driver(&ambtemp_driver);
}

MODULE_AUTHOR("Jeff Garrett <jgarrett@teamhpc.com>");
MODULE_DESCRIPTION("Intel AMB temperature monitor");
MODULE_LICENSE("GPL");

module_init(ambtemp_init)
module_exit(ambtemp_exit)

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] AMB FB DIMM driver

[Rudolf Marek]

Hi Jeffrey,
> To find your sensors, one can scan the 128k area of memory for what
> "looks" right.  The ticket suggested looking for a vendor id of 0x8086
> (Intel) starting the particular 2k slot.  This works on the DSBF-D, but
> I have a Tyan board which is less agreeable.  But on both boards, the
> non-DIMM areas in that space have vendor id:device id equal to
> 0xffff:0xffff.  In other words, we can just look for anything not all
> ones.  I haven't found anything on paper that justifies this, but it
> works on the boards I have.  ;)

Yes it should work. Anything other than 0xffff should be considered valid ID.

> /*
>  * ambtemp.c - Linux kernel module for hardware monitoring
>  *
>  * Copyright (C) 2007 Jeff Garrett <jgarrett@teamhpc.com>
>  *
>  * Inspired (stolen?) from the k8temp driver.

Well inspired sounds better.

>  *
>  * 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>
> #include <asm/bitops.h>
> 
> 
> #define TEMP_FROM_REG(val)  ((((u16)(val)) * 500))
> #define AMBASE_HIGH         0x48
> #define AMB_EXTENT          0x20000
> #define TEMP_OFFSET         ((3*256)+0x85)

Careful about tabs versus spaces!

> 
> struct ambtemp_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 */
> 
> 	/* register values */
> 	void __iomem *base;
> 	u64 valid_sensors;	/* bitmap for which sensors are present */
> 	u8  temp[64];		/* all 64 possible sensors */
> };
> 
> static struct ambtemp_data *ambtemp_update_device(struct device *dev)
> {
> 	struct ambtemp_data *data = dev_get_drvdata(dev);
> 	int i;
> 
> 	mutex_lock(&data->update_lock);
> 
> 	if (!data->valid
> 		|| time_after(jiffies, data->last_updated + HZ)) {
> 
> 		/* Read the sensors */
> 		for (i = 0; i < 64; i++)
> 			if (test_bit(i, &data->valid_sensors))
> 				data->temp[i] = readb(data->base + 2048*i + TEMP_OFFSET);
> 
> 		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 ambtemp_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 *attr > 		to_sensor_dev_attr(devattr);
> 	int sensor = attr->index;
> 	struct ambtemp_data *data = ambtemp_update_device(dev);
> 
> 	return sprintf(buf, "%d\n",
> 			   TEMP_FROM_REG(data->temp[sensor]));
> }
> 
> 
> /* There are a total of 4 channels, with 16 possible AMBs per channel */
> static struct sensor_device_attribute temp_input[] = {
> 	SENSOR_ATTR(temp00_input, S_IRUGO, show_temp, NULL, 0),
Without leading zero and decimal please. Maybe you can even do this in dynamic 
fashion. I think there was at least one driver doing it this way, but I dont 
recall right now.


> 	SENSOR_ATTR(temp01_input, S_IRUGO, show_temp, NULL, 1),
> 	SENSOR_ATTR(temp02_input, S_IRUGO, show_temp, NULL, 2),
> 	SENSOR_ATTR(temp03_input, S_IRUGO, show_temp, NULL, 3),
> 	SENSOR_ATTR(temp04_input, S_IRUGO, show_temp, NULL, 4),
> 	SENSOR_ATTR(temp05_input, S_IRUGO, show_temp, NULL, 5),
> 	SENSOR_ATTR(temp06_input, S_IRUGO, show_temp, NULL, 6),
> 	SENSOR_ATTR(temp07_input, S_IRUGO, show_temp, NULL, 7),
> 	SENSOR_ATTR(temp08_input, S_IRUGO, show_temp, NULL, 8),
> 	SENSOR_ATTR(temp09_input, S_IRUGO, show_temp, NULL, 9),
> 	SENSOR_ATTR(temp0a_input, S_IRUGO, show_temp, NULL, 10),
> 	SENSOR_ATTR(temp0b_input, S_IRUGO, show_temp, NULL, 11),
> 	SENSOR_ATTR(temp0c_input, S_IRUGO, show_temp, NULL, 12),
> 	SENSOR_ATTR(temp0d_input, S_IRUGO, show_temp, NULL, 13),
> 	SENSOR_ATTR(temp0e_input, S_IRUGO, show_temp, NULL, 14),
> 	SENSOR_ATTR(temp0f_input, S_IRUGO, show_temp, NULL, 15),
> 	SENSOR_ATTR(temp10_input, S_IRUGO, show_temp, NULL, 16),
> 	SENSOR_ATTR(temp11_input, S_IRUGO, show_temp, NULL, 17),
> 	SENSOR_ATTR(temp12_input, S_IRUGO, show_temp, NULL, 18),
> 	SENSOR_ATTR(temp13_input, S_IRUGO, show_temp, NULL, 19),
> 	SENSOR_ATTR(temp14_input, S_IRUGO, show_temp, NULL, 20),
> 	SENSOR_ATTR(temp15_input, S_IRUGO, show_temp, NULL, 21),
> 	SENSOR_ATTR(temp16_input, S_IRUGO, show_temp, NULL, 22),
> 	SENSOR_ATTR(temp17_input, S_IRUGO, show_temp, NULL, 23),
> 	SENSOR_ATTR(temp18_input, S_IRUGO, show_temp, NULL, 24),
> 	SENSOR_ATTR(temp19_input, S_IRUGO, show_temp, NULL, 25),
> 	SENSOR_ATTR(temp1a_input, S_IRUGO, show_temp, NULL, 26),
> 	SENSOR_ATTR(temp1b_input, S_IRUGO, show_temp, NULL, 27),
> 	SENSOR_ATTR(temp1c_input, S_IRUGO, show_temp, NULL, 28),
> 	SENSOR_ATTR(temp1d_input, S_IRUGO, show_temp, NULL, 29),
> 	SENSOR_ATTR(temp1e_input, S_IRUGO, show_temp, NULL, 30),
> 	SENSOR_ATTR(temp1f_input, S_IRUGO, show_temp, NULL, 31),
> 	SENSOR_ATTR(temp20_input, S_IRUGO, show_temp, NULL, 32),
> 	SENSOR_ATTR(temp21_input, S_IRUGO, show_temp, NULL, 33),
> 	SENSOR_ATTR(temp22_input, S_IRUGO, show_temp, NULL, 34),
> 	SENSOR_ATTR(temp23_input, S_IRUGO, show_temp, NULL, 35),
> 	SENSOR_ATTR(temp24_input, S_IRUGO, show_temp, NULL, 36),
> 	SENSOR_ATTR(temp25_input, S_IRUGO, show_temp, NULL, 37),
> 	SENSOR_ATTR(temp26_input, S_IRUGO, show_temp, NULL, 38),
> 	SENSOR_ATTR(temp27_input, S_IRUGO, show_temp, NULL, 39),
> 	SENSOR_ATTR(temp28_input, S_IRUGO, show_temp, NULL, 40),
> 	SENSOR_ATTR(temp29_input, S_IRUGO, show_temp, NULL, 41),
> 	SENSOR_ATTR(temp2a_input, S_IRUGO, show_temp, NULL, 42),
> 	SENSOR_ATTR(temp2b_input, S_IRUGO, show_temp, NULL, 43),
> 	SENSOR_ATTR(temp2c_input, S_IRUGO, show_temp, NULL, 44),
> 	SENSOR_ATTR(temp2d_input, S_IRUGO, show_temp, NULL, 45),
> 	SENSOR_ATTR(temp2e_input, S_IRUGO, show_temp, NULL, 46),
> 	SENSOR_ATTR(temp2f_input, S_IRUGO, show_temp, NULL, 47),
> 	SENSOR_ATTR(temp30_input, S_IRUGO, show_temp, NULL, 48),
> 	SENSOR_ATTR(temp31_input, S_IRUGO, show_temp, NULL, 49),
> 	SENSOR_ATTR(temp32_input, S_IRUGO, show_temp, NULL, 50),
> 	SENSOR_ATTR(temp33_input, S_IRUGO, show_temp, NULL, 51),
> 	SENSOR_ATTR(temp34_input, S_IRUGO, show_temp, NULL, 52),
> 	SENSOR_ATTR(temp35_input, S_IRUGO, show_temp, NULL, 53),
> 	SENSOR_ATTR(temp36_input, S_IRUGO, show_temp, NULL, 54),
> 	SENSOR_ATTR(temp37_input, S_IRUGO, show_temp, NULL, 55),
> 	SENSOR_ATTR(temp38_input, S_IRUGO, show_temp, NULL, 56),
> 	SENSOR_ATTR(temp39_input, S_IRUGO, show_temp, NULL, 57),
> 	SENSOR_ATTR(temp3a_input, S_IRUGO, show_temp, NULL, 58),
> 	SENSOR_ATTR(temp3b_input, S_IRUGO, show_temp, NULL, 59),
> 	SENSOR_ATTR(temp3c_input, S_IRUGO, show_temp, NULL, 60),
> 	SENSOR_ATTR(temp3d_input, S_IRUGO, show_temp, NULL, 61),
> 	SENSOR_ATTR(temp3e_input, S_IRUGO, show_temp, NULL, 62),
> 	SENSOR_ATTR(temp3f_input, S_IRUGO, show_temp, NULL, 63),
> };
> 
> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> 
> static struct pci_device_id ambtemp_ids[] = {
> 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x25f0) },

the 0x25f0 should go to the pciids.h Moreover I do not recall if this is used 
also for something else? I mean has this PCI device some other function?

> 	{ 0 },
> };
> 
> MODULE_DEVICE_TABLE(pci, ambtemp_ids);
> 
> static int __devinit ambtemp_probe(struct pci_dev *pdev,
> 				  const struct pci_device_id *id)
> {
> 	int err;
> 	int i;
> 	void *addr;
> 	unsigned long c;
> 	
> 	struct ambtemp_data *data;
> 
> 	/* The Error Reporting Registers we piggy back on show up at
> 	 * 10.0, 10.1, and 10.2.  We only want the first function.  The
> 	 * AMBASE slot on the others is wrong.
> 	 */
> 	if (!(PCI_SLOT(pdev->devfn) = 0x10 && PCI_FUNC(pdev->devfn) = 0x00))
> 	{
> 		/* Go away */
> 		err = 0;
> 		goto exit;
> 	}

Maybe just function check is enough?
  if (PCI_FUNC(pdev->devfn) !=...



> 
> 	if (!(data = kzalloc(sizeof(struct ambtemp_data), GFP_KERNEL))) {
> 		err = -ENOMEM;
> 		goto exit;
> 	}
> 
> 	printk(KERN_INFO "ambtemp: Device found at %04x:%02x.%01x\n", 
> 		pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
perhaps just dev_info will work better

> 	printk(KERN_INFO "ambtemp: Scanning for sensors...\n");
this should be dev_dbg


> 
> 	/* Reserve the sensor range starting at AMBASE */
> 	pci_read_config_dword(pdev, AMBASE_HIGH, &addr);

Perhaps mask out lowest bits? They are reserved 16:0
Also it may be even set 64bit so question is if we should support this. Maybe 
some check for 32bit valid address and check for 0x0 ?

> 	data->base = ioremap(addr, AMB_EXTENT);


> 	if (!data->base)
> 	{
> 		err = -EBUSY;
> 		goto exit;
> 	}
> 
> 	for (i = 0; i < 64; i++)
> 	{
> 		c = readl(data->base + 2048*i);
> 		if (c != 0xffffffffl)
> 		{
> 			set_bit(i, &data->valid_sensors);
> 			printk(KERN_INFO "ambtemp: Sensor at offset 0x%02x\n", i);
	dev_dbg
> 		}
> 	}
> 
> 	data->name = "ambtemp";
> 	mutex_init(&data->update_lock);
> 	dev_set_drvdata(&pdev->dev, data);
> 
> 	/* Register sysfs hooks */
> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
> 		if (test_bit(i, &data->valid_sensors))
> 		{
> 			err = device_create_file(&pdev->dev, &temp_input[i].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:
> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
> 		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);
> 
> 	device_remove_file(&pdev->dev, &dev_attr_name);
> 	dev_set_drvdata(&pdev->dev, NULL);
> 	kfree(data);
> exit:
> 	return err;
> }
> 
> static void __devexit ambtemp_remove(struct pci_dev *pdev)
> {
> 	int i;
> 	struct ambtemp_data *data = dev_get_drvdata(&pdev->dev);
> 
> 	hwmon_device_unregister(data->class_dev);
> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
> 		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);
> 
> 	device_remove_file(&pdev->dev, &dev_attr_name);
> 	dev_set_drvdata(&pdev->dev, NULL);
> 
> 	iounmap(data->base);
> 	kfree(data);
> }
> 
> static struct pci_driver ambtemp_driver = {
> 	.name = "ambtemp",
> 	.id_table = ambtemp_ids,
> 	.probe = ambtemp_probe,
> 	.remove = __devexit_p(ambtemp_remove),
> };
> 
> static int __init ambtemp_init(void)
> {
> 	return pci_register_driver(&ambtemp_driver);
> }
> 
> static void __exit ambtemp_exit(void)
> {
> 	pci_unregister_driver(&ambtemp_driver);
> }
> 
> MODULE_AUTHOR("Jeff Garrett <jgarrett@teamhpc.com>");
> MODULE_DESCRIPTION("Intel AMB temperature monitor");
> MODULE_LICENSE("GPL");
> 
> module_init(ambtemp_init)
> module_exit(ambtemp_exit)

Ok so this was just very fast review. For a future reference here are the 
datasheets:
http://www.intel.com/design/chipsets/datashts/313071.htm
http://www.intel.com/design/chipsets/datashts/313072.htm

I hope it helps,

Rudolf

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] AMB FB DIMM driver

[Jeffrey Garrett]

[-- Attachment #1: Type: text/plain, Size: 13663 bytes --]

Hi,

I have another attempt.  :)  Patch is attached, and comments to the
original quick review are below.  Mostly ACKs.

On Wed, May 23, 2007 at 11:40:47PM +0200, Rudolf Marek wrote:
> Hi Jeffrey,
>> To find your sensors, one can scan the 128k area of memory for what
>> "looks" right.  The ticket suggested looking for a vendor id of 0x8086
>> (Intel) starting the particular 2k slot.  This works on the DSBF-D, but
>> I have a Tyan board which is less agreeable.  But on both boards, the
>> non-DIMM areas in that space have vendor id:device id equal to
>> 0xffff:0xffff.  In other words, we can just look for anything not all
>> ones.  I haven't found anything on paper that justifies this, but it
>> works on the boards I have.  ;)
>
> Yes it should work. Anything other than 0xffff should be considered valid 
> ID.

Good to know

>> /*
>>  * ambtemp.c - Linux kernel module for hardware monitoring
>>  *
>>  * Copyright (C) 2007 Jeff Garrett <jgarrett@teamhpc.com>
>>  *
>>  * Inspired (stolen?) from the k8temp driver.
>
> Well inspired sounds better.

Point taken.  Not funny.  :)

>>  *
>>  * 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>
>> #include <asm/bitops.h>
>> #define TEMP_FROM_REG(val)  ((((u16)(val)) * 500))
>> #define AMBASE_HIGH         0x48
>> #define AMB_EXTENT          0x20000
>> #define TEMP_OFFSET         ((3*256)+0x85)
>
> Careful about tabs versus spaces!

Noted

>> struct ambtemp_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 */
>> 	/* register values */
>> 	void __iomem *base;
>> 	u64 valid_sensors;	/* bitmap for which sensors are present */
>> 	u8  temp[64];		/* all 64 possible sensors */
>> };
>> static struct ambtemp_data *ambtemp_update_device(struct device *dev)
>> {
>> 	struct ambtemp_data *data = dev_get_drvdata(dev);
>> 	int i;
>> 	mutex_lock(&data->update_lock);
>> 	if (!data->valid
>> 		|| time_after(jiffies, data->last_updated + HZ)) {
>> 		/* Read the sensors */
>> 		for (i = 0; i < 64; i++)
>> 			if (test_bit(i, &data->valid_sensors))
>> 				data->temp[i] = readb(data->base + 2048*i + TEMP_OFFSET);
>> 		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 ambtemp_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 *attr =
>> 		to_sensor_dev_attr(devattr);
>> 	int sensor = attr->index;
>> 	struct ambtemp_data *data = ambtemp_update_device(dev);
>> 	return sprintf(buf, "%d\n",
>> 			   TEMP_FROM_REG(data->temp[sensor]));
>> }
>> /* There are a total of 4 channels, with 16 possible AMBs per channel */
>> static struct sensor_device_attribute temp_input[] = {
>> 	SENSOR_ATTR(temp00_input, S_IRUGO, show_temp, NULL, 0),
> Without leading zero and decimal please. Maybe you can even do this in 
> dynamic fashion. I think there was at least one driver doing it this way, 
> but I dont recall right now.

Noted.  Should there be any communication with the user about names
then?  For example, in the DSBF-D manual it calls the DIMMs DIMM_00,
DIMM_01, etc. based on channel & number. And this would correlate to
temp00_input, etc.  In other words, locating a hot DIMM is relatively
easy with this information.  If they're called, say,
temp{1,2,17,18,33,34,49,50}_input, then a user can still figure out
which DIMM is which, but has to realize how to correlate 17 to DIMM_10.
It's a little bit more opaque.  And if they're called
temp{1,2,3,4,5,6,7,8}_input, it's even more so.

So, should there be a separate sysfs attribute, something like
temp1_name which may read DIMM_00, for example?

Also, which of the two options above is preferable [no gaps, or gaps but
numbers that are stable...  temp17 always points to the same slot]?

I made it kinda sorta dynamic...   i.e. the structs are in data, so they
are allocated dynamically, not statically, and initialized when needed.
But it still allocates space for 64...

>> 	SENSOR_ATTR(temp01_input, S_IRUGO, show_temp, NULL, 1),
>> 	SENSOR_ATTR(temp02_input, S_IRUGO, show_temp, NULL, 2),
>> 	SENSOR_ATTR(temp03_input, S_IRUGO, show_temp, NULL, 3),
>> 	SENSOR_ATTR(temp04_input, S_IRUGO, show_temp, NULL, 4),
>> 	SENSOR_ATTR(temp05_input, S_IRUGO, show_temp, NULL, 5),
>> 	SENSOR_ATTR(temp06_input, S_IRUGO, show_temp, NULL, 6),
>> 	SENSOR_ATTR(temp07_input, S_IRUGO, show_temp, NULL, 7),
>> 	SENSOR_ATTR(temp08_input, S_IRUGO, show_temp, NULL, 8),
>> 	SENSOR_ATTR(temp09_input, S_IRUGO, show_temp, NULL, 9),
>> 	SENSOR_ATTR(temp0a_input, S_IRUGO, show_temp, NULL, 10),
>> 	SENSOR_ATTR(temp0b_input, S_IRUGO, show_temp, NULL, 11),
>> 	SENSOR_ATTR(temp0c_input, S_IRUGO, show_temp, NULL, 12),
>> 	SENSOR_ATTR(temp0d_input, S_IRUGO, show_temp, NULL, 13),
>> 	SENSOR_ATTR(temp0e_input, S_IRUGO, show_temp, NULL, 14),
>> 	SENSOR_ATTR(temp0f_input, S_IRUGO, show_temp, NULL, 15),
>> 	SENSOR_ATTR(temp10_input, S_IRUGO, show_temp, NULL, 16),
>> 	SENSOR_ATTR(temp11_input, S_IRUGO, show_temp, NULL, 17),
>> 	SENSOR_ATTR(temp12_input, S_IRUGO, show_temp, NULL, 18),
>> 	SENSOR_ATTR(temp13_input, S_IRUGO, show_temp, NULL, 19),
>> 	SENSOR_ATTR(temp14_input, S_IRUGO, show_temp, NULL, 20),
>> 	SENSOR_ATTR(temp15_input, S_IRUGO, show_temp, NULL, 21),
>> 	SENSOR_ATTR(temp16_input, S_IRUGO, show_temp, NULL, 22),
>> 	SENSOR_ATTR(temp17_input, S_IRUGO, show_temp, NULL, 23),
>> 	SENSOR_ATTR(temp18_input, S_IRUGO, show_temp, NULL, 24),
>> 	SENSOR_ATTR(temp19_input, S_IRUGO, show_temp, NULL, 25),
>> 	SENSOR_ATTR(temp1a_input, S_IRUGO, show_temp, NULL, 26),
>> 	SENSOR_ATTR(temp1b_input, S_IRUGO, show_temp, NULL, 27),
>> 	SENSOR_ATTR(temp1c_input, S_IRUGO, show_temp, NULL, 28),
>> 	SENSOR_ATTR(temp1d_input, S_IRUGO, show_temp, NULL, 29),
>> 	SENSOR_ATTR(temp1e_input, S_IRUGO, show_temp, NULL, 30),
>> 	SENSOR_ATTR(temp1f_input, S_IRUGO, show_temp, NULL, 31),
>> 	SENSOR_ATTR(temp20_input, S_IRUGO, show_temp, NULL, 32),
>> 	SENSOR_ATTR(temp21_input, S_IRUGO, show_temp, NULL, 33),
>> 	SENSOR_ATTR(temp22_input, S_IRUGO, show_temp, NULL, 34),
>> 	SENSOR_ATTR(temp23_input, S_IRUGO, show_temp, NULL, 35),
>> 	SENSOR_ATTR(temp24_input, S_IRUGO, show_temp, NULL, 36),
>> 	SENSOR_ATTR(temp25_input, S_IRUGO, show_temp, NULL, 37),
>> 	SENSOR_ATTR(temp26_input, S_IRUGO, show_temp, NULL, 38),
>> 	SENSOR_ATTR(temp27_input, S_IRUGO, show_temp, NULL, 39),
>> 	SENSOR_ATTR(temp28_input, S_IRUGO, show_temp, NULL, 40),
>> 	SENSOR_ATTR(temp29_input, S_IRUGO, show_temp, NULL, 41),
>> 	SENSOR_ATTR(temp2a_input, S_IRUGO, show_temp, NULL, 42),
>> 	SENSOR_ATTR(temp2b_input, S_IRUGO, show_temp, NULL, 43),
>> 	SENSOR_ATTR(temp2c_input, S_IRUGO, show_temp, NULL, 44),
>> 	SENSOR_ATTR(temp2d_input, S_IRUGO, show_temp, NULL, 45),
>> 	SENSOR_ATTR(temp2e_input, S_IRUGO, show_temp, NULL, 46),
>> 	SENSOR_ATTR(temp2f_input, S_IRUGO, show_temp, NULL, 47),
>> 	SENSOR_ATTR(temp30_input, S_IRUGO, show_temp, NULL, 48),
>> 	SENSOR_ATTR(temp31_input, S_IRUGO, show_temp, NULL, 49),
>> 	SENSOR_ATTR(temp32_input, S_IRUGO, show_temp, NULL, 50),
>> 	SENSOR_ATTR(temp33_input, S_IRUGO, show_temp, NULL, 51),
>> 	SENSOR_ATTR(temp34_input, S_IRUGO, show_temp, NULL, 52),
>> 	SENSOR_ATTR(temp35_input, S_IRUGO, show_temp, NULL, 53),
>> 	SENSOR_ATTR(temp36_input, S_IRUGO, show_temp, NULL, 54),
>> 	SENSOR_ATTR(temp37_input, S_IRUGO, show_temp, NULL, 55),
>> 	SENSOR_ATTR(temp38_input, S_IRUGO, show_temp, NULL, 56),
>> 	SENSOR_ATTR(temp39_input, S_IRUGO, show_temp, NULL, 57),
>> 	SENSOR_ATTR(temp3a_input, S_IRUGO, show_temp, NULL, 58),
>> 	SENSOR_ATTR(temp3b_input, S_IRUGO, show_temp, NULL, 59),
>> 	SENSOR_ATTR(temp3c_input, S_IRUGO, show_temp, NULL, 60),
>> 	SENSOR_ATTR(temp3d_input, S_IRUGO, show_temp, NULL, 61),
>> 	SENSOR_ATTR(temp3e_input, S_IRUGO, show_temp, NULL, 62),
>> 	SENSOR_ATTR(temp3f_input, S_IRUGO, show_temp, NULL, 63),
>> };
>> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>> static struct pci_device_id ambtemp_ids[] = {
>> 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x25f0) },
>
> the 0x25f0 should go to the pciids.h Moreover I do not recall if this is 
> used also for something else? I mean has this PCI device some other 
> function?

It's called the error reporting registers by lspci.  As far as I can
tell, it's just a bunch of registers associated to the MCH, and nothing
else in the kernel claims it.

>> 	{ 0 },
>> };
>> MODULE_DEVICE_TABLE(pci, ambtemp_ids);
>> static int __devinit ambtemp_probe(struct pci_dev *pdev,
>> 				  const struct pci_device_id *id)
>> {
>> 	int err;
>> 	int i;
>> 	void *addr;
>> 	unsigned long c;
>> 	
>> 	struct ambtemp_data *data;
>> 	/* The Error Reporting Registers we piggy back on show up at
>> 	 * 10.0, 10.1, and 10.2.  We only want the first function.  The
>> 	 * AMBASE slot on the others is wrong.
>> 	 */
>> 	if (!(PCI_SLOT(pdev->devfn) == 0x10 && PCI_FUNC(pdev->devfn) == 0x00))
>> 	{
>> 		/* Go away */
>> 		err = 0;
>> 		goto exit;
>> 	}
>
> Maybe just function check is enough?
>  if (PCI_FUNC(pdev->devfn) !=...

Noted.

>> 	if (!(data = kzalloc(sizeof(struct ambtemp_data), GFP_KERNEL))) {
>> 		err = -ENOMEM;
>> 		goto exit;
>> 	}
>> 	printk(KERN_INFO "ambtemp: Device found at %04x:%02x.%01x\n", 
>> 		pdev->bus->number, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> perhaps just dev_info will work better

Indeed. :)

>> 	printk(KERN_INFO "ambtemp: Scanning for sensors...\n");
> this should be dev_dbg

Ok

>> 	/* Reserve the sensor range starting at AMBASE */
>> 	pci_read_config_dword(pdev, AMBASE_HIGH, &addr);
>
> Perhaps mask out lowest bits? They are reserved 16:0
> Also it may be even set 64bit so question is if we should support this. 
> Maybe some check for 32bit valid address and check for 0x0 ?

Noted.

>> 	data->base = ioremap(addr, AMB_EXTENT);
>
>
>> 	if (!data->base)
>> 	{
>> 		err = -EBUSY;
>> 		goto exit;
>> 	}
>> 	for (i = 0; i < 64; i++)
>> 	{
>> 		c = readl(data->base + 2048*i);
>> 		if (c != 0xffffffffl)
>> 		{
>> 			set_bit(i, &data->valid_sensors);
>> 			printk(KERN_INFO "ambtemp: Sensor at offset 0x%02x\n", i);
> 	dev_dbg

Ok.

>> 		}
>> 	}
>> 	data->name = "ambtemp";
>> 	mutex_init(&data->update_lock);
>> 	dev_set_drvdata(&pdev->dev, data);
>> 	/* Register sysfs hooks */
>> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
>> 		if (test_bit(i, &data->valid_sensors))
>> 		{
>> 			err = device_create_file(&pdev->dev, &temp_input[i].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:
>> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
>> 		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);
>> 	device_remove_file(&pdev->dev, &dev_attr_name);
>> 	dev_set_drvdata(&pdev->dev, NULL);
>> 	kfree(data);
>> exit:
>> 	return err;
>> }
>> static void __devexit ambtemp_remove(struct pci_dev *pdev)
>> {
>> 	int i;
>> 	struct ambtemp_data *data = dev_get_drvdata(&pdev->dev);
>> 	hwmon_device_unregister(data->class_dev);
>> 	for (i = 0; i < ARRAY_SIZE(temp_input); i++)
>> 		device_remove_file(&pdev->dev, &temp_input[i].dev_attr);
>> 	device_remove_file(&pdev->dev, &dev_attr_name);
>> 	dev_set_drvdata(&pdev->dev, NULL);
>> 	iounmap(data->base);
>> 	kfree(data);
>> }
>> static struct pci_driver ambtemp_driver = {
>> 	.name = "ambtemp",
>> 	.id_table = ambtemp_ids,
>> 	.probe = ambtemp_probe,
>> 	.remove = __devexit_p(ambtemp_remove),
>> };
>> static int __init ambtemp_init(void)
>> {
>> 	return pci_register_driver(&ambtemp_driver);
>> }
>> static void __exit ambtemp_exit(void)
>> {
>> 	pci_unregister_driver(&ambtemp_driver);
>> }
>> MODULE_AUTHOR("Jeff Garrett <jgarrett@teamhpc.com>");
>> MODULE_DESCRIPTION("Intel AMB temperature monitor");
>> MODULE_LICENSE("GPL");
>> module_init(ambtemp_init)
>> module_exit(ambtemp_exit)
>
> Ok so this was just very fast review. For a future reference here are the 
> datasheets:
> http://www.intel.com/design/chipsets/datashts/313071.htm
> http://www.intel.com/design/chipsets/datashts/313072.htm

Yea, I failed to mention it, but I had found those.  I hadn't spent very
long with them though.

> I hope it helps,
>
> Rudolf

Helps immensely.  I'm a bit new to this.  Thanks very much,

Jeff

[-- Attachment #2: ambtemp.patch --]
[-- Type: text/x-diff, Size: 7999 bytes --]

diff --git a/drivers/hwmon/ambtemp.c b/drivers/hwmon/ambtemp.c
new file mode 100644
index 0000000..bf38c54
--- /dev/null
+++ b/drivers/hwmon/ambtemp.c
@@ -0,0 +1,278 @@
+/*
+ * ambtemp.c - Linux kernel module for hardware monitoring
+ *
+ * Copyright (C) 2007 Jeff Garrett <jgarrett@teamhpc.com>
+ *
+ * Inspired from the k8temp driver.
+ *
+ * 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>
+#include <linux/bitmap.h>
+
+
+#define TEMP_FROM_REG(val)	((((u16)(val)) * 500))
+#define AMBASE_LOW_REG		0x48
+#define AMBASE_HIGH_REG		0x4C
+#define AMBASE_MASK		0x000000FFFFFE0000ULL
+#define AMB_MAX_DEVS		64
+#define AMB_DEV_LENGTH		2048	/* Length of a single sensor region */
+#define TEMP_OFFSET		((3*256)+0x85)
+
+/*
+ * These differ from their analogues in linux/sysfs.h and linux/hwmon-sysfs.h
+ * by not stringifying _name.  That is, _name is allowed to be runtime
+ * evaluable.
+ */
+#define __ATTR_DYNAMIC(_name,_mode,_show,_store) { \
+	.attr = { .name = _name, .mode = _mode, .owner = THIS_MODULE }, \
+	.show   = _show,					\
+	.store  = _store,					\
+}
+#define SENSOR_ATTR_DYNAMIC(_name, _mode, _show, _store, _index)	\
+	{ .dev_attr = __ATTR_DYNAMIC(_name, _mode, _show, _store),	\
+	  .index = _index }
+
+
+struct ambtemp_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 */
+
+	/* register values */
+	void __iomem *base;
+	DECLARE_BITMAP(valid_sensors, AMB_MAX_DEVS);	/* bitmap for which sensors are present */
+	u8 temp[AMB_MAX_DEVS];				/* all possible sensors */
+
+	struct sensor_device_attribute temp_input[AMB_MAX_DEVS];
+};
+
+static struct ambtemp_data *ambtemp_update_device(struct device *dev)
+{
+	struct ambtemp_data *data = dev_get_drvdata(dev);
+	int i;
+
+	mutex_lock(&data->update_lock);
+
+	if (!data->valid
+		|| time_after(jiffies, data->last_updated + HZ)) {
+
+		/* Read the sensors */
+		for (i = 0; i < AMB_MAX_DEVS; i++)
+			if (test_bit(i, &data->valid_sensors))
+				data->temp[i] = readb(data->base + i*AMB_DEV_LENGTH + TEMP_OFFSET);
+
+		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 ambtemp_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 *attr =
+		to_sensor_dev_attr(devattr);
+	int sensor = attr->index;
+	struct ambtemp_data *data = ambtemp_update_device(dev);
+
+	return sprintf(buf, "%d\n",
+			   TEMP_FROM_REG(data->temp[sensor]));
+}
+
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static struct pci_device_id ambtemp_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_MCH_AMB) },
+	{ 0 },
+};
+
+MODULE_DEVICE_TABLE(pci, ambtemp_ids);
+
+static int __devinit ambtemp_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	int err;
+	int i;
+	u32 low, high;
+	u64 addr;
+	unsigned long c;
+	char *name;
+	
+	struct ambtemp_data *data;
+
+	/* The Error Reporting Registers we piggy back on show up at
+	 * 10.0, 10.1, and 10.2.  We only want the first function.  The
+	 * AMBASE slot on the others is wrong.
+	 */
+	if (PCI_FUNC(pdev->devfn) != 0x00) {
+		/* Go away */
+		err = 0;
+		goto exit;
+	}
+
+	if (!(data = kzalloc(sizeof(struct ambtemp_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	dev_info(&pdev->dev, "Device found\n");
+	dev_dbg(&pdev->dev, "Scanning for sensors...\n");
+
+	/* Reserve the sensor range starting at AMBASE */
+	pci_read_config_dword(pdev, AMBASE_LOW_REG, &low);
+	pci_read_config_dword(pdev, AMBASE_HIGH_REG, &high);
+
+	addr = ((u64)high << 32 | low) & AMBASE_MASK;
+	if (!addr) {
+		err = -EBUSY;
+		goto exit;
+	}
+
+	data->base = ioremap(addr, AMB_MAX_DEVS * AMB_DEV_LENGTH);
+	if (!data->base) {
+		err = -EBUSY;
+		goto exit;
+	}
+
+	for (i = 0; i < AMB_MAX_DEVS; i++) {
+		c = readl(data->base + i*AMB_DEV_LENGTH);
+		if (c != 0xffffffffl) {
+			set_bit(i, &data->valid_sensors);
+			dev_dbg(&pdev->dev, "Sensor at offset 0x%02x\n", i);
+		}
+	}
+
+	data->name = "ambtemp";
+	mutex_init(&data->update_lock);
+	dev_set_drvdata(&pdev->dev, data);
+
+	/* Register sysfs hooks */
+	for (i = 0; i < ARRAY_SIZE(data->temp_input); i++)
+		if (test_bit(i, &data->valid_sensors)) {
+			if (!(name = kasprintf(GFP_KERNEL, "temp%d_input", i + 1))) {
+				err = -ENOMEM;
+				goto exit_remove;
+			}
+
+			data->temp_input[i] = (struct sensor_device_attribute)
+				SENSOR_ATTR_DYNAMIC(name, S_IRUGO, show_temp, NULL, i);
+
+			err = device_create_file(&pdev->dev, &data->temp_input[i].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:
+	for (i = 0; i < ARRAY_SIZE(data->temp_input); i++)
+		if (data->temp_input[i].dev_attr.attr.name) {
+			device_remove_file(&pdev->dev, &data->temp_input[i].dev_attr);
+			kfree(data->temp_input[i].dev_attr.attr.name);
+		}
+
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	dev_set_drvdata(&pdev->dev, NULL);
+	kfree(data);
+exit:
+	return err;
+}
+
+static void __devexit ambtemp_remove(struct pci_dev *pdev)
+{
+	int i;
+	struct ambtemp_data *data = dev_get_drvdata(&pdev->dev);
+
+	if (!data)
+		return;
+
+	hwmon_device_unregister(data->class_dev);
+	for (i = 0; i < ARRAY_SIZE(data->temp_input); i++)
+		if (data->temp_input[i].dev_attr.attr.name) {
+			device_remove_file(&pdev->dev, &data->temp_input[i].dev_attr);
+			kfree(data->temp_input[i].dev_attr.attr.name);
+		}
+
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	iounmap(data->base);
+	kfree(data);
+}
+
+static struct pci_driver ambtemp_driver = {
+	.name = "ambtemp",
+	.id_table = ambtemp_ids,
+	.probe = ambtemp_probe,
+	.remove = __devexit_p(ambtemp_remove),
+};
+
+static int __init ambtemp_init(void)
+{
+	return pci_register_driver(&ambtemp_driver);
+}
+
+static void __exit ambtemp_exit(void)
+{
+	pci_unregister_driver(&ambtemp_driver);
+}
+
+MODULE_AUTHOR("Jeff Garrett <jgarrett@teamhpc.com>");
+MODULE_DESCRIPTION("Intel AMB temperature monitor");
+MODULE_LICENSE("GPL");
+
+module_init(ambtemp_init)
+module_exit(ambtemp_exit)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4712e26..9b55239 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2146,6 +2146,7 @@
 
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
+#define PCI_DEVICE_ID_INTEL_MCH_AMB	0x25F0
 #define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
 #define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
 #define PCI_DEVICE_ID_INTEL_PXH_0	0x0329

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] AMB FB DIMM driver

[Rudolf Marek]

Hello Jeffrey,

Sorry for the delay,

>>> static struct sensor_device_attribute temp_input[] = {
>>> 	SENSOR_ATTR(temp00_input, S_IRUGO, show_temp, NULL, 0),
>> Without leading zero and decimal please. Maybe you can even do this in 
>> dynamic fashion. I think there was at least one driver doing it this way, 
>> but I dont recall right now.
> 
> Noted.  Should there be any communication with the user about names
> then?  For example, in the DSBF-D manual it calls the DIMMs DIMM_00,
> DIMM_01, etc. based on channel & number. And this would correlate to
> temp00_input, etc.  In other words, locating a hot DIMM is relatively
> easy with this information.  If they're called, say,
> temp{1,2,17,18,33,34,49,50}_input, then a user can still figure out
> which DIMM is which, but has to realize how to correlate 17 to DIMM_10.
> It's a little bit more opaque.  And if they're called
> temp{1,2,3,4,5,6,7,8}_input, it's even more so.

> So, should there be a separate sysfs attribute, something like
> temp1_name which may read DIMM_00, for example?

Yes we have _label (coretemp driver for example). Please fill the label with string.

> Also, which of the two options above is preferable [no gaps, or gaps but
> numbers that are stable...  temp17 always points to the same slot]?

If we have _label files so perhaps static approach is easiest.

> I made it kinda sorta dynamic...   i.e. the structs are in data, so they
> are allocated dynamically, not statically, and initialized when needed.
> But it still allocates space for 64...

Ok I dont know if Mark will like it, anyway this may be changed in next iteration.



>>> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>>> static struct pci_device_id ambtemp_ids[] = {
>>> 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x25f0) },
>> the 0x25f0 should go to the pciids.h Moreover I do not recall if this is 
>> used also for something else? I mean has this PCI device some other 
>> function?
> 
> It's called the error reporting registers by lspci.  As far as I can
> tell, it's just a bunch of registers associated to the MCH, and nothing
> else in the kernel claims it.

And what about EDAC? (check the edac project please) maybe we need to do same 
trick as for i2c-viapro driver, return -ENODEV to PCI layer but normally 
request_region for the MEM region.

> Helps immensely.  I'm a bit new to this.  Thanks very much,

Good. Sorry for the delays, this week I have first opportunity to have free 
evening :/

Hope it helps a bit.

Rudolf

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [patch] AMB FB DIMM driver

[Jeff Garrett]

[-- Attachment #1: Type: text/plain, Size: 3373 bytes --]

Darrick, I'm cc'ing you since you aren't subscribed to the list and
since a recent posting suggests you will be interested in this thread.
Some previous (really short) threads:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-May/019782.html
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-June/019979.html

Hello, Rudolf,

>Sorry for the delay,

Same here.  Work always seems to get in the way.  :)  Attached is the
next version of the driver.

>>>> static struct sensor_device_attribute temp_input[] = {
>>>> 	SENSOR_ATTR(temp00_input, S_IRUGO, show_temp, NULL, 0),
>>> Without leading zero and decimal please. Maybe you can even do this in 
>>> dynamic fashion. I think there was at least one driver doing it this way, 
>>> but I dont recall right now.
>> 
>> Noted.  Should there be any communication with the user about names
>> then?  For example, in the DSBF-D manual it calls the DIMMs DIMM_00,
>> DIMM_01, etc. based on channel & number. And this would correlate to
>> temp00_input, etc.  In other words, locating a hot DIMM is relatively
>> easy with this information.  If they're called, say,
>> temp{1,2,17,18,33,34,49,50}_input, then a user can still figure out
>> which DIMM is which, but has to realize how to correlate 17 to DIMM_10.
>> It's a little bit more opaque.  And if they're called
>> temp{1,2,3,4,5,6,7,8}_input, it's even more so.

>> So, should there be a separate sysfs attribute, something like
>> temp1_name which may read DIMM_00, for example?

>Yes we have _label (coretemp driver for example). Please fill the label with string.

Done.

>> Also, which of the two options above is preferable [no gaps, or gaps but
>> numbers that are stable...  temp17 always points to the same slot]?

>If we have _label files so perhaps static approach is easiest.

>> I made it kinda sorta dynamic...   i.e. the structs are in data, so they
>> are allocated dynamically, not statically, and initialized when needed.
>> But it still allocates space for 64...

>Ok I dont know if Mark will like it, anyway this may be changed in next iteration.

OK, so I found it easy to switch over to a struct list_head and
dynamically allocate all the sensors.  This made it easy as well to
number them numerically.  So right now, you have temp1_input,
temp2_input, etc.  And there are the _label's as well.

>>>> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>>>> static struct pci_device_id ambtemp_ids[] = {
>>>> 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x25f0) },
>>> the 0x25f0 should go to the pciids.h Moreover I do not recall if this is 
>>> used also for something else? I mean has this PCI device some other 
>>> function?
>> 
>> It's called the error reporting registers by lspci.  As far as I can
>> tell, it's just a bunch of registers associated to the MCH, and nothing
>> else in the kernel claims it.

>And what about EDAC? (check the edac project please) maybe we need to do same 
>trick as for i2c-viapro driver, return -ENODEV to PCI layer but normally 
>request_region for the MEM region.

You are right.  I had all but forgot about EDAC and they do need access to
the memory error registers, so I've modified it to return -ENODEV.

>> Helps immensely.  I'm a bit new to this.  Thanks very much,

>Good. Sorry for the delays, this week I have first opportunity to have free 
>evening :/

>Hope it helps a bit.

It's great!  Thanks,

Jeff

[-- Attachment #2: ambtemp-v3.patch --]
[-- Type: text/x-patch, Size: 11380 bytes --]

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4d1cb5b..151bef2 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -128,6 +128,17 @@ config SENSORS_K8TEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called k8temp.
 
+config SENSORS_AMBTEMP
+	tristate "Intel AMB FB-DIMM temperature sensors"
+	depends on X86 && PCI && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the temperature
+	  sensor(s) on the Advanced Memory Buffer on fully buffered
+	  DIMMs.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ambtemp.
+
 config SENSORS_AMS
 	tristate "Apple Motion Sensor driver"
 	depends on PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || (ADB_PMU && !I2C) || I2C) && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index cfaf338..895d94f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
+obj-$(CONFIG_SENSORS_AMBTEMP)	+= ambtemp.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
diff --git a/drivers/hwmon/ambtemp.c b/drivers/hwmon/ambtemp.c
new file mode 100644
index 0000000..bd6bddb
--- /dev/null
+++ b/drivers/hwmon/ambtemp.c
@@ -0,0 +1,371 @@
+/*
+ * ambtemp.c - Linux kernel module for hardware monitoring
+ *
+ * Copyright (C) 2007 Jeff Garrett <jgarrett@teamhpc.com>
+ *
+ * Inspired from the k8temp driver.  Thanks also to Rudolf Marek.
+ *
+ * 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>
+#include <linux/list.h>
+
+
+/*
+ * Please see the datasheets:
+ *   http://www.intel.com/design/chipsets/datashts/313071.htm
+ *   http://www.intel.com/design/chipsets/datashts/313072.htm
+ */
+#define TEMP_FROM_REG(val)	(500*((unsigned long)val))
+#define AMBASE_LOW_REG		0x48
+#define AMBASE_HIGH_REG		0x4C
+#define AMBASE_MASK		0x000000FFFFFE0000ULL
+
+#define AMB_MAX_DEVS		64
+#define AMB_DEV_LENGTH		2048	/* Length of a single sensor region */
+#define AMB_RGN_LENGTH		(AMB_MAX_DEVS * AMB_DEV_LENGTH)
+
+#define AMB_INDEX(base, off)	((off - base)/AMB_DEV_LENGTH)
+
+#define TEMP_OFFSET		0x385
+
+/*
+ * These differ from their analogues in linux/sysfs.h and linux/hwmon-sysfs.h
+ * by not stringifying _name.  That is, _name is allowed to be runtime
+ * evaluable.
+ */
+#define __ATTR_DYNAMIC(_name,_mode,_show,_store) {			\
+	.attr = { .name = _name, .mode = _mode, .owner = THIS_MODULE },	\
+	.show   = _show,						\
+	.store  = _store,						\
+}
+#define SENSOR_ATTR_DYNAMIC(_name, _mode, _show, _store, _index)	\
+	(struct sensor_device_attribute)				\
+	{ .dev_attr = __ATTR_DYNAMIC(_name, _mode, _show, _store),	\
+	  .index = _index }
+
+
+struct ambtemp_sensor {
+	unsigned int idx;
+	unsigned long value;
+	void __iomem *address;
+
+	/* Sysfs */
+	char *input_file;	/* e.g. "temp1_input" */
+	char *label_file;	/* e.g. "temp1_label" */
+	struct sensor_device_attribute temp_input;
+	struct sensor_device_attribute temp_label;
+	char *label;		/* e.g. "DIMM_00" */
+
+	struct list_head list;
+};
+
+struct ambtemp_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 */
+
+	/* register values */
+	void __iomem *base;
+	struct ambtemp_sensor temp;
+};
+
+static struct ambtemp_data *ambtemp_update_device(struct device *dev)
+{
+	struct ambtemp_data *data = dev_get_drvdata(dev);
+	struct ambtemp_sensor *p;
+
+	mutex_lock(&data->update_lock);
+
+	if (!data->valid
+		|| time_after(jiffies, data->last_updated + HZ)) {
+
+		/* Read the sensors */
+		list_for_each_entry(p, &data->temp.list, list)
+			p->value = TEMP_FROM_REG(readb(p->address));
+
+		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 ambtemp_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 *attr = to_sensor_dev_attr(devattr);
+	struct ambtemp_data *data = ambtemp_update_device(dev);
+	struct ambtemp_sensor *p;
+
+	list_for_each_entry(p, &data->temp.list, list)
+		if (p->idx == attr->index)
+			return sprintf(buf, "%ld\n", p->value);
+	return sprintf(buf, "\n");
+}
+
+
+static ssize_t show_label(struct device *dev,
+			  struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct ambtemp_data *data = dev_get_drvdata(dev);
+	struct ambtemp_sensor *p;
+
+	list_for_each_entry(p, &data->temp.list, list)
+		if (p->idx == attr->index)
+			return sprintf(buf, "%s\n", p->label);
+	return sprintf(buf, "\n");
+}
+
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static struct pci_device_id ambtemp_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_MCH_AMB) },
+	{ 0 },
+};
+
+MODULE_DEVICE_TABLE(pci, ambtemp_ids);
+
+static int __devinit ambtemp_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	int err;
+	u32 low, high;
+	u64 addr;
+	void __iomem *l;
+	
+	struct ambtemp_data *data;
+
+	struct ambtemp_sensor *p, *q;
+	unsigned int idx = 0;
+
+	/* The Error Reporting Registers we piggy back on show up at
+	 * 10.0, 10.1, and 10.2.  We only want the first function.  The
+	 * AMBASE slot on the others is wrong.
+	 */
+	if (PCI_FUNC(pdev->devfn) != 0x00) {
+		/* Go away */
+		err = -ENODEV;
+		goto exit;
+	}
+
+	data = kzalloc(sizeof(struct ambtemp_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	INIT_LIST_HEAD(&data->temp.list);
+
+	dev_info(&pdev->dev, "Device found\n");
+	dev_dbg(&pdev->dev, "Scanning for sensors...\n");
+
+	/* Reserve the sensor range starting at AMBASE */
+	pci_read_config_dword(pdev, AMBASE_LOW_REG, &low);
+	pci_read_config_dword(pdev, AMBASE_HIGH_REG, &high);
+
+	addr = ((u64)high << 32 | low) & AMBASE_MASK;
+	if (!addr) {
+		err = -EBUSY;
+		goto exit;
+	}
+
+	data->base = ioremap(addr, AMB_RGN_LENGTH);
+	if (!data->base) {
+		err = -EBUSY;
+		goto exit;
+	}
+
+	for (l = data->base; l < data->base + AMB_RGN_LENGTH;
+	     l += AMB_DEV_LENGTH) {
+		if (readl(l) == 0xFFFFFFFFL)
+			continue;
+
+		p = kzalloc(sizeof(struct ambtemp_sensor), GFP_KERNEL);
+		if (!p) {
+			err = -ENOMEM;
+			goto exit;
+		}
+
+		p->idx = ++idx;
+		p->address = l + TEMP_OFFSET;
+
+		list_add_tail(&p->list, &data->temp.list);
+		dev_dbg(&pdev->dev, "Sensor at offset 0x%02x\n",
+		        AMB_INDEX(data->base, l));
+	}
+
+	data->name = "ambtemp";
+	mutex_init(&data->update_lock);
+	dev_set_drvdata(&pdev->dev, data);
+
+	/* Register sysfs hooks */
+	list_for_each_entry(p, &data->temp.list, list) {
+		p->input_file = kasprintf(GFP_KERNEL, "temp%d_input", p->idx);
+		if (!p->input_file) {
+			err = -ENOMEM;
+			goto exit_remove;
+		}
+
+		p->temp_input = SENSOR_ATTR_DYNAMIC(p->input_file, S_IRUGO,
+						    show_temp, NULL, p->idx);
+
+		err = device_create_file(&pdev->dev, &p->temp_input.dev_attr);
+		if (err)
+			goto exit_remove;
+
+		p->label_file = kasprintf(GFP_KERNEL, "temp%d_label", p->idx);
+		if (!p->label_file) {
+			err = -ENOMEM;
+			goto exit_remove;
+		}
+
+		p->temp_label = SENSOR_ATTR_DYNAMIC(p->label_file, S_IRUGO,
+						    show_label, NULL, p->idx);
+
+		err = device_create_file(&pdev->dev, &p->temp_label.dev_attr);
+		if (err)
+			goto exit_remove;
+
+		p->label = kasprintf(GFP_KERNEL, "DIMM_%02lx",
+		                     AMB_INDEX(data->base, p->address));
+		if (!p->label) {
+			err = -ENOMEM;
+			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;
+	}
+
+	/* Let others bind to this PCI device */
+	return -ENODEV;
+
+exit_remove:
+	list_for_each_entry_safe(p, q, &data->temp.list, list) {
+		if (p->input_file) {
+			device_remove_file(&pdev->dev, &p->temp_input.dev_attr);
+			kfree(p->input_file);
+		}
+
+		if (p->label_file) {
+			device_remove_file(&pdev->dev, &p->temp_label.dev_attr);
+			kfree(p->label_file);
+		}
+
+		kfree(p->label);
+		list_del(&p->list);
+		kfree(p);
+	}
+
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	dev_set_drvdata(&pdev->dev, NULL);
+	kfree(data);
+exit:
+	return err;
+}
+
+static void __devexit ambtemp_remove(struct pci_dev *pdev)
+{
+	struct ambtemp_data *data = dev_get_drvdata(&pdev->dev);
+	struct ambtemp_sensor *p, *q;
+
+	if (!data)
+		return;
+
+	hwmon_device_unregister(data->class_dev);
+	list_for_each_entry_safe(p, q, &data->temp.list, list) {
+		if (p->input_file) {
+			device_remove_file(&pdev->dev, &p->temp_input.dev_attr);
+			kfree(p->input_file);
+		}
+
+		if (p->label_file) {
+			device_remove_file(&pdev->dev, &p->temp_label.dev_attr);
+			kfree(p->label_file);
+		}
+
+		kfree(p->label);
+		list_del(&p->list);
+		kfree(p);
+	}
+
+	device_remove_file(&pdev->dev, &dev_attr_name);
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	iounmap(data->base);
+	kfree(data);
+}
+
+static struct pci_driver ambtemp_driver = {
+	.name = "ambtemp",
+	.id_table = ambtemp_ids,
+	.probe = ambtemp_probe,
+	.remove = __devexit_p(ambtemp_remove),
+};
+
+static int __init ambtemp_init(void)
+{
+	return pci_register_driver(&ambtemp_driver);
+}
+
+static void __exit ambtemp_exit(void)
+{
+	pci_unregister_driver(&ambtemp_driver);
+}
+
+MODULE_AUTHOR("Jeff Garrett <jgarrett@teamhpc.com>");
+MODULE_DESCRIPTION("Intel AMB temperature monitor");
+MODULE_LICENSE("GPL");
+
+module_init(ambtemp_init)
+module_exit(ambtemp_exit)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4712e26..9b55239 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2146,6 +2146,7 @@
 
 #define PCI_VENDOR_ID_INTEL		0x8086
 #define PCI_DEVICE_ID_INTEL_EESSC	0x0008
+#define PCI_DEVICE_ID_INTEL_MCH_AMB	0x25F0
 #define PCI_DEVICE_ID_INTEL_PXHD_0	0x0320
 #define PCI_DEVICE_ID_INTEL_PXHD_1	0x0321
 #define PCI_DEVICE_ID_INTEL_PXH_0	0x0329

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [patch] AMB FB DIMM driver

[Rudolf Marek]

Hi all,

I'm leaving soon for two weeks to the summer camp. I will be offline there so I 
cant resolve this soon. Maybe others will help?

It seems we have two drivers now... So question is what now? Let win better one ;) ?

Thanks,
Rudolf

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [patch] AMB FB DIMM driver

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 578 bytes --]

On Tue, Jul 17, 2007 at 05:24:00PM +0200, Rudolf Marek wrote:
> Hi all,
> 
> I'm leaving soon for two weeks to the summer camp. I will be offline there 
> so I cant resolve this soon. Maybe others will help?
> 
> It seems we have two drivers now... So question is what now? Let win better 
> one ;) ?

Mr. Garrett's driver looks fine to me... except that the driver ought to
be a bit clearer that it's for Intel chipsets only--perhaps call it
'intel-ambtemp' to avoid confusion if/when other chipset designers
expose the AMB registers via different methods.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors