[PATCH 9/10] introduce intel_menlow platform specific driver

[PATCH 9/10] introduce intel_menlow platform specific driver

[Zhang Rui]

From: Thomas Sujith <sujith.thomas@intel.com>

Intel menlow platform specific driver for thermal management.

Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/misc/Kconfig        |   10 
 drivers/misc/Makefile       |    1 
 drivers/misc/intel_menlow.c |  527 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 538 insertions(+)

Index: linux-2.6/drivers/misc/intel_menlow.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/misc/intel_menlow.c
@@ -0,0 +1,527 @@
+/*
+*  intel_menlow.c - Intel menlow Driver for thermal management extension
+*
+*  Copyright (C) 2008 Intel Corp
+*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
+*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
+*  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+*
+*  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; version 2 of the License.
+*
+*  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.,
+*  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+*
+* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+*
+*  This driver creates the sys I/F for programming the sensors.
+*  It also implements the driver for intel menlow memory controller (hardware
+*  id is INT0002) which makes use of the platform specific ACPI methods
+*  to get/set bandwidth.
+*/
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/pm.h>
+
+#include <linux/thermal.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Thomas Sujith");
+MODULE_AUTHOR("Zhang Rui");
+MODULE_DESCRIPTION("Intel Menlow platform specific driver");
+MODULE_LICENSE("GPL");
+
+/*
+ * Memory controller device control
+ */
+
+#define MEMORY_GET_BANDWIDTH "GTHS"
+#define MEMORY_SET_BANDWIDTH "STHS"
+#define MEMORY_ARG_CUR_BANDWIDTH 1
+#define MEMORY_ARG_MAX_BANDWIDTH 0
+
+static int
+memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
+			     unsigned long *max_state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	*max_state = value - 1;
+	return 0;
+}
+
+static int
+memory_get_max_bandwidth(struct thermal_cooling_device *cdev, char *buf)
+{
+	unsigned long value;
+	if (memory_get_int_max_bandwidth(cdev,&value))
+		return -EINVAL;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int
+memory_get_cur_bandwidth(struct thermal_cooling_device *cdev, char *buf)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_CUR_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int
+memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
+			 unsigned int state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+	int temp;
+	unsigned long max_state;
+
+	if (memory_get_int_max_bandwidth(cdev,&max_state))
+		return -EFAULT;
+
+	if (max_state < 0 || state > max_state)
+		return -EINVAL;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = state;
+
+	status =
+	    acpi_evaluate_integer(handle, MEMORY_SET_BANDWIDTH, &arg_list,
+				  (unsigned long *)&temp);
+
+	printk(KERN_INFO
+		"Bandwidth value was %d: status is %d\n", state, status);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return 0;
+}
+
+static struct thermal_cooling_device_ops memory_cooling_ops = {
+	.get_max_state = memory_get_max_bandwidth,
+	.get_cur_state = memory_get_cur_bandwidth,
+	.set_cur_state = memory_set_cur_bandwidth,
+};
+
+/*
+ * Memory Device Management
+ */
+static int intel_menlow_memory_add(struct acpi_device *device)
+{
+	int result = -ENODEV;
+	acpi_status status = AE_OK;
+	acpi_handle dummy;
+	struct thermal_cooling_device *cdev;
+
+	if (!device)
+		return -EINVAL;
+
+	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	cdev = thermal_cooling_device_register("Memory controller", device,
+					       &memory_cooling_ops);
+	acpi_driver_data(device) = cdev;
+	if (!cdev)
+		result = -ENODEV;
+	else {
+		result =
+		    sysfs_create_link(&device->dev.kobj, &cdev->device.kobj,
+				      "thermal_cooling");
+		if (result)
+			goto end;
+		result =
+		    sysfs_create_link(&cdev->device.kobj, &device->dev.kobj,
+				      "device");
+		if (result)
+			goto end;
+	}
+
+      end:
+	return result;
+}
+
+static int intel_menlow_memory_remove(struct acpi_device *device, int type)
+{
+	struct thermal_cooling_device *cdev = acpi_driver_data(device);
+
+	if (!device || !cdev)
+		return -EINVAL;
+
+	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+	sysfs_remove_link(&cdev->device.kobj, "device");
+	thermal_cooling_device_unregister(cdev);
+
+	return 0;
+}
+
+const static struct acpi_device_id intel_menlow_memory_ids[] = {
+	{"INT0002", 0},
+	{"", 0},
+};
+
+static struct acpi_driver intel_menlow_memory_driver = {
+	.name = "intel_menlow_thermal_control",
+	.ids = intel_menlow_memory_ids,
+	.ops = {
+		.add = intel_menlow_memory_add,
+		.remove = intel_menlow_memory_remove,
+		},
+};
+
+/*
+ * Sensor control on menlow platform
+ */
+
+#define THERMAL_AUX0 0
+#define THERMAL_AUX1 1
+#define GET_AUX0 "GAX0"
+#define GET_AUX1 "GAX1"
+#define SET_AUX0 "SAX0"
+#define SET_AUX1 "SAX1"
+
+struct intel_menlow_attribute {
+	struct device_attribute attr;
+	struct device *device;
+	acpi_handle handle;
+	struct list_head node;
+};
+
+static LIST_HEAD(intel_menlow_attr_list);
+static DEFINE_MUTEX(intel_menlow_attr_lock);
+
+/*
+ * sensor_get_auxtrip
+ * -----------------
+ * get the current auxtrip value from sensor through proprietory ACPI methods
+ * name: Thermalzone name
+ * auxtype : AUX0/AUX1
+ * buf: syfs buffer
+ */
+static int sensor_get_auxtrip(acpi_handle handle, int index, int *value)
+{
+	acpi_status status;
+
+	if ((index != 0 && index != 1) || !value)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
+				       NULL, (unsigned long *)value);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
+/*
+ * sensor_set_auxtrip
+ * -----------------
+ * set the new auxtrip value to sensor through proprietory ACPI methods
+ * name: Thermalzone name
+ * auxtype : AUX0/AUX1
+ * buf: syfs buffer
+ */
+static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
+{
+	acpi_status status;
+	union acpi_object arg = {
+		ACPI_TYPE_INTEGER
+	};
+	struct acpi_object_list args = {
+		1, &arg
+	};
+	int temp;
+
+	if (index != 0 && index != 1)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
+				       NULL, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	if ((index && value < temp) || (!index && value > temp))
+		return -EINVAL;
+
+	arg.integer.value = value;
+	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
+				       &args, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* do we need to check the return value of SAX0/SAX1 ? */
+
+	return 0;
+}
+
+#define to_intel_menlow_attr(_attr)	\
+	container_of(_attr, struct intel_menlow_attribute, attr)
+
+static ssize_t aux0_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 0, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux1_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 1, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux0_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 0, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+static ssize_t aux1_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 1, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+/* BIOS can enable/disable the thermal user application in dabney platform */
+#define BIOS_ENABLED "\\_TZ.GSTS"
+static ssize_t bios_enabled_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	acpi_status status;
+	unsigned long bios_enabled;
+
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
+}
+
+static int intel_menlow_add_one_attribute(char *name, int mode, void *show,
+					  void *store, struct device *dev,
+					  acpi_handle handle)
+{
+	struct intel_menlow_attribute *attr;
+	int result;
+
+	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->attr.attr.name = name;
+	attr->attr.attr.mode = mode;
+	attr->attr.show = show;
+	attr->attr.store = store;
+	attr->device = dev;
+	attr->handle = handle;
+
+	result = device_create_file(dev, &attr->attr);
+	if (result)
+		return result;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_add_tail(&attr->node, &intel_menlow_attr_list);
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return 0;
+}
+
+static acpi_status
+intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
+			     void *context, void **rv)
+{
+	acpi_status status;
+	acpi_handle dummy;
+	struct thermal_zone_device *thermal;
+	int result;
+
+	result = acpi_bus_get_private_data(handle, (void **)&thermal);
+	if (result)
+		return 0;
+
+	/* _TZ must have the AUX0/1 methods */
+	status = acpi_get_handle(handle, GET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux0", 0644,
+						aux0_show, aux0_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	status = acpi_get_handle(handle, GET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux1", 0644,
+						aux1_show, aux1_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	/*
+	 * create the "dabney_enabled" attribute which means the user app
+	 * should be loaded or not
+	 */
+
+	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
+						bios_enabled_show, NULL,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+      not_found:
+	if (status == AE_NOT_FOUND)
+		return AE_OK;
+	else
+		return status;
+}
+
+static void intel_menlow_unregister_sensor(void)
+{
+	struct intel_menlow_attribute *pos, *next;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_for_each_entry_safe(pos, next, &intel_menlow_attr_list, node) {
+		list_del(&pos->node);
+		device_remove_file(pos->device, &pos->attr);
+		kfree(pos);
+	}
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return;
+}
+
+static int __init intel_menlow_module_init(void)
+{
+	int result = -ENODEV;
+	acpi_status status;
+	unsigned long enable;
+
+	if (acpi_disabled)
+		return result;
+
+	/* Looking for the \_TZ.GSTS method */
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
+	if (ACPI_FAILURE(status) || !enable)
+		return -ENODEV;
+
+	/* Looking for ACPI device MEM0 with hardware id INT0002 */
+	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
+	if (result)
+		return result;
+
+	/* Looking for sensors in each ACPI thermal zone */
+	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX,
+				     intel_menlow_register_sensor, NULL, NULL);
+	if (ACPI_FAILURE(status))
+		result = -ENODEV;
+
+	return 0;
+}
+
+static void __exit intel_menlow_module_exit(void)
+{
+	acpi_bus_unregister_driver(&intel_menlow_memory_driver);
+	intel_menlow_unregister_sensor();
+}
+
+module_init(intel_menlow_module_init);
+module_exit(intel_menlow_module_exit);
Index: linux-2.6/drivers/misc/Kconfig
===================================================================
--- linux-2.6.orig/drivers/misc/Kconfig
+++ linux-2.6/drivers/misc/Kconfig
@@ -232,4 +232,14 @@ config ATMEL_SSC
 
 	  If unsure, say N.
 
+config INTEL_MENLOW
+        tristate "Thermal Management driver for Intel menlow platform"
+        depends on ACPI_THERMAL
+	default n
+        ---help---
+          ACPI thermal management enhancement driver on
+	  Intel Menlow platform.
+
+          If you are not sure, say N here.
+
 endif # MISC_DEVICES
Index: linux-2.6/drivers/misc/Makefile
===================================================================
--- linux-2.6.orig/drivers/misc/Makefile
+++ linux-2.6/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP)	+= sony-laptop
 obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
 obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
 obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
+obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Sam Ravnborg]

>  
> +config INTEL_MENLOW
> +        tristate "Thermal Management driver for Intel menlow platform"
> +        depends on ACPI_THERMAL
> +	default n

default n is not needed - it is default.

	Sam

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Randy Dunlap]

On Thu, 17 Jan 2008 15:51:17 +0800 Zhang Rui wrote:

> From: Thomas Sujith <sujith.thomas@intel.com>
> 
> Intel menlow platform specific driver for thermal management.
> 
> Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/misc/Kconfig        |   10 
>  drivers/misc/Makefile       |    1 
>  drivers/misc/intel_menlow.c |  527 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 538 insertions(+)
> 
> Index: linux-2.6/drivers/misc/intel_menlow.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/drivers/misc/intel_menlow.c
> @@ -0,0 +1,527 @@
> +/*
> +*  intel_menlow.c - Intel menlow Driver for thermal management extension
> +*
> +*  Copyright (C) 2008 Intel Corp
> +*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> +*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +*
> +*  This driver creates the sys I/F for programming the sensors.
> +*  It also implements the driver for intel menlow memory controller (hardware
> +*  id is INT0002) which makes use of the platform specific ACPI methods
> +*  to get/set bandwidth.
> +*/
> +

> +static int
> +memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
> +			     unsigned long *max_state)

Don't put 'static int' (return type etc.) on a line by itself.
That format is not wanted in Linux.   (many places here)

> +{
> +	struct acpi_device *device = cdev->devdata;
> +	acpi_handle handle = device->handle;
> +	unsigned long value;
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +	acpi_status status = AE_OK;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
> +	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
> +				       &arg_list, &value);
> +	if (ACPI_FAILURE(status))
> +		return -EFAULT;
> +
> +	*max_state = value - 1;
> +	return 0;
> +}
> +
> +
> +/*
> + * Memory Device Management
> + */
> +static int intel_menlow_memory_add(struct acpi_device *device)
> +{
> +	int result = -ENODEV;
> +	acpi_status status = AE_OK;
> +	acpi_handle dummy;
> +	struct thermal_cooling_device *cdev;
> +
> +	if (!device)
> +		return -EINVAL;
> +
> +	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto end;
> +
> +	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto end;
> +
> +	cdev = thermal_cooling_device_register("Memory controller", device,
> +					       &memory_cooling_ops);
> +	acpi_driver_data(device) = cdev;
> +	if (!cdev)
> +		result = -ENODEV;
> +	else {
> +		result =
> +		    sysfs_create_link(&device->dev.kobj, &cdev->device.kobj,
> +				      "thermal_cooling");
> +		if (result)
> +			goto end;
> +		result =
> +		    sysfs_create_link(&cdev->device.kobj, &device->dev.kobj,
> +				      "device");
> +		if (result)
> +			goto end;
> +	}
> +
> +      end:

Labels should begin in column 0 or 1.  Only.

> +	return result;
> +}
> +
> +
> +const static struct acpi_device_id intel_menlow_memory_ids[] = {
> +	{"INT0002", 0},
> +	{"", 0},

or just	{} for the terminating entry, right?

> +};

> +/*
> + * sensor_get_auxtrip
> + * -----------------
> + * get the current auxtrip value from sensor through proprietory ACPI methods
> + * name: Thermalzone name
> + * auxtype : AUX0/AUX1
> + * buf: syfs buffer
> + */

It would be Good to use kernel-doc format if someone is going to
add function documentation at all...  (ref. multiple places)

See Documentation/kernel-doc-nano-HOWTO.txt .

> +static int sensor_get_auxtrip(acpi_handle handle, int index, int *value)
> +{
> +	acpi_status status;
> +
> +	if ((index != 0 && index != 1) || !value)
> +		return -EINVAL;
> +
> +	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> +				       NULL, (unsigned long *)value);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +

> +static acpi_status
> +intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> +			     void *context, void **rv)
> +{
> +	acpi_status status;
> +	acpi_handle dummy;
> +	struct thermal_zone_device *thermal;
> +	int result;
> +
> +	result = acpi_bus_get_private_data(handle, (void **)&thermal);
> +	if (result)
> +		return 0;
> +
> +	/* _TZ must have the AUX0/1 methods */
> +	status = acpi_get_handle(handle, GET_AUX0, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	status = acpi_get_handle(handle, SET_AUX0, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	result = intel_menlow_add_one_attribute("aux0", 0644,
> +						aux0_show, aux0_store,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> +	status = acpi_get_handle(handle, GET_AUX1, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	status = acpi_get_handle(handle, SET_AUX1, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	result = intel_menlow_add_one_attribute("aux1", 0644,
> +						aux1_show, aux1_store,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> +	/*
> +	 * create the "dabney_enabled" attribute which means the user app
> +	 * should be loaded or not
> +	 */
> +
> +	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> +						bios_enabled_show, NULL,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> +      not_found:

Don't indent labels so much.  It hides them.

> +	if (status == AE_NOT_FOUND)
> +		return AE_OK;
> +	else
> +		return status;
> +}

> Index: linux-2.6/drivers/misc/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/misc/Kconfig
> +++ linux-2.6/drivers/misc/Kconfig
> @@ -232,4 +232,14 @@ config ATMEL_SSC
>  
>  	  If unsure, say N.
>  
> +config INTEL_MENLOW
> +        tristate "Thermal Management driver for Intel menlow platform"
> +        depends on ACPI_THERMAL
> +	default n

Documentation/CodingStyle has info on Kconfig style.
Please read & use it and be more careful.

> +        ---help---
> +          ACPI thermal management enhancement driver on
> +	  Intel Menlow platform.
> +
> +          If you are not sure, say N here.
> +
>  endif # MISC_DEVICES


---
~Randy

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Len Brown]

> > +static int
> > +memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
> > +			     unsigned long *max_state)
> 
> Don't put 'static int' (return type etc.) on a line by itself.
> That format is not wanted in Linux.   (many places here)

if checkpatch.pl and Lindent are happey (which they are not, totally)
then I'm satisifed witht he Linux style of the code.

Randy, if you feel strongly about this this style nit,
then change Lindent to implement it -- nagging
developers about trivia is a waste of everybody's time.

thanks,
-Len

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Randy Dunlap]

Len Brown wrote:
>>> +static int
>>> +memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
>>> +			     unsigned long *max_state)
>> Don't put 'static int' (return type etc.) on a line by itself.
>> That format is not wanted in Linux.   (many places here)
> 
> if checkpatch.pl and Lindent are happey (which they are not, totally)
> then I'm satisifed witht he Linux style of the code.
> 
> Randy, if you feel strongly about this this style nit,
> then change Lindent to implement it -- nagging
> developers about trivia is a waste of everybody's time.
> 
> thanks,
> -Len

http://marc.info/?l=linux-kernel&m=105451996829576&w=2

Thanks.

-- 
~Randy

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Zhang Rui]

Hi, Randy and Sam,

> > +             if (result)
> > +                     goto end;
> > +     }
> > +
> > +      end:
Hah, Lindent always does this for me,
> Labels should begin in column 0 or 1.  Only.
yes, and checkpatch.pl begins to complain about this
after using Lindent...
I think we should change one of them, :)

> > +     return result;
> > +}
> > +
> > +
> > +const static struct acpi_device_id intel_menlow_memory_ids[] = {
> > +     {"INT0002", 0},
> > +     {"", 0},
> 
> or just {} for the terminating entry, right?
Yes, I use this just to be consistent with other ACPI device drivers.

Thanks for your comments, please review the patch below,

From: Thomas Sujith <sujith.thomas@intel.com>

Intel menlow platform specific driver for thermal management extension.

Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/misc/Kconfig        |    9 
 drivers/misc/Makefile       |    1 
 drivers/misc/intel_menlow.c |  520 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 530 insertions(+)

Index: linux-2.6/drivers/misc/intel_menlow.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/misc/intel_menlow.c
@@ -0,0 +1,520 @@
+/*
+*  intel_menlow.c - Intel menlow Driver for thermal management extension
+*
+*  Copyright (C) 2008 Intel Corp
+*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
+*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
+*  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+*
+*  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; version 2 of the License.
+*
+*  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.,
+*  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+*
+* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+*
+*  This driver creates the sys I/F for programming the sensors.
+*  It also implements the driver for intel menlow memory controller (hardware
+*  id is INT0002) which makes use of the platform specific ACPI methods
+*  to get/set bandwidth.
+*/
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/pm.h>
+
+#include <linux/thermal.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Thomas Sujith");
+MODULE_AUTHOR("Zhang Rui");
+MODULE_DESCRIPTION("Intel Menlow platform specific driver");
+MODULE_LICENSE("GPL");
+
+/*
+ * Memory controller device control
+ */
+
+#define MEMORY_GET_BANDWIDTH "GTHS"
+#define MEMORY_SET_BANDWIDTH "STHS"
+#define MEMORY_ARG_CUR_BANDWIDTH 1
+#define MEMORY_ARG_MAX_BANDWIDTH 0
+
+static int memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
+					unsigned long *max_state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	*max_state = value - 1;
+	return 0;
+}
+
+static int memory_get_max_bandwidth(struct thermal_cooling_device *cdev,
+				    char *buf)
+{
+	unsigned long value;
+	if (memory_get_int_max_bandwidth(cdev, &value))
+		return -EINVAL;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int memory_get_cur_bandwidth(struct thermal_cooling_device *cdev,
+				    char *buf)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_CUR_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
+				    unsigned int state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+	int temp;
+	unsigned long max_state;
+
+	if (memory_get_int_max_bandwidth(cdev, &max_state))
+		return -EFAULT;
+
+	if (max_state < 0 || state > max_state)
+		return -EINVAL;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = state;
+
+	status =
+	    acpi_evaluate_integer(handle, MEMORY_SET_BANDWIDTH, &arg_list,
+				  (unsigned long *)&temp);
+
+	printk(KERN_INFO
+	       "Bandwidth value was %d: status is %d\n", state, status);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return 0;
+}
+
+static struct thermal_cooling_device_ops memory_cooling_ops = {
+	.get_max_state = memory_get_max_bandwidth,
+	.get_cur_state = memory_get_cur_bandwidth,
+	.set_cur_state = memory_set_cur_bandwidth,
+};
+
+/*
+ * Memory Device Management
+ */
+static int intel_menlow_memory_add(struct acpi_device *device)
+{
+	int result = -ENODEV;
+	acpi_status status = AE_OK;
+	acpi_handle dummy;
+	struct thermal_cooling_device *cdev;
+
+	if (!device)
+		return -EINVAL;
+
+	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	cdev = thermal_cooling_device_register("Memory controller", device,
+					       &memory_cooling_ops);
+	acpi_driver_data(device) = cdev;
+	if (!cdev)
+		result = -ENODEV;
+	else {
+		result =
+		    sysfs_create_link(&device->dev.kobj, &cdev->device.kobj,
+				      "thermal_cooling");
+		if (result)
+			goto end;
+		result =
+		    sysfs_create_link(&cdev->device.kobj, &device->dev.kobj,
+				      "device");
+		if (result)
+			goto end;
+	}
+
+ end:
+	return result;
+}
+
+static int intel_menlow_memory_remove(struct acpi_device *device, int type)
+{
+	struct thermal_cooling_device *cdev = acpi_driver_data(device);
+
+	if (!device || !cdev)
+		return -EINVAL;
+
+	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+	sysfs_remove_link(&cdev->device.kobj, "device");
+	thermal_cooling_device_unregister(cdev);
+
+	return 0;
+}
+
+const static struct acpi_device_id intel_menlow_memory_ids[] = {
+	{"INT0002", 0},
+	{"", 0},
+};
+
+static struct acpi_driver intel_menlow_memory_driver = {
+	.name = "intel_menlow_thermal_control",
+	.ids = intel_menlow_memory_ids,
+	.ops = {
+		.add = intel_menlow_memory_add,
+		.remove = intel_menlow_memory_remove,
+		},
+};
+
+/*
+ * Sensor control on menlow platform
+ */
+
+#define THERMAL_AUX0 0
+#define THERMAL_AUX1 1
+#define GET_AUX0 "GAX0"
+#define GET_AUX1 "GAX1"
+#define SET_AUX0 "SAX0"
+#define SET_AUX1 "SAX1"
+
+struct intel_menlow_attribute {
+	struct device_attribute attr;
+	struct device *device;
+	acpi_handle handle;
+	struct list_head node;
+};
+
+static LIST_HEAD(intel_menlow_attr_list);
+static DEFINE_MUTEX(intel_menlow_attr_lock);
+
+/*
+ * sensor_get_auxtrip - get the current auxtrip value from sensor
+ * @name: Thermalzone name
+ * @auxtype : AUX0/AUX1
+ * @buf: syfs buffer
+ */
+static int sensor_get_auxtrip(acpi_handle handle, int index, int *value)
+{
+	acpi_status status;
+
+	if ((index != 0 && index != 1) || !value)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
+				       NULL, (unsigned long *)value);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
+/*
+ * sensor_set_auxtrip - set the new auxtrip value to sensor
+ * @name: Thermalzone name
+ * @auxtype : AUX0/AUX1
+ * @buf: syfs buffer
+ */
+static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
+{
+	acpi_status status;
+	union acpi_object arg = {
+		ACPI_TYPE_INTEGER
+	};
+	struct acpi_object_list args = {
+		1, &arg
+	};
+	int temp;
+
+	if (index != 0 && index != 1)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
+				       NULL, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	if ((index && value < temp) || (!index && value > temp))
+		return -EINVAL;
+
+	arg.integer.value = value;
+	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
+				       &args, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* do we need to check the return value of SAX0/SAX1 ? */
+
+	return 0;
+}
+
+#define to_intel_menlow_attr(_attr)	\
+	container_of(_attr, struct intel_menlow_attribute, attr)
+
+static ssize_t aux0_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 0, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux1_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 1, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux0_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 0, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+static ssize_t aux1_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 1, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+/* BIOS can enable/disable the thermal user application in dabney platform */
+#define BIOS_ENABLED "\\_TZ.GSTS"
+static ssize_t bios_enabled_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	acpi_status status;
+	unsigned long bios_enabled;
+
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
+}
+
+static int intel_menlow_add_one_attribute(char *name, int mode, void *show,
+					  void *store, struct device *dev,
+					  acpi_handle handle)
+{
+	struct intel_menlow_attribute *attr;
+	int result;
+
+	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->attr.attr.name = name;
+	attr->attr.attr.mode = mode;
+	attr->attr.show = show;
+	attr->attr.store = store;
+	attr->device = dev;
+	attr->handle = handle;
+
+	result = device_create_file(dev, &attr->attr);
+	if (result)
+		return result;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_add_tail(&attr->node, &intel_menlow_attr_list);
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return 0;
+}
+
+static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
+						void *context, void **rv)
+{
+	acpi_status status;
+	acpi_handle dummy;
+	struct thermal_zone_device *thermal;
+	int result;
+
+	result = acpi_bus_get_private_data(handle, (void **)&thermal);
+	if (result)
+		return 0;
+
+	/* _TZ must have the AUX0/1 methods */
+	status = acpi_get_handle(handle, GET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux0", 0644,
+						aux0_show, aux0_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	status = acpi_get_handle(handle, GET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux1", 0644,
+						aux1_show, aux1_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	/*
+	 * create the "dabney_enabled" attribute which means the user app
+	 * should be loaded or not
+	 */
+
+	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
+						bios_enabled_show, NULL,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+ not_found:
+	if (status == AE_NOT_FOUND)
+		return AE_OK;
+	else
+		return status;
+}
+
+static void intel_menlow_unregister_sensor(void)
+{
+	struct intel_menlow_attribute *pos, *next;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_for_each_entry_safe(pos, next, &intel_menlow_attr_list, node) {
+		list_del(&pos->node);
+		device_remove_file(pos->device, &pos->attr);
+		kfree(pos);
+	}
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return;
+}
+
+static int __init intel_menlow_module_init(void)
+{
+	int result = -ENODEV;
+	acpi_status status;
+	unsigned long enable;
+
+	if (acpi_disabled)
+		return result;
+
+	/* Looking for the \_TZ.GSTS method */
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
+	if (ACPI_FAILURE(status) || !enable)
+		return -ENODEV;
+
+	/* Looking for ACPI device MEM0 with hardware id INT0002 */
+	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
+	if (result)
+		return result;
+
+	/* Looking for sensors in each ACPI thermal zone */
+	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX,
+				     intel_menlow_register_sensor, NULL, NULL);
+	if (ACPI_FAILURE(status))
+		result = -ENODEV;
+
+	return 0;
+}
+
+static void __exit intel_menlow_module_exit(void)
+{
+	acpi_bus_unregister_driver(&intel_menlow_memory_driver);
+	intel_menlow_unregister_sensor();
+}
+
+module_init(intel_menlow_module_init);
+module_exit(intel_menlow_module_exit);
Index: linux-2.6/drivers/misc/Kconfig
===================================================================
--- linux-2.6.orig/drivers/misc/Kconfig
+++ linux-2.6/drivers/misc/Kconfig
@@ -232,4 +232,13 @@ config ATMEL_SSC
 
 	  If unsure, say N.
 
+config INTEL_MENLOW
+	tristate "Thermal Management driver for Intel menlow platform"
+	depends on ACPI_THERMAL
+	---help---
+	  ACPI thermal management enhancement driver on
+	  Intel Menlow platform.
+
+	  If unsure, say N.
+
 endif # MISC_DEVICES
Index: linux-2.6/drivers/misc/Makefile
===================================================================
--- linux-2.6.orig/drivers/misc/Makefile
+++ linux-2.6/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP)	+= sony-laptop
 obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
 obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
 obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
+obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Li Zefan]

Zhang Rui 写道:
> Hi, Randy and Sam,
> 
>>> +             if (result)
>>> +                     goto end;acpi_bus_get_private_data
>>> +     }
>>> +
>>> +      end:
> Hah, Lindent always does this for me,
>> Labels should begin in column 0 or 1.  Only.
> yes, and checkpatch.pl begins to complain about this
> after using Lindent...
> I think we should change one of them, :)
> 
>>> +     return result;
>>> +}
>>> +
>>> +
>>> +const static struct acpi_device_id intel_menlow_memory_ids[] = {
>>> +     {"INT0002", 0},
>>> +     {"", 0},
>> or just {} for the terminating entry, right?
> Yes, I use this just to be consistent with other ACPI device drivers.
> 
> Thanks for your comments, please review the patch below,
> 
> From: Thomas Sujith <sujith.thomas@intel.com>
> 
> Intel menlow platform specific driver for thermal management extension.
> 
> Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/misc/Kconfig        |    9 
>  drivers/misc/Makefile       |    1 
>  drivers/misc/intel_menlow.c |  520 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 530 insertions(+)
> 
> Index: linux-2.6/drivers/misc/intel_menlow.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/drivers/misc/intel_menlow.c
> @@ -0,0 +1,520 @@
> +/*
> +*  intel_menlow.c - Intel menlow Driver for thermal management extension
> +*
> +*  Copyright (C) 2008 Intel Corp
> +*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> +*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> +*  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +*
> +*  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; version 2 of the License.
> +*
> +*  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.,
> +*  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> +*
> +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +*
> +*  This driver creates the sys I/F for programming the sensors.
> +*  It also implements the driver for intel menlow memory controller (hardware
> +*  id is INT0002) which makes use of the platform specific ACPI methods
> +*  to get/set bandwidth.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/pci.h>
> +#include <linux/pm.h>
> +
> +#include <linux/thermal.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +
> +MODULE_AUTHOR("Thomas Sujith");
> +MODULE_AUTHOR("Zhang Rui");
> +MODULE_DESCRIPTION("Intel Menlow platform specific driver");
> +MODULE_LICENSE("GPL");
> +
> +/*
> + * Memory controller device control
> + */
> +
> +#define MEMORY_GET_BANDWIDTH "GTHS"
> +#define MEMORY_SET_BANDWIDTH "STHS"
> +#define MEMORY_ARG_CUR_BANDWIDTH 1
> +#define MEMORY_ARG_MAX_BANDWIDTH 0
> +
> +static int memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
> +					unsigned long *max_state)
> +{
> +	struct acpi_device *device = cdev->devdata;
> +	acpi_handle handle = device->handle;
> +	unsigned long value;
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +	acpi_status status = AE_OK;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
> +	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
> +				       &arg_list, &value);
> +	if (ACPI_FAILURE(status))
> +		return -EFAULT;
> +
> +	*max_state = value - 1;
> +	return 0;
> +}
> +
> +static int memory_get_max_bandwidth(struct thermal_cooling_device *cdev,
> +				    char *buf)
> +{
> +	unsigned long value;
> +	if (memory_get_int_max_bandwidth(cdev, &value))
> +		return -EINVAL;
> +
> +	return sprintf(buf, "%ld\n", value);
> +}
> +

<...>

> +static int memory_get_cur_bandwidth(struct thermal_cooling_device *cdev,
> +				    char *buf)
> +{
> +	struct acpi_device *device = cdev->devdata;
> +	acpi_handle handle = device->handle;
> +	unsigned long value;
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +	acpi_status status = AE_OK;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = MEMORY_ARG_CUR_BANDWIDTH;
> +	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
> +				       &arg_list, &value);
> +	if (ACPI_FAILURE(status))
> +		return -EFAULT;
> +
> +	return sprintf(buf, "%ld\n", value);
> +}
> +
> +static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
> +				    unsigned int state)
> +{
> +	struct acpi_device *device = cdev->devdata;
> +	acpi_handle handle = device->handle;
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +	acpi_status status;
> +	int temp;
> +	unsigned long max_state;
> +
> +	if (memory_get_int_max_bandwidth(cdev, &max_state))
> +		return -EFAULT;
> +
> +	if (max_state < 0 || state > max_state)
> +		return -EINVAL;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = state;
> +
> +	status =
> +	    acpi_evaluate_integer(handle, MEMORY_SET_BANDWIDTH, &arg_list,
> +				  (unsigned long *)&temp);
> +
> +	printk(KERN_INFO
> +	       "Bandwidth value was %d: status is %d\n", state, status);
> +	if (ACPI_FAILURE(status))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static struct thermal_cooling_device_ops memory_cooling_ops = {
> +	.get_max_state = memory_get_max_bandwidth,
> +	.get_cur_state = memory_get_cur_bandwidth,
> +	.set_cur_state = memory_set_cur_bandwidth,
> +};
> +

<...>

> +/*
> + * Memory Device Management
> + */
> +static int intel_menlow_memory_add(struct acpi_device *device)
> +{
> +	int result = -ENODEV;
> +	acpi_status status = AE_OK;
> +	acpi_handle dummy;
> +	struct thermal_cooling_device *cdev;
> +
> +	if (!device)
> +		return -EINVAL;
> +
> +	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto end;
> +
> +	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto end;
> +
> +	cdev = thermal_cooling_device_register("Memory controller", device,
> +					       &memory_cooling_ops);
> +	acpi_driver_data(device) = cdev;
> +	if (!cdev)
> +		result = -ENODEV;
> +	else {
> +		result =
> +		    sysfs_create_link(&device->dev.kobj, &cdev->device.kobj,
> +				      "thermal_cooling");
> +		if (result)
> +			goto end;
> +		result =
> +		    sysfs_create_link(&cdev->device.kobj, &device->dev.kobj,
> +				      "device");
> +		if (result)
> +			goto end;

This if condition is needless.

> +	}
> +
> + end:
> +	return result;

No need to call thermal_cooling_device_unregister() and sysfs_remove_link() in
some error routines ?

> +}
> +
> +static int intel_menlow_memory_remove(struct acpi_device *device, int type)
> +{
> +	struct thermal_cooling_device *cdev = acpi_driver_data(device);
> +
> +	if (!device || !cdev)
> +		return -EINVAL;
> +
> +	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> +	sysfs_remove_link(&cdev->device.kobj, "device");
> +	thermal_cooling_device_unregister(cdev);
> +
> +	return 0;
> +}
> +
> +const static struct acpi_device_id intel_menlow_memory_ids[] = {
> +	{"INT0002", 0},
> +	{"", 0},
> +};
> +
> +static struct acpi_driver intel_menlow_memory_driver = {
> +	.name = "intel_menlow_thermal_control",
> +	.ids = intel_menlow_memory_ids,
> +	.ops = {
> +		.add = intel_menlow_memory_add,
> +		.remove = intel_menlow_memory_remove,
> +		},
> +};
> +
> +/*
> + * Sensor control on menlow platform
> + */
> +
> +#define THERMAL_AUX0 0
> +#define THERMAL_AUX1 1
> +#define GET_AUX0 "GAX0"
> +#define GET_AUX1 "GAX1"
> +#define SET_AUX0 "SAX0"
> +#define SET_AUX1 "SAX1"
> +
> +struct intel_menlow_attribute {
> +	struct device_attribute attr;
> +	struct device *device;
> +	acpi_handle handle;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(intel_menlow_attr_list);
> +static DEFINE_MUTEX(intel_menlow_attr_lock);
> +
> +/*
> + * sensor_get_auxtrip - get the current auxtrip value from sensor
> + * @name: Thermalzone name
> + * @auxtype : AUX0/AUX1
> + * @buf: syfs buffer
> + */
> +static int sensor_get_auxtrip(acpi_handle handle, int index, int *value)
> +{
> +	acpi_status status;
> +
> +	if ((index != 0 && index != 1) || !value)
> +		return -EINVAL;
> +
> +	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> +				       NULL, (unsigned long *)value);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +

<...>

> +/*
> + * sensor_set_auxtrip - set the new auxtrip value to sensor
> + * @name: Thermalzone name
> + * @auxtype : AUX0/AUX1
> + * @buf: syfs buffer
> + */
> +static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> +{
> +	acpi_status status;
> +	union acpi_object arg = {
> +		ACPI_TYPE_INTEGER
> +	};
> +	struct acpi_object_list args = {
> +		1, &arg
> +	};
> +	int temp;
> +
> +	if (index != 0 && index != 1)
> +		return -EINVAL;
> +
> +	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> +				       NULL, (unsigned long *)&temp);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +	if ((index && value < temp) || (!index && value > temp))
> +		return -EINVAL;
> +
> +	arg.integer.value = value;
> +	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> +				       &args, (unsigned long *)&temp);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	/* do we need to check the return value of SAX0/SAX1 ? */
> +
> +	return 0;
> +}
> +
> +#define to_intel_menlow_attr(_attr)	\
> +	container_of(_attr, struct intel_menlow_attribute, attr)
> +
> +static ssize_t aux0_show(struct device *dev,
> +			 struct device_attribute *dev_attr, char *buf)
> +{
> +	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> +	int value;
> +	int result;
> +
> +	result = sensor_get_auxtrip(attr->handle, 0, &value);
> +
> +	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
> +}
> +
> +static ssize_t aux1_show(struct device *dev,
> +			 struct device_attribute *dev_attr, char *buf)
> +{
> +	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> +	int value;
> +	int result;
> +
> +	result = sensor_get_auxtrip(attr->handle, 1, &value);
> +
> +	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
> +}
> +
> +static ssize_t aux0_store(struct device *dev,
> +			  struct device_attribute *dev_attr,
> +			  const char *buf, size_t count)
> +{
> +	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> +	int value;
> +	int result;
> +
> +	/*Sanity check; should be a positive integer */
> +	if (!sscanf(buf, "%d", &value))
> +		return -EINVAL;
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	result = sensor_set_auxtrip(attr->handle, 0, CELSIUS_TO_KELVIN(value));
> +	return result ? result : count;
> +}
> +

<...>

> +static ssize_t aux1_store(struct device *dev,
> +			  struct device_attribute *dev_attr,
> +			  const char *buf, size_t count)
> +{
> +	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
> +	int value;
> +	int result;
> +
> +	/*Sanity check; should be a positive integer */
> +	if (!sscanf(buf, "%d", &value))
> +		return -EINVAL;
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	result = sensor_set_auxtrip(attr->handle, 1, CELSIUS_TO_KELVIN(value));
> +	return result ? result : count;
> +}
> +
> +/* BIOS can enable/disable the thermal user application in dabney platform */
> +#define BIOS_ENABLED "\\_TZ.GSTS"
> +static ssize_t bios_enabled_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	acpi_status status;
> +	unsigned long bios_enabled;
> +
> +	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> +}
> +
> +static int intel_menlow_add_one_attribute(char *name, int mode, void *show,
> +					  void *store, struct device *dev,
> +					  acpi_handle handle)
> +{
> +	struct intel_menlow_attribute *attr;
> +	int result;
> +
> +	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> +	if (!attr)
> +		return -ENOMEM;
> +
> +	attr->attr.attr.name = name;
> +	attr->attr.attr.mode = mode;
> +	attr->attr.show = show;
> +	attr->attr.store = store;
> +	attr->device = dev;
> +	attr->handle = handle;
> +
> +	result = device_create_file(dev, &attr->attr);
> +	if (result)
> +		return result;
> +
> +	mutex_lock(&intel_menlow_attr_lock);
> +	list_add_tail(&attr->node, &intel_menlow_attr_list);
> +	mutex_unlock(&intel_menlow_attr_lock);
> +
> +	return 0;
> +}
> +

<...>

> +static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> +						void *context, void **rv)
> +{
> +	acpi_status status;
> +	acpi_handle dummy;
> +	struct thermal_zone_device *thermal;
> +	int result;
> +
> +	result = acpi_bus_get_private_data(handle, (void **)&thermal);
> +	if (result)
> +		return 0;
> +
> +	/* _TZ must have the AUX0/1 methods */
> +	status = acpi_get_handle(handle, GET_AUX0, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	status = acpi_get_handle(handle, SET_AUX0, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	result = intel_menlow_add_one_attribute("aux0", 0644,
> +						aux0_show, aux0_store,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> +	status = acpi_get_handle(handle, GET_AUX1, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	status = acpi_get_handle(handle, SET_AUX1, &dummy);
> +	if (ACPI_FAILURE(status))
> +		goto not_found;
> +
> +	result = intel_menlow_add_one_attribute("aux1", 0644,
> +						aux1_show, aux1_store,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> +	/*
> +	 * create the "dabney_enabled" attribute which means the user app
> +	 * should be loaded or not
> +	 */
> +
> +	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> +						bios_enabled_show, NULL,
> +						&thermal->device, handle);
> +	if (result)
> +		return AE_ERROR;
> +
> + not_found:
> +	if (status == AE_NOT_FOUND)
> +		return AE_OK;
> +	else
> +		return status;
> +}
> +
> +static void intel_menlow_unregister_sensor(void)
> +{
> +	struct intel_menlow_attribute *pos, *next;
> +
> +	mutex_lock(&intel_menlow_attr_lock);
> +	list_for_each_entry_safe(pos, next, &intel_menlow_attr_list, node) {
> +		list_del(&pos->node);
> +		device_remove_file(pos->device, &pos->attr);
> +		kfree(pos);
> +	}
> +	mutex_unlock(&intel_menlow_attr_lock);
> +
> +	return;
> +}
> +
> +static int __init intel_menlow_module_init(void)
> +{
> +	int result = -ENODEV;
> +	acpi_status status;
> +	unsigned long enable;
> +
> +	if (acpi_disabled)
> +		return result;
> +
> +	/* Looking for the \_TZ.GSTS method */
> +	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
> +	if (ACPI_FAILURE(status) || !enable)
> +		return -ENODEV;
> +
> +	/* Looking for ACPI device MEM0 with hardware id INT0002 */
> +	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
> +	if (result)
> +		return result;
> +
> +	/* Looking for sensors in each ACPI thermal zone */
> +	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
> +				     ACPI_UINT32_MAX,
> +				     intel_menlow_register_sensor, NULL, NULL);
> +	if (ACPI_FAILURE(status))
> +		result = -ENODEV;
> +

It seems to me this should be 'return -ENODEV;'

You can just eliminate variable result, because result will be
-ENODEV only.

> +	return 0;
> +}
> +
> +static void __exit intel_menlow_module_exit(void)
> +{
> +	acpi_bus_unregister_driver(&intel_menlow_memory_driver);
> +	intel_menlow_unregister_sensor();
> +}
> +
> +module_init(intel_menlow_module_init);
> +module_exit(intel_menlow_module_exit);
> Index: linux-2.6/drivers/misc/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/misc/Kconfig
> +++ linux-2.6/drivers/misc/Kconfig
> @@ -232,4 +232,13 @@ config ATMEL_SSC
>  
>  	  If unsure, say N.
>  
> +config INTEL_MENLOW
> +	tristate "Thermal Management driver for Intel menlow platform"
> +	depends on ACPI_THERMAL
> +	---help---
> +	  ACPI thermal management enhancement driver on
> +	  Intel Menlow platform.
> +
> +	  If unsure, say N.
> +
>  endif # MISC_DEVICES
> Index: linux-2.6/drivers/misc/Makefile
> ===================================================================
> --- linux-2.6.orig/drivers/misc/Makefile
> +++ linux-2.6/drivers/misc/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP)	+= sony-laptop
>  obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>  obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
>  obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
> +obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o
> 
> 

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 9/10] introduce intel_menlow platform specific driver

[Thomas, Sujith]

> -----Original Message-----
> From: Li Zefan [mailto:lizf@cn.fujitsu.com]
> Sent: Friday, January 18, 2008 9:25 AM
> To: Zhang, Rui
> Cc: Randy Dunlap; sam@ravnborg.org; lenb@kernel.org; linux-
> acpi@vger.kernel.org; linux-pm@lists.linux-foundation.org; linux-
> kernel@vger.kernel.org; Thomas, Sujith
> Subject: Re: [PATCH 9/10] introduce intel_menlow platform specific driver
> 
> Zhang Rui 写道:
> > Hi, Randy and Sam,
> >
> >>> +             if (result)
> >>> +                     goto end;acpi_bus_get_private_data
> >>> +     }
> >>> +
> >>> +      end:
> > Hah, Lindent always does this for me,
> >> Labels should begin in column 0 or 1.  Only.
> > yes, and checkpatch.pl begins to complain about this
> > after using Lindent...
> > I think we should change one of them, :)
> >
> >>> +     return result;
> >>> +}
> >>> +
> >>> +
> >>> +const static struct acpi_device_id intel_menlow_memory_ids[] = {
> >>> +     {"INT0002", 0},
> >>> +     {"", 0},
> >> or just {} for the terminating entry, right?
> > Yes, I use this just to be consistent with other ACPI device drivers.
> >
> > Thanks for your comments, please review the patch below,
> >
> > From: Thomas Sujith <sujith.thomas@intel.com>
> >
> > Intel menlow platform specific driver for thermal management extension.
> >
> > Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/misc/Kconfig        |    9
> >  drivers/misc/Makefile       |    1
> >  drivers/misc/intel_menlow.c |  520
> ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 530 insertions(+)
> >
> > Index: linux-2.6/drivers/misc/intel_menlow.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/drivers/misc/intel_menlow.c
> > @@ -0,0 +1,520 @@
> > +/*
> > +*  intel_menlow.c - Intel menlow Driver for thermal management
> extension
> > +*
> > +*  Copyright (C) 2008 Intel Corp
> > +*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> > +*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> > +*
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +*
> > +*  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; version 2 of the License.
> > +*
> > +*  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.,
> > +*  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> > +*
> > +*
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +*
> > +*  This driver creates the sys I/F for programming the sensors.
> > +*  It also implements the driver for intel menlow memory controller
> (hardware
> > +*  id is INT0002) which makes use of the platform specific ACPI methods
> > +*  to get/set bandwidth.
> > +*/
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/pci.h>
> > +#include <linux/pm.h>
> > +
> > +#include <linux/thermal.h>
> > +#include <acpi/acpi_bus.h>
> > +#include <acpi/acpi_drivers.h>
> > +
> > +MODULE_AUTHOR("Thomas Sujith");
> > +MODULE_AUTHOR("Zhang Rui");
> > +MODULE_DESCRIPTION("Intel Menlow platform specific driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +/*
> > + * Memory controller device control
> > + */
> > +
> > +#define MEMORY_GET_BANDWIDTH "GTHS"
> > +#define MEMORY_SET_BANDWIDTH "STHS"
> > +#define MEMORY_ARG_CUR_BANDWIDTH 1
> > +#define MEMORY_ARG_MAX_BANDWIDTH 0
> > +
> > +static int memory_get_int_max_bandwidth(struct thermal_cooling_device
> *cdev,
> > +					unsigned long *max_state)
> > +{
> > +	struct acpi_device *device = cdev->devdata;
> > +	acpi_handle handle = device->handle;
> > +	unsigned long value;
> > +	struct acpi_object_list arg_list;
> > +	union acpi_object arg;
> > +	acpi_status status = AE_OK;
> > +
> > +	arg_list.count = 1;
> > +	arg_list.pointer = &arg;
> > +	arg.type = ACPI_TYPE_INTEGER;
> > +	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
> > +	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
> > +				       &arg_list, &value);
> > +	if (ACPI_FAILURE(status))
> > +		return -EFAULT;
> > +
> > +	*max_state = value - 1;
> > +	return 0;
> > +}
> > +
> > +static int memory_get_max_bandwidth(struct thermal_cooling_device
> *cdev,
> > +				    char *buf)
> > +{
> > +	unsigned long value;
> > +	if (memory_get_int_max_bandwidth(cdev, &value))
> > +		return -EINVAL;
> > +
> > +	return sprintf(buf, "%ld\n", value);
> > +}
> > +
> 
> <...>
> 
> > +static int memory_get_cur_bandwidth(struct thermal_cooling_device
> *cdev,
> > +				    char *buf)
> > +{
> > +	struct acpi_device *device = cdev->devdata;
> > +	acpi_handle handle = device->handle;
> > +	unsigned long value;
> > +	struct acpi_object_list arg_list;
> > +	union acpi_object arg;
> > +	acpi_status status = AE_OK;
> > +
> > +	arg_list.count = 1;
> > +	arg_list.pointer = &arg;
> > +	arg.type = ACPI_TYPE_INTEGER;
> > +	arg.integer.value = MEMORY_ARG_CUR_BANDWIDTH;
> > +	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
> > +				       &arg_list, &value);
> > +	if (ACPI_FAILURE(status))
> > +		return -EFAULT;
> > +
> > +	return sprintf(buf, "%ld\n", value);
> > +}
> > +
> > +static int memory_set_cur_bandwidth(struct thermal_cooling_device
> *cdev,
> > +				    unsigned int state)
> > +{
> > +	struct acpi_device *device = cdev->devdata;
> > +	acpi_handle handle = device->handle;
> > +	struct acpi_object_list arg_list;
> > +	union acpi_object arg;
> > +	acpi_status status;
> > +	int temp;
> > +	unsigned long max_state;
> > +
> > +	if (memory_get_int_max_bandwidth(cdev, &max_state))
> > +		return -EFAULT;
> > +
> > +	if (max_state < 0 || state > max_state)
> > +		return -EINVAL;
> > +
> > +	arg_list.count = 1;
> > +	arg_list.pointer = &arg;
> > +	arg.type = ACPI_TYPE_INTEGER;
> > +	arg.integer.value = state;
> > +
> > +	status =
> > +	    acpi_evaluate_integer(handle, MEMORY_SET_BANDWIDTH, &arg_list,
> > +				  (unsigned long *)&temp);
> > +
> > +	printk(KERN_INFO
> > +	       "Bandwidth value was %d: status is %d\n", state, status);
> > +	if (ACPI_FAILURE(status))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct thermal_cooling_device_ops memory_cooling_ops = {
> > +	.get_max_state = memory_get_max_bandwidth,
> > +	.get_cur_state = memory_get_cur_bandwidth,
> > +	.set_cur_state = memory_set_cur_bandwidth,
> > +};
> > +
> 
> <...>
> 
> > +/*
> > + * Memory Device Management
> > + */
> > +static int intel_menlow_memory_add(struct acpi_device *device)
> > +{
> > +	int result = -ENODEV;
> > +	acpi_status status = AE_OK;
> > +	acpi_handle dummy;
> > +	struct thermal_cooling_device *cdev;
> > +
> > +	if (!device)
> > +		return -EINVAL;
> > +
> > +	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH,
> &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto end;
> > +
> > +	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH,
> &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto end;
> > +
> > +	cdev = thermal_cooling_device_register("Memory controller", device,
> > +					       &memory_cooling_ops);
> > +	acpi_driver_data(device) = cdev;
> > +	if (!cdev)
> > +		result = -ENODEV;
> > +	else {
> > +		result =
> > +		    sysfs_create_link(&device->dev.kobj, &cdev->device.kobj,
> > +				      "thermal_cooling");
> > +		if (result)
> > +			goto end;
> > +		result =
> > +		    sysfs_create_link(&cdev->device.kobj, &device->dev.kobj,
> > +				      "device");
> > +		if (result)
> > +			goto end;
> 
> This if condition is needless.
I agree. This will be taken care of.
> 
> > +	}
> > +
> > + end:
> > +	return result;
> 
> No need to call thermal_cooling_device_unregister() and
> sysfs_remove_link() in
> some error routines ?
Yes, I agree. We need to call those on error routines
> 
> > +}
> > +
> > +static int intel_menlow_memory_remove(struct acpi_device *device, int
> type)
> > +{
> > +	struct thermal_cooling_device *cdev = acpi_driver_data(device);
> > +
> > +	if (!device || !cdev)
> > +		return -EINVAL;
> > +
> > +	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> > +	sysfs_remove_link(&cdev->device.kobj, "device");
> > +	thermal_cooling_device_unregister(cdev);
> > +
> > +	return 0;
> > +}
> > +
> > +const static struct acpi_device_id intel_menlow_memory_ids[] = {
> > +	{"INT0002", 0},
> > +	{"", 0},
> > +};
> > +
> > +static struct acpi_driver intel_menlow_memory_driver = {
> > +	.name = "intel_menlow_thermal_control",
> > +	.ids = intel_menlow_memory_ids,
> > +	.ops = {
> > +		.add = intel_menlow_memory_add,
> > +		.remove = intel_menlow_memory_remove,
> > +		},
> > +};
> > +
> > +/*
> > + * Sensor control on menlow platform
> > + */
> > +
> > +#define THERMAL_AUX0 0
> > +#define THERMAL_AUX1 1
> > +#define GET_AUX0 "GAX0"
> > +#define GET_AUX1 "GAX1"
> > +#define SET_AUX0 "SAX0"
> > +#define SET_AUX1 "SAX1"
> > +
> > +struct intel_menlow_attribute {
> > +	struct device_attribute attr;
> > +	struct device *device;
> > +	acpi_handle handle;
> > +	struct list_head node;
> > +};
> > +
> > +static LIST_HEAD(intel_menlow_attr_list);
> > +static DEFINE_MUTEX(intel_menlow_attr_lock);
> > +
> > +/*
> > + * sensor_get_auxtrip - get the current auxtrip value from sensor
> > + * @name: Thermalzone name
> > + * @auxtype : AUX0/AUX1
> > + * @buf: syfs buffer
> > + */
> > +static int sensor_get_auxtrip(acpi_handle handle, int index, int
> *value)
> > +{
> > +	acpi_status status;
> > +
> > +	if ((index != 0 && index != 1) || !value)
> > +		return -EINVAL;
> > +
> > +	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
> > +				       NULL, (unsigned long *)value);
> > +	if (ACPI_FAILURE(status))
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> 
> <...>
> 
> > +/*
> > + * sensor_set_auxtrip - set the new auxtrip value to sensor
> > + * @name: Thermalzone name
> > + * @auxtype : AUX0/AUX1
> > + * @buf: syfs buffer
> > + */
> > +static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
> > +{
> > +	acpi_status status;
> > +	union acpi_object arg = {
> > +		ACPI_TYPE_INTEGER
> > +	};
> > +	struct acpi_object_list args = {
> > +		1, &arg
> > +	};
> > +	int temp;
> > +
> > +	if (index != 0 && index != 1)
> > +		return -EINVAL;
> > +
> > +	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
> > +				       NULL, (unsigned long *)&temp);
> > +	if (ACPI_FAILURE(status))
> > +		return -EIO;
> > +	if ((index && value < temp) || (!index && value > temp))
> > +		return -EINVAL;
> > +
> > +	arg.integer.value = value;
> > +	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
> > +				       &args, (unsigned long *)&temp);
> > +	if (ACPI_FAILURE(status))
> > +		return -EIO;
> > +
> > +	/* do we need to check the return value of SAX0/SAX1 ? */
> > +
> > +	return 0;
> > +}
> > +
> > +#define to_intel_menlow_attr(_attr)	\
> > +	container_of(_attr, struct intel_menlow_attribute, attr)
> > +
> > +static ssize_t aux0_show(struct device *dev,
> > +			 struct device_attribute *dev_attr, char *buf)
> > +{
> > +	struct intel_menlow_attribute *attr =
> to_intel_menlow_attr(dev_attr);
> > +	int value;
> > +	int result;
> > +
> > +	result = sensor_get_auxtrip(attr->handle, 0, &value);
> > +
> > +	return result ? result : sprintf(buf, "%lu",
> KELVIN_TO_CELSIUS(value));
> > +}
> > +
> > +static ssize_t aux1_show(struct device *dev,
> > +			 struct device_attribute *dev_attr, char *buf)
> > +{
> > +	struct intel_menlow_attribute *attr =
> to_intel_menlow_attr(dev_attr);
> > +	int value;
> > +	int result;
> > +
> > +	result = sensor_get_auxtrip(attr->handle, 1, &value);
> > +
> > +	return result ? result : sprintf(buf, "%lu",
> KELVIN_TO_CELSIUS(value));
> > +}
> > +
> > +static ssize_t aux0_store(struct device *dev,
> > +			  struct device_attribute *dev_attr,
> > +			  const char *buf, size_t count)
> > +{
> > +	struct intel_menlow_attribute *attr =
> to_intel_menlow_attr(dev_attr);
> > +	int value;
> > +	int result;
> > +
> > +	/*Sanity check; should be a positive integer */
> > +	if (!sscanf(buf, "%d", &value))
> > +		return -EINVAL;
> > +
> > +	if (value < 0)
> > +		return -EINVAL;
> > +
> > +	result = sensor_set_auxtrip(attr->handle, 0,
> CELSIUS_TO_KELVIN(value));
> > +	return result ? result : count;
> > +}
> > +
> 
> <...>
> 
> > +static ssize_t aux1_store(struct device *dev,
> > +			  struct device_attribute *dev_attr,
> > +			  const char *buf, size_t count)
> > +{
> > +	struct intel_menlow_attribute *attr =
> to_intel_menlow_attr(dev_attr);
> > +	int value;
> > +	int result;
> > +
> > +	/*Sanity check; should be a positive integer */
> > +	if (!sscanf(buf, "%d", &value))
> > +		return -EINVAL;
> > +
> > +	if (value < 0)
> > +		return -EINVAL;
> > +
> > +	result = sensor_set_auxtrip(attr->handle, 1,
> CELSIUS_TO_KELVIN(value));
> > +	return result ? result : count;
> > +}
> > +
> > +/* BIOS can enable/disable the thermal user application in dabney
> platform */
> > +#define BIOS_ENABLED "\\_TZ.GSTS"
> > +static ssize_t bios_enabled_show(struct device *dev,
> > +				 struct device_attribute *attr, char *buf)
> > +{
> > +	acpi_status status;
> > +	unsigned long bios_enabled;
> > +
> > +	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL,
> &bios_enabled);
> > +	if (ACPI_FAILURE(status))
> > +		return -ENODEV;
> > +
> > +	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> > +}
> > +
> > +static int intel_menlow_add_one_attribute(char *name, int mode, void
> *show,
> > +					  void *store, struct device *dev,
> > +					  acpi_handle handle)
> > +{
> > +	struct intel_menlow_attribute *attr;
> > +	int result;
> > +
> > +	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> > +	if (!attr)
> > +		return -ENOMEM;
> > +
> > +	attr->attr.attr.name = name;
> > +	attr->attr.attr.mode = mode;
> > +	attr->attr.show = show;
> > +	attr->attr.store = store;
> > +	attr->device = dev;
> > +	attr->handle = handle;
> > +
> > +	result = device_create_file(dev, &attr->attr);
> > +	if (result)
> > +		return result;
> > +
> > +	mutex_lock(&intel_menlow_attr_lock);
> > +	list_add_tail(&attr->node, &intel_menlow_attr_list);
> > +	mutex_unlock(&intel_menlow_attr_lock);
> > +
> > +	return 0;
> > +}
> > +
> 
> <...>
> 
> > +static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32
> lvl,
> > +						void *context, void **rv)
> > +{
> > +	acpi_status status;
> > +	acpi_handle dummy;
> > +	struct thermal_zone_device *thermal;
> > +	int result;
> > +
> > +	result = acpi_bus_get_private_data(handle, (void **)&thermal);
> > +	if (result)
> > +		return 0;
> > +
> > +	/* _TZ must have the AUX0/1 methods */
> > +	status = acpi_get_handle(handle, GET_AUX0, &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto not_found;
> > +
> > +	status = acpi_get_handle(handle, SET_AUX0, &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto not_found;
> > +
> > +	result = intel_menlow_add_one_attribute("aux0", 0644,
> > +						aux0_show, aux0_store,
> > +						&thermal->device, handle);
> > +	if (result)
> > +		return AE_ERROR;
> > +
> > +	status = acpi_get_handle(handle, GET_AUX1, &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto not_found;
> > +
> > +	status = acpi_get_handle(handle, SET_AUX1, &dummy);
> > +	if (ACPI_FAILURE(status))
> > +		goto not_found;
> > +
> > +	result = intel_menlow_add_one_attribute("aux1", 0644,
> > +						aux1_show, aux1_store,
> > +						&thermal->device, handle);
> > +	if (result)
> > +		return AE_ERROR;
> > +
> > +	/*
> > +	 * create the "dabney_enabled" attribute which means the user app
> > +	 * should be loaded or not
> > +	 */
> > +
> > +	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
> > +						bios_enabled_show, NULL,
> > +						&thermal->device, handle);
> > +	if (result)
> > +		return AE_ERROR;
> > +
> > + not_found:
> > +	if (status == AE_NOT_FOUND)
> > +		return AE_OK;
> > +	else
> > +		return status;
> > +}
> > +
> > +static void intel_menlow_unregister_sensor(void)
> > +{
> > +	struct intel_menlow_attribute *pos, *next;
> > +
> > +	mutex_lock(&intel_menlow_attr_lock);
> > +	list_for_each_entry_safe(pos, next, &intel_menlow_attr_list, node) {
> > +		list_del(&pos->node);
> > +		device_remove_file(pos->device, &pos->attr);
> > +		kfree(pos);
> > +	}
> > +	mutex_unlock(&intel_menlow_attr_lock);
> > +
> > +	return;
> > +}
> > +
> > +static int __init intel_menlow_module_init(void)
> > +{
> > +	int result = -ENODEV;
> > +	acpi_status status;
> > +	unsigned long enable;
> > +
> > +	if (acpi_disabled)
> > +		return result;
> > +
> > +	/* Looking for the \_TZ.GSTS method */
> > +	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
> > +	if (ACPI_FAILURE(status) || !enable)
> > +		return -ENODEV;
> > +
> > +	/* Looking for ACPI device MEM0 with hardware id INT0002 */
> > +	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
> > +	if (result)
> > +		return result;
> > +
> > +	/* Looking for sensors in each ACPI thermal zone */
> > +	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
> > +				     ACPI_UINT32_MAX,
> > +				     intel_menlow_register_sensor, NULL, NULL);
> > +	if (ACPI_FAILURE(status))
> > +		result = -ENODEV;
> > +
> 
> It seems to me this should be 'return -ENODEV;'
> 
> You can just eliminate variable result, because result will be
> -ENODEV only.
I disagree. "result" is required to store the return value from acpi_bus_register_driver which may be different from ENODEV. Otherwise it's all -ENODEV.

:-Sujith
> 
> > +	return 0;
> > +}
> > +
> > +static void __exit intel_menlow_module_exit(void)
> > +{
> > +	acpi_bus_unregister_driver(&intel_menlow_memory_driver);
> > +	intel_menlow_unregister_sensor();
> > +}
> > +
> > +module_init(intel_menlow_module_init);
> > +module_exit(intel_menlow_module_exit);
> > Index: linux-2.6/drivers/misc/Kconfig
> > ===================================================================
> > --- linux-2.6.orig/drivers/misc/Kconfig
> > +++ linux-2.6/drivers/misc/Kconfig
> > @@ -232,4 +232,13 @@ config ATMEL_SSC
> >
> >  	  If unsure, say N.
> >
> > +config INTEL_MENLOW
> > +	tristate "Thermal Management driver for Intel menlow platform"
> > +	depends on ACPI_THERMAL
> > +	---help---
> > +	  ACPI thermal management enhancement driver on
> > +	  Intel Menlow platform.
> > +
> > +	  If unsure, say N.
> > +
> >  endif # MISC_DEVICES
> > Index: linux-2.6/drivers/misc/Makefile
> > ===================================================================
> > --- linux-2.6.orig/drivers/misc/Makefile
> > +++ linux-2.6/drivers/misc/Makefile
> > @@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP)	+= sony-laptop
> >  obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
> >  obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
> >  obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
> > +obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o
> >
> >

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Li Zefan]

>>> +static int __init intel_menlow_module_init(void)
>>> +{
>>> +	int result = -ENODEV;
>>> +	acpi_status status;
>>> +	unsigned long enable;
>>> +
>>> +	if (acpi_disabled)
>>> +		return result;
>>> +
>>> +	/* Looking for the \_TZ.GSTS method */
>>> +	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
>>> +	if (ACPI_FAILURE(status) || !enable)
>>> +		return -ENODEV;
>>> +
>>> +	/* Looking for ACPI device MEM0 with hardware id INT0002 */
>>> +	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
>>> +	if (result)
>>> +		return result;
>>> +
>>> +	/* Looking for sensors in each ACPI thermal zone */
>>> +	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
>>> +				     ACPI_UINT32_MAX,
>>> +				     intel_menlow_register_sensor, NULL, NULL);
>>> +	if (ACPI_FAILURE(status))
>>> +		result = -ENODEV;
>>> +
>> It seems to me this should be 'return -ENODEV;'
>>
>> You can just eliminate variable result, because result will be
>> -ENODEV only.
> I disagree. "result" is required to store the return value from acpi_bus_register_driver which may be different from ENODEV. Otherwise it's all -ENODEV.
> 
> :-Sujith

Indead, I overlooked it.

But the above 'result = -ENODEV;' should be 'return -ENODEV;', right?

>>> +	return 0;
>>> +}

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Christoph Hellwig]

On Thu, Jan 17, 2008 at 03:51:17PM +0800, Zhang Rui wrote:
> From: Thomas Sujith <sujith.thomas@intel.com>
> 
> Intel menlow platform specific driver for thermal management.
> 
> Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/misc/Kconfig        |   10 
>  drivers/misc/Makefile       |    1 
>  drivers/misc/intel_menlow.c |  527 ++++++++++++++++++++++++++++++++++++++++++++

Why is this in drivers/misc?  I don't have a thermal.h in mainline, but
if this is a new subsystem your adding care to create a directory under
drivers/ for it?

> +/*
> +*  intel_menlow.c - Intel menlow Driver for thermal management extension
> +*
> +*  Copyright (C) 2008 Intel Corp
> +*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> +*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>

Please add a whitespace before the * so they line up.

> +MODULE_AUTHOR("Thomas Sujith");
> +MODULE_AUTHOR("Zhang Rui");

I've never seen a driver with two MODULE_AUTHOR statements before.  Does
this actually work?  What does modinfo -F author say for your module?

RE: [PATCH 9/10] introduce intel_menlow platform specific driver

[Thomas, Sujith]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Monday, January 21, 2008 3:24 PM
> To: Zhang, Rui
> Cc: lenb@kernel.org; linux-acpi@vger.kernel.org; linux-pm@lists.linux-
> foundation.org; linux-kernel@vger.kernel.org; Thomas, Sujith
> Subject: Re: [PATCH 9/10] introduce intel_menlow platform specific
driver
> 
> On Thu, Jan 17, 2008 at 03:51:17PM +0800, Zhang Rui wrote:
> > From: Thomas Sujith <sujith.thomas@intel.com>
> >
> > Intel menlow platform specific driver for thermal management.
> >
> > Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/misc/Kconfig        |   10
> >  drivers/misc/Makefile       |    1
> >  drivers/misc/intel_menlow.c |  527
> ++++++++++++++++++++++++++++++++++++++++++++
> 
> Why is this in drivers/misc?  I don't have a thermal.h in mainline,
but
> if this is a new subsystem your adding care to create a directory
under
> drivers/ for it?
This patch series adds a new "thermal" subsystem and is rightly placed
under drivers/thermal/ . intel_menlow.c isn't fully a thermal driver. It
does all the platform specific stuff (thermal is one of them). It's not
fitting under any of the subsystems hence moved to drivers/misc
> 
> > +/*
> > +*  intel_menlow.c - Intel menlow Driver for thermal management
> extension
> > +*
> > +*  Copyright (C) 2008 Intel Corp
> > +*  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> > +*  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> 
> Please add a whitespace before the * so they line up.
Will take care of this.
> 
> > +MODULE_AUTHOR("Thomas Sujith");
> > +MODULE_AUTHOR("Zhang Rui");
> 
> I've never seen a driver with two MODULE_AUTHOR statements before.
Does
> this actually work?  What does modinfo -F author say for your module?
There is nothing wrong in giving 2 MODULE_AUTHORS. Both will be
displayed in the command modinfo -F author.

:-Sujith

RE: [PATCH 9/10] introduce intel_menlow platform specific driver

[Jan Engelhardt]

On Jan 21 2008 19:49, Thomas, Sujith wrote:
>>> 
>>> +MODULE_AUTHOR("Thomas Sujith");
>>> +MODULE_AUTHOR("Zhang Rui");
>> 
>> I've never seen a driver with two MODULE_AUTHOR statements before.
>> Does this actually work?  What does modinfo -F author say for your module?
>
>There is nothing wrong in giving 2 MODULE_AUTHORS. Both will be
>displayed in the command modinfo -F author.

Coding-wise, it is no problem. MODULE_* gets transformed into char
__mod_*line, so as long as you have the two MODULE_AUTHOR() macros on
different lines, it will work.

It is cool to actually see this working out-of-the-box with modinfo
too (even without the -F option). Could decouple a few AUTHOR lines.

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Len Brown]

On Monday 21 January 2008 04:53, Christoph Hellwig wrote:
> On Thu, Jan 17, 2008 at 03:51:17PM +0800, Zhang Rui wrote:
> > From: Thomas Sujith <sujith.thomas@intel.com>
> > 
> > Intel menlow platform specific driver for thermal management.
> > 
> > Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/misc/Kconfig        |   10 
> >  drivers/misc/Makefile       |    1 
> >  drivers/misc/intel_menlow.c |  527 ++++++++++++++++++++++++++++++++++++++++++++
> 
> Why is this in drivers/misc?  I don't have a thermal.h in mainline, but
> if this is a new subsystem your adding care to create a directory under
> drivers/ for it?

Hi Christoph,

I'm sure it was me who suggested putting intel_menlo.c under drivers/misc.

A while back I refused to let any new platform specific drivers under
drivers/acpi, and asked that the ones here already move out.
This is because they happne to use ACPI, but are not actually
part of the ACPI sub-system (any more than ATA is).
Also, I wanted the maintainer roles to be clear -- these
drivers have primary maintainers other than me, and those
guys are in the heroic, but tragic, business of writing
platform-specific drivers for systems with no documentation or vendor support.

drivers/misc/asus-laptop.c
drivers/misc/fujitsu-laptop.c
drivers/misc/msi-laptop.c
drivers/misc/sony-laptop.c
drivers/misc/thinkpad_acpi.c
drivers/misc/sony-laptop.c

intel_menlo.c fits in with this group, the only difference is that
its primary maintainers actually have documentation:-)

is there a better place than drivers/misc for this group?

thanks,
-Len

Re: [PATCH 9/10] introduce intel_menlow platform specific driver

[Zhang Rui]

On Mon, 2008-01-21 at 13:45 +0800, Li Zefan wrote:
> >>> +static int __init intel_menlow_module_init(void)
> >>> +{
> >>> +   int result = -ENODEV;
> >>> +   acpi_status status;
> >>> +   unsigned long enable;
> >>> +
> >>> +   if (acpi_disabled)
> >>> +           return result;
> >>> +
> >>> +   /* Looking for the \_TZ.GSTS method */
> >>> +   status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL,
> &enable);
> >>> +   if (ACPI_FAILURE(status) || !enable)
> >>> +           return -ENODEV;
> >>> +
> >>> +   /* Looking for ACPI device MEM0 with hardware id INT0002 */
> >>> +   result =
> acpi_bus_register_driver(&intel_menlow_memory_driver);
> >>> +   if (result)
> >>> +           return result;
> >>> +
> >>> +   /* Looking for sensors in each ACPI thermal zone */
> >>> +   status = acpi_walk_namespace(ACPI_TYPE_THERMAL,
> ACPI_ROOT_OBJECT,
> >>> +                                ACPI_UINT32_MAX,
> >>> +                                intel_menlow_register_sensor,
> NULL, NULL);
> >>> +   if (ACPI_FAILURE(status))
> >>> +           result = -ENODEV;
> >>> +
> >> It seems to me this should be 'return -ENODEV;'
> >>
> >> You can just eliminate variable result, because result will be
> >> -ENODEV only.
> > I disagree. "result" is required to store the return value from
> acpi_bus_register_driver which may be different from ENODEV. Otherwise
> it's all -ENODEV.
> >
> > :-Sujith
> 
> Indead, I overlooked it.
> 
> But the above 'result = -ENODEV;' should be 'return -ENODEV;', right?
Yes, please review the refreshed patches.

From: Thomas Sujith <sujith.thomas@intel.com>

Intel menlow platform specific driver for thermal management extension.

Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/misc/Kconfig        |    9 
 drivers/misc/Makefile       |    1 
 drivers/misc/intel_menlow.c |  526 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 536 insertions(+)

Index: linux-2.6/drivers/misc/intel_menlow.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/misc/intel_menlow.c
@@ -0,0 +1,526 @@
+/*
+ *  intel_menlow.c - Intel menlow Driver for thermal management extension
+ *
+ *  Copyright (C) 2008 Intel Corp
+ *  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
+ *  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  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; version 2 of the License.
+ *
+ *  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.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This driver creates the sys I/F for programming the sensors.
+ *  It also implements the driver for intel menlow memory controller (hardware
+ *  id is INT0002) which makes use of the platform specific ACPI methods
+ *  to get/set bandwidth.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/pm.h>
+
+#include <linux/thermal.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Thomas Sujith");
+MODULE_AUTHOR("Zhang Rui");
+MODULE_DESCRIPTION("Intel Menlow platform specific driver");
+MODULE_LICENSE("GPL");
+
+/*
+ * Memory controller device control
+ */
+
+#define MEMORY_GET_BANDWIDTH "GTHS"
+#define MEMORY_SET_BANDWIDTH "STHS"
+#define MEMORY_ARG_CUR_BANDWIDTH 1
+#define MEMORY_ARG_MAX_BANDWIDTH 0
+
+static int memory_get_int_max_bandwidth(struct thermal_cooling_device *cdev,
+					unsigned long *max_state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_MAX_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	*max_state = value - 1;
+	return 0;
+}
+
+static int memory_get_max_bandwidth(struct thermal_cooling_device *cdev,
+				    char *buf)
+{
+	unsigned long value;
+	if (memory_get_int_max_bandwidth(cdev, &value))
+		return -EINVAL;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int memory_get_cur_bandwidth(struct thermal_cooling_device *cdev,
+				    char *buf)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	unsigned long value;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status = AE_OK;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = MEMORY_ARG_CUR_BANDWIDTH;
+	status = acpi_evaluate_integer(handle, MEMORY_GET_BANDWIDTH,
+				       &arg_list, &value);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return sprintf(buf, "%ld\n", value);
+}
+
+static int memory_set_cur_bandwidth(struct thermal_cooling_device *cdev,
+				    unsigned int state)
+{
+	struct acpi_device *device = cdev->devdata;
+	acpi_handle handle = device->handle;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+	int temp;
+	unsigned long max_state;
+
+	if (memory_get_int_max_bandwidth(cdev, &max_state))
+		return -EFAULT;
+
+	if (max_state < 0 || state > max_state)
+		return -EINVAL;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = state;
+
+	status =
+	    acpi_evaluate_integer(handle, MEMORY_SET_BANDWIDTH, &arg_list,
+				  (unsigned long *)&temp);
+
+	printk(KERN_INFO
+	       "Bandwidth value was %d: status is %d\n", state, status);
+	if (ACPI_FAILURE(status))
+		return -EFAULT;
+
+	return 0;
+}
+
+static struct thermal_cooling_device_ops memory_cooling_ops = {
+	.get_max_state = memory_get_max_bandwidth,
+	.get_cur_state = memory_get_cur_bandwidth,
+	.set_cur_state = memory_set_cur_bandwidth,
+};
+
+/*
+ * Memory Device Management
+ */
+static int intel_menlow_memory_add(struct acpi_device *device)
+{
+	int result = -ENODEV;
+	acpi_status status = AE_OK;
+	acpi_handle dummy;
+	struct thermal_cooling_device *cdev;
+
+	if (!device)
+		return -EINVAL;
+
+	status = acpi_get_handle(device->handle, MEMORY_GET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	status = acpi_get_handle(device->handle, MEMORY_SET_BANDWIDTH, &dummy);
+	if (ACPI_FAILURE(status))
+		goto end;
+
+	cdev = thermal_cooling_device_register("Memory controller", device,
+					       &memory_cooling_ops);
+	acpi_driver_data(device) = cdev;
+	if (!cdev)
+		result = -ENODEV;
+	else {
+		result = sysfs_create_link(&device->dev.kobj,
+					&cdev->device.kobj, "thermal_cooling");
+		if (result)
+			goto unregister;
+
+		result = sysfs_create_link(&cdev->device.kobj,
+					&device->dev.kobj, "device");
+		if (result) {
+			sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+			goto unregister;
+		}
+	}
+
+ end:
+	return result;
+
+ unregister:
+	thermal_cooling_device_unregister(cdev);
+	return result;
+
+}
+
+static int intel_menlow_memory_remove(struct acpi_device *device, int type)
+{
+	struct thermal_cooling_device *cdev = acpi_driver_data(device);
+
+	if (!device || !cdev)
+		return -EINVAL;
+
+	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+	sysfs_remove_link(&cdev->device.kobj, "device");
+	thermal_cooling_device_unregister(cdev);
+
+	return 0;
+}
+
+const static struct acpi_device_id intel_menlow_memory_ids[] = {
+	{"INT0002", 0},
+	{"", 0},
+};
+
+static struct acpi_driver intel_menlow_memory_driver = {
+	.name = "intel_menlow_thermal_control",
+	.ids = intel_menlow_memory_ids,
+	.ops = {
+		.add = intel_menlow_memory_add,
+		.remove = intel_menlow_memory_remove,
+		},
+};
+
+/*
+ * Sensor control on menlow platform
+ */
+
+#define THERMAL_AUX0 0
+#define THERMAL_AUX1 1
+#define GET_AUX0 "GAX0"
+#define GET_AUX1 "GAX1"
+#define SET_AUX0 "SAX0"
+#define SET_AUX1 "SAX1"
+
+struct intel_menlow_attribute {
+	struct device_attribute attr;
+	struct device *device;
+	acpi_handle handle;
+	struct list_head node;
+};
+
+static LIST_HEAD(intel_menlow_attr_list);
+static DEFINE_MUTEX(intel_menlow_attr_lock);
+
+/*
+ * sensor_get_auxtrip - get the current auxtrip value from sensor
+ * @name: Thermalzone name
+ * @auxtype : AUX0/AUX1
+ * @buf: syfs buffer
+ */
+static int sensor_get_auxtrip(acpi_handle handle, int index, int *value)
+{
+	acpi_status status;
+
+	if ((index != 0 && index != 1) || !value)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX1 : GET_AUX0,
+				       NULL, (unsigned long *)value);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
+/*
+ * sensor_set_auxtrip - set the new auxtrip value to sensor
+ * @name: Thermalzone name
+ * @auxtype : AUX0/AUX1
+ * @buf: syfs buffer
+ */
+static int sensor_set_auxtrip(acpi_handle handle, int index, int value)
+{
+	acpi_status status;
+	union acpi_object arg = {
+		ACPI_TYPE_INTEGER
+	};
+	struct acpi_object_list args = {
+		1, &arg
+	};
+	int temp;
+
+	if (index != 0 && index != 1)
+		return -EINVAL;
+
+	status = acpi_evaluate_integer(handle, index ? GET_AUX0 : GET_AUX1,
+				       NULL, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	if ((index && value < temp) || (!index && value > temp))
+		return -EINVAL;
+
+	arg.integer.value = value;
+	status = acpi_evaluate_integer(handle, index ? SET_AUX1 : SET_AUX0,
+				       &args, (unsigned long *)&temp);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	/* do we need to check the return value of SAX0/SAX1 ? */
+
+	return 0;
+}
+
+#define to_intel_menlow_attr(_attr)	\
+	container_of(_attr, struct intel_menlow_attribute, attr)
+
+static ssize_t aux0_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 0, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux1_show(struct device *dev,
+			 struct device_attribute *dev_attr, char *buf)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	result = sensor_get_auxtrip(attr->handle, 1, &value);
+
+	return result ? result : sprintf(buf, "%lu", KELVIN_TO_CELSIUS(value));
+}
+
+static ssize_t aux0_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 0, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+static ssize_t aux1_store(struct device *dev,
+			  struct device_attribute *dev_attr,
+			  const char *buf, size_t count)
+{
+	struct intel_menlow_attribute *attr = to_intel_menlow_attr(dev_attr);
+	int value;
+	int result;
+
+	/*Sanity check; should be a positive integer */
+	if (!sscanf(buf, "%d", &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	result = sensor_set_auxtrip(attr->handle, 1, CELSIUS_TO_KELVIN(value));
+	return result ? result : count;
+}
+
+/* BIOS can enable/disable the thermal user application in dabney platform */
+#define BIOS_ENABLED "\\_TZ.GSTS"
+static ssize_t bios_enabled_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	acpi_status status;
+	unsigned long bios_enabled;
+
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &bios_enabled);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
+}
+
+static int intel_menlow_add_one_attribute(char *name, int mode, void *show,
+					  void *store, struct device *dev,
+					  acpi_handle handle)
+{
+	struct intel_menlow_attribute *attr;
+	int result;
+
+	attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->attr.attr.name = name;
+	attr->attr.attr.mode = mode;
+	attr->attr.show = show;
+	attr->attr.store = store;
+	attr->device = dev;
+	attr->handle = handle;
+
+	result = device_create_file(dev, &attr->attr);
+	if (result)
+		return result;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_add_tail(&attr->node, &intel_menlow_attr_list);
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return 0;
+}
+
+static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
+						void *context, void **rv)
+{
+	acpi_status status;
+	acpi_handle dummy;
+	struct thermal_zone_device *thermal;
+	int result;
+
+	result = acpi_bus_get_private_data(handle, (void **)&thermal);
+	if (result)
+		return 0;
+
+	/* _TZ must have the AUX0/1 methods */
+	status = acpi_get_handle(handle, GET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX0, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux0", 0644,
+						aux0_show, aux0_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	status = acpi_get_handle(handle, GET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	status = acpi_get_handle(handle, SET_AUX1, &dummy);
+	if (ACPI_FAILURE(status))
+		goto not_found;
+
+	result = intel_menlow_add_one_attribute("aux1", 0644,
+						aux1_show, aux1_store,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+	/*
+	 * create the "dabney_enabled" attribute which means the user app
+	 * should be loaded or not
+	 */
+
+	result = intel_menlow_add_one_attribute("bios_enabled", 0444,
+						bios_enabled_show, NULL,
+						&thermal->device, handle);
+	if (result)
+		return AE_ERROR;
+
+ not_found:
+	if (status == AE_NOT_FOUND)
+		return AE_OK;
+	else
+		return status;
+}
+
+static void intel_menlow_unregister_sensor(void)
+{
+	struct intel_menlow_attribute *pos, *next;
+
+	mutex_lock(&intel_menlow_attr_lock);
+	list_for_each_entry_safe(pos, next, &intel_menlow_attr_list, node) {
+		list_del(&pos->node);
+		device_remove_file(pos->device, &pos->attr);
+		kfree(pos);
+	}
+	mutex_unlock(&intel_menlow_attr_lock);
+
+	return;
+}
+
+static int __init intel_menlow_module_init(void)
+{
+	int result = -ENODEV;
+	acpi_status status;
+	unsigned long enable;
+
+	if (acpi_disabled)
+		return result;
+
+	/* Looking for the \_TZ.GSTS method */
+	status = acpi_evaluate_integer(NULL, BIOS_ENABLED, NULL, &enable);
+	if (ACPI_FAILURE(status) || !enable)
+		return -ENODEV;
+
+	/* Looking for ACPI device MEM0 with hardware id INT0002 */
+	result = acpi_bus_register_driver(&intel_menlow_memory_driver);
+	if (result)
+		return result;
+
+	/* Looking for sensors in each ACPI thermal zone */
+	status = acpi_walk_namespace(ACPI_TYPE_THERMAL, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX,
+				     intel_menlow_register_sensor, NULL, NULL);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return 0;
+}
+
+static void __exit intel_menlow_module_exit(void)
+{
+	acpi_bus_unregister_driver(&intel_menlow_memory_driver);
+	intel_menlow_unregister_sensor();
+}
+
+module_init(intel_menlow_module_init);
+module_exit(intel_menlow_module_exit);
Index: linux-2.6/drivers/misc/Kconfig
===================================================================
--- linux-2.6.orig/drivers/misc/Kconfig
+++ linux-2.6/drivers/misc/Kconfig
@@ -232,4 +232,13 @@ config ATMEL_SSC
 
 	  If unsure, say N.
 
+config INTEL_MENLOW
+	tristate "Thermal Management driver for Intel menlow platform"
+	depends on ACPI_THERMAL
+	---help---
+	  ACPI thermal management enhancement driver on
+	  Intel Menlow platform.
+
+	  If unsure, say N.
+
 endif # MISC_DEVICES
Index: linux-2.6/drivers/misc/Makefile
===================================================================
--- linux-2.6.orig/drivers/misc/Makefile
+++ linux-2.6/drivers/misc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_SONY_LAPTOP)	+= sony-laptop
 obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
 obj-$(CONFIG_FUJITSU_LAPTOP)	+= fujitsu-laptop.o
 obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
+obj-$(CONFIG_INTEL_MENLOW)	+= intel_menlow.o