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@xxxxxxx> --- 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; } - To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
