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

[Hans de Goede <hdegoede@redhat.com>]

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
>