| Message ID | 20230307120540.389920-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: apple_bl: Use acpi_video_get_backlight_type() | expand |
On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: > On some MacBooks both the apple_bl and the apple-gmux backlight drivers > may be able to export a /sys/class/backlight device. > > To avoid having 2 backlight devices for one LCD panel until now > the apple-gmux driver has been calling apple_bl_unregister() to move > the apple_bl backlight device out of the way when it loads. > > Similar problems exist on other x86 laptops and all backlight drivers > which may be used on x86 laptops have moved to using > acpi_video_get_backlight_type() to determine whether they should load > or not. > > Switch apple_bl to this model too, so that it is consistent with all > the other x86 backlight drivers. > > Besides code-simplification and consistency this has 2 other benefits: > > 1) It removes a race during boot where userspace will briefly see > an apple_bl backlight and then have it disappear again, leading to e.g.: > https://bbs.archlinux.org/viewtopic.php?id=269920 > > 2) This allows user to switch between the drivers by passing > acpi_backlight=apple_gmux or acpi_backlight=vendor on the kernel > commandline. ... > + depends on ACPI_VIDEO=n || ACPI_VIDEO I'm wondering if "imply ACPI_VIDEO" (i.o.w. weak dependency) is what suitable here.
Hi, On 3/7/23 13:27, Andy Shevchenko wrote: > On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: >> On some MacBooks both the apple_bl and the apple-gmux backlight drivers >> may be able to export a /sys/class/backlight device. >> >> To avoid having 2 backlight devices for one LCD panel until now >> the apple-gmux driver has been calling apple_bl_unregister() to move >> the apple_bl backlight device out of the way when it loads. >> >> Similar problems exist on other x86 laptops and all backlight drivers >> which may be used on x86 laptops have moved to using >> acpi_video_get_backlight_type() to determine whether they should load >> or not. >> >> Switch apple_bl to this model too, so that it is consistent with all >> the other x86 backlight drivers. >> >> Besides code-simplification and consistency this has 2 other benefits: >> >> 1) It removes a race during boot where userspace will briefly see >> an apple_bl backlight and then have it disappear again, leading to e.g.: >> https://bbs.archlinux.org/viewtopic.php?id=269920 >> >> 2) This allows user to switch between the drivers by passing >> acpi_backlight=apple_gmux or acpi_backlight=vendor on the kernel >> commandline. > > ... > >> + depends on ACPI_VIDEO=n || ACPI_VIDEO > > I'm wondering if "imply ACPI_VIDEO" (i.o.w. weak dependency) is what suitable > here. No, because then if apple_bl is builtin and acpi_video is a module we will get unresolved dependencies. The quoted bit of Kconfig magic is exactly to avoid that scenario, other combinations are fine (due to stubs in acpi/video.h when disabled). Regards, Hans
On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: > On some MacBooks both the apple_bl and the apple-gmux backlight drivers > may be able to export a /sys/class/backlight device. > > To avoid having 2 backlight devices for one LCD panel until now > the apple-gmux driver has been calling apple_bl_unregister() to move > the apple_bl backlight device out of the way when it loads. > > Similar problems exist on other x86 laptops and all backlight drivers > which may be used on x86 laptops have moved to using > acpi_video_get_backlight_type() to determine whether they should load > or not. > > Switch apple_bl to this model too, so that it is consistent with all > the other x86 backlight drivers. > [snip] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> but... > --- > A note to the backlight class / subsystem maintainers, this change > applies on top of a similar patch for drivers/platform/x86/apple-gmux.c > which makes that driver use acpi_video_get_backlight_type(). See: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > I believe it is easiest to also merge this patch through > the platform-drivers-x86 tree, may I please have your Ack for this ? > --- ... please don't treat above as an ack. Lee Jones will hopefully be along shortly to discuss that! Daniel.
Hi, On 3/9/23 18:09, Daniel Thompson wrote: > On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: >> On some MacBooks both the apple_bl and the apple-gmux backlight drivers >> may be able to export a /sys/class/backlight device. >> >> To avoid having 2 backlight devices for one LCD panel until now >> the apple-gmux driver has been calling apple_bl_unregister() to move >> the apple_bl backlight device out of the way when it loads. >> >> Similar problems exist on other x86 laptops and all backlight drivers >> which may be used on x86 laptops have moved to using >> acpi_video_get_backlight_type() to determine whether they should load >> or not. >> >> Switch apple_bl to this model too, so that it is consistent with all >> the other x86 backlight drivers. >> [snip] >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> > > but... Thank you. >> --- >> A note to the backlight class / subsystem maintainers, this change >> applies on top of a similar patch for drivers/platform/x86/apple-gmux.c >> which makes that driver use acpi_video_get_backlight_type(). See: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> I believe it is easiest to also merge this patch through >> the platform-drivers-x86 tree, may I please have your Ack for this ? >> --- > > ... please don't treat above as an ack. Lee Jones will hopefully be > along shortly to discuss that! Understood. Regards, Hans
On Thu, 09 Mar 2023, Daniel Thompson wrote: > On Tue, Mar 07, 2023 at 01:05:40PM +0100, Hans de Goede wrote: > > On some MacBooks both the apple_bl and the apple-gmux backlight drivers > > may be able to export a /sys/class/backlight device. > > > > To avoid having 2 backlight devices for one LCD panel until now > > the apple-gmux driver has been calling apple_bl_unregister() to move > > the apple_bl backlight device out of the way when it loads. > > > > Similar problems exist on other x86 laptops and all backlight drivers > > which may be used on x86 laptops have moved to using > > acpi_video_get_backlight_type() to determine whether they should load > > or not. > > > > Switch apple_bl to this model too, so that it is consistent with all > > the other x86 backlight drivers. > > [snip] > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> > > but... > > > --- > > A note to the backlight class / subsystem maintainers, this change > > applies on top of a similar patch for drivers/platform/x86/apple-gmux.c > > which makes that driver use acpi_video_get_backlight_type(). See: > > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > > > I believe it is easiest to also merge this patch through > > the platform-drivers-x86 tree, may I please have your Ack for this ? > > --- > > ... please don't treat above as an ack. Lee Jones will hopefully be > along shortly to discuss that! That's fine. I will need a succinct, immutable pull-request though. Acked-by: Lee Jones <lee@kernel.org> -- Lee Jones [李琼斯]
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index d2619e7025c7..aa8df8d4aee9 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -206,7 +206,6 @@ config APPLE_GMUX depends on ACPI && PCI depends on PNP depends on BACKLIGHT_CLASS_DEVICE - depends on BACKLIGHT_APPLE=n || BACKLIGHT_APPLE help This driver provides support for the gmux device found on many Apple laptops, which controls the display mux for the hybrid diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 787cf2a7e268..f490565e1ed1 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -16,7 +16,6 @@ #include <linux/backlight.h> #include <linux/acpi.h> #include <linux/pnp.h> -#include <linux/apple_bl.h> #include <linux/apple-gmux.h> #include <linux/slab.h> #include <linux/delay.h> @@ -884,14 +883,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) gmux_data->bdev = bdev; bdev->props.brightness = gmux_get_brightness(bdev); backlight_update_status(bdev); - - /* - * The backlight situation on Macs is complicated. If the gmux is - * present it's the best choice, because it always works for - * backlight control and supports more levels than other options. - * Disable the other backlight choices. - */ - apple_bl_unregister(); } gmux_data->power_state = VGA_SWITCHEROO_ON; @@ -1008,8 +999,6 @@ static void gmux_remove(struct pnp_dev *pnp) release_region(gmux_data->iostart, gmux_data->iolen); apple_gmux_data = NULL; kfree(gmux_data); - - apple_bl_register(); } static const struct pnp_device_id gmux_device_ids[] = { diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 4c33e971c0f0..51387b1ef012 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -285,6 +285,7 @@ config BACKLIGHT_MT6370 config BACKLIGHT_APPLE tristate "Apple Backlight Driver" depends on X86 && ACPI + depends on ACPI_VIDEO=n || ACPI_VIDEO help If you have an Intel-based Apple say Y to enable a driver for its backlight. diff --git a/drivers/video/backlight/apple_bl.c b/drivers/video/backlight/apple_bl.c index e9e7acb577bf..aaa824437a2a 100644 --- a/drivers/video/backlight/apple_bl.c +++ b/drivers/video/backlight/apple_bl.c @@ -24,7 +24,7 @@ #include <linux/pci.h> #include <linux/acpi.h> #include <linux/atomic.h> -#include <linux/apple_bl.h> +#include <acpi/video.h> static struct backlight_device *apple_backlight_device; @@ -215,32 +215,21 @@ static struct acpi_driver apple_bl_driver = { }, }; -static atomic_t apple_bl_registered = ATOMIC_INIT(0); - -int apple_bl_register(void) -{ - if (atomic_xchg(&apple_bl_registered, 1) == 0) - return acpi_bus_register_driver(&apple_bl_driver); - - return 0; -} -EXPORT_SYMBOL_GPL(apple_bl_register); - -void apple_bl_unregister(void) -{ - if (atomic_xchg(&apple_bl_registered, 0) == 1) - acpi_bus_unregister_driver(&apple_bl_driver); -} -EXPORT_SYMBOL_GPL(apple_bl_unregister); - static int __init apple_bl_init(void) { - return apple_bl_register(); + /* + * Use ACPI video detection code to see if this driver should register + * or if another driver, e.g. the apple-gmux driver should be used. + */ + if (acpi_video_get_backlight_type() != acpi_backlight_vendor) + return -ENODEV; + + return acpi_bus_register_driver(&apple_bl_driver); } static void __exit apple_bl_exit(void) { - apple_bl_unregister(); + acpi_bus_unregister_driver(&apple_bl_driver); } module_init(apple_bl_init); diff --git a/include/linux/apple_bl.h b/include/linux/apple_bl.h deleted file mode 100644 index 445af2e3cc21..000000000000 --- a/include/linux/apple_bl.h +++ /dev/null @@ -1,27 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -/* - * apple_bl exported symbols - */ - -#ifndef _LINUX_APPLE_BL_H -#define _LINUX_APPLE_BL_H - -#if defined(CONFIG_BACKLIGHT_APPLE) || defined(CONFIG_BACKLIGHT_APPLE_MODULE) - -extern int apple_bl_register(void); -extern void apple_bl_unregister(void); - -#else /* !CONFIG_BACKLIGHT_APPLE */ - -static inline int apple_bl_register(void) -{ - return 0; -} - -static inline void apple_bl_unregister(void) -{ -} - -#endif /* !CONFIG_BACKLIGHT_APPLE */ - -#endif /* _LINUX_APPLE_BL_H */
On some MacBooks both the apple_bl and the apple-gmux backlight drivers may be able to export a /sys/class/backlight device. To avoid having 2 backlight devices for one LCD panel until now the apple-gmux driver has been calling apple_bl_unregister() to move the apple_bl backlight device out of the way when it loads. Similar problems exist on other x86 laptops and all backlight drivers which may be used on x86 laptops have moved to using acpi_video_get_backlight_type() to determine whether they should load or not. Switch apple_bl to this model too, so that it is consistent with all the other x86 backlight drivers. Besides code-simplification and consistency this has 2 other benefits: 1) It removes a race during boot where userspace will briefly see an apple_bl backlight and then have it disappear again, leading to e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 2) This allows user to switch between the drivers by passing acpi_backlight=apple_gmux or acpi_backlight=vendor on the kernel commandline. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- A note to the backlight class / subsystem maintainers, this change applies on top of a similar patch for drivers/platform/x86/apple-gmux.c which makes that driver use acpi_video_get_backlight_type(). See: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I believe it is easiest to also merge this patch through the platform-drivers-x86 tree, may I please have your Ack for this ? --- drivers/platform/x86/Kconfig | 1 - drivers/platform/x86/apple-gmux.c | 11 ----------- drivers/video/backlight/Kconfig | 1 + drivers/video/backlight/apple_bl.c | 31 ++++++++++-------------------- include/linux/apple_bl.h | 27 -------------------------- 5 files changed, 11 insertions(+), 60 deletions(-) delete mode 100644 include/linux/apple_bl.h