Re: [PATCH v1] ASoC: Intel: sof_nau8825: add support for nau8825 with amp nau8318

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

On 12/9/22 09:05, Ajye Huang wrote:
> This patch adds the driver data for two nau8318 speaker amplifiers on
> SSP1 and nau8825 on SSP0 for ADL platform.
> And reusing max98360's topology since DAI setting could be leveraged.
> 
> Signed-off-by: Ajye Huang <ajye_huang@compal.corp-partner.google.com>
> ---
>  sound/soc/intel/boards/Kconfig                |  1 +
>  sound/soc/intel/boards/sof_nau8825.c          | 23 +++++++++++++++++++
>  .../intel/common/soc-acpi-intel-adl-match.c   | 12 ++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
> index a472de1909f4..3f68e9edd853 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -554,6 +554,7 @@ config SND_SOC_INTEL_SOF_NAU8825_MACH
>  	select SND_SOC_RT1015P
>  	select SND_SOC_MAX98373_I2C
>  	select SND_SOC_MAX98357A
> +	select SND_SOC_NAU8315

This looks inconsistent with the commit message. There are separate
Kconfigs for different codecs.

SND_SOC_NAU8315
SND_SOC_NAU8825

Which is it?

>  	select SND_SOC_DMIC
>  	select SND_SOC_HDAC_HDMI
>  	select SND_SOC_INTEL_HDA_DSP_COMMON
> diff --git a/sound/soc/intel/boards/sof_nau8825.c b/sound/soc/intel/boards/sof_nau8825.c
> index 27880224359d..0936450be153 100644
> --- a/sound/soc/intel/boards/sof_nau8825.c
> +++ b/sound/soc/intel/boards/sof_nau8825.c
> @@ -48,6 +48,7 @@
>  #define SOF_MAX98373_SPEAKER_AMP_PRESENT	BIT(15)
>  #define SOF_MAX98360A_SPEAKER_AMP_PRESENT	BIT(16)
>  #define SOF_RT1015P_SPEAKER_AMP_PRESENT	BIT(17)
> +#define SOF_NAU8318_SPEAKER_AMP_PRESENT	BIT(18)
>  
>  static unsigned long sof_nau8825_quirk = SOF_NAU8825_SSP_CODEC(0);
>  
> @@ -338,6 +339,13 @@ static struct snd_soc_dai_link_component rt1019p_component[] = {
>  	}
>  };
>  
> +static struct snd_soc_dai_link_component nau8318_components[] = {
> +	{
> +		.name = "NVTN2012:00",

Deep sigh...

This ACPI HID is already used to match with the 8815, so this is not
going to work if we confuse two independent drivers...

sound/soc/codecs/nau8315.c:

#ifdef CONFIG_ACPI
static const struct acpi_device_id nau8315_acpi_match[] = {
	{ "NVTN2010", 0 },
	{ "NVTN2012", 0},
	{},
};
MODULE_DEVICE_TABLE(acpi, nau8315_acpi_match);
#endif

How does this identify a NAU8825?


> +		.dai_name = "nau8315-hifi",

and again this makes a reference to 8815.

I will stop the review here.

NAK for this v1. Please clarify which codec you are using and make sure
all references are consistent.


> +	}
> +};
> +
>  static struct snd_soc_dai_link_component dummy_component[] = {
>  	{
>  		.name = "snd-soc-dummy",
> @@ -486,6 +494,11 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev,
>  			max_98360a_dai_link(&links[id]);
>  		} else if (sof_nau8825_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) {
>  			sof_rt1015p_dai_link(&links[id]);
> +		} else if (sof_nau8825_quirk &
> +				SOF_NAU8318_SPEAKER_AMP_PRESENT) {
> +			links[id].codecs = nau8318_components;
> +			links[id].num_codecs = ARRAY_SIZE(nau8318_components);
> +			links[id].init = speaker_codec_init;
>  		} else {
>  			goto devm_err;
>  		}
> @@ -657,6 +670,16 @@ static const struct platform_device_id board_ids[] = {
>  					SOF_BT_OFFLOAD_SSP(2) |
>  					SOF_SSP_BT_OFFLOAD_PRESENT),
>  	},
> +	{
> +		.name = "adl_nau8318_nau8825",
> +		.driver_data = (kernel_ulong_t)(SOF_NAU8825_SSP_CODEC(0) |
> +					SOF_SPEAKER_AMP_PRESENT |
> +					SOF_NAU8318_SPEAKER_AMP_PRESENT |
> +					SOF_NAU8825_SSP_AMP(1) |
> +					SOF_NAU8825_NUM_HDMIDEV(4) |
> +					SOF_BT_OFFLOAD_SSP(2) |
> +					SOF_SSP_BT_OFFLOAD_PRESENT),
> +	},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(platform, board_ids);
> diff --git a/sound/soc/intel/common/soc-acpi-intel-adl-match.c b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> index 60aee56f94bd..282b9c966ce6 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> @@ -450,6 +450,11 @@ static const struct snd_soc_acpi_codecs adl_lt6911_hdmi = {
>  	.codecs = {"INTC10B0"}
>  };
>  
> +static const struct snd_soc_acpi_codecs adl_nau8318_amp = {
> +	.num_codecs = 1,
> +	.codecs = {"NVTN2012"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	{
>  		.comp_ids = &adl_rt5682_rt5682s_hp,
> @@ -507,6 +512,13 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  		.quirk_data = &adl_rt1015p_amp,
>  		.sof_tplg_filename = "sof-adl-rt1015-nau8825.tplg",
>  	},
> +	{
> +		.id = "10508825",
> +		.drv_name = "adl_nau8318_nau8825",
> +		.machine_quirk = snd_soc_acpi_codec_list,
> +		.quirk_data = &adl_nau8318_amp,
> +		.sof_tplg_filename = "sof-adl-max98360a-nau8825.tplg",
> +	},
>  	{
>  		.id = "10508825",
>  		.drv_name = "sof_nau8825",