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 <[email protected]>
> ---
> 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",
Hi Pierre
On Fri, Dec 9, 2022 at 11:52 PM Pierre-Louis Bossart
<[email protected]> wrote:
> This looks inconsistent with the commit message. There are separate
> Kconfigs for different codecs.
>
> SND_SOC_NAU8315
> SND_SOC_NAU8825
>
> Which is it?
>
Sorry about confusing you, I think it is better to change the title as
ASoC: Intel: sof_nau8825: add combination of nau8825 headset codec
with nau8318 Amp.
And add some information about Nuvoton chips in the comment message.
***
The nau8315 and nau8318 are both Nuvoton Amp chips. They use the same
Amp driver nau8315.c. The acpi_device_id for nau8315 is "NVTN2010",
for nau8318 is "NVTN2012".
The nau8825 is one of Nuvoton headset codec, and its acpi_device_id is
"10508825".
***
>
> 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?
>
The NAU8825 Is headset codec, we added a new combination of nau8825
headset codec + nau8318 amp in the machine driver.
>
> > + .dai_name = "nau8315-hifi",
>
> and again this makes a reference to 8815.
>
Because the Amp driver is the same file nau8315.c , so nau8318 use the
same dai_name "nau8315-hifi"
> I will stop the review here.
>
> NAK for this v1. Please clarify which codec you are using and make sure
> all references are consistent.
>
>
I apologize for the unclear comment message, please give me any
suggestions if needed, and I will send v2 for review.
thanks.
>> This looks inconsistent with the commit message. There are separate
>> Kconfigs for different codecs.
>>
>> SND_SOC_NAU8315
>> SND_SOC_NAU8825
>>
>> Which is it?
>>
>
> Sorry about confusing you, I think it is better to change the title as
> ASoC: Intel: sof_nau8825: add combination of nau8825 headset codec
> with nau8318 Amp.
Suggested edit:
ASoC: Intel: sof_nau8825: add variant with nau8318 amplifier.
>
> And add some information about Nuvoton chips in the comment message.
> ***
> The nau8315 and nau8318 are both Nuvoton Amp chips. They use the same
> Amp driver nau8315.c. The acpi_device_id for nau8315 is "NVTN2010",
> for nau8318 is "NVTN2012".
That should be added in the commit message please.
>> NAK for this v1. Please clarify which codec you are using and make sure
>> all references are consistent.
>>
>>
>
> I apologize for the unclear comment message, please give me any
> suggestions if needed, and I will send v2 for review.
> thanks.
Ok, makes sense now. Please do include the explanations on 8315/8318
variants, I couldn't figure out what chips were used.
I would also not use the same topology name as a different platform with
another amplifier. I appreciate you trying to reuse when possible, but
it's just better to create a new topology file. That way if you want any
update down the road you can do so, it's more maintainable and risk-free.
HI Pierre,
On Sat, Dec 10, 2022 at 1:31 AM Pierre-Louis Bossart
<[email protected]> wrote:
> Suggested edit:
>
> ASoC: Intel: sof_nau8825: add variant with nau8318 amplifier.
>
> That should be added in the commit message please.
>
Thank you for the suggestion title and I will add those explanations
in the comment message.
> Ok, makes sense now. Please do include the explanations on 8315/8318
> variants, I couldn't figure out what chips were used.
>
> I would also not use the same topology name as a different platform with
> another amplifier. I appreciate you trying to reuse when possible, but
> it's just better to create a new topology file. That way if you want any
> update down the road you can do so, it's more maintainable and risk-free.
OK, The v2 patch will be sent after the new topology file is created. Thank you.