Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 7/22/19 8:01 AM, Takashi Iwai wrote:
> On Mon, 22 Jul 2019 14:58:44 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>> On 7/22/19 7:26 AM, Takashi Iwai wrote:
>>> On Mon, 22 Jul 2019 14:14:28 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>>
>>>> On 7/22/19 3:54 AM, Takashi Iwai wrote:
>>>>> On Sat, 20 Jul 2019 23:06:46 +0200,
>>>>> Cezary Rojewski wrote:
>>>>>>
>>>>>>> --- a/sound/hda/Kconfig
>>>>>>> +++ b/sound/hda/Kconfig
>>>>>>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE
>>>>>>>        	  Note that the pre-allocation size can be changed dynamically
>>>>>>>      	  via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too.
>>>>>>> +
>>>>>>> +config SND_INTEL_NHLT
>>>>>>> +	tristate
>>>>>>
>>>>>> If above is true, "depends on ACPI" would be expected.
>>>>>
>>>>> This won't fix things in practice as the Kconfig reverse selection
>>>>> ignores the dependencies of the selected item.  It'd be as a help for
>>>>> readers, though.
>>>>
>>>> There is a fallback if ACPI is not defined, so the code would always
>>>> compile. Configurations which select SND_INTEL_NHLT already depend on
>>>> ACPI.
>>>
>>> IIUC, the question above came from the point:
>>>
>>>    #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT)
>>>    ....
>>>    #else
>>>    ....
>>>    #endif
>>>
>>> and here Cezary suggested to drop IS_ENABLED(CONFIG_ACPI) *iff* the
>>> dependency can be assured in Kconfig side.  But for that assurance,
>>> putting "depends on ACPI" in config SND_INTEL_NHLT block won't
>>> suffice; that was my followup.
>>>
>>> So, as of the current code, we can drop IS_ENABLED(CONFIG_ACPI) from
>>> the ifdef above, yes.  But the dependency is no rock solid at this
>>> point, so either some comments or keeping the extra ifdef like the
>>> above would be needed, IMO.
>>
>> this extra ifdef is a bit overkill, I added it to make sure that the
>> fallbacks are used in nonsensical configurations w/ randconfig. In
>> practice, all Intel drivers already depend on ACPI and for the legacy
>> we already have:
>>
>> select SND_INTEL_NHLT if ACPI
>>
>> Not sure if we need to do anything more.
> 
> The missing piece is this implicit dependency information.
> You can just put some comments somewhere mentioning it.

ok, will do. thanks for the guidance.