[PATCH 2/2] acer-wmi: add Acer Aspire 5750G to video vendor list but keep acpi video driver

[PATCH 2/2] acer-wmi: add Acer Aspire 5750G to video vendor list but keep acpi video driver

[Chun-Yi Lee]

From: Lee, Chun-Yi <jlee@suse.com>

After Andrzej's testing, we found the acpi backlight methods broken on Acer
Aspire 5750G but the i915 backlight control works when we set to vendor mode.
And, we still want to keep the acpi/video driver for transfer acpi event to key
event but not unregister whole acpi/video driver.

This patch introduced a new capability flag is ACER_CAP_KEEP_VIDEO_KEY, it
indicates the machine works fine with acpi/video driver for key event but want
to unregister the backlight interface of acpi/video.

Reference: bko#35622
        https://bugzilla.kernel.org/show_bug.cgi?id=35622

Tested-by: Andrzej Krentosz <endrjux@gmail.com>
Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Corentin Chary <corentincj@iksaif.net>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Aaron Lu <aaron.lu@intel.com>
Cc: Thomas Renninger <trenn@suse.de>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/platform/x86/acer-wmi.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index c9076bd..2a02409 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -207,6 +207,7 @@ struct hotkey_function_type_aa {
 #define ACER_CAP_BRIGHTNESS		(1<<3)
 #define ACER_CAP_THREEG			(1<<4)
 #define ACER_CAP_ACCEL			(1<<5)
+#define ACER_CAP_KEEP_VIDEO_KEY		(1<<6)
 #define ACER_CAP_ANY			(0xFFFFFFFF)
 
 /*
@@ -539,6 +540,15 @@ static int video_set_backlight_video_vendor(const struct dmi_system_id *d)
 	return 0;
 }
 
+static int video_set_backlight_video_vendor_keep_acpi_video(
+		const struct dmi_system_id *d)
+{
+	video_set_backlight_video_vendor(d);
+	interface->capability |= ACER_CAP_KEEP_VIDEO_KEY;
+	pr_info("Keep acpi video driver for emit keycode against backlight change\n");
+	return 0;
+}
+
 static const struct dmi_system_id video_vendor_dmi_table[] = {
 	{
 		.callback = video_set_backlight_video_vendor,
@@ -572,6 +582,14 @@ static const struct dmi_system_id video_vendor_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"),
 		},
 	},
+	{
+		.callback = video_set_backlight_video_vendor_keep_acpi_video,
+		.ident = "Acer Aspire 5750G",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750G"),
+		},
+	},
 	{}
 };
 
@@ -2228,6 +2246,8 @@ static int __init acer_wmi_init(void)
 	if (acpi_video_backlight_support()) {
 		interface->capability &= ~ACER_CAP_BRIGHTNESS;
 		pr_info("Brightness must be controlled by acpi video driver\n");
+	} else if (interface->capability & ACER_CAP_KEEP_VIDEO_KEY) {
+		acpi_video_backlight_unregister();
 	} else {
 		pr_info("Disabling ACPI video driver\n");
 		acpi_video_unregister();
-- 
1.6.0.2

[PATCH 1/2] acpi: video: add function to support unregister backlight

[Chun-Yi Lee]

From: "Lee, Chun-Yi" <jlee@suse.com>

There have 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.

Reference: bko#35622
        https://bugzilla.kernel.org/show_bug.cgi?id=35622

Tested-by: Andrzej Krentosz <endrjux@gmail.com>
Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Corentin Chary <corentincj@iksaif.net>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Aaron Lu <aaron.lu@intel.com>
Cc: Thomas Renninger <trenn@suse.de>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/acpi/video.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/video.h |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 313f959..acd2e7a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1793,6 +1793,52 @@ 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 = NULL;
+	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);
+			}
+		}
+		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)
 {
-- 
1.8.1.4

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[Rafael J. Wysocki]

On Monday, April 22, 2013 08:39:15 PM Chun-Yi Lee wrote:
> From: "Lee, Chun-Yi" <jlee@suse.com>
> 
> There have 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.
> 
> Reference: bko#35622
>         https://bugzilla.kernel.org/show_bug.cgi?id=35622

Rui, any comments?

Rafael


> Tested-by: Andrzej Krentosz <endrjux@gmail.com>
> Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> Cc: Corentin Chary <corentincj@iksaif.net>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Aaron Lu <aaron.lu@intel.com>
> Cc: Thomas Renninger <trenn@suse.de>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  drivers/acpi/video.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/video.h |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..acd2e7a 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1793,6 +1793,52 @@ 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 = NULL;
> +	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);
> +			}
> +		}
> +		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)
>  {
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] acer-wmi: add Acer Aspire 5750G to video vendor list but keep acpi video driver

[Dmitry Torokhov]

On Mon, Apr 22, 2013 at 08:39:14PM +0800, Chun-Yi Lee wrote:
> From: Lee, Chun-Yi <jlee@suse.com>
> 
> After Andrzej's testing, we found the acpi backlight methods broken on Acer
> Aspire 5750G but the i915 backlight control works when we set to vendor mode.
> And, we still want to keep the acpi/video driver for transfer acpi event to key
> event but not unregister whole acpi/video driver.
> 
> This patch introduced a new capability flag is ACER_CAP_KEEP_VIDEO_KEY, it
> indicates the machine works fine with acpi/video driver for key event but want
> to unregister the backlight interface of acpi/video.
> 
> Reference: bko#35622
>         https://bugzilla.kernel.org/show_bug.cgi?id=35622
> 
> Tested-by: Andrzej Krentosz <endrjux@gmail.com>
> Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> Cc: Corentin Chary <corentincj@iksaif.net>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Aaron Lu <aaron.lu@intel.com>
> Cc: Thomas Renninger <trenn@suse.de>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  drivers/platform/x86/acer-wmi.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index c9076bd..2a02409 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -207,6 +207,7 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_BRIGHTNESS		(1<<3)
>  #define ACER_CAP_THREEG			(1<<4)
>  #define ACER_CAP_ACCEL			(1<<5)
> +#define ACER_CAP_KEEP_VIDEO_KEY		(1<<6)
>  #define ACER_CAP_ANY			(0xFFFFFFFF)
>  
>  /*
> @@ -539,6 +540,15 @@ static int video_set_backlight_video_vendor(const struct dmi_system_id *d)
>  	return 0;
>  }
>  
> +static int video_set_backlight_video_vendor_keep_acpi_video(
> +		const struct dmi_system_id *d)
> +{
> +	video_set_backlight_video_vendor(d);
> +	interface->capability |= ACER_CAP_KEEP_VIDEO_KEY;
> +	pr_info("Keep acpi video driver for emit keycode against backlight change\n");

Huh?

"Keeping acpi video driver active to emit backlight brightness change
key events"?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[Dmitry Torokhov]

On Mon, Apr 22, 2013 at 08:39:15PM +0800, Chun-Yi Lee wrote:
> From: "Lee, Chun-Yi" <jlee@suse.com>
> +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 = NULL;

Gratuitous initialization of local variables prevents compiler from
warning when you using variable uninitialized.

> +	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);
> +			}
> +		}
> +		mutex_unlock(&video->device_list_lock);
> +		acpi_video_bus_start_devices(video);
> +	}
> +	return AE_OK;
> +}
> +
> +void acpi_video_backlight_unregister(void)
> +{
> +	if (!register_count) {

Locking? It looks like the rest of the driver ignores locking too...

> +		/*
> +		 * 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)
>  {
> -- 
> 1.8.1.4
> 
> 

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[Aaron Lu]

On 04/22/2013 08:39 PM, Chun-Yi Lee wrote:
> From: "Lee, Chun-Yi" <jlee@suse.com>
>
> There have 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.
>
> Reference: bko#35622
>          https://bugzilla.kernel.org/show_bug.cgi?id=35622
>
> Tested-by: Andrzej Krentosz <endrjux@gmail.com>
> Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> Cc: Corentin Chary <corentincj@iksaif.net>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Aaron Lu <aaron.lu@intel.com>
> Cc: Thomas Renninger <trenn@suse.de>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>   drivers/acpi/video.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/acpi/video.h |  2 ++
>   2 files changed, 48 insertions(+)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..acd2e7a 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1793,6 +1793,52 @@ 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 = NULL;
> +	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);
> +			}

The cooling_dev should also be unregistered I think.

Thanks,
Aaron

> +		}
> +		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)
>   {
>

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[joeyli]

Hi Dmitry, 

Thanks for your review and suggestions first!

於 一，2013-04-22 於 21:12 -0700，Dmitry Torokhov 提到：
> On Mon, Apr 22, 2013 at 08:39:15PM +0800, Chun-Yi Lee wrote:
> > From: "Lee, Chun-Yi" <jlee@suse.com>
> > +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 = NULL;
> 
> Gratuitous initialization of local variables prevents compiler from
> warning when you using variable uninitialized.

Yes, I agreed should not put gratuitous initialization, I will remove it
in v2 patch.

> 
> > +	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);
> > +			}
> > +		}
> > +		mutex_unlock(&video->device_list_lock);
> > +		acpi_video_bus_start_devices(video);
> > +	}
> > +	return AE_OK;
> > +}
> > +
> > +void acpi_video_backlight_unregister(void)
> > +{
> > +	if (!register_count) {
> 
> Locking? It looks like the rest of the driver ignores locking too...

Did you mean locking video_device_list? 

The acpi/video locks video_device_list when add, remove and notify acpi
video bus driver. It always do the mutex lock before control
video_device_list, so I also add lock when unregister all backlight of
devices.


Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[joeyli]

Hi Dmitry, 

Thanks for your review and suggestions first!

於 一，2013-04-22 於 21:12 -0700，Dmitry Torokhov 提到：
> On Mon, Apr 22, 2013 at 08:39:15PM +0800, Chun-Yi Lee wrote:
> > From: "Lee, Chun-Yi" <jlee@suse.com>
> > +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 = NULL;
> 
> Gratuitous initialization of local variables prevents compiler from
> warning when you using variable uninitialized.

Yes, I agreed should not put gratuitous initialization, I will remove it
in v2 patch.

> 
> > +	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);
> > +			}
> > +		}
> > +		mutex_unlock(&video->device_list_lock);
> > +		acpi_video_bus_start_devices(video);
> > +	}
> > +	return AE_OK;
> > +}
> > +
> > +void acpi_video_backlight_unregister(void)
> > +{
> > +	if (!register_count) {
> 
> Locking? It looks like the rest of the driver ignores locking too...

Did you mean locking video_device_list? 

The acpi/video locks video_device_list when add, remove and notify acpi
video bus driver. It always do the mutex lock before control
video_device_list, so I also add lock when unregister all backlight of
devices.


Thanks a lot!
Joey Lee

Re: [PATCH 2/2] acer-wmi: add Acer Aspire 5750G to video vendor list but keep acpi video driver

[joeyli]

Hi Dmitry, 

Thanks for your review and suggestion!

於 一，2013-04-22 於 21:04 -0700，Dmitry Torokhov 提到：
> On Mon, Apr 22, 2013 at 08:39:14PM +0800, Chun-Yi Lee wrote:
> > From: Lee, Chun-Yi <jlee@suse.com>
> > 
> > After Andrzej's testing, we found the acpi backlight methods broken on Acer
> > Aspire 5750G but the i915 backlight control works when we set to vendor mode.
> > And, we still want to keep the acpi/video driver for transfer acpi event to key
> > event but not unregister whole acpi/video driver.
> > 
> > This patch introduced a new capability flag is ACER_CAP_KEEP_VIDEO_KEY, it
> > indicates the machine works fine with acpi/video driver for key event but want
> > to unregister the backlight interface of acpi/video.
> > 
> > Reference: bko#35622
> >         https://bugzilla.kernel.org/show_bug.cgi?id=35622
> > 
> > Tested-by: Andrzej Krentosz <endrjux@gmail.com>
> > Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
> > Cc: Matthew Garrett <mjg@redhat.com>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> > Cc: Corentin Chary <corentincj@iksaif.net>
> > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > Cc: Aaron Lu <aaron.lu@intel.com>
> > Cc: Thomas Renninger <trenn@suse.de>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  drivers/platform/x86/acer-wmi.c |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> > index c9076bd..2a02409 100644
> > --- a/drivers/platform/x86/acer-wmi.c
> > +++ b/drivers/platform/x86/acer-wmi.c
> > @@ -207,6 +207,7 @@ struct hotkey_function_type_aa {
> >  #define ACER_CAP_BRIGHTNESS		(1<<3)
> >  #define ACER_CAP_THREEG			(1<<4)
> >  #define ACER_CAP_ACCEL			(1<<5)
> > +#define ACER_CAP_KEEP_VIDEO_KEY		(1<<6)
> >  #define ACER_CAP_ANY			(0xFFFFFFFF)
> >  
> >  /*
> > @@ -539,6 +540,15 @@ static int video_set_backlight_video_vendor(const struct dmi_system_id *d)
> >  	return 0;
> >  }
> >  
> > +static int video_set_backlight_video_vendor_keep_acpi_video(
> > +		const struct dmi_system_id *d)
> > +{
> > +	video_set_backlight_video_vendor(d);
> > +	interface->capability |= ACER_CAP_KEEP_VIDEO_KEY;
> > +	pr_info("Keep acpi video driver for emit keycode against backlight change\n");
> 
> Huh?
> 
> "Keeping acpi video driver active to emit backlight brightness change
> key events"?
> 
> Thanks.
> 

Yes, this statement is more fluent, I will put to v2 patch.


Thanks a lot!
Joey Lee

Re: [PATCH 1/2] acpi: video: add function to support unregister backlight

[joeyli]

於 五，2013-04-26 於 13:24 +0800，Aaron Lu 提到：
> On 04/22/2013 08:39 PM, Chun-Yi Lee wrote:
> > From: "Lee, Chun-Yi" <jlee@suse.com>
> >
> > There have 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.
> >
> > Reference: bko#35622
> >          https://bugzilla.kernel.org/show_bug.cgi?id=35622
> >
> > Tested-by: Andrzej Krentosz <endrjux@gmail.com>
> > Cc: Carlos Corbacho <carlos@strangeworlds.co.uk>
> > Cc: Matthew Garrett <mjg@redhat.com>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> > Cc: Corentin Chary <corentincj@iksaif.net>
> > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > Cc: Aaron Lu <aaron.lu@intel.com>
> > Cc: Thomas Renninger <trenn@suse.de>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >   drivers/acpi/video.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >   include/acpi/video.h |  2 ++
> >   2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 313f959..acd2e7a 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1793,6 +1793,52 @@ 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 = NULL;
> > +	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);
> > +			}
> 
> The cooling_dev should also be unregistered I think.
> 
> Thanks,
> Aaron
> 

Yes, you are right!

Unregistering  backlight interface of video devices means the backlight
control functions broken, so it can not used for throttling.

I will unregister cooling_dev in v2 patch.


Thanks a lot!
Joey Lee