Re: [PATCH v2 1/2] acpi: video: add function to support unregisterbacklight interface

[Aaron Lu]

On 05/31/2013 12:10 PM, Lee, Chun-Yi wrote:
> There have some situation we unregister whole acpi/video driver by downstream
> driver just want to remove backlight control interface of acpi/video. It caues
> we lost other functions of acpi/video, e.g. transfer acpi event to input event.
> 
> So, this patch add a new function, find_video_unregister_backlight, it provide
> the interface let downstream driver can tell acpi/video to unregister backlight
> interface of all acpi video devices. Then we can keep functions of acpi/video
> but only remove backlight support.

It doesn't seem to be the best way to solve this problem to me, as the
platform driver doesn't need to be dependent on ACPI video driver and
ACPI video driver shouldn't handle things like these.

The current backlight model has limitations to solve problems like this,
also bear in mind we have some thinkpad models that have similar
problems. I sent the email hoping we can have a discussion on this
topic a while ago:
http://marc.info/?l=linux-acpi&m=136507538826872&w=2
Unfortunately nobody seems interested. 

I'm thinking how we can deal with such problems altogether, introducing
something like backlight manager seems to be a necessary thing to do now.

Thanks,
Aaron

> 
> Reference: bko#35622
>         https://bugzilla.kernel.org/show_bug.cgi?id=35622
> 
> v2: Also unregister cooling devices.
> 
> Tested-by: Andrzej Krentosz <endrjux@xxxxxxxxx>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> Cc: Len Brown <lenb@xxxxxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxx>
> Cc: Carlos Corbacho <carlos@xxxxxxxxxxxxxxxxxxx>
> Cc: Matthew Garrett <mjg@xxxxxxxxxx>
> Cc: Dmitry Torokhov <dtor@xxxxxxx>
> Cc: Corentin Chary <corentincj@xxxxxxxxxx>
> Cc: Aaron Lu <aaron.lu@xxxxxxxxx>
> Cc: Thomas Renninger <trenn@xxxxxxx>
> Signed-off-by: Lee, Chun-Yi <jlee@xxxxxxxx>
> ---
>  drivers/acpi/video.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/video.h |    2 +
>  2 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index c3932d0..f21104d 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1861,6 +1861,60 @@ static int __init intel_opregion_present(void)
>  	return opregion;
>  }
>  
> +static acpi_status
> +find_video_unregister_backlight(acpi_handle handle, u32 lvl, void *context,
> +				void **rv)
> +{
> +	struct acpi_device *acpi_dev;
> +	struct acpi_video_bus *video;
> +	struct acpi_video_device *dev, *next;
> +
> +	if (acpi_bus_get_device(handle, &acpi_dev))
> +		return AE_OK;
> +
> +	if (!acpi_match_device_ids(acpi_dev, video_device_ids)) {
> +		video = acpi_driver_data(acpi_dev);
> +		acpi_video_bus_stop_devices(video);
> +		mutex_lock(&video->device_list_lock);
> +		list_for_each_entry_safe(dev, next, &video->video_device_list,
> +					entry) {
> +			if (dev->backlight) {
> +				backlight_device_unregister(dev->backlight);
> +				dev->backlight = NULL;
> +				kfree(dev->brightness->levels);
> +				kfree(dev->brightness);
> +			}
> +			if (dev->cooling_dev) {
> +				sysfs_remove_link(&dev->dev->dev.kobj,
> +						  "thermal_cooling");
> +				sysfs_remove_link(&dev->cooling_dev->device.kobj,
> +						  "device");
> +				thermal_cooling_device_unregister(dev->cooling_dev);
> +				dev->cooling_dev = NULL;
> +			}
> +		}
> +		mutex_unlock(&video->device_list_lock);
> +		acpi_video_bus_start_devices(video);
> +	}
> +	return AE_OK;
> +}
> +
> +void acpi_video_backlight_unregister(void)
> +{
> +	if (!register_count) {
> +		/*
> +		 * If the acpi video bus is already unloaded, don't
> +		 * unregister backlight of devices and return directly.
> +		 */
> +		return;
> +	}
> +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +			    ACPI_UINT32_MAX, find_video_unregister_backlight,
> +			    NULL, NULL, NULL);
> +	return;
> +}
> +EXPORT_SYMBOL(acpi_video_backlight_unregister);
> +
>  int acpi_video_register(void)
>  {
>  	int result = 0;
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 61109f2..1e810a1 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -19,11 +19,13 @@ struct acpi_device;
>  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
>  extern int acpi_video_register(void);
>  extern void acpi_video_unregister(void);
> +extern void acpi_video_backlight_unregister(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
>  #else
>  static inline int acpi_video_register(void) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
> +static inline void acpi_video_backlight_unregister(void) { return; }
>  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  				      int device_id, void **edid)
>  {
> 

--
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/