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

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

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