| Message ID | 1433941292-21530-20-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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.
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
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(-)