[PATCH 0/3] apple_bl: Add support for gmux backlight control

[PATCH 0/3] apple_bl: Add support for gmux backlight control

[Seth Forshee]

Many models of Apple laptops have a gmux device, whose primary purpose
seems to be controlling the display mux in these machines. The gmux
often also supports adjustment of screen brightness, and in some cases
it is the only working method available for adjusting the backlight.

This series adds support to apple_bl for gmux backlight control, along
with some driver reworking to better support this device. While I was at
it I went ahead and threw in a patch to convert the printks to
pr_<level>.

I've been able to get these changes tested on several different models
of MacBooks, and the functioning of the existing backlight support is
unaffected. On some models the gmux is present but does not control
screen brightness. I haven't found any way to detect this in software
though, as the reads and writes to the backlight control ports behave
identically to when the gmux can control the backlight.

Thanks,
Seth


Seth Forshee (3):
  apple_bl: Convert printks to pr_<level>
  apple_bl: Rework in advance of gmux backlight support
  apple_bl: Add support for gmux backlight control

 drivers/video/backlight/apple_bl.c |  329 +++++++++++++++++++++++++-----------
 1 files changed, 230 insertions(+), 99 deletions(-)

[PATCH 1/3] apple_bl: Convert printks to pr_<level>

[Seth Forshee]

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/video/backlight/apple_bl.c |   29 ++++++++---------------------
 1 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
index be98d15..66d5bec 100644
--- a/drivers/video/backlight/apple_bl.c
+++ b/drivers/video/backlight/apple_bl.c
@@ -16,6 +16,8 @@
  *  get at the firmware code in order to figure out what it's actually doing.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -38,13 +40,6 @@ struct hw_data {
 
 static const struct hw_data *hw_data;
 
-#define DRIVER "apple_backlight: "
-
-/* Module parameters. */
-static int debug;
-module_param_named(debug, debug, int, 0644);
-MODULE_PARM_DESC(debug, "Set to one to enable debugging messages.");
-
 /*
  * Implementation for machines with Intel chipset.
  */
@@ -58,9 +53,7 @@ static int intel_chipset_send_intensity(struct backlight_device *bd)
 {
 	int intensity = bd->props.brightness;
 
-	if (debug)
-		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
-		       intensity);
+	pr_debug("setting brightness to %d\n", intensity);
 
 	intel_chipset_set_brightness(intensity);
 	return 0;
@@ -74,9 +67,7 @@ static int intel_chipset_get_intensity(struct backlight_device *bd)
 	outb(0xbf, 0xb2);
 	intensity = inb(0xb3) >> 4;
 
-	if (debug)
-		printk(KERN_DEBUG DRIVER "read brightness of %d\n",
-		       intensity);
+	pr_debug("read brightness of %d\n", intensity);
 
 	return intensity;
 }
@@ -105,9 +96,7 @@ static int nvidia_chipset_send_intensity(struct backlight_device *bd)
 {
 	int intensity = bd->props.brightness;
 
-	if (debug)
-		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
-		       intensity);
+	pr_debug("setting brightness to %d\n", intensity);
 
 	nvidia_chipset_set_brightness(intensity);
 	return 0;
@@ -121,9 +110,7 @@ static int nvidia_chipset_get_intensity(struct backlight_device *bd)
 	outb(0xbf, 0x52e);
 	intensity = inb(0x52f) >> 4;
 
-	if (debug)
-		printk(KERN_DEBUG DRIVER "read brightness of %d\n",
-		       intensity);
+	pr_debug("read brightness of %d\n", intensity);
 
 	return intensity;
 }
@@ -148,7 +135,7 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 	host = pci_get_bus_and_slot(0, 0);
 
 	if (!host) {
-		printk(KERN_ERR DRIVER "unable to find PCI host\n");
+		pr_err("unable to find PCI host\n");
 		return -ENODEV;
 	}
 
@@ -160,7 +147,7 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 	pci_dev_put(host);
 
 	if (!hw_data) {
-		printk(KERN_ERR DRIVER "unknown hardware\n");
+		pr_err("unknown hardware\n");
 		return -ENODEV;
 	}
 
-- 
1.7.8.3

Re: [PATCH 1/3] apple_bl: Convert printks to pr_<level>

[Joe Perches]

On Fri, 2012-02-03 at 14:28 -0600, Seth Forshee wrote:
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
[]
> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> @@ -58,9 +53,7 @@ static int intel_chipset_send_intensity(struct backlight_device *bd)
>  {
>  	int intensity = bd->props.brightness;
>  
> -	if (debug)
> -		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
> -		       intensity);
> +	pr_debug("setting brightness to %d\n", intensity);

You may need to add "#define DEBUG" somewhere for
this to work as you might expect it to.

An "allyesconfig" does effectively add one.

Re: [PATCH 1/3] apple_bl: Convert printks to pr_<level>

[Ryan Mallon]

On 04/02/12 07:28, Seth Forshee wrote:

> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/video/backlight/apple_bl.c |   29 ++++++++---------------------
>  1 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> index be98d15..66d5bec 100644
> --- a/drivers/video/backlight/apple_bl.c
> +++ b/drivers/video/backlight/apple_bl.c
> @@ -16,6 +16,8 @@
>   *  get at the firmware code in order to figure out what it's actually doing.
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> @@ -38,13 +40,6 @@ struct hw_data {
>  
>  static const struct hw_data *hw_data;
>  
> -#define DRIVER "apple_backlight: "
> -
> -/* Module parameters. */
> -static int debug;
> -module_param_named(debug, debug, int, 0644);
> -MODULE_PARM_DESC(debug, "Set to one to enable debugging messages.");


Removal of this module parameter is not noted in the changelog.

> -
>  /*
>   * Implementation for machines with Intel chipset.
>   */
> @@ -58,9 +53,7 @@ static int intel_chipset_send_intensity(struct backlight_device *bd)
>  {
>  	int intensity = bd->props.brightness;
>  
> -	if (debug)
> -		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
> -		       intensity);
> +	pr_debug("setting brightness to %d\n", intensity);


As Joe points out, this no longer has the same behaviour as it did
previously. The pr_debug will only show up if you have DEBUG defined.
Again, this should probably be mentioned in the changelog.

~Ryan

Re: [PATCH 1/3] apple_bl: Convert printks to pr_<level>

[Seth Forshee]

On Mon, Feb 06, 2012 at 10:23:13AM +1100, Ryan Mallon wrote:
> On 04/02/12 07:28, Seth Forshee wrote:
> 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  drivers/video/backlight/apple_bl.c |   29 ++++++++---------------------
> >  1 files changed, 8 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> > index be98d15..66d5bec 100644
> > --- a/drivers/video/backlight/apple_bl.c
> > +++ b/drivers/video/backlight/apple_bl.c
> > @@ -16,6 +16,8 @@
> >   *  get at the firmware code in order to figure out what it's actually doing.
> >   */
> >  
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> >  #include <linux/init.h>
> > @@ -38,13 +40,6 @@ struct hw_data {
> >  
> >  static const struct hw_data *hw_data;
> >  
> > -#define DRIVER "apple_backlight: "
> > -
> > -/* Module parameters. */
> > -static int debug;
> > -module_param_named(debug, debug, int, 0644);
> > -MODULE_PARM_DESC(debug, "Set to one to enable debugging messages.");
> 
> 
> Removal of this module parameter is not noted in the changelog.
> 
> > -
> >  /*
> >   * Implementation for machines with Intel chipset.
> >   */
> > @@ -58,9 +53,7 @@ static int intel_chipset_send_intensity(struct backlight_device *bd)
> >  {
> >  	int intensity = bd->props.brightness;
> >  
> > -	if (debug)
> > -		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
> > -		       intensity);
> > +	pr_debug("setting brightness to %d\n", intensity);
> 
> 
> As Joe points out, this no longer has the same behaviour as it did
> previously. The pr_debug will only show up if you have DEBUG defined.
> Again, this should probably be mentioned in the changelog.

I doesn't matter to me one way or the other whether these statements are
enabled by module parameter, I was just thinking that pr_debug was
generally preferred. I can put the module parameter back or denote this
in the changelog, whichever is more acceptable.

Seth

Re: [PATCH 1/3] apple_bl: Convert printks to pr_<level>

[Ryan Mallon]

On 07/02/12 01:35, Seth Forshee wrote:

> On Mon, Feb 06, 2012 at 10:23:13AM +1100, Ryan Mallon wrote:
>> On 04/02/12 07:28, Seth Forshee wrote:
>>
>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>> ---
>>>  drivers/video/backlight/apple_bl.c |   29 ++++++++---------------------
>>>  1 files changed, 8 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
>>> index be98d15..66d5bec 100644
>>> --- a/drivers/video/backlight/apple_bl.c
>>> +++ b/drivers/video/backlight/apple_bl.c
>>> @@ -16,6 +16,8 @@
>>>   *  get at the firmware code in order to figure out what it's actually doing.
>>>   */
>>>  
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>>  #include <linux/module.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/init.h>
>>> @@ -38,13 +40,6 @@ struct hw_data {
>>>  
>>>  static const struct hw_data *hw_data;
>>>  
>>> -#define DRIVER "apple_backlight: "
>>> -
>>> -/* Module parameters. */
>>> -static int debug;
>>> -module_param_named(debug, debug, int, 0644);
>>> -MODULE_PARM_DESC(debug, "Set to one to enable debugging messages.");
>>
>>
>> Removal of this module parameter is not noted in the changelog.
>>
>>> -
>>>  /*
>>>   * Implementation for machines with Intel chipset.
>>>   */
>>> @@ -58,9 +53,7 @@ static int intel_chipset_send_intensity(struct backlight_device *bd)
>>>  {
>>>  	int intensity = bd->props.brightness;
>>>  
>>> -	if (debug)
>>> -		printk(KERN_DEBUG DRIVER "setting brightness to %d\n",
>>> -		       intensity);
>>> +	pr_debug("setting brightness to %d\n", intensity);
>>
>>
>> As Joe points out, this no longer has the same behaviour as it did
>> previously. The pr_debug will only show up if you have DEBUG defined.
>> Again, this should probably be mentioned in the changelog.
> 
> I doesn't matter to me one way or the other whether these statements are
> enabled by module parameter, I was just thinking that pr_debug was
> generally preferred. I can put the module parameter back or denote this
> in the changelog, whichever is more acceptable.


Actually, you can just remove the debug print. The backlight core has a
similar message already, so no need to duplicate it here. Note this
reasoning in the changelog though.

~Ryan

[PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Seth Forshee]

Make it easier to support backlights without a fixed I/O range, and
remove use of global variables to allow having multiple backlights
concurrently.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/video/backlight/apple_bl.c |  163 +++++++++++++++++++-----------------
 1 files changed, 85 insertions(+), 78 deletions(-)

diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
index 66d5bec..e65b459 100644
--- a/drivers/video/backlight/apple_bl.c
+++ b/drivers/video/backlight/apple_bl.c
@@ -27,39 +27,30 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 
-static struct backlight_device *apple_backlight_device;
-
-struct hw_data {
+struct apple_bl_data {
 	/* I/O resource to allocate. */
 	unsigned long iostart;
 	unsigned long iolen;
+
+	/* Backlight device */
+	struct backlight_device *bdev;
+
 	/* Backlight operations structure. */
-	const struct backlight_ops backlight_ops;
-	void (*set_brightness)(int);
+	int (*get_brightness)(struct apple_bl_data *);
+	void (*set_brightness)(struct apple_bl_data *, int);
 };
 
-static const struct hw_data *hw_data;
-
 /*
  * Implementation for machines with Intel chipset.
  */
-static void intel_chipset_set_brightness(int intensity)
+static void intel_chipset_set_brightness(struct apple_bl_data *bl_data,
+					 int intensity)
 {
 	outb(0x04 | (intensity << 4), 0xb3);
 	outb(0xbf, 0xb2);
 }
 
-static int intel_chipset_send_intensity(struct backlight_device *bd)
-{
-	int intensity = bd->props.brightness;
-
-	pr_debug("setting brightness to %d\n", intensity);
-
-	intel_chipset_set_brightness(intensity);
-	return 0;
-}
-
-static int intel_chipset_get_intensity(struct backlight_device *bd)
+static int intel_chipset_get_brightness(struct apple_bl_data *bl_data)
 {
 	int intensity;
 
@@ -72,37 +63,17 @@ static int intel_chipset_get_intensity(struct backlight_device *bd)
 	return intensity;
 }
 
-static const struct hw_data intel_chipset_data = {
-	.iostart = 0xb2,
-	.iolen = 2,
-	.backlight_ops	= {
-		.options	= BL_CORE_SUSPENDRESUME,
-		.get_brightness	= intel_chipset_get_intensity,
-		.update_status	= intel_chipset_send_intensity,
-	},
-	.set_brightness = intel_chipset_set_brightness,
-};
-
 /*
  * Implementation for machines with Nvidia chipset.
  */
-static void nvidia_chipset_set_brightness(int intensity)
+static void nvidia_chipset_set_brightness(struct apple_bl_data *bl_data,
+					  int intensity)
 {
 	outb(0x04 | (intensity << 4), 0x52f);
 	outb(0xbf, 0x52e);
 }
 
-static int nvidia_chipset_send_intensity(struct backlight_device *bd)
-{
-	int intensity = bd->props.brightness;
-
-	pr_debug("setting brightness to %d\n", intensity);
-
-	nvidia_chipset_set_brightness(intensity);
-	return 0;
-}
-
-static int nvidia_chipset_get_intensity(struct backlight_device *bd)
+static int nvidia_chipset_get_brightness(struct apple_bl_data *bl_data)
 {
 	int intensity;
 
@@ -115,82 +86,118 @@ static int nvidia_chipset_get_intensity(struct backlight_device *bd)
 	return intensity;
 }
 
-static const struct hw_data nvidia_chipset_data = {
-	.iostart = 0x52e,
-	.iolen = 2,
-	.backlight_ops		= {
-		.options	= BL_CORE_SUSPENDRESUME,
-		.get_brightness	= nvidia_chipset_get_intensity,
-		.update_status	= nvidia_chipset_send_intensity
-	},
-	.set_brightness = nvidia_chipset_set_brightness,
+/*
+ * Backlight device class operations
+ */
+static int apple_bl_get_brightness(struct backlight_device *bd)
+{
+	struct apple_bl_data *bl_data = bl_get_data(bd);
+	return bl_data->get_brightness(bl_data);
+}
+
+static int apple_bl_update_status(struct backlight_device *bd)
+{
+	struct apple_bl_data *bl_data = bl_get_data(bd);
+
+	bl_data->set_brightness(bl_data, bd->props.brightness);
+	return 0;
+}
+
+static const struct backlight_ops apple_bl_ops = {
+	.get_brightness = apple_bl_get_brightness,
+	.update_status = apple_bl_update_status,
 };
 
 static int __devinit apple_bl_add(struct acpi_device *dev)
 {
+	struct apple_bl_data *bl_data;
 	struct backlight_properties props;
+	struct backlight_device *bdev;
 	struct pci_dev *host;
+	unsigned short vendor;
 	int intensity;
+	int ret = -ENODEV;
+
+	bl_data = kzalloc(sizeof(*bl_data), GFP_KERNEL);
+	if (!bl_data)
+		return -ENOMEM;
+	dev->driver_data = dev;
 
 	host = pci_get_bus_and_slot(0, 0);
 
 	if (!host) {
 		pr_err("unable to find PCI host\n");
-		return -ENODEV;
+		goto err_free;
 	}
 
-	if (host->vendor == PCI_VENDOR_ID_INTEL)
-		hw_data = &intel_chipset_data;
-	else if (host->vendor == PCI_VENDOR_ID_NVIDIA)
-		hw_data = &nvidia_chipset_data;
-
+	vendor = host->vendor;
 	pci_dev_put(host);
 
-	if (!hw_data) {
+	if (vendor == PCI_VENDOR_ID_INTEL) {
+		bl_data->iostart = 0xb2;
+		bl_data->iolen = 2;
+		bl_data->get_brightness = intel_chipset_get_brightness;
+		bl_data->set_brightness = intel_chipset_set_brightness;
+	} else if (vendor == PCI_VENDOR_ID_NVIDIA) {
+		bl_data->iostart = 0x52e;
+		bl_data->iolen = 2;
+		bl_data->get_brightness = nvidia_chipset_get_brightness;
+		bl_data->set_brightness = nvidia_chipset_set_brightness;
+	} else {
 		pr_err("unknown hardware\n");
-		return -ENODEV;
+		goto err_free;
 	}
 
 	/* Check that the hardware responds - this may not work under EFI */
 
-	intensity = hw_data->backlight_ops.get_brightness(NULL);
+	intensity = bl_data->get_brightness(bl_data);
 
 	if (!intensity) {
-		hw_data->set_brightness(1);
-		if (!hw_data->backlight_ops.get_brightness(NULL))
-			return -ENODEV;
+		bl_data->set_brightness(bl_data, 1);
+		if (!bl_data->get_brightness(bl_data))
+			goto err_free;
 
-		hw_data->set_brightness(0);
+		bl_data->set_brightness(bl_data, 0);
 	}
 
-	if (!request_region(hw_data->iostart, hw_data->iolen,
-			    "Apple backlight"))
-		return -ENXIO;
+	if (!request_region(bl_data->iostart, bl_data->iolen,
+			    "Apple backlight")) {
+		ret = -ENXIO;
+		goto err_free;
+	}
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = 15;
-	apple_backlight_device = backlight_device_register("apple_backlight",
-				  NULL, NULL, &hw_data->backlight_ops, &props);
+	bdev = backlight_device_register("apple_backlight", NULL, bl_data,
+					 &apple_bl_ops, &props);
 
-	if (IS_ERR(apple_backlight_device)) {
-		release_region(hw_data->iostart, hw_data->iolen);
-		return PTR_ERR(apple_backlight_device);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		goto err_release;
 	}
 
-	apple_backlight_device->props.brightness =
-		hw_data->backlight_ops.get_brightness(apple_backlight_device);
-	backlight_update_status(apple_backlight_device);
+	bl_data->bdev = bdev;
+	bdev->props.brightness = bl_data->get_brightness(bl_data);
+	backlight_update_status(bdev);
 
 	return 0;
+
+err_release:
+	release_region(bl_data->iostart, bl_data->iolen);
+err_free:
+	kfree(bl_data);
+	return ret;
 }
 
 static int __devexit apple_bl_remove(struct acpi_device *dev, int type)
 {
-	backlight_device_unregister(apple_backlight_device);
+	struct apple_bl_data *bl_data = dev->driver_data;
+
+	backlight_device_unregister(bl_data->bdev);
+	release_region(bl_data->iostart, bl_data->iolen);
+	kfree(bl_data);
 
-	release_region(hw_data->iostart, hw_data->iolen);
-	hw_data = NULL;
 	return 0;
 }
 
-- 
1.7.8.3

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Lars-Peter Clausen]

On 02/03/2012 09:28 PM, Seth Forshee wrote:
> Make it easier to support backlights without a fixed I/O range, and
> remove use of global variables to allow having multiple backlights
> concurrently.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/video/backlight/apple_bl.c |  163 +++++++++++++++++++-----------------
>  1 files changed, 85 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> index 66d5bec..e65b459 100644
> --- a/drivers/video/backlight/apple_bl.c
> +++ b/drivers/video/backlight/apple_bl.c
> @@ -27,39 +27,30 @@
>  #include <linux/pci.h>
>  #include <linux/acpi.h>
> [...]
> + */
> +static int apple_bl_get_brightness(struct backlight_device *bd)
> +{
> +	struct apple_bl_data *bl_data = bl_get_data(bd);
> +	return bl_data->get_brightness(bl_data);
> +}
> +
> +static int apple_bl_update_status(struct backlight_device *bd)
> +{
> +	struct apple_bl_data *bl_data = bl_get_data(bd);
> +
> +	bl_data->set_brightness(bl_data, bd->props.brightness);
> +	return 0;
> +}
> +
> +static const struct backlight_ops apple_bl_ops = {
> +	.get_brightness = apple_bl_get_brightness,
> +	.update_status = apple_bl_update_status,
>  };

Adding this extra indirection here isn't so nice and isn't necessary either.
Just define one set of backlight ops for the intel case and one for the nvidia
case and use it accordingly when registering the backlight device.

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Seth Forshee]

On Fri, Feb 03, 2012 at 11:25:12PM +0100, Lars-Peter Clausen wrote:
> On 02/03/2012 09:28 PM, Seth Forshee wrote:
> > Make it easier to support backlights without a fixed I/O range, and
> > remove use of global variables to allow having multiple backlights
> > concurrently.
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  drivers/video/backlight/apple_bl.c |  163 +++++++++++++++++++-----------------
> >  1 files changed, 85 insertions(+), 78 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> > index 66d5bec..e65b459 100644
> > --- a/drivers/video/backlight/apple_bl.c
> > +++ b/drivers/video/backlight/apple_bl.c
> > @@ -27,39 +27,30 @@
> >  #include <linux/pci.h>
> >  #include <linux/acpi.h>
> > [...]
> > + */
> > +static int apple_bl_get_brightness(struct backlight_device *bd)
> > +{
> > +	struct apple_bl_data *bl_data = bl_get_data(bd);
> > +	return bl_data->get_brightness(bl_data);
> > +}
> > +
> > +static int apple_bl_update_status(struct backlight_device *bd)
> > +{
> > +	struct apple_bl_data *bl_data = bl_get_data(bd);
> > +
> > +	bl_data->set_brightness(bl_data, bd->props.brightness);
> > +	return 0;
> > +}
> > +
> > +static const struct backlight_ops apple_bl_ops = {
> > +	.get_brightness = apple_bl_get_brightness,
> > +	.update_status = apple_bl_update_status,
> >  };
> 
> Adding this extra indirection here isn't so nice and isn't necessary either.
> Just define one set of backlight ops for the intel case and one for the nvidia
> case and use it accordingly when registering the backlight device.

There's a reason for the extra level of indirection to be there. The
driver uses {get,set}_brightness before the backlight device has been
allocated to test whether or not the backlight interface actually works.
This worked okay previously because the functions didn't need any extra
data; they just access fixed port addresses (really it only half-worked,
the update_status actually already has this indirection to support the
test, duplicated for each interface). But for the gmux backlight we're
getting the I/O address range from ACPI, so it needs to get at the data.

Of course there are a couple of ways we could get around this. Not
calling the backlight ops in the gmux case would be an option; then you
don't get the check, but so far as I know right now the check doesn't
work for the gmux backlight anyway. Or allocating the backlight device
first before doing the check, but I don't see that as a good option.

I feel like what I've done is the cleanest way to accommodate the test,
and the extra level of indirection really isn't all that bad imho.

Seth

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Lars-Peter Clausen]

On 02/03/2012 11:51 PM, Seth Forshee wrote:
> On Fri, Feb 03, 2012 at 11:25:12PM +0100, Lars-Peter Clausen wrote:
>> On 02/03/2012 09:28 PM, Seth Forshee wrote:
>>> Make it easier to support backlights without a fixed I/O range, and
>>> remove use of global variables to allow having multiple backlights
>>> concurrently.
>>>
>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>> ---
>>>  drivers/video/backlight/apple_bl.c |  163 +++++++++++++++++++-----------------
>>>  1 files changed, 85 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
>>> index 66d5bec..e65b459 100644
>>> --- a/drivers/video/backlight/apple_bl.c
>>> +++ b/drivers/video/backlight/apple_bl.c
>>> @@ -27,39 +27,30 @@
>>>  #include <linux/pci.h>
>>>  #include <linux/acpi.h>
>>> [...]
>>> + */
>>> +static int apple_bl_get_brightness(struct backlight_device *bd)
>>> +{
>>> +	struct apple_bl_data *bl_data = bl_get_data(bd);
>>> +	return bl_data->get_brightness(bl_data);
>>> +}
>>> +
>>> +static int apple_bl_update_status(struct backlight_device *bd)
>>> +{
>>> +	struct apple_bl_data *bl_data = bl_get_data(bd);
>>> +
>>> +	bl_data->set_brightness(bl_data, bd->props.brightness);
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct backlight_ops apple_bl_ops = {
>>> +	.get_brightness = apple_bl_get_brightness,
>>> +	.update_status = apple_bl_update_status,
>>>  };
>>
>> Adding this extra indirection here isn't so nice and isn't necessary either.
>> Just define one set of backlight ops for the intel case and one for the nvidia
>> case and use it accordingly when registering the backlight device.
> 
> There's a reason for the extra level of indirection to be there. The
> driver uses {get,set}_brightness before the backlight device has been
> allocated to test whether or not the backlight interface actually works.
> This worked okay previously because the functions didn't need any extra
> data; they just access fixed port addresses (really it only half-worked,
> the update_status actually already has this indirection to support the
> test, duplicated for each interface). But for the gmux backlight we're
> getting the I/O address range from ACPI, so it needs to get at the data.
> 

Ok, I see. Btw. am I missing something or are the intel and nvidia
{set,get}_brightness functions identical except for the IO base address? If
yes, I think they could be merged since you now pass the pass base address into
the function when calling it.

Something that would also be good to fix is to move the request_region(...) on
the IO address before actually accessing it.

> Of course there are a couple of ways we could get around this. Not
> calling the backlight ops in the gmux case would be an option; then you
> don't get the check, but so far as I know right now the check doesn't
> work for the gmux backlight anyway. Or allocating the backlight device
> first before doing the check, but I don't see that as a good option.
> 

Hm, if that the check doesn't do anything for gmux is there acutally any code
shared between the gmux and the legacy path in the driver? If not would make
sense to put the gmux backlight support into its own driver?

- Lars

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Seth Forshee]

On Sat, Feb 04, 2012 at 06:25:53PM +0100, Lars-Peter Clausen wrote:
> On 02/03/2012 11:51 PM, Seth Forshee wrote:
> > On Fri, Feb 03, 2012 at 11:25:12PM +0100, Lars-Peter Clausen wrote:
> >> On 02/03/2012 09:28 PM, Seth Forshee wrote:
> >>> Make it easier to support backlights without a fixed I/O range, and
> >>> remove use of global variables to allow having multiple backlights
> >>> concurrently.
> >>>
> >>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >>> ---
> >>>  drivers/video/backlight/apple_bl.c |  163 +++++++++++++++++++-----------------
> >>>  1 files changed, 85 insertions(+), 78 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
> >>> index 66d5bec..e65b459 100644
> >>> --- a/drivers/video/backlight/apple_bl.c
> >>> +++ b/drivers/video/backlight/apple_bl.c
> >>> @@ -27,39 +27,30 @@
> >>>  #include <linux/pci.h>
> >>>  #include <linux/acpi.h>
> >>> [...]
> >>> + */
> >>> +static int apple_bl_get_brightness(struct backlight_device *bd)
> >>> +{
> >>> +	struct apple_bl_data *bl_data = bl_get_data(bd);
> >>> +	return bl_data->get_brightness(bl_data);
> >>> +}
> >>> +
> >>> +static int apple_bl_update_status(struct backlight_device *bd)
> >>> +{
> >>> +	struct apple_bl_data *bl_data = bl_get_data(bd);
> >>> +
> >>> +	bl_data->set_brightness(bl_data, bd->props.brightness);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static const struct backlight_ops apple_bl_ops = {
> >>> +	.get_brightness = apple_bl_get_brightness,
> >>> +	.update_status = apple_bl_update_status,
> >>>  };
> >>
> >> Adding this extra indirection here isn't so nice and isn't necessary either.
> >> Just define one set of backlight ops for the intel case and one for the nvidia
> >> case and use it accordingly when registering the backlight device.
> > 
> > There's a reason for the extra level of indirection to be there. The
> > driver uses {get,set}_brightness before the backlight device has been
> > allocated to test whether or not the backlight interface actually works.
> > This worked okay previously because the functions didn't need any extra
> > data; they just access fixed port addresses (really it only half-worked,
> > the update_status actually already has this indirection to support the
> > test, duplicated for each interface). But for the gmux backlight we're
> > getting the I/O address range from ACPI, so it needs to get at the data.
> > 
> 
> Ok, I see. Btw. am I missing something or are the intel and nvidia
> {set,get}_brightness functions identical except for the IO base address? If
> yes, I think they could be merged since you now pass the pass base address into
> the function when calling it.
> 
> Something that would also be good to fix is to move the request_region(...) on
> the IO address before actually accessing it.

Okay, that makes sense.

> > Of course there are a couple of ways we could get around this. Not
> > calling the backlight ops in the gmux case would be an option; then you
> > don't get the check, but so far as I know right now the check doesn't
> > work for the gmux backlight anyway. Or allocating the backlight device
> > first before doing the check, but I don't see that as a good option.
> > 
> 
> Hm, if that the check doesn't do anything for gmux is there acutally any code
> shared between the gmux and the legacy path in the driver? If not would make
> sense to put the gmux backlight support into its own driver?

What I know is that the check doesn't work for the gmux on one
particular model. On that machine the gmux device is present but doesn't
control the backlight, yet reads and writes to the backlight ports
behave the same as if it did control the backlight. That's the only
model with a gmux that doesn't control the backlight that I've been able
to get any testing on, so I don't know how other models behave.

But you're right, by and large all the legacy and gmux paths share are
boilerplate code, other than the fact that they support the same class
of machines. The way apple_bl is done it seems natural to extended it
with other backlight interfaces for Apple hardware, but an argument can
be made for separting it out as well.

Seth

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Seth Forshee]

On Mon, Feb 06, 2012 at 08:56:30AM -0600, Seth Forshee wrote:
> > > Of course there are a couple of ways we could get around this. Not
> > > calling the backlight ops in the gmux case would be an option; then you
> > > don't get the check, but so far as I know right now the check doesn't
> > > work for the gmux backlight anyway. Or allocating the backlight device
> > > first before doing the check, but I don't see that as a good option.
> > > 
> > 
> > Hm, if that the check doesn't do anything for gmux is there acutally any code
> > shared between the gmux and the legacy path in the driver? If not would make
> > sense to put the gmux backlight support into its own driver?
> 
> What I know is that the check doesn't work for the gmux on one
> particular model. On that machine the gmux device is present but doesn't
> control the backlight, yet reads and writes to the backlight ports
> behave the same as if it did control the backlight. That's the only
> model with a gmux that doesn't control the backlight that I've been able
> to get any testing on, so I don't know how other models behave.
> 
> But you're right, by and large all the legacy and gmux paths share are
> boilerplate code, other than the fact that they support the same class
> of machines. The way apple_bl is done it seems natural to extended it
> with other backlight interfaces for Apple hardware, but an argument can
> be made for separting it out as well.

I managed to get some more information that will allow me to do a sanity
check on the gmux. This will be completely separate from the check for
the legacy path, meaning they diverge even more. As a result I think I
will go ahead and put the gmux support in its own driver. This probably
makes the most sense anyway as the driver is likely to grow to include
support for the display mux functionality.

But with it being its own driver, I don't think it really belongs in
drivers/video/backlight even though the driver will initially only
supply backlight control. Anyone have suggestions where a driver for a
display mux device should live? I'm not seeing anywhere that looks like
an obviously correct location.

Thanks,
Seth

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Lars-Peter Clausen]

On 02/09/2012 06:20 PM, Seth Forshee wrote:
> On Mon, Feb 06, 2012 at 08:56:30AM -0600, Seth Forshee wrote:
>>>> Of course there are a couple of ways we could get around this. Not
>>>> calling the backlight ops in the gmux case would be an option; then you
>>>> don't get the check, but so far as I know right now the check doesn't
>>>> work for the gmux backlight anyway. Or allocating the backlight device
>>>> first before doing the check, but I don't see that as a good option.
>>>>
>>>
>>> Hm, if that the check doesn't do anything for gmux is there acutally any code
>>> shared between the gmux and the legacy path in the driver? If not would make
>>> sense to put the gmux backlight support into its own driver?
>>
>> What I know is that the check doesn't work for the gmux on one
>> particular model. On that machine the gmux device is present but doesn't
>> control the backlight, yet reads and writes to the backlight ports
>> behave the same as if it did control the backlight. That's the only
>> model with a gmux that doesn't control the backlight that I've been able
>> to get any testing on, so I don't know how other models behave.
>>
>> But you're right, by and large all the legacy and gmux paths share are
>> boilerplate code, other than the fact that they support the same class
>> of machines. The way apple_bl is done it seems natural to extended it
>> with other backlight interfaces for Apple hardware, but an argument can
>> be made for separting it out as well.
> 
> I managed to get some more information that will allow me to do a sanity
> check on the gmux. This will be completely separate from the check for
> the legacy path, meaning they diverge even more. As a result I think I
> will go ahead and put the gmux support in its own driver. This probably
> makes the most sense anyway as the driver is likely to grow to include
> support for the display mux functionality.
> 
> But with it being its own driver, I don't think it really belongs in
> drivers/video/backlight even though the driver will initially only
> supply backlight control. Anyone have suggestions where a driver for a
> display mux device should live? I'm not seeing anywhere that looks like
> an obviously correct location.
> 

drivers/platform/x86 might be a good place to start. This were all the
laptop platform drivers go. But if the display mux is for switching between
multiple video outputs it probably wants to be integrated with the DRM
framework on the long run.

- Lars

Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

[Seth Forshee]

On Fri, Feb 10, 2012 at 10:25:34AM +0100, Lars-Peter Clausen wrote:
> On 02/09/2012 06:20 PM, Seth Forshee wrote:
> > On Mon, Feb 06, 2012 at 08:56:30AM -0600, Seth Forshee wrote:
> >>>> Of course there are a couple of ways we could get around this. Not
> >>>> calling the backlight ops in the gmux case would be an option; then you
> >>>> don't get the check, but so far as I know right now the check doesn't
> >>>> work for the gmux backlight anyway. Or allocating the backlight device
> >>>> first before doing the check, but I don't see that as a good option.
> >>>>
> >>>
> >>> Hm, if that the check doesn't do anything for gmux is there acutally any code
> >>> shared between the gmux and the legacy path in the driver? If not would make
> >>> sense to put the gmux backlight support into its own driver?
> >>
> >> What I know is that the check doesn't work for the gmux on one
> >> particular model. On that machine the gmux device is present but doesn't
> >> control the backlight, yet reads and writes to the backlight ports
> >> behave the same as if it did control the backlight. That's the only
> >> model with a gmux that doesn't control the backlight that I've been able
> >> to get any testing on, so I don't know how other models behave.
> >>
> >> But you're right, by and large all the legacy and gmux paths share are
> >> boilerplate code, other than the fact that they support the same class
> >> of machines. The way apple_bl is done it seems natural to extended it
> >> with other backlight interfaces for Apple hardware, but an argument can
> >> be made for separting it out as well.
> > 
> > I managed to get some more information that will allow me to do a sanity
> > check on the gmux. This will be completely separate from the check for
> > the legacy path, meaning they diverge even more. As a result I think I
> > will go ahead and put the gmux support in its own driver. This probably
> > makes the most sense anyway as the driver is likely to grow to include
> > support for the display mux functionality.
> > 
> > But with it being its own driver, I don't think it really belongs in
> > drivers/video/backlight even though the driver will initially only
> > supply backlight control. Anyone have suggestions where a driver for a
> > display mux device should live? I'm not seeing anywhere that looks like
> > an obviously correct location.
> > 
> 
> drivers/platform/x86 might be a good place to start. This were all the
> laptop platform drivers go. But if the display mux is for switching between
> multiple video outputs it probably wants to be integrated with the DRM
> framework on the long run.

Yeah, while this seems a little different than most of the platform-x86
drivers, that may be the best choice available.

The display mux is for switching between the video outputs between the
two GPUs in the system, which would be handled by registering a
vga_switcheroo handler. I hope to get to that eventually, but this
machine has more serious issues to get resolved first. The hybrid
graphics is only supported when doing native EFI booting, which atm
requires patching the video drivers anyway, so it's not a common use
case.

Thanks,
Seth

[PATCH 3/3] apple_bl: Add support for gmux backlight control

[Seth Forshee]

On many Apple models the gmux device, which controls the display mux,
also supports control of the display backlight. On some models this is
the only way to adjust screen brightness.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/video/backlight/apple_bl.c |  183 +++++++++++++++++++++++++++++++-----
 1 files changed, 160 insertions(+), 23 deletions(-)

diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c
index e65b459..b705dc2 100644
--- a/drivers/video/backlight/apple_bl.c
+++ b/drivers/video/backlight/apple_bl.c
@@ -27,7 +27,13 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 
+#define APPLE_BL_ID	"APP0002"
+#define APPLE_GMUX_ID	"APP000B"
+
 struct apple_bl_data {
+	const char *name;
+	int max_brightness;
+
 	/* I/O resource to allocate. */
 	unsigned long iostart;
 	unsigned long iolen;
@@ -41,6 +47,37 @@ struct apple_bl_data {
 };
 
 /*
+ * gmux port offsets. Many of these are not yet used, but may be in the
+ * future, and it's useful to have them documented here anyhow.
+ */
+#define GMUX_PORT_VERSION_MAJOR		0x04
+#define GMUX_PORT_VERSION_MINOR		0x05
+#define GMUX_PORT_VERSION_RELEASE	0x06
+#define GMUX_PORT_SWITCH_DISPLAY	0x10
+#define GMUX_PORT_SWITCH_GET_DISPLAY	0x11
+#define GMUX_PORT_INTERRUPT_ENABLE	0x14
+#define GMUX_PORT_INTERRUPT_STATUS	0x16
+#define GMUX_PORT_SWITCH_DDC		0x28
+#define GMUX_PORT_SWITCH_EXTERNAL	0x40
+#define GMUX_PORT_SWITCH_GET_EXTERNAL	0x41
+#define GMUX_PORT_DISCRETE_POWER	0x50
+#define GMUX_PORT_MAX_BRIGHTNESS	0x70
+#define GMUX_PORT_BRIGHTNESS		0x74
+
+#define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
+
+#define GMUX_INTERRUPT_ENABLE		0xff
+#define GMUX_INTERRUPT_DISABLE		0x00
+
+#define GMUX_INTERRUPT_STATUS_ACTIVE	0
+#define GMUX_INTERRUPT_STATUS_DISPLAY	(1 << 0)
+#define GMUX_INTERRUPT_STATUS_POWER	(1 << 2)
+#define GMUX_INTERRUPT_STATUS_HOTPLUG	(1 << 3)
+
+#define GMUX_BRIGHTNESS_MASK		0x00ffffff
+#define GMUX_MAX_BRIGHTNESS		GMUX_BRIGHTNESS_MASK
+
+/*
  * Implementation for machines with Intel chipset.
  */
 static void intel_chipset_set_brightness(struct apple_bl_data *bl_data,
@@ -87,6 +124,28 @@ static int nvidia_chipset_get_brightness(struct apple_bl_data *bl_data)
 }
 
 /*
+ * Implementation for gmux backlight control
+ */
+static void gmux_set_brightness(struct apple_bl_data *bl_data, int intensity)
+{
+	/*
+	 * Older versions of gmux require writing out lower bytes first
+	 * then setting upper byte to 0 to flush values. Newer versions
+	 * accept a single u32 write, but the old method works as well
+	 * so just use it for everything.
+	 */
+	outb(intensity, bl_data->iostart + GMUX_PORT_BRIGHTNESS);
+	outb(intensity >> 8, bl_data->iostart + GMUX_PORT_BRIGHTNESS + 1);
+	outb(intensity >> 16, bl_data->iostart + GMUX_PORT_BRIGHTNESS + 2);
+	outb(0, bl_data->iostart + GMUX_PORT_BRIGHTNESS + 3);
+}
+
+static int gmux_get_brightness(struct apple_bl_data *bl_data)
+{
+	return inl(bl_data->iostart + GMUX_PORT_BRIGHTNESS) & GMUX_BRIGHTNESS_MASK;
+}
+
+/*
  * Backlight device class operations
  */
 static int apple_bl_get_brightness(struct backlight_device *bd)
@@ -108,54 +167,131 @@ static const struct backlight_ops apple_bl_ops = {
 	.update_status = apple_bl_update_status,
 };
 
-static int __devinit apple_bl_add(struct acpi_device *dev)
+static int __devinit legacy_bl_init(struct apple_bl_data *bl_data,
+				    struct acpi_device *dev)
 {
-	struct apple_bl_data *bl_data;
-	struct backlight_properties props;
-	struct backlight_device *bdev;
 	struct pci_dev *host;
 	unsigned short vendor;
-	int intensity;
-	int ret = -ENODEV;
-
-	bl_data = kzalloc(sizeof(*bl_data), GFP_KERNEL);
-	if (!bl_data)
-		return -ENOMEM;
-	dev->driver_data = dev;
 
 	host = pci_get_bus_and_slot(0, 0);
-
 	if (!host) {
 		pr_err("unable to find PCI host\n");
-		goto err_free;
+		return -ENODEV;
 	}
 
 	vendor = host->vendor;
 	pci_dev_put(host);
 
-	if (vendor == PCI_VENDOR_ID_INTEL) {
+	switch (vendor) {
+	case PCI_VENDOR_ID_INTEL:
 		bl_data->iostart = 0xb2;
 		bl_data->iolen = 2;
 		bl_data->get_brightness = intel_chipset_get_brightness;
 		bl_data->set_brightness = intel_chipset_set_brightness;
-	} else if (vendor == PCI_VENDOR_ID_NVIDIA) {
+		break;
+	case PCI_VENDOR_ID_NVIDIA:
 		bl_data->iostart = 0x52e;
 		bl_data->iolen = 2;
 		bl_data->get_brightness = nvidia_chipset_get_brightness;
 		bl_data->set_brightness = nvidia_chipset_set_brightness;
-	} else {
-		pr_err("unknown hardware\n");
-		goto err_free;
+		break;
+	default:
+		pr_err("no backlight support for PCI vendor %hu\n", vendor);
+		return -ENODEV;
+	}
+
+	bl_data->name = "apple_backlight";
+	bl_data->max_brightness = 15;
+	return 0;
+}
+
+static acpi_status __devinit gmux_get_resources(struct acpi_resource *res,
+						void *context)
+{
+	struct apple_bl_data *bl_data = context;
+	struct acpi_resource_io *io;
+
+	if (res->type == ACPI_RESOURCE_TYPE_IO) {
+		io = &res->data.io;
+		bl_data->iostart = io->minimum;
+		bl_data->iolen = io->maximum - io->minimum;
+
+		return AE_CTRL_TERMINATE;
+	}
+
+	return AE_OK;
+}
+
+static int __devinit gmux_init(struct apple_bl_data *bl_data,
+			       struct acpi_device *dev)
+{
+	acpi_status status;
+
+	status = acpi_walk_resources(dev->handle, METHOD_NAME__CRS,
+				     gmux_get_resources, bl_data);
+	if (ACPI_FAILURE(status))
+		return -ENXIO;
+
+	if (!bl_data->iostart) {
+		pr_err("Failed to find gmux I/O resources\n");
+		return -ENXIO;
+	}
+
+	if (bl_data->iolen < GMUX_MIN_IO_LEN) {
+		pr_err("gmux I/O region too small (%lu < %u)\n",
+		       bl_data->iolen, GMUX_MIN_IO_LEN);
+		return -ENXIO;
 	}
 
+	bl_data->name = "gmux_backlight";
+	bl_data->get_brightness = gmux_get_brightness;
+	bl_data->set_brightness = gmux_set_brightness;
+	bl_data->max_brightness =
+		inl(bl_data->iostart + GMUX_PORT_MAX_BRIGHTNESS);
+
+	/*
+	 * Currently it's assumed that the maximum brightness is less
+	 * than 2^24 for compatibility with old gmux versions. Cap the
+	 * max brightness at this max value, but print a warning if
+	 * the hardware reports something higher so it can be fixed.
+	 */
+	if (WARN_ON(bl_data->max_brightness > GMUX_MAX_BRIGHTNESS))
+		bl_data->max_brightness = GMUX_MAX_BRIGHTNESS;
+
+	return 0;
+}
+
+static int __devinit apple_bl_add(struct acpi_device *dev)
+{
+	struct apple_bl_data *bl_data;
+	struct backlight_properties props;
+	struct backlight_device *bdev;
+	int intensity;
+	int ret = -ENODEV;
+
+	bl_data = kzalloc(sizeof(*bl_data), GFP_KERNEL);
+	if (!bl_data)
+		return -ENOMEM;
+	dev->driver_data = bl_data;
+
+	if (!strcmp(acpi_device_hid(dev), APPLE_GMUX_ID))
+		ret = gmux_init(bl_data, dev);
+	else
+		ret = legacy_bl_init(bl_data, dev);
+
+	if (ret)
+		goto err_free;
+
 	/* Check that the hardware responds - this may not work under EFI */
 
 	intensity = bl_data->get_brightness(bl_data);
 
 	if (!intensity) {
 		bl_data->set_brightness(bl_data, 1);
-		if (!bl_data->get_brightness(bl_data))
+		if (!bl_data->get_brightness(bl_data)) {
+			ret = -ENODEV;
 			goto err_free;
+		}
 
 		bl_data->set_brightness(bl_data, 0);
 	}
@@ -168,8 +304,8 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
-	props.max_brightness = 15;
-	bdev = backlight_device_register("apple_backlight", NULL, bl_data,
+	props.max_brightness = bl_data->max_brightness;
+	bdev = backlight_device_register(bl_data->name, &dev->dev, bl_data,
 					 &apple_bl_ops, &props);
 
 	if (IS_ERR(bdev)) {
@@ -178,7 +314,7 @@ static int __devinit apple_bl_add(struct acpi_device *dev)
 	}
 
 	bl_data->bdev = bdev;
-	bdev->props.brightness = bl_data->get_brightness(bl_data);
+	bdev->props.brightness = intensity;
 	backlight_update_status(bdev);
 
 	return 0;
@@ -202,7 +338,8 @@ static int __devexit apple_bl_remove(struct acpi_device *dev, int type)
 }
 
 static const struct acpi_device_id apple_bl_ids[] = {
-	{"APP0002", 0},
+	{APPLE_BL_ID, 0},
+	{APPLE_GMUX_ID, 0},
 	{"", 0},
 };
 
-- 
1.7.8.3