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

[Takashi Iwai]

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.



Takashi