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

[Thomas, Sujith]

> -----Original Message-----
> From: Li Zefan [mailto:lizf@xxxxxxxxxxxxxx]
> Sent: Friday, January 18, 2008 9:25 AM
> To: Zhang, Rui
> Cc: Randy Dunlap; sam@xxxxxxxxxxxx; lenb@xxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx>
> >
> > Intel menlow platform specific driver for thermal management extension.
> >
> > Signed-off-by: Thomas Sujith <sujith.thomas@xxxxxxxxx>
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> > +*  Copyright (C) 2008 Zhang Rui <rui.zhang@xxxxxxxxx>
> > +*
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +*
> > +*  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
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/