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

["Pali Rohár" <pali.rohar@gmail.com>]

On Thursday 11 June 2015 14:29:24 Hans de Goede wrote:
> 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

Ok. This patch series is just refactor/cleanup which does not change
functional behavior. I'm ok with this dell-laptop change, so you can add
my Acked-By.

Once you will create new patches for backlight (maybe for intel i915),
please CC me, so I can discuss about them too.

-- 
Pali Rohár
pali.rohar@gmail.com