Re: [PATCH 2/3] platform/x86: apple-gmux: Add apple_gmux_detect() helper

[Lukas Wunner <lukas@wunner.de>]

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