Message-ID: <4F2D6A21.7030903@metafoo.de>
Date:	Sat, 04 Feb 2012 18:25:53 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	linux-kernel@...r.kernel.org, Richard Purdie <rpurdie@...ys.net>,
	Matthew Garrett <mjg@...hat.com>
Subject: Re: [PATCH 2/3] apple_bl: Rework in advance of gmux backlight support

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@...onical.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists