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

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

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