| Message ID | 20230123113750.462144-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | ACPI: video/apple-gmux: Improve apple-gmux backlight detection | expand |
On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: > --- a/include/linux/apple-gmux.h > +++ b/include/linux/apple-gmux.h [...] > +static inline bool apple_gmux_is_indexed(unsigned long iostart) > +{ > + u16 val; > + > + outb(0xaa, iostart + 0xcc); > + outb(0x55, iostart + 0xcd); > + outb(0x00, iostart + 0xce); > + > + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); > + if (val == 0x55aa) > + return true; > + > + return false; > +} Something like this, and especially the large apple_gmux_detect() below, should not live in a header file. Why can't apple_gmux.ko just export a detection function which is used both internally and as a helper by the backlight detection? Thanks, Lukas > > /** > - * apple_gmux_present() - detect if gmux is built into the machine > + * apple_gmux_detect() - detect if gmux is built into the machine > + * > + * @pnp_dev: Device to probe or NULL to use the first matching device > + * @indexed_ret: Returns (by reference) if the gmux is indexed or not > + * > + * Detect if a supported gmux device is present by actually probing it. > + * This avoids the false positives returned on some models by > + * apple_gmux_present(). > + * > + * Return: %true if a supported gmux ACPI device is detected and the kernel > + * was configured with CONFIG_APPLE_GMUX, %false otherwise. > + */ > +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) > +{ > + u8 ver_major, ver_minor, ver_release; > + struct resource *res; > + bool indexed = false; > + > + if (!pnp_dev) { > + struct acpi_device *adev; > + struct device *dev; > + > + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); > + if (!adev) > + return false; > + > + dev = acpi_get_first_physical_node(adev); > + if (!dev) > + return false; > + > + pnp_dev = to_pnp_dev(dev); > + } > + > + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); > + if (!res) > + return false; > + > + if (resource_size(res) < GMUX_MIN_IO_LEN) > + return false; > + > + /* > + * Invalid version information may indicate either that the gmux > + * device isn't present or that it's a new one that uses indexed io. > + */ > + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); > + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); > + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); > + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { > + indexed = apple_gmux_is_indexed(res->start); > + if (!indexed) > + return false; > + } > + > + if (indexed_ret) > + *indexed_ret = indexed; > + > + return true; > +} > + > +/** > + * apple_gmux_present() - check if gmux ACPI device is present > * > * Drivers may use this to activate quirks specific to dual GPU MacBook Pros > * and Mac Pros, e.g. for deferred probing, runtime pm and backlight. > * > - * Return: %true if gmux is present and the kernel was configured > + * Return: %true if gmux ACPI device is present and the kernel was configured > * with CONFIG_APPLE_GMUX, %false otherwise. > */ > static inline bool apple_gmux_present(void) > @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void) > return false; > } > > +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) > +{ > + return false; > +} > + > #endif /* !CONFIG_APPLE_GMUX */ > > #endif /* LINUX_APPLE_GMUX_H */ > -- > 2.39.0
Hi, On 1/23/23 14:49, Lukas Wunner wrote: > On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >> --- a/include/linux/apple-gmux.h >> +++ b/include/linux/apple-gmux.h > [...] >> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >> +{ >> + u16 val; >> + >> + outb(0xaa, iostart + 0xcc); >> + outb(0x55, iostart + 0xcd); >> + outb(0x00, iostart + 0xce); >> + >> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >> + if (val == 0x55aa) >> + return true; >> + >> + return false; >> +} > > Something like this, and especially the large apple_gmux_detect() below, > should not live in a header file. I understand where you are coming from, but these functions really are not that large. > Why can't apple_gmux.ko just export a detection function which is used > both internally and as a helper by the backlight detection? Both the acpi-video code and the apple-gmux code can be built as modules. So this will break if the acpi-video code get builtin and the apple-gmux code does not. This can be worked around in Kconfig by adding something like: depends on APPLE_GMUX || APPLE_GMUX=n to the ACPI_VIDEO Kconfig bits and then cross our fingers we don't get some Kconfig circular dep thing causing things to error out. The whole idea behind the video-detect.c code is that it does as little as possible to figure out which backlight control method to use. It e.g. on purpose does not depend on the GPU drivers to see if native GPU backlight control is available that would bring in a whole lot of dependencies. So the do not depend on other kernel-modules / driver-code is part of the design of video-detect.c (in so far as it was ever designed, since it also very much has organically grown / evolved into its current code). If we forgo this design principle then we evt would end adding similar Kconfig snippets for each backlight device-type which the video-detect code supports this quickly gets unwieldly. And doing this also means that video.ko now starts depending on not just apple-gmux.ko but also on its dependencies, although in this case that would not bring in any extra dependencies. But for ohter types there might very well be significant dependencies. So waying the cons and pros here, as well as trying to be consistent and not add dependencies on other kernel-modules just for detection purposes, I would prefer to keep using the static inline approach for this. Regards, Hans >> >> /** >> - * apple_gmux_present() - detect if gmux is built into the machine >> + * apple_gmux_detect() - detect if gmux is built into the machine >> + * >> + * @pnp_dev: Device to probe or NULL to use the first matching device >> + * @indexed_ret: Returns (by reference) if the gmux is indexed or not >> + * >> + * Detect if a supported gmux device is present by actually probing it. >> + * This avoids the false positives returned on some models by >> + * apple_gmux_present(). >> + * >> + * Return: %true if a supported gmux ACPI device is detected and the kernel >> + * was configured with CONFIG_APPLE_GMUX, %false otherwise. >> + */ >> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) >> +{ >> + u8 ver_major, ver_minor, ver_release; >> + struct resource *res; >> + bool indexed = false; >> + >> + if (!pnp_dev) { >> + struct acpi_device *adev; >> + struct device *dev; >> + >> + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); >> + if (!adev) >> + return false; >> + >> + dev = acpi_get_first_physical_node(adev); >> + if (!dev) >> + return false; >> + >> + pnp_dev = to_pnp_dev(dev); >> + } >> + >> + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); >> + if (!res) >> + return false; >> + >> + if (resource_size(res) < GMUX_MIN_IO_LEN) >> + return false; >> + >> + /* >> + * Invalid version information may indicate either that the gmux >> + * device isn't present or that it's a new one that uses indexed io. >> + */ >> + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); >> + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); >> + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); >> + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { >> + indexed = apple_gmux_is_indexed(res->start); >> + if (!indexed) >> + return false; >> + } >> + >> + if (indexed_ret) >> + *indexed_ret = indexed; >> + >> + return true; >> +} >> + >> +/** >> + * apple_gmux_present() - check if gmux ACPI device is present >> * >> * Drivers may use this to activate quirks specific to dual GPU MacBook Pros >> * and Mac Pros, e.g. for deferred probing, runtime pm and backlight. >> * >> - * Return: %true if gmux is present and the kernel was configured >> + * Return: %true if gmux ACPI device is present and the kernel was configured >> * with CONFIG_APPLE_GMUX, %false otherwise. >> */ >> static inline bool apple_gmux_present(void) >> @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void) >> return false; >> } >> >> +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) >> +{ >> + return false; >> +} >> + >> #endif /* !CONFIG_APPLE_GMUX */ >> >> #endif /* LINUX_APPLE_GMUX_H */ >> -- >> 2.39.0 >
On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote: > On 1/23/23 14:49, Lukas Wunner wrote: > > On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: > > > --- a/include/linux/apple-gmux.h > > > +++ b/include/linux/apple-gmux.h > > [...] > > > +static inline bool apple_gmux_is_indexed(unsigned long iostart) > > > +{ > > > + u16 val; > > > + > > > + outb(0xaa, iostart + 0xcc); > > > + outb(0x55, iostart + 0xcd); > > > + outb(0x00, iostart + 0xce); > > > + > > > + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); > > > + if (val == 0x55aa) > > > + return true; > > > + > > > + return false; > > > +} > > > > Something like this, and especially the large apple_gmux_detect() below, > > should not live in a header file. > > I understand where you are coming from, but these functions really > are not that large. > > > Why can't apple_gmux.ko just export a detection function which is used > > both internally and as a helper by the backlight detection? > > Both the acpi-video code and the apple-gmux code can be built as > modules. So this will break if the acpi-video code get builtin > and the apple-gmux code does not. > > This can be worked around in Kconfig by adding something like: > > depends on APPLE_GMUX || APPLE_GMUX=n > > to the ACPI_VIDEO Kconfig bits and then cross our fingers > we don't get some Kconfig circular dep thing causing things > to error out. Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is? Would making APPLE_GMUX depend on ACPI_VIDEO (instead of "ACPI_VIDEO=n || ACPI_VIDEO") achieve that? I believe APPLE_GMUX would then inherit the setting of ACPI_VIDEO? Thanks, Lukas
Hi, On 1/23/23 15:23, Lukas Wunner wrote: > On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote: >> On 1/23/23 14:49, Lukas Wunner wrote: >>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >>>> --- a/include/linux/apple-gmux.h >>>> +++ b/include/linux/apple-gmux.h >>> [...] >>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >>>> +{ >>>> + u16 val; >>>> + >>>> + outb(0xaa, iostart + 0xcc); >>>> + outb(0x55, iostart + 0xcd); >>>> + outb(0x00, iostart + 0xce); >>>> + >>>> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >>>> + if (val == 0x55aa) >>>> + return true; >>>> + >>>> + return false; >>>> +} >>> >>> Something like this, and especially the large apple_gmux_detect() below, >>> should not live in a header file. >> >> I understand where you are coming from, but these functions really >> are not that large. >> >>> Why can't apple_gmux.ko just export a detection function which is used >>> both internally and as a helper by the backlight detection? >> >> Both the acpi-video code and the apple-gmux code can be built as >> modules. So this will break if the acpi-video code get builtin >> and the apple-gmux code does not. >> >> This can be worked around in Kconfig by adding something like: >> >> depends on APPLE_GMUX || APPLE_GMUX=n >> >> to the ACPI_VIDEO Kconfig bits and then cross our fingers >> we don't get some Kconfig circular dep thing causing things >> to error out. > > Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is? > > Would making APPLE_GMUX depend on ACPI_VIDEO (instead of > "ACPI_VIDEO=n || ACPI_VIDEO") achieve that? I believe > APPLE_GMUX would then inherit the setting of ACPI_VIDEO? I'm afraid that won't work, make it depend on ACPI_VIDEO would not make it inherit ACPI_VIDEO's setting instead it would be limited to ACPI_VIDEO's setting. So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m then APPLE_GMUX can only be m. Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO" bit is obsolete and should be dropped (I have already prepared a patch for this), the apple_gmux code no longer depends on any of the ACPI_VIDEO symbols. Which does make it possible to add a dependency the other way. I just tried the following: --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -210,6 +210,8 @@ config ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE depends on INPUT depends on ACPI_WMI || !X86 + # ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled + depends on APPLE_GMUX || APPLE_GMUX=n select THERMAL help This driver implements the ACPI Extensions For Display Adapters And that does not cause any circular dep issues it seems, so If we really want to have the detection code inside apple_gmux then we could use the above and have the acpi-video code depend on apple_gmux.ko. I'm not a fan of that though, as mentioned before the intent for the acpi-video code's detection parts is to be as much standalone code as possible. Regards, Hans
Hi, On 1/23/23 16:05, Hans de Goede wrote: > Hi, > > On 1/23/23 15:23, Lukas Wunner wrote: >> On Mon, Jan 23, 2023 at 03:13:28PM +0100, Hans de Goede wrote: >>> On 1/23/23 14:49, Lukas Wunner wrote: >>>> On Mon, Jan 23, 2023 at 12:37:49PM +0100, Hans de Goede wrote: >>>>> --- a/include/linux/apple-gmux.h >>>>> +++ b/include/linux/apple-gmux.h >>>> [...] >>>>> +static inline bool apple_gmux_is_indexed(unsigned long iostart) >>>>> +{ >>>>> + u16 val; >>>>> + >>>>> + outb(0xaa, iostart + 0xcc); >>>>> + outb(0x55, iostart + 0xcd); >>>>> + outb(0x00, iostart + 0xce); >>>>> + >>>>> + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); >>>>> + if (val == 0x55aa) >>>>> + return true; >>>>> + >>>>> + return false; >>>>> +} >>>> >>>> Something like this, and especially the large apple_gmux_detect() below, >>>> should not live in a header file. >>> >>> I understand where you are coming from, but these functions really >>> are not that large. >>> >>>> Why can't apple_gmux.ko just export a detection function which is used >>>> both internally and as a helper by the backlight detection? >>> >>> Both the acpi-video code and the apple-gmux code can be built as >>> modules. So this will break if the acpi-video code get builtin >>> and the apple-gmux code does not. >>> >>> This can be worked around in Kconfig by adding something like: >>> >>> depends on APPLE_GMUX || APPLE_GMUX=n >>> >>> to the ACPI_VIDEO Kconfig bits and then cross our fingers >>> we don't get some Kconfig circular dep thing causing things >>> to error out. >> >> Can we force APPLE_GMUX to be built-in if ACPI_VIDEO is? >> >> Would making APPLE_GMUX depend on ACPI_VIDEO (instead of >> "ACPI_VIDEO=n || ACPI_VIDEO") achieve that? I believe >> APPLE_GMUX would then inherit the setting of ACPI_VIDEO? > > I'm afraid that won't work, make it depend on ACPI_VIDEO would not > make it inherit ACPI_VIDEO's setting instead it would be limited > to ACPI_VIDEO's setting. > > So if we make APPLE_GMUX "depends on ACPI_VIDEO" and ACPI_VIDEO=y > then APPLE_GMUX can be both Y or M, where as if ACPI_VIDEO=m > then APPLE_GMUX can only be m. > > Note that the APPLE_GMUX Kconfig "ACPI_VIDEO=n || ACPI_VIDEO" > bit is obsolete and should be dropped (I have already prepared > a patch for this), the apple_gmux code no longer depends on any > of the ACPI_VIDEO symbols. Which does make it possible to > add a dependency the other way. > > I just tried the following: > > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -210,6 +210,8 @@ config ACPI_VIDEO > depends on BACKLIGHT_CLASS_DEVICE > depends on INPUT > depends on ACPI_WMI || !X86 > + # ACPI_VIDEO uses symbols from APPLE_GMUX if that is enabled > + depends on APPLE_GMUX || APPLE_GMUX=n > select THERMAL > help > This driver implements the ACPI Extensions For Display Adapters > > And that does not cause any circular dep issues it seems And 10 seconds after hitting send I realized that things are not this simple, because a bunch of other Kconfig bits do "select ACPI_VIDEO" and Kconfig bits doing "select FOO" much make sure all the dependencies of FOO are met and we have just added a new dependency... So all those other places then would need something similar. Kconfig is great, but with intra dependencies it really can get quite hairy. So IMHO the (really not that big) static inline detect function really is the best solution here. Regards, Hans
On Mon, Jan 23, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Add a new (static inline) apple_gmux_detect() helper to apple-gmux.h > which can be used for gmux detection instead of apple_gmux_present(). > > The latter is not really reliable since an ACPI device with a HID > of APP000B is present on some devices without a gmux at all, as well > as on devices with a newer (unsupported) MMIO based gmux model. > > This causes apple_gmux_present() to return false-postives on positives > a number of different Apple laptop models. > > This new helper uses the same probing as the actual apple-gmux > driver, so that it does not return false positives. > > To avoid code duplication the gmux_probe() function of the actual > driver is also moved over to using the new apple_gmux_detect() helper. ... > + if (!apple_gmux_detect(pnp, &indexed)) { > + pr_info("gmux device not present\n"); You may start using dev_info(&pnp->dev, ...) if I'm not mistaken. > + return -ENODEV; > + } ... > +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) > +{ > + u8 ver_major, ver_minor, ver_release; > + struct resource *res; > + bool indexed = false; > + > + if (!pnp_dev) { > + struct acpi_device *adev; > + struct device *dev; > + > + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); > + if (!adev) > + return false; > + > + dev = acpi_get_first_physical_node(adev); > + if (!dev) I remember I saw something like this in your tree(?). I hope it's not pending upstream (yet) because of a leak here. Don't forget to call acpi_dev_put() after you finish with adev. Recently I have fixed a bunch of similar issues in ASoC Intel. > + return false; > + > + pnp_dev = to_pnp_dev(dev); > + } > + > + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); > + if (!res) > + return false; > + > + if (resource_size(res) < GMUX_MIN_IO_LEN) > + return false; > + > + /* > + * Invalid version information may indicate either that the gmux > + * device isn't present or that it's a new one that uses indexed io. > + */ > + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); > + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); > + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); > + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { > + indexed = apple_gmux_is_indexed(res->start); > + if (!indexed) > + return false; > + } > + > + if (indexed_ret) > + *indexed_ret = indexed; > + > + return true; > +}
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index a0af01f6a0fd..9333f82cfa8a 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -226,23 +226,6 @@ static void gmux_write32(struct apple_gmux_data *gmux_data, int port, gmux_pio_write32(gmux_data, port, val); } -static bool gmux_is_indexed(struct apple_gmux_data *gmux_data) -{ - u16 val; - - outb(0xaa, gmux_data->iostart + 0xcc); - outb(0x55, gmux_data->iostart + 0xcd); - outb(0x00, gmux_data->iostart + 0xce); - - val = inb(gmux_data->iostart + 0xcc) | - (inb(gmux_data->iostart + 0xcd) << 8); - - if (val == 0x55aa) - return true; - - return false; -} - /** * DOC: Backlight control * @@ -582,60 +565,43 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) int ret = -ENXIO; acpi_status status; unsigned long long gpe; + bool indexed = false; + u32 version; if (apple_gmux_data) return -EBUSY; + if (!apple_gmux_detect(pnp, &indexed)) { + pr_info("gmux device not present\n"); + return -ENODEV; + } + gmux_data = kzalloc(sizeof(*gmux_data), GFP_KERNEL); if (!gmux_data) return -ENOMEM; pnp_set_drvdata(pnp, gmux_data); res = pnp_get_resource(pnp, IORESOURCE_IO, 0); - if (!res) { - pr_err("Failed to find gmux I/O resource\n"); - goto err_free; - } - gmux_data->iostart = res->start; gmux_data->iolen = resource_size(res); - if (gmux_data->iolen < GMUX_MIN_IO_LEN) { - pr_err("gmux I/O region too small (%lu < %u)\n", - gmux_data->iolen, GMUX_MIN_IO_LEN); - goto err_free; - } - if (!request_region(gmux_data->iostart, gmux_data->iolen, "Apple gmux")) { pr_err("gmux I/O already in use\n"); goto err_free; } - /* - * Invalid version information may indicate either that the gmux - * device isn't present or that it's a new one that uses indexed - * io - */ - - ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR); - ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR); - ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE); - if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { - if (gmux_is_indexed(gmux_data)) { - u32 version; - mutex_init(&gmux_data->index_lock); - gmux_data->indexed = true; - version = gmux_read32(gmux_data, - GMUX_PORT_VERSION_MAJOR); - ver_major = (version >> 24) & 0xff; - ver_minor = (version >> 16) & 0xff; - ver_release = (version >> 8) & 0xff; - } else { - pr_info("gmux device not present\n"); - ret = -ENODEV; - goto err_release; - } + if (indexed) { + mutex_init(&gmux_data->index_lock); + gmux_data->indexed = true; + version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR); + ver_major = (version >> 24) & 0xff; + ver_minor = (version >> 16) & 0xff; + ver_release = (version >> 8) & 0xff; + } else { + ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR); + ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR); + ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE); } pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor, ver_release, (gmux_data->indexed ? "indexed" : "classic")); diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h index 80efaaf89e07..47c04e7e3672 100644 --- a/include/linux/apple-gmux.h +++ b/include/linux/apple-gmux.h @@ -8,6 +8,8 @@ #define LINUX_APPLE_GMUX_H #include <linux/acpi.h> +#include <linux/io.h> +#include <linux/pnp.h> #define GMUX_ACPI_HID "APP000B" @@ -35,14 +37,88 @@ #define GMUX_MIN_IO_LEN (GMUX_PORT_BRIGHTNESS + 4) #if IS_ENABLED(CONFIG_APPLE_GMUX) +static inline bool apple_gmux_is_indexed(unsigned long iostart) +{ + u16 val; + + outb(0xaa, iostart + 0xcc); + outb(0x55, iostart + 0xcd); + outb(0x00, iostart + 0xce); + + val = inb(iostart + 0xcc) | (inb(iostart + 0xcd) << 8); + if (val == 0x55aa) + return true; + + return false; +} /** - * apple_gmux_present() - detect if gmux is built into the machine + * apple_gmux_detect() - detect if gmux is built into the machine + * + * @pnp_dev: Device to probe or NULL to use the first matching device + * @indexed_ret: Returns (by reference) if the gmux is indexed or not + * + * Detect if a supported gmux device is present by actually probing it. + * This avoids the false positives returned on some models by + * apple_gmux_present(). + * + * Return: %true if a supported gmux ACPI device is detected and the kernel + * was configured with CONFIG_APPLE_GMUX, %false otherwise. + */ +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) +{ + u8 ver_major, ver_minor, ver_release; + struct resource *res; + bool indexed = false; + + if (!pnp_dev) { + struct acpi_device *adev; + struct device *dev; + + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); + if (!adev) + return false; + + dev = acpi_get_first_physical_node(adev); + if (!dev) + return false; + + pnp_dev = to_pnp_dev(dev); + } + + res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0); + if (!res) + return false; + + if (resource_size(res) < GMUX_MIN_IO_LEN) + return false; + + /* + * Invalid version information may indicate either that the gmux + * device isn't present or that it's a new one that uses indexed io. + */ + ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR); + ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR); + ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE); + if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) { + indexed = apple_gmux_is_indexed(res->start); + if (!indexed) + return false; + } + + if (indexed_ret) + *indexed_ret = indexed; + + return true; +} + +/** + * apple_gmux_present() - check if gmux ACPI device is present * * Drivers may use this to activate quirks specific to dual GPU MacBook Pros * and Mac Pros, e.g. for deferred probing, runtime pm and backlight. * - * Return: %true if gmux is present and the kernel was configured + * Return: %true if gmux ACPI device is present and the kernel was configured * with CONFIG_APPLE_GMUX, %false otherwise. */ static inline bool apple_gmux_present(void) @@ -57,6 +133,11 @@ static inline bool apple_gmux_present(void) return false; } +static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret) +{ + return false; +} + #endif /* !CONFIG_APPLE_GMUX */ #endif /* LINUX_APPLE_GMUX_H */
Add a new (static inline) apple_gmux_detect() helper to apple-gmux.h which can be used for gmux detection instead of apple_gmux_present(). The latter is not really reliable since an ACPI device with a HID of APP000B is present on some devices without a gmux at all, as well as on devices with a newer (unsupported) MMIO based gmux model. This causes apple_gmux_present() to return false-postives on a number of different Apple laptop models. This new helper uses the same probing as the actual apple-gmux driver, so that it does not return false positives. To avoid code duplication the gmux_probe() function of the actual driver is also moved over to using the new apple_gmux_detect() helper. Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/apple-gmux.c | 70 +++++++------------------ include/linux/apple-gmux.h | 85 ++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 54 deletions(-)