[alsa-devel] [RFC 06/10] ASoC: hdac_hda: add ASoC based HDA codec driver

[Ughreja, Rakesh A]

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Saturday, December 2, 2017 1:07 AM
>To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; alsa-devel at alsa-
>project.org; broonie at kernel.org; tiwai at suse.de; liam.r.girdwood at linux.intel.com
>Cc: Koul, Vinod <vinod.koul at intel.com>; Patches Audio
><patches.audio at intel.com>
>Subject: Re: [alsa-devel] [RFC 06/10] ASoC: hdac_hda: add ASoC based HDA
>codec driver
>
>On 12/1/17 3:14 AM, Rakesh Ughreja wrote:
>> This patch adds ASoC based HDA codec driver that can be used with
>> all Intel platforms.
>>
>> FIXME:
>> KConfig change allows users to compile legacy HDA codec drivers either
>> as ASoC or ALSA codec driver. This is not a good approach and need
>> suggestion to improve it.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja at intel.com>
>> ---
>>   sound/pci/hda/Kconfig         | 11 +++++
>>   sound/pci/hda/hda_codec.h     | 26 +++++++++++-
>>   sound/soc/codecs/Kconfig      |  6 +++
>>   sound/soc/codecs/Makefile     |  2 +
>>   sound/soc/codecs/hdac_hda.c   | 95
>+++++++++++++++++++++++++++++++++++++++++++
>>   sound/soc/codecs/hdac_hda.h   | 16 ++++++++
>>   sound/soc/intel/skylake/skl.c | 18 +++++++-
>>   7 files changed, 171 insertions(+), 3 deletions(-)
>>   create mode 100644 sound/soc/codecs/hdac_hda.c
>>   create mode 100644 sound/soc/codecs/hdac_hda.h
>>
>> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
>> index 7f3b5ed..010dd7d 100644
>> --- a/sound/pci/hda/Kconfig
>> +++ b/sound/pci/hda/Kconfig
>> @@ -215,6 +215,17 @@ config SND_HDA_GENERIC
>>   comment "Set to Y if you want auto-loading the codec driver"
>>   	depends on SND_HDA=y && SND_HDA_GENERIC=m
>>
>> +config SND_ASOC_HDA_GENERIC
>> +        bool "Build HDA drivers as HDA ASoC Codec drivers"
>> +	select SND_SOC_HDAC_HDA
>> +        def_bool n
>> +        help
>> +          Select to build legacy HD-audio codec drivers as ASoC
>> +          codec drivers.
>> +
>> +comment "Set to Y if you want to enable support for HDA audio codec with
>> +	 DSP on Intel platforms"
>> +
>>   config SND_HDA_POWER_SAVE_DEFAULT
>>   	int "Default time-out for HD-audio power-save mode"
>>   	depends on PM
>> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
>> index 2d02d02..67ce2e6 100644
>> --- a/sound/pci/hda/hda_codec.h
>> +++ b/sound/pci/hda/hda_codec.h
>> @@ -104,9 +104,33 @@ int __hda_codec_driver_register(struct
>hda_codec_driver *drv, const char *name,
>>   #define hda_codec_driver_register(drv) \
>>   	__hda_codec_driver_register(drv, KBUILD_MODNAME, THIS_MODULE)
>>   void hda_codec_driver_unregister(struct hda_codec_driver *drv);
>> +
>> +/*
>> + * FIXME:
>> + * In this implementation one can compile codec drivers either as ALSA or as
>> + * ASoC, but not both.
>> + * This may be not the be the best way to build the HDA codec drivers as ALSA
>> + * vs ASoC driver. Need to find a better way, where the driver can be built
>> + * as ALSA as well as ASoC at the same time. Even though practically people
>> + * would never use them at the same time. But may be disto guys may want to
>> + * do it.
>
>Distros *have* to do it, since the DSP usage can be changed in
>user-visible BIOS settings or disabled.

Yes, agree.

>
>> + */
>> +
>> +#ifdef CONFIG_SND_ASOC_HDA_GENERIC
>> +int __hdac_hda_codec_driver_register(struct hda_codec_driver *drv,
>> +	const char *name, struct module *owner);
>> +#define hdac_hda_codec_driver_register(drv) \
>> +	__hdac_hda_codec_driver_register(drv, KBUILD_MODNAME,
>THIS_MODULE)
>> +void hdac_hda_codec_driver_unregister(struct hda_codec_driver *drv);
>> +
>> +#define module_hda_codec_driver(drv) \
>> +	module_driver(drv, hdac_hda_codec_driver_register, \
>> +			hdac_hda_codec_driver_unregister)
>> +#else
>>   #define module_hda_codec_driver(drv) \
>>   	module_driver(drv, hda_codec_driver_register, \
>> -		      hda_codec_driver_unregister)
>> +			hda_codec_driver_unregister)
>> +#endif
>
>Couldn't you use a common registration that internally checks if you are
>running in legacy or DSP/ASoC mode and branch accordingly?

Yes, this requires change in the legacy codec driver. Takashi also
suggested that we need to change the legacy driver code in order to make
it work. Now that we are okay to change the legacy driver source code,
I will come up with some proposal.