Re: [PATCH 19/32] dell-laptop: Port to new backlight interface selection API

From: Hans de Goede <hdegoede@redhat.com>
To: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>,
	Ike Panhc <ike.pan@canonical.com>,
	linux-acpi@vger.kernel.org, ibm-acpi-devel@lists.sourceforge.net,
	Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>,
	acpi4asus-user@lists.sourceforge.net,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	dri-devel@lists.freedesktop.org,
	Azael Avalos <coproscefalo@gmail.com>,
	Lee Chun-Yi <jlee@suse.com>, Mattia Dongili <malattia@linux.it>,
	Cezary Jackiewicz <cezary.jackiewicz@gmail.com>,
	Ben Skeggs <bskeggs@redhat.com>,
	Corentin Chary <corentin.chary@gmail.com>,
	Darren Hart <dvhart@infradead.org>,
	platform-driver-x86@vger.kernel.org,
	Aaron Lu <aaron.lu@intel.com>
Subject: Re: [PATCH 19/32] dell-laptop: Port to new backlight interface selection API
Date: Thu, 11 Jun 2015 14:29:24 +0200	[thread overview]
Message-ID: <55797F24.8060500@redhat.com> (raw)
In-Reply-To: <20150611114709.GD19820@pali>

Hi,

On 11-06-15 13:47, Pali Rohár wrote:
> On Wednesday 10 June 2015 15:01:19 Hans de Goede wrote:
>> Port the backlight selection logic to the new backlight interface
>> selection API.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/platform/x86/dell-laptop.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>> index d688d80..db2e031 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -31,6 +31,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/debugfs.h>
>>   #include <linux/seq_file.h>
>> +#include <acpi/video.h>
>>   #include "../../firmware/dcdbas.h"
>>
>>   #define BRIGHTNESS_TOKEN 0x7d
>> @@ -1921,10 +1922,7 @@ static int __init dell_init(void)
>>   				    &dell_debugfs_fops);
>>
>>   #ifdef CONFIG_ACPI
>> -	/* In the event of an ACPI backlight being available, don't
>> -	 * register the platform controller.
>> -	 */
>> -	if (acpi_video_backlight_support())
>> +	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>>   		return 0;
>>   #endif
>>
>
> Now I'm thinking... Is this correct condition? And do we need it? This
> Dell backlight interface works even if ACPI or intel i915 driver
> controls backlight. Why we should prevent dell-laptop.ko to register
> Dell backlight interface if ACPI video already register one? And why not
> prevent when intel i915 driver register another?

3 things:

1) This is mostly a cleanup series, it does not make any behavioral
changes (other then renaming the kernelcmdline option and we've
already discussed fixing that).

2) All 3 drivers ultimately bit-bang the same hardware, we really should
have only one driver banging the hardware, so
	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
		return 0;

Definitely is the right thing todo here.

3) You're right that the intel_backlight still registering even if
acpi_video_get_backlight_type() != acpi_backlight_native is weird, but
that is how it was before this series, and userspace prefers firmware
type backlight sysfs entries over platform /raw ones so it will
automatically pick the right one.  We should consider not registering
the backlight sysfs class interface from the intel code when
acpi_video_get_backlight_type() != acpi_backlight_native but that is
something for later...

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel