[PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[Zhang Rui]

From: Zhang Rui <rui.zhang@intel.com>

Register ACPI video device as thermal cooling devices as they may be listed
in _TZD method and the backlight control can be used for throttling.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Thomas Sujith <sujith.thomas@intel.com>
---
 drivers/acpi/video.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -34,6 +34,7 @@
 #include <linux/seq_file.h>
 #include <linux/input.h>
 #include <linux/backlight.h>
+#include <linux/thermal.h>
 #include <linux/video_output.h>
 #include <asm/uaccess.h>
 
@@ -179,6 +180,7 @@ struct acpi_video_device {
 	struct acpi_device *dev;
 	struct acpi_video_device_brightness *brightness;
 	struct backlight_device *backlight;
+	struct thermal_cooling_device *cdev;
 	struct output_device *output_dev;
 };
 
@@ -334,6 +336,54 @@ static struct output_properties acpi_out
 	.set_state = acpi_video_output_set,
 	.get_status = acpi_video_output_get,
 };
+
+
+/* thermal cooling device callbacks */
+static int video_get_max_state(struct thermal_cooling_device *cdev, char *buf)
+{
+	struct acpi_device *device = cdev->devdata;
+	struct acpi_video_device *video = acpi_driver_data(device);
+
+	return sprintf(buf, "%d\n", video->brightness->count - 3);
+}
+
+static int video_get_cur_state(struct thermal_cooling_device *cdev, char *buf)
+{
+	struct acpi_device *device = cdev->devdata;
+	struct acpi_video_device *video = acpi_driver_data(device);
+	unsigned long level;
+	int state;
+
+	acpi_video_device_lcd_get_level_current(video, &level);
+	for (state = 2; state < video->brightness->count; state++)
+		if (level == video->brightness->levels[state])
+			return sprintf(buf, "%d\n",
+				       video->brightness->count - state - 1);
+
+	return -EINVAL;
+}
+
+static int
+video_set_cur_state(struct thermal_cooling_device *cdev, unsigned int state)
+{
+	struct acpi_device *device = cdev->devdata;
+	struct acpi_video_device *video = acpi_driver_data(device);
+	int level;
+
+	if ( state >= video->brightness->count - 2)
+		return -EINVAL;
+
+	state = video->brightness->count - state;
+	level = video->brightness->levels[state -1];
+	return acpi_video_device_lcd_set_level(video, level);
+}
+
+static struct thermal_cooling_device_ops video_cooling_ops = {
+	.get_max_state = video_get_max_state,
+	.get_cur_state = video_get_cur_state,
+	.set_cur_state = video_set_cur_state,
+};
+
 /* --------------------------------------------------------------------------
                                Video Management
    -------------------------------------------------------------------------- */
@@ -653,6 +703,7 @@ static void acpi_video_device_find_cap(s
 
 	if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
 		unsigned long tmp;
+		int result;
 		static int count = 0;
 		char *name;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
@@ -666,8 +717,25 @@ static void acpi_video_device_find_cap(s
 		device->backlight->props.max_brightness = max_level;
 		device->backlight->props.brightness = (int)tmp;
 		backlight_update_status(device->backlight);
-
 		kfree(name);
+
+		device->cdev = thermal_cooling_device_register("LCD",
+					device->dev, &video_cooling_ops);
+		if (device->cdev) {
+			printk(KERN_INFO PREFIX
+				"%s is registered as cooling_device%d\n",
+				device->dev->dev.bus_id, device->cdev->id);
+			result = sysfs_create_link(&device->dev->dev.kobj,
+					  &device->cdev->device.kobj,
+					  "thermal_cooling");
+			if (result)
+				printk(KERN_ERR PREFIX "Create sysfs link\n");
+			result = sysfs_create_link(&device->cdev->device.kobj,
+					  &device->dev->dev.kobj,
+					  "device");
+                        if (result)
+				printk(KERN_ERR PREFIX "Create sysfs link\n");
+		}
 	}
 	if (device->cap._DCS && device->cap._DSS){
 		static int count = 0;
@@ -1729,6 +1797,14 @@ static int acpi_video_bus_put_one_device
 					    ACPI_DEVICE_NOTIFY,
 					    acpi_video_device_notify);
 	backlight_device_unregister(device->backlight);
+	if (device->cdev) {
+		sysfs_remove_link(&device->dev->dev.kobj,
+				  "thermal_cooling");
+		sysfs_remove_link(&device->cdev->device.kobj,
+				  "device");
+		thermal_cooling_device_unregister(device->cdev);
+		device->cdev = NULL;
+	}
 	video_output_unregister(device->output_dev);
 
 	return 0;

Re: [PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[Matthew Garrett]

On Thu, Jan 17, 2008 at 03:51:22PM +0800, Zhang Rui wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> 
> Register ACPI video device as thermal cooling devices as they may be listed
> in _TZD method and the backlight control can be used for throttling.

I'm worried to some extent about how ungeneric this is. A thermal zone 
may cover any devices, not just processor and video ones. Perhaps this 
should be added to the acpi_device struct instead, and then let 
individual drivers implement whatever callbacks are appropriate?

> +static int
> +video_set_cur_state(struct thermal_cooling_device *cdev, unsigned int state)
> +{
> +	struct acpi_device *device = cdev->devdata;
> +	struct acpi_video_device *video = acpi_driver_data(device);
> +	int level;
> +
> +	if ( state >= video->brightness->count - 2)
> +		return -EINVAL;
> +
> +	state = video->brightness->count - state;
> +	level = video->brightness->levels[state -1];
> +	return acpi_video_device_lcd_set_level(video, level);

This all seems like duplication of the backlight interface?

(Speaking of which - Len, what's happening with my patches to the video 
driver? Some feedback would be nice)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[Zhang Rui]

Hi, Mattew,

thanks for your comments.

On Thu, 2008-01-17 at 20:24 +0800, Matthew Garrett wrote:
> On Thu, Jan 17, 2008 at 03:51:22PM +0800, Zhang Rui wrote:
> > From: Zhang Rui <rui.zhang@intel.com>
> >
> > Register ACPI video device as thermal cooling devices as they may be
> listed
> > in _TZD method and the backlight control can be used for throttling.
> 
> I'm worried to some extent about how ungeneric this is. A thermal zone
> may cover any devices, not just processor and video ones.
>  Perhaps this
> should be added to the acpi_device struct instead, and then let
> individual drivers implement whatever callbacks are appropriate?
Yes, we can do this. But it's kinda early to add the
thermal_cooling_device struct to the acpi_device struct now.
I don't think any other individual drivers know whether they can or how
to throttle the device except processor, fan and lcd currently.

Just like I don't think lcd should be used for ACPI thermal management
before I saw it is listed in _TZD and intel_menlow requires to throttle
it when overheating, why not let the individual drivers implement the
callbacks if there is clearly a request to do this.
And we can add this to the generic acpi_device struct then if this is a
common feature for all ACPI devices.
> 
> > +static int
> > +video_set_cur_state(struct thermal_cooling_device *cdev, unsigned
> int state)
> > +{
> > +     struct acpi_device *device = cdev->devdata;
> > +     struct acpi_video_device *video = acpi_driver_data(device);
> > +     int level;
> > +
> > +     if ( state >= video->brightness->count - 2)
> > +             return -EINVAL;
> > +
> > +     state = video->brightness->count - state;
> > +     level = video->brightness->levels[state -1];
> > +     return acpi_video_device_lcd_set_level(video, level);
> 
> This all seems like duplication of the backlight interface?
Well, you're right.
But in order to throttle the lcd, this is reasonable, right?

Thanks,
Rui

Re: [PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[Matthew Garrett]

On Fri, Jan 18, 2008 at 09:31:40AM +0800, Zhang Rui wrote:

> Just like I don't think lcd should be used for ACPI thermal management
> before I saw it is listed in _TZD and intel_menlow requires to throttle
> it when overheating, why not let the individual drivers implement the
> callbacks if there is clearly a request to do this.
> And we can add this to the generic acpi_device struct then if this is a
> common feature for all ACPI devices.

It'll probably never be common for all ACPI devices, but it's already 
required for three types. I think that's a strong argument for making it 
generic.

> Well, you're right.
> But in order to throttle the lcd, this is reasonable, right?

Moving the common code into its own routine and then calling that from 
each of the others would probably work.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 6/10] ACPI: register ACPI Video LCD as generic thermal cooling device

[Zhang Rui]

Hi, Matthew,

On Fri, 2008-01-18 at 09:42 +0800, Matthew Garrett wrote:
> On Fri, Jan 18, 2008 at 09:31:40AM +0800, Zhang Rui wrote:
> 
> > Just like I don't think lcd should be used for ACPI thermal
> management
> > before I saw it is listed in _TZD and intel_menlow requires to
> throttle
> > it when overheating, why not let the individual drivers implement
> the
> > callbacks if there is clearly a request to do this.
> > And we can add this to the generic acpi_device struct then if this
> is a
> > common feature for all ACPI devices.
> 
> It'll probably never be common for all ACPI devices,
I agree.
>  but it's already
> required for three types. I think that's a strong argument for making
> it generic.
I don't think it's worth doing this as it's only the common feature for
three ACPI devices.
> > Well, you're right.
> > But in order to throttle the lcd, this is reasonable, right?
> 
> Moving the common code into its own routine and then calling that from
> each of the others would probably work.
Yes.
I can send an on top patch if the patch "Rationalise ACPI backlight
implementation" is applied.

Thanks,
Rui