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

[Randy Dunlap <randy.dunlap@oracle.com>]

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