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
