Re: [PATCH v2] ACPI video: poke display on lid open

Dmitry Torokhov
2007-06-17T04:08:36Z

Hi Daniel,

On Saturday 16 June 2007 20:03, Daniel Drake wrote:
> Many Dell laptops have the DSDT coded to power down the display when the lid
> is closed, and leave it off when it is opened.
>
> http://bugzilla.kernel.org/show_bug.cgi?id=5155
>
> Based on ideas from Len Brown and Dmitry Torokhov, this patch creates an input
> handler in the video driver which monitors for lid input events. When a lid
> open event is detected, the video driver reactivates the LCD.
>

Appears to be working on my Inspiron 8100. Couple of comments:

> +
> +static int lid_connect(struct input_handler *handler, struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct acpi_video_bus *video = handler->private;
> + int r;
> +
> + if (dev->id.product != ACPI_BUTTON_TYPE_LID ||
> + strcmp(dev->name, ACPI_BUTTON_DEVICE_NAME_LID) != 0)
> + return -ENODEV;
> +
> + if (video->lid_handle != NULL) {
> + printk(KERN_ERR PREFIX "trying to bind to multiple lids?\n");
> + return -ENODEV;

If we do not support multiple LIDs -EBUSY would be a better error code.

> + }
> +
> + video->lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> + if (!video->lid_handle)
> + return -ENOMEM;
> + video->lid_handle->dev = dev;
> + video->lid_handle->handler = handler;
> + video->lid_handle->private = video;

Need to setup handler->name, otherwise "cat /proc/bus/input/devices" looks
"interesting".

> +
> +static void lid_event(struct input_handle *handle, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct acpi_video_device *dev, *tmp;
> + struct acpi_video_bus *video = handle->private;
> +
> + if (type != EV_SW || value != 0)
> + return;
> +
> + list_for_each_entry_safe(dev, tmp, &video->video_device_list, entry) {

Safe from what? I don't see anything altering list state in the body
of this loop. Where's list locking? Also, once input core locking is
in place handler_>event will be called under a spinlock with interrupts
off. Can acpi_video_device_set_state be used in this case?

Below is a small cleanup patch you may want to fold into yours.

But I guess more important question if this is a good solution overall.
My box already generates video bus events when I close and open the lid
so maybe video.c should just call acpi_video__device_set_state() from
acpi_video_bus_notify()?

--
Dmitry

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

drivers/acpi/video.c | 24 +++++++++++++++---------
1 files changed, 15 insertions(+), 9 deletions(-)

Index: work/drivers/acpi/video.c
===================================================================
--- work.orig/drivers/acpi/video.c
+++ work/drivers/acpi/video.c
@@ -1737,6 +1737,7 @@ static int lid_connect(struct input_hand
const struct input_device_id *id)
{
struct acpi_video_bus *video = handler->private;
+ struct input_handle *lid_handle;
int r;

if (dev->id.product != ACPI_BUTTON_TYPE_LID ||
@@ -1748,18 +1749,22 @@ static int lid_connect(struct input_hand
return -ENODEV;
}

- video->lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
- if (!video->lid_handle)
+ lid_handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
+ if (!lid_handle)
return -ENOMEM;
- video->lid_handle->dev = dev;
- video->lid_handle->handler = handler;
- video->lid_handle->private = video;

- r = input_register_handle(video->lid_handle);
+ video->lid_handle = lid_handle;
+
+ lid_handle->dev = dev;
+ lid_handle->handler = handler;
+ lid_handle->name = "acpi-video";
+ lid_handle->private = video;
+
+ r = input_register_handle(lid_handle);
if (r)
goto err;

- r = input_open_device(video->lid_handle);
+ r = input_open_device(lid_handle);
if (r)
goto err_unregister;

@@ -1777,9 +1782,10 @@ err:
static void lid_disconnect(struct input_handle *handle)
{
struct acpi_video_bus *video = handle->private;
- input_unregister_handle(handle);
+
input_close_device(handle);
- kfree(video->lid_handle);
+ input_unregister_handle(handle);
+ kfree(handle);
video->lid_handle = NULL;
}