On 12/16/22 09:49, Ajye Huang wrote:
> This patch adds the driver data for two nau8318 speaker amplifiers on
> SSP1 and nau8825 on SSP0 for ADL platform.
So here you are making reference to two amplifiers...
> +static struct snd_soc_dai_link_component nau8318_components[] = {
> + {
> + .name = "NVTN2012:00",
> + .dai_name = "nau8315-hifi",
> + }
> +};
but here there's only one? I was expecting something like what we've
used for Maxim amplifiers with a codec configuration and dailink
components that list the two amplifiers.
static struct snd_soc_codec_conf max_98373_codec_conf[] = {
{
.dlc = COMP_CODEC_CONF(MAX_98373_DEV0_NAME),
.name_prefix = "Right",
},
{
.dlc = COMP_CODEC_CONF(MAX_98373_DEV1_NAME),
.name_prefix = "Left",
},
};
struct snd_soc_dai_link_component max_98373_components[] = {
{ /* For Right */
.name = MAX_98373_DEV0_NAME,
.dai_name = MAX_98373_CODEC_DAI,
},
{ /* For Left */
.name = MAX_98373_DEV1_NAME,
.dai_name = MAX_98373_CODEC_DAI,
},
};
Or is this a commit message problem and there's really only one amplifier?
> +
> 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;
The rest looks fine, I only have this one/two amplifier question.
Hi Pierre
On Sat, Dec 17, 2022 at 12:03 AM Pierre-Louis Bossart
<[email protected]> wrote:
> On 12/16/22 09:49, Ajye Huang wrote:
> > This patch adds the driver data for two nau8318 speaker amplifiers on
> > SSP1 and nau8825 on SSP0 for ADL platform.
>
> So here you are making reference to two amplifiers...
>
> > +static struct snd_soc_dai_link_component nau8318_components[] = {
> > + {
> > + .name = "NVTN2012:00",
> > + .dai_name = "nau8315-hifi",
> > + }
> > +};
>
> but here there's only one? I was expecting something like what we've
> used for Maxim amplifiers with a codec configuration and dailink
> components that list the two amplifiers.
>
> static struct snd_soc_codec_conf max_98373_codec_conf[] = {
> {
> .dlc = COMP_CODEC_CONF(MAX_98373_DEV0_NAME),
> .name_prefix = "Right",
> },
> {
> .dlc = COMP_CODEC_CONF(MAX_98373_DEV1_NAME),
> .name_prefix = "Left",
> },
> };
>
> struct snd_soc_dai_link_component max_98373_components[] = {
> { /* For Right */
> .name = MAX_98373_DEV0_NAME,
> .dai_name = MAX_98373_CODEC_DAI,
> },
> { /* For Left */
> .name = MAX_98373_DEV1_NAME,
> .dai_name = MAX_98373_CODEC_DAI,
> },
> };
>
> Or is this a commit message problem and there's really only one amplifier?
Really , it has two speakers. The nau8318 is an auto mode Amplifier
chip, similar to the max98360a amp chip.
EX: Sof_maxim_common.c (sound\soc\intel\boards):
static struct snd_soc_dai_link_component max_98360a_components[] = {
{
.name = MAX_98360A_DEV0_NAME,
.dai_name = MAX_98357A_CODEC_DAI,
}
};
It is not an i2c interface, from the nau8318 data sheet, there are
five pins used mainly. one for enable, others for I2S.
EN-- enable pin
FSR-- Frame Sync, Right
FSL-- Frame Sync, Left
BCLK-- bit clock
DACIN-- Input i2s data
The FSR and FSL pins are for Left and Right channels used.
thanks
On 12/16/22 10:55, Ajye Huang wrote:
> Hi Pierre
>
> On Sat, Dec 17, 2022 at 12:03 AM Pierre-Louis Bossart
> <[email protected]> wrote:
>
>> On 12/16/22 09:49, Ajye Huang wrote:
>>> This patch adds the driver data for two nau8318 speaker amplifiers on
>>> SSP1 and nau8825 on SSP0 for ADL platform.
>>
>> So here you are making reference to two amplifiers...
>>
>>> +static struct snd_soc_dai_link_component nau8318_components[] = {
>>> + {
>>> + .name = "NVTN2012:00",
>>> + .dai_name = "nau8315-hifi",
>>> + }
>>> +};
>>
>> but here there's only one? I was expecting something like what we've
>> used for Maxim amplifiers with a codec configuration and dailink
>> components that list the two amplifiers.
>>
>> static struct snd_soc_codec_conf max_98373_codec_conf[] = {
>> {
>> .dlc = COMP_CODEC_CONF(MAX_98373_DEV0_NAME),
>> .name_prefix = "Right",
>> },
>> {
>> .dlc = COMP_CODEC_CONF(MAX_98373_DEV1_NAME),
>> .name_prefix = "Left",
>> },
>> };
>>
>> struct snd_soc_dai_link_component max_98373_components[] = {
>> { /* For Right */
>> .name = MAX_98373_DEV0_NAME,
>> .dai_name = MAX_98373_CODEC_DAI,
>> },
>> { /* For Left */
>> .name = MAX_98373_DEV1_NAME,
>> .dai_name = MAX_98373_CODEC_DAI,
>> },
>> };
>>
>> Or is this a commit message problem and there's really only one amplifier?
>
> Really , it has two speakers. The nau8318 is an auto mode Amplifier
> chip, similar to the max98360a amp chip.
> EX: Sof_maxim_common.c (sound\soc\intel\boards):
> static struct snd_soc_dai_link_component max_98360a_components[] = {
> {
> .name = MAX_98360A_DEV0_NAME,
> .dai_name = MAX_98357A_CODEC_DAI,
> }
> };
> It is not an i2c interface, from the nau8318 data sheet, there are
> five pins used mainly. one for enable, others for I2S.
> EN-- enable pin
> FSR-- Frame Sync, Right
> FSL-- Frame Sync, Left
> BCLK-- bit clock
> DACIN-- Input i2s data
>
> The FSR and FSL pins are for Left and Right channels used.
> thanks
Ok, thanks for the explanations.
Acked-by: Pierre-Louis Bossart <[email protected]>
Dear Mark,
On Sat, Dec 17, 2022 at 1:37 AM Pierre-Louis Bossart
<[email protected]> wrote:
>
> Acked-by: Pierre-Louis Bossart <[email protected]>
First of all, I apologize for this letter of inquiry.
I got "Acked-by" from Pierre .
Please kindly check this when you are free, thank you so much.
Hi Pierre
On Sat, Dec 17, 2022 at 1:37 AM Pierre-Louis Bossart
<[email protected]> wrote:
>
> Ok, thanks for the explanations.
>
> Acked-by: Pierre-Louis Bossart <[email protected]>
Yesterday, I saw Arnd Bergmann sent this patch "ASoC: Intel:
sof-nau8825: fix module alias overflow " for reducing the string to
prevent over length,https://patchwork.kernel.org/project/alsa-devel/patch/[email protected]/.
so, I need to check with you, should my string need to change the
format style with his, even the my string does not over length , from
.drv_name = "adl_nau8318_nau8825" to .drv_name = "adl_nau8318_8825",
align with his format style?
thanks
On 12/21/22 17:30, Ajye Huang wrote:
> Hi Pierre
>
> On Sat, Dec 17, 2022 at 1:37 AM Pierre-Louis Bossart
> <[email protected]> wrote:
>
>>
>> Ok, thanks for the explanations.
>>
>> Acked-by: Pierre-Louis Bossart <[email protected]>
>
> Yesterday, I saw Arnd Bergmann sent this patch "ASoC: Intel:
> sof-nau8825: fix module alias overflow " for reducing the string to
> prevent over length,https://patchwork.kernel.org/project/alsa-devel/patch/[email protected]/.
>
> so, I need to check with you, should my string need to change the
> format style with his, even the my string does not over length , from
> .drv_name = "adl_nau8318_nau8825" to .drv_name = "adl_nau8318_8825",
> align with his format style?
That would be more consistent indeed, no objections from me.
Hi Pierre
On Thu, Dec 22, 2022 at 8:27 AM Pierre-Louis Bossart
<[email protected]> wrote:
>
>
> That would be more consistent indeed, no objections from me.
Thank you , I will send the v3 patch with modified string "adl_nau8318_8825".