Re: [PATCH v2] platform/x86: Add driver for Apple gmux device

[Seth Forshee <seth.forshee@canonical.com>]

On Mon, Mar 12, 2012 at 03:07:17PM +0000, Matthew Garrett wrote:
> The alternative is for apple_bl to export its unregister function and 
> have gmux call that - downside is obviously that that results in gmux 
> depending on apple_bl. Maybe some sort of notification list in the 
> backlight core?

I've been think about this, and I'm not sure a notification in the
backlight core makes sense; it seems likely to be limited in use to only
this one use case. Other ideas might be to have an interface to disable
specific backlight devices, or a way to score backlights which leaves
only the "best" device of a given type enabled. Either of these might
affect userspace ABI however.

For now at least if something like this is needed maybe it makes the
most sense to have apple_bl export a function. What about something like
the patch at the end of this message (compile tested only)?

> > We also have the problem of gmux_backlight versus acpi_video. On most
> > machines with a gmux the acpi_video backlight interface is present but
> > just doesn't work. This problem isn't just limited to Apples. I'm of the
> > opinion that we need a more generalized solution for arbitrating between
> > the backlight interfaces present on a given machine, but I haven't had a
> > chance to really think about what that would look like.
> 
> The ACPI code appears to be trapping into system management mode, so 
> figuring out what it's meant to do is going to be awkward. I think 
> having a hook in the ACPI video driver to deregister it in cases where 
> we know it doesn't work is legitimate, but since it presumably works 
> under Windows it'd be nice to figure out what's broken about it.

I've been experimenting with a MBP 8,2. This actually has 2 ACPI
backlights, one associated with the radeon GPU (acpi_video0) and the
other with the Intel (acpi_video1).

Turns out that acpi_video0 does work under Windows, and it also works
under Linux depending on how the OS is booted. Doing a boot camp style
boot gives us working acpi_video0 and apple_bl backlights. acpi_video1
doesn't show up in a legacy boot situation, as evaluating _BCM gives an
error.

Under EFI boot, both acpi_video backlights show up but neither works.
The readback verify in apple_bl's probe fails, so that one doesn't show
up. Only gmux_backlight actually works.

Using rEFIt is an oddball case. A BIOS-compatible boot gives non-working
acpi_video and apple_bl backlights. I guess the Apple bootloader must
enable some extra legacy support that rEFIt does not, but I haven't been
able to find any way for us to enable it after Linux has started.

Seth


diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
index be98d15..9b16d58 100644
--- a/drivers/video/backlight/apple_bl.c
+++ b/drivers/video/backlight/apple_bl.c
@@ -27,6 +27,19 @@
 
 static struct backlight_device *apple_backlight_device;
 
+/*
+ * apple_bl may be disabled by other modules. We need to track the state
+ * of the device to know how to respond to various events.
+ */
+enum {
+	APPLE_BL_STATE_UNUSED,
+	APPLE_BL_STATE_IN_USE,
+	APPLE_BL_STATE_DISABLED,
+};
+
+static int apple_bl_state = APPLE_BL_STATE_UNUSED;
+DEFINE_MUTEX(apple_bl_mutex);
+
 struct hw_data {
 	/* I/O resource to allocate. */
 	unsigned long iostart;
@@ -139,17 +152,46 @@ static const struct hw_data nvidia_chipset_data = {
 	.set_brightness = nvidia_chipset_set_brightness,
 };
 
+static void apple_bl_unregister(void)
+{
+	backlight_device_unregister(apple_backlight_device);
+	release_region(hw_data->iostart, hw_data->iolen);
+	hw_data = NULL;
+}
+
+void apple_bl_disable(void)
+{
+	mutex_lock(&apple_bl_mutex);
+	if (apple_bl_state == APPLE_BL_STATE_IN_USE)
+		apple_bl_unregister();
+	apple_bl_state = APPLE_BL_STATE_DISABLED;
+	mutex_unlock(&apple_bl_mutex);
+}
+EXPORT_SYMBOL_GPL(apple_bl_disable);
+
 static int __devinit apple_bl_add(struct acpi_device *dev)
 {
 	struct backlight_properties props;
 	struct pci_dev *host;
 	int intensity;
+	int ret = -ENODEV;
+
+	mutex_lock(&apple_bl_mutex);
+
+	switch (apple_bl_state) {
+	case APPLE_BL_STATE_IN_USE:
+		ret = -EBUSY;
+		goto out;
+	case APPLE_BL_STATE_DISABLED:
+		printk(KERN_ERR DRIVER "apple_bl disabled, ignoring device\n");
+		goto out;
+	}
 
 	host = pci_get_bus_and_slot(0, 0);
 
 	if (!host) {
 		printk(KERN_ERR DRIVER "unable to find PCI host\n");
-		return -ENODEV;
+		goto out;
 	}
 
 	if (host->vendor == PCI_VENDOR_ID_INTEL)
@@ -161,7 +203,7 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 
 	if (!hw_data) {
 		printk(KERN_ERR DRIVER "unknown hardware\n");
-		return -ENODEV;
+		goto out;
 	}
 
 	/* Check that the hardware responds - this may not work under EFI */
@@ -171,14 +213,16 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 	if (!intensity) {
 		hw_data->set_brightness(1);
 		if (!hw_data->backlight_ops.get_brightness(NULL))
-			return -ENODEV;
+			goto out;
 
 		hw_data->set_brightness(0);
 	}
 
 	if (!request_region(hw_data->iostart, hw_data->iolen,
-			    "Apple backlight"))
-		return -ENXIO;
+			    "Apple backlight")) {
+		ret = -ENXIO;
+		goto out;
+	}
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
@@ -188,22 +232,30 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 
 	if (IS_ERR(apple_backlight_device)) {
 		release_region(hw_data->iostart, hw_data->iolen);
-		return PTR_ERR(apple_backlight_device);
+		ret = PTR_ERR(apple_backlight_device);
+		goto out;
 	}
 
 	apple_backlight_device->props.brightness =
 		hw_data->backlight_ops.get_brightness(apple_backlight_device);
 	backlight_update_status(apple_backlight_device);
 
-	return 0;
+	apple_bl_state = APPLE_BL_STATE_IN_USE;
+	ret = 0;
+
+out:
+	mutex_unlock(&apple_bl_mutex);
+	return ret;
 }
 
 static int __devexit apple_bl_remove(struct acpi_device *dev, int type)
 {
-	backlight_device_unregister(apple_backlight_device);
-
-	release_region(hw_data->iostart, hw_data->iolen);
-	hw_data = NULL;
+	mutex_lock(&apple_bl_mutex);
+	if (apple_bl_state == APPLE_BL_STATE_IN_USE) {
+		apple_bl_unregister();
+		apple_bl_state = APPLE_BL_STATE_UNUSED;
+	}
+	mutex_unlock(&apple_bl_mutex);
 	return 0;
 }
 
diff --git a/include/linux/apple_bl.h b/include/linux/apple_bl.h
new file mode 100644
index 0000000..7a4b6bc
--- /dev/null
+++ b/include/linux/apple_bl.h
@@ -0,0 +1,10 @@
+/*
+ * apple_bl exported symbols
+ */
+
+#ifndef _LINUX_APPLE_BL_H
+#define _LINUX_APPLE_BL_H
+
+extern void apple_bl_disable(void);
+
+#endif