LinuxLists.cc - [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

On 5/21/20 11:25 AM, Lukasz Majczak wrote:
> Split be_hw_params_fixup function for different codecs as current common
> function, leads to crash while trying to get snd_soc_dpcm with
> container_of() macro in kabylake_ssp_fixup().
> The crash call path looks as below:
> soc_pcm_hw_params()
> snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
> rtd->dai_link->be_hw_params_fixup(rtd, params)
> kabylake_ssp_fixup()
> In this case, codec_params is just a copy of an internal structure and is
> not embedded into struct snd_soc_dpcm thus we cannot use
> container_of() on it.
>
> v1 -> v2:
> - Extract dmic from SSP0 as every BE should have own fixup function.
> v2 -> v3:
> - Restore naming in the dapm route table to not confuse with other
> drivers
> - Fixed indentations

you need the changelog to be below the --- marker two lines down.

>
> Signed-off-by: Lukasz Majczak <[email protected]>
> ---

[...]

> +static int kabylake_dmic01_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + int ret = 0;
> +
> + ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0xF, 0, 8, 16);
> + if (ret < 0) {
> + dev_err(rtd->dev, "set TDM slot err:%d\n", ret);
> + return ret;
> + }
> +
> + ret = snd_soc_dai_set_sysclk(rtd->codec_dai,
> + RT5514_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN);
> + if (ret < 0) {
> + dev_err(rtd->dev, "set sysclk err: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static struct snd_soc_ops kabylake_ssp0_ops = {
> .hw_params = kabylake_ssp0_hw_params,
> };
>
> +static struct snd_soc_ops kabylake_dmic01_ops = {
> + .hw_params = kabylake_dmic01_hw_params,
> +};
> +
> static const unsigned int channels_dmic[] = {
> 4,
> };
> @@ -507,14 +528,19 @@ SND_SOC_DAILINK_DEF(ssp0_pin,
> SND_SOC_DAILINK_DEF(ssp0_codec,
> DAILINK_COMP_ARRAY(
> /* Left */ COMP_CODEC(MAXIM_DEV0_NAME, KBL_MAXIM_CODEC_DAI),
> - /* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI),
> - /* dmic */ COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI)));
> + /* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI)));
>
> SND_SOC_DAILINK_DEF(ssp1_pin,
> DAILINK_COMP_ARRAY(COMP_CPU("SSP1 Pin")));
> SND_SOC_DAILINK_DEF(ssp1_codec,
> DAILINK_COMP_ARRAY(COMP_CODEC(RT5663_DEV_NAME, KBL_REALTEK_CODEC_DAI)));
>
> +SND_SOC_DAILINK_DEF(dmic01_pin,
> + DAILINK_COMP_ARRAY(COMP_CPU("DMIC01 Pin")));
> +SND_SOC_DAILINK_DEF(dmic01_codec,
> + DAILINK_COMP_ARRAY(
> + COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI)));
> +
> SND_SOC_DAILINK_DEF(idisp1_pin,
> DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin")));
> SND_SOC_DAILINK_DEF(idisp1_codec,
> @@ -618,9 +644,8 @@ static struct snd_soc_dai_link kabylake_dais[] = {
> SND_SOC_DAIFMT_NB_NF |
> SND_SOC_DAIFMT_CBS_CFS,
> .ignore_pmdown_time = 1,
> - .be_hw_params_fixup = kabylake_ssp_fixup,
> + .be_hw_params_fixup = kabylake_ssp0_fixup,
> .dpcm_playback = 1,
> - .dpcm_capture = 1,
> .ops = &kabylake_ssp0_ops,
> SND_SOC_DAILINK_REG(ssp0_pin, ssp0_codec, platform),
> },
> @@ -632,12 +657,25 @@ static struct snd_soc_dai_link kabylake_dais[] = {
> .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> SND_SOC_DAIFMT_CBS_CFS,
> .ignore_pmdown_time = 1,
> - .be_hw_params_fixup = kabylake_ssp_fixup,
> + .be_hw_params_fixup = kabylake_ssp1_fixup,
> .ops = &kabylake_rt5663_ops,
> .dpcm_playback = 1,
> .dpcm_capture = 1,
> SND_SOC_DAILINK_REG(ssp1_pin, ssp1_codec, platform),
> },
> + {
> + .name = "dmic01",
> + .id = 2,
> + .no_pcm = 1,
> + .dai_fmt = SND_SOC_DAIFMT_DSP_B |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_CBS_CFS,
> + .ignore_pmdown_time = 1,
> + .be_hw_params_fixup = kabylake_dmic_fixup,
> + .dpcm_capture = 1,
> + .ops = &kabylake_dmic01_ops,
> + SND_SOC_DAILINK_REG(dmic01_pin, dmic01_codec, platform),
> + },

don't add a new dailink, this is not right.

> {
> .name = "iDisp1",
> .id = 3,
>
> base-commit: a4f6fc98cd2fa1774bcaeb248c67156ef9402a56
>

2020-05-21 17:11:00

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

>
> don't add a new dailink, this is not right.
>
Can you advise a better solution how to assign different fixup
functions to mic and to speakers? I was looking at "dmic01" dailink in
skl_nau88l25_max98357a.c as an example.

Best regards,
Lukasz

2020-05-21 17:20:07

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

On 5/21/20 12:08 PM, Łukasz Majczak wrote:
>>
>> don't add a new dailink, this is not right.
>>
> Can you advise a better solution how to assign different fixup
> functions to mic and to speakers? I was looking at "dmic01" dailink in
> skl_nau88l25_max98357a.c as an example.

I am not sure I follow. the DMICs are handled on a shared SSP, so how
would one set a different fixup? The word length have to be the same.

2020-05-21 17:35:00

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

Hi Pierre

If you will take a look at the original kabylake_ssp_fixup() you will
see that it is checking whether the related FE is "Kbl Audio Port",
"Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio
DMIC cap" - then for the first 3 cases it sets min/max channels to 2
while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm
trying to split this, but maybe I'm missing here something.

Best regards,
Lukasz

czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
<[email protected]> napisał(a):
>
>
>
> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> >>
> >> don't add a new dailink, this is not right.
> >>
> > Can you advise a better solution how to assign different fixup
> > functions to mic and to speakers? I was looking at "dmic01" dailink in
> > skl_nau88l25_max98357a.c as an example.
>
> I am not sure I follow. the DMICs are handled on a shared SSP, so how
> would one set a different fixup? The word length have to be the same.

2020-05-21 18:12:52

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

On 5/21/20 12:30 PM, Łukasz Majczak wrote:
> Hi Pierre
>
> If you will take a look at the original kabylake_ssp_fixup() you will
> see that it is checking whether the related FE is "Kbl Audio Port",
> "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio
> DMIC cap" - then for the first 3 cases it sets min/max channels to 2
> while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm
> trying to split this, but maybe I'm missing here something.

I don't understand this code either.

I believe the intent is that for all SSP1-RT5663 usages, we should use

rate->min = rate->max = 48000;
chan->min = chan->max = 2;
snd_mask_none(fmt);
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);

That is pretty easy to move to a dedicated ssp1 fixup.

for SSP0, we have RT5514 for capture and max98927 for playback, but the
existing code does not explicitly deal with rate/channels/format for all
cases, so it's not clear what should happen.

Harsha, can you help here?

>
> Best regards,
> Lukasz
>
> czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
> <[email protected]> napisał(a):
>>
>>
>>
>> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
>>>>
>>>> don't add a new dailink, this is not right.
>>>>
>>> Can you advise a better solution how to assign different fixup
>>> functions to mic and to speakers? I was looking at "dmic01" dailink in
>>> skl_nau88l25_max98357a.c as an example.
>>
>> I am not sure I follow. the DMICs are handled on a shared SSP, so how
>> would one set a different fixup? The word length have to be the same.

2020-06-29 18:43:39

[permalink] [raw]

Subject: RE: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

> -----Original Message-----
> From: Łukasz Majczak <[email protected]>
> Sent: Monday, June 29, 2020 4:11 AM
> To: N, Harshapriya <[email protected]>
> Cc: [email protected]; Pierre-Louis Bossart <pierre-
> [email protected]>; Jie Yang <[email protected]>; Radoslaw
> Biernacki <[email protected]>; Ross Zwisler <[email protected]>; linux-
> [email protected]; Liam Girdwood <[email protected]>;
> Bob Brandt <[email protected]>; Marcin Wojtas <[email protected]>; Alex
> Levin <[email protected]>
> Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split
> be_hw_params_fixup function
>
> Hi Harsha,
>
> We would like to continue the work on this, could you please suggest the
> correct approach.
>
> Best regards,
> Lukasz
>
> czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre-
> [email protected]> napisał(a):
> >
> >
> >
> > On 5/21/20 12:30 PM, Łukasz Majczak wrote:
> > > Hi Pierre
> > >
> > > If you will take a look at the original kabylake_ssp_fixup() you
> > > will see that it is checking whether the related FE is "Kbl Audio
> > > Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or
> > > "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max
> > > channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's
> > > is why I'm trying to split this, but maybe I'm missing here something.
> >
> > I don't understand this code either.
> >
> > I believe the intent is that for all SSP1-RT5663 usages, we should use
> >
> > rate->min = rate->max = 48000;
> > chan->min = chan->max = 2;
> > snd_mask_none(fmt);
> > snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
> >
> > That is pretty easy to move to a dedicated ssp1 fixup.
> >
> > for SSP0, we have RT5514 for capture and max98927 for playback, but
> > the existing code does not explicitly deal with rate/channels/format
> > for all cases, so it's not clear what should happen.
> >
> > Harsha, can you help here?
Apologies for missing the email I had to respond to.

SSP0 - has the speakers
SSP1 - has headset and dmic
For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai
For speakers, however support only 16 bit, so we set it back to 16 bit
If the front end dai is dmic, then the channel is set to either 2 or 4 dmic_constraints. No other formats need to be set.

All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above)
Most parameters are same for speakers and headset which are on different SSP. This is the reason we had a single fixup function.

Is there a reason why the fixup function needs to be split?

> >
> > >
> > > Best regards,
> > > Lukasz
> > >
> > > czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
> > > <[email protected]> napisał(a):
> > >>
> > >>
> > >>
> > >> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> > >>>>
> > >>>> don't add a new dailink, this is not right.
> > >>>>
> > >>> Can you advise a better solution how to assign different fixup
> > >>> functions to mic and to speakers? I was looking at "dmic01"
> > >>> dailink in skl_nau88l25_max98357a.c as an example.
> > >>
> > >> I am not sure I follow. the DMICs are handled on a shared SSP, so
> > >> how would one set a different fixup? The word length have to be the same.

2020-06-29 21:02:58

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

Hi Harsha,

We would like to continue the work on this, could you please suggest
the correct approach.

Best regards,
Lukasz

czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart
<[email protected]> napisał(a):
>
>
>
> On 5/21/20 12:30 PM, Łukasz Majczak wrote:
> > Hi Pierre
> >
> > If you will take a look at the original kabylake_ssp_fixup() you will
> > see that it is checking whether the related FE is "Kbl Audio Port",
> > "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio
> > DMIC cap" - then for the first 3 cases it sets min/max channels to 2
> > while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm
> > trying to split this, but maybe I'm missing here something.
>
> I don't understand this code either.
>
> I believe the intent is that for all SSP1-RT5663 usages, we should use
>
> rate->min = rate->max = 48000;
> chan->min = chan->max = 2;
> snd_mask_none(fmt);
> snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
>
> That is pretty easy to move to a dedicated ssp1 fixup.
>
> for SSP0, we have RT5514 for capture and max98927 for playback, but the
> existing code does not explicitly deal with rate/channels/format for all
> cases, so it's not clear what should happen.
>
> Harsha, can you help here?
>
> >
> > Best regards,
> > Lukasz
> >
> > czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
> > <[email protected]> napisał(a):
> >>
> >>
> >>
> >> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> >>>>
> >>>> don't add a new dailink, this is not right.
> >>>>
> >>> Can you advise a better solution how to assign different fixup
> >>> functions to mic and to speakers? I was looking at "dmic01" dailink in
> >>> skl_nau88l25_max98357a.c as an example.
> >>
> >> I am not sure I follow. the DMICs are handled on a shared SSP, so how
> >> would one set a different fixup? The word length have to be the same.

2020-06-29 21:47:36

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

Hi Harsha,

I've put the reason for this change in the commit message.
I had to split be_hw_params_fixup function for different codecs
because current approach (made for kernel 4.4) used in kernel 5.4,
leads to crash while trying to get snd_soc_dpcm with container_of()
macro in kabylake_ssp_fixup().
The crash call path looks as below:
soc_pcm_hw_params()
snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
rtd->dai_link->be_hw_params_fixup(rtd, params)
kabylake_ssp_fixup()
In this case, codec_params is just a copy of an internal structure and is
not embedded into struct snd_soc_dpcm thus we cannot use
container_of() on it.

Best regards,
Lukasz

pon., 29 cze 2020 o 18:51 N, Harshapriya <[email protected]> napisał(a):
>
> > -----Original Message-----
> > From: Łukasz Majczak <[email protected]>
> > Sent: Monday, June 29, 2020 4:11 AM
> > To: N, Harshapriya <[email protected]>
> > Cc: [email protected]; Pierre-Louis Bossart <pierre-
> > [email protected]>; Jie Yang <[email protected]>; Radoslaw
> > Biernacki <[email protected]>; Ross Zwisler <[email protected]>; linux-
> > [email protected]; Liam Girdwood <[email protected]>;
> > Bob Brandt <[email protected]>; Marcin Wojtas <[email protected]>; Alex
> > Levin <[email protected]>
> > Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split
> > be_hw_params_fixup function
> >
> > Hi Harsha,
> >
> > We would like to continue the work on this, could you please suggest the
> > correct approach.
> >
> > Best regards,
> > Lukasz
> >
> > czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre-
> > [email protected]> napisał(a):
> > >
> > >
> > >
> > > On 5/21/20 12:30 PM, Łukasz Majczak wrote:
> > > > Hi Pierre
> > > >
> > > > If you will take a look at the original kabylake_ssp_fixup() you
> > > > will see that it is checking whether the related FE is "Kbl Audio
> > > > Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or
> > > > "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max
> > > > channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's
> > > > is why I'm trying to split this, but maybe I'm missing here something.
> > >
> > > I don't understand this code either.
> > >
> > > I believe the intent is that for all SSP1-RT5663 usages, we should use
> > >
> > > rate->min = rate->max = 48000;
> > > chan->min = chan->max = 2;
> > > snd_mask_none(fmt);
> > > snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
> > >
> > > That is pretty easy to move to a dedicated ssp1 fixup.
> > >
> > > for SSP0, we have RT5514 for capture and max98927 for playback, but
> > > the existing code does not explicitly deal with rate/channels/format
> > > for all cases, so it's not clear what should happen.
> > >
> > > Harsha, can you help here?
> Apologies for missing the email I had to respond to.
>
> SSP0 - has the speakers
> SSP1 - has headset and dmic
> For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai
> For speakers, however support only 16 bit, so we set it back to 16 bit
> If the front end dai is dmic, then the channel is set to either 2 or 4 dmic_constraints. No other formats need to be set.
>
> All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above)
> Most parameters are same for speakers and headset which are on different SSP. This is the reason we had a single fixup function.
>
> Is there a reason why the fixup function needs to be split?
>
> > >
> > > >
> > > > Best regards,
> > > > Lukasz
> > > >
> > > > czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
> > > > <[email protected]> napisał(a):
> > > >>
> > > >>
> > > >>
> > > >> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> > > >>>>
> > > >>>> don't add a new dailink, this is not right.
> > > >>>>
> > > >>> Can you advise a better solution how to assign different fixup
> > > >>> functions to mic and to speakers? I was looking at "dmic01"
> > > >>> dailink in skl_nau88l25_max98357a.c as an example.
> > > >>
> > > >> I am not sure I follow. the DMICs are handled on a shared SSP, so
> > > >> how would one set a different fixup? The word length have to be the same.

2020-06-29 22:33:21

[permalink] [raw]

Subject: RE: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

>
> Hi Harsha,
>
> I've put the reason for this change in the commit message.
> I had to split be_hw_params_fixup function for different codecs because
> current approach (made for kernel 4.4) used in kernel 5.4, leads to crash while
> trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup().
> The crash call path looks as below:
> soc_pcm_hw_params()
> snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
> rtd->dai_link->be_hw_params_fixup(rtd, params)
> kabylake_ssp_fixup()
> In this case, codec_params is just a copy of an internal structure and is not
> embedded into struct snd_soc_dpcm thus we cannot use
> container_of() on it.
addressing it below to keep the context

>
> Best regards,
> Lukasz
>
> pon., 29 cze 2020 o 18:51 N, Harshapriya <[email protected]>
> napisał(a):
> >
> > > -----Original Message-----
> > > From: Łukasz Majczak <[email protected]>
> > > Sent: Monday, June 29, 2020 4:11 AM
> > > To: N, Harshapriya <[email protected]>
> > > Cc: [email protected]; Pierre-Louis Bossart <pierre-
> > > [email protected]>; Jie Yang <[email protected]>;
> > > Radoslaw Biernacki <[email protected]>; Ross Zwisler
> > > <[email protected]>; linux- [email protected]; Liam Girdwood
> > > <[email protected]>;
> > > Bob Brandt <[email protected]>; Marcin Wojtas <[email protected]>;
> Alex
> > > Levin <[email protected]>
> > > Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927:
> > > Split be_hw_params_fixup function
> > >
> > > Hi Harsha,
> > >
> > > We would like to continue the work on this, could you please suggest
> > > the correct approach.
> > >
> > > Best regards,
> > > Lukasz
> > >
> > > czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre-
> > > [email protected]> napisał(a):
> > > >
> > > >
> > > >
> > > > On 5/21/20 12:30 PM, Łukasz Majczak wrote:
> > > > > Hi Pierre
> > > > >
> > > > > If you will take a look at the original kabylake_ssp_fixup() you
> > > > > will see that it is checking whether the related FE is "Kbl
> > > > > Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture
> > > > > Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it
> > > > > sets min/max channels to 2 while for the "Kbl DMIC cap" it can
> > > > > be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing
> here something.
> > > >
> > > > I don't understand this code either.
> > > >
> > > > I believe the intent is that for all SSP1-RT5663 usages, we should
> > > > use
> > > >
> > > > rate->min = rate->max = 48000;
> > > > chan->min = chan->max = 2;
> > > > snd_mask_none(fmt);
> > > > snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
> > > >
> > > > That is pretty easy to move to a dedicated ssp1 fixup.
Agree. SSP1 can be moved to a separate function as it only hosts headset codec

> > > >
> > > > for SSP0, we have RT5514 for capture and max98927 for playback,
> > > > but the existing code does not explicitly deal with
> > > > rate/channels/format for all cases, so it's not clear what should happen.
> > > >
> > > > Harsha, can you help here?
> > Apologies for missing the email I had to respond to.
> >
> > SSP0 - has the speakers
> > SSP1 - has headset and dmic
Correction dmic is on SSP0

> > For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting
> > is done based on the front-end dai For speakers, however support only
> > 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel
> is set to either 2 or 4 dmic_constraints. No other formats need to be set.
> >
> > All the SSP1 usages do not have the same parameters (as dmic is on
> > SSP1 and its different as given above) Most parameters are same for
> speakers and headset which are on different SSP. This is the reason we had a
> single fixup function.
On SSP1, for dmic we need to fix the channels which is derived from
dmic_num of the snd_soc_acpi_mach structure based on the number of dmic on the board.
The channel is something that might be different from speakers.
We might not want to constraint the dmic capture to always be 48Khz as well.
Given this, there seems to me, 2 ways to set it:
1. Derive if the fixup is being called for dmic or speaker
2. Having a new dailink

If #2 is not preferred (going by Pierre's comments), can we use rtd->cpu_dai/codec_dai->name to
figure out if its for dmic or speaker?
I can test this and get back to you.

> >
> > Is there a reason why the fixup function needs to be split?
> >
> > > >
> > > > >
> > > > > Best regards,
> > > > > Lukasz
> > > > >
> > > > > czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart
> > > > > <[email protected]> napisał(a):
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> > > > >>>>
> > > > >>>> don't add a new dailink, this is not right.
> > > > >>>>
> > > > >>> Can you advise a better solution how to assign different fixup
> > > > >>> functions to mic and to speakers? I was looking at "dmic01"
> > > > >>> dailink in skl_nau88l25_max98357a.c as an example.
> > > > >>
> > > > >> I am not sure I follow. the DMICs are handled on a shared SSP,
> > > > >> so how would one set a different fixup? The word length have to be the
> same.

2020-07-01 16:38:41

[permalink] [raw]

Subject: RE: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

>
> > > For speakers and headsets its 48Khz, 2 ch and 24 bit and this
> > > setting is done based on the front-end dai For speakers, however
> > > support only
> > > 16 bit, so we set it back to 16 bit If the front end dai is dmic,
> > > then the channel
> > is set to either 2 or 4 dmic_constraints. No other formats need to be set.
> > >
> > > All the SSP1 usages do not have the same parameters (as dmic is on
> > > SSP1 and its different as given above) Most parameters are same for
> > speakers and headset which are on different SSP. This is the reason we
> > had a single fixup function.
> On SSP1, for dmic we need to fix the channels which is derived from dmic_num
> of the snd_soc_acpi_mach structure based on the number of dmic on the
> board.
> The channel is something that might be different from speakers.
> We might not want to constraint the dmic capture to always be 48Khz as well.
> Given this, there seems to me, 2 ways to set it:
> 1. Derive if the fixup is being called for dmic or speaker 2. Having a new dailink
>
> If #2 is not preferred (going by Pierre's comments), can we use rtd-
> >cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
> I can test this and get back to you.
Tested and the following is something we can use without creating a new dailink.
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
if (params_channels(params) == 2 || DMIC_CH(dmic_constraints) == 2)
channels->min = channels->max = 2;
else
channels->min = channels->max = 4;
} else {
rate->min = rate->max = 48000;
channels->min = channels->max = 2;
snd_mask_none(fmt);
snd_mask_set_format(fmt, pcm_fmt);
}

Pierre, thoughts?

2020-07-01 16:49:28

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

On 7/1/20 11:37 AM, N, Harshapriya wrote:
>>
>>>> For speakers and headsets its 48Khz, 2 ch and 24 bit and this
>>>> setting is done based on the front-end dai For speakers, however
>>>> support only
>>>> 16 bit, so we set it back to 16 bit If the front end dai is dmic,
>>>> then the channel
>>> is set to either 2 or 4 dmic_constraints. No other formats need to be set.
>>>>
>>>> All the SSP1 usages do not have the same parameters (as dmic is on
>>>> SSP1 and its different as given above) Most parameters are same for
>>> speakers and headset which are on different SSP. This is the reason we
>>> had a single fixup function.
>> On SSP1, for dmic we need to fix the channels which is derived from dmic_num
>> of the snd_soc_acpi_mach structure based on the number of dmic on the
>> board.
>> The channel is something that might be different from speakers.
>> We might not want to constraint the dmic capture to always be 48Khz as well.
>> Given this, there seems to me, 2 ways to set it:
>> 1. Derive if the fixup is being called for dmic or speaker 2. Having a new dailink
>>
>> If #2 is not preferred (going by Pierre's comments), can we use rtd-
>>> cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
>> I can test this and get back to you.
> Tested and the following is something we can use without creating a new dailink.
> struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
> if (params_channels(params) == 2 || DMIC_CH(dmic_constraints) == 2)
> channels->min = channels->max = 2;
> else
> channels->min = channels->max = 4;
> } else {
> rate->min = rate->max = 48000;
> channels->min = channels->max = 2;
> snd_mask_none(fmt);
> snd_mask_set_format(fmt, pcm_fmt);
> }
>
> Pierre, thoughts?

thanks Harsha, that looks like what I had in mind, but my earlier
question was why we deal with the rates and formats only in the last case?

2020-07-01 17:01:28

[permalink] [raw]

Subject: RE: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

> >>
> >>>> For speakers and headsets its 48Khz, 2 ch and 24 bit and this
> >>>> setting is done based on the front-end dai For speakers, however
> >>>> support only
> >>>> 16 bit, so we set it back to 16 bit If the front end dai is dmic,
> >>>> then the channel
> >>> is set to either 2 or 4 dmic_constraints. No other formats need to be set.
> >>>>
> >>>> All the SSP1 usages do not have the same parameters (as dmic is on
> >>>> SSP1 and its different as given above) Most parameters are same for
> >>> speakers and headset which are on different SSP. This is the reason
> >>> we had a single fixup function.
> >> On SSP1, for dmic we need to fix the channels which is derived from
> >> dmic_num of the snd_soc_acpi_mach structure based on the number of
> >> dmic on the board.
> >> The channel is something that might be different from speakers.
> >> We might not want to constraint the dmic capture to always be 48Khz as
> well.
> >> Given this, there seems to me, 2 ways to set it:
> >> 1. Derive if the fixup is being called for dmic or speaker 2. Having
> >> a new dailink
> >>
> >> If #2 is not preferred (going by Pierre's comments), can we use rtd-
> >>> cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
> >> I can test this and get back to you.
> > Tested and the following is something we can use without creating a new
> dailink.
> > struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
> > if (params_channels(params) == 2 ||
> DMIC_CH(dmic_constraints) == 2)
> > channels->min = channels->max = 2;
> > else
> > channels->min = channels->max = 4;
> > } else {
> > rate->min = rate->max = 48000;
> > channels->min = channels->max = 2;
> > snd_mask_none(fmt);
> > snd_mask_set_format(fmt, pcm_fmt);
> > }
> >
> > Pierre, thoughts?
>
> thanks Harsha, that looks like what I had in mind, but my earlier question was
> why we deal with the rates and formats only in the last case?
The speaker codec supported only 16 bit. (Vendor mentioned)
For playback on this platform, only 48Khz was used.

2020-07-01 17:11:28

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

>>> Tested and the following is something we can use without creating a new
>> dailink.
>>> struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>>> if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
>>> if (params_channels(params) == 2 ||
>> DMIC_CH(dmic_constraints) == 2)
>>> channels->min = channels->max = 2;
>>> else
>>> channels->min = channels->max = 4;
>>> } else {
>>> rate->min = rate->max = 48000;
>>> channels->min = channels->max = 2;
>>> snd_mask_none(fmt);
>>> snd_mask_set_format(fmt, pcm_fmt);
>>> }
>>>
>>> Pierre, thoughts?
>>
>> thanks Harsha, that looks like what I had in mind, but my earlier question was
>> why we deal with the rates and formats only in the last case?
> The speaker codec supported only 16 bit. (Vendor mentioned)
> For playback on this platform, only 48Khz was used.

ok then, as long as Harsha and Lukasz are aligned I'm fine. Thanks!

2020-07-02 10:11:01

[permalink] [raw]

Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function

Hi,

I am also ok with Harsha patch. I have checked it on my Eve and it looks ok.

Best regards,

Lukasz

śr., 1 lip 2020 o 19:08 Pierre-Louis Bossart
<[email protected]> napisał(a):
>
>
> >>> Tested and the following is something we can use without creating a new
> >> dailink.
> >>> struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> >>> if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
> >>> if (params_channels(params) == 2 ||
> >> DMIC_CH(dmic_constraints) == 2)
> >>> channels->min = channels->max = 2;
> >>> else
> >>> channels->min = channels->max = 4;
> >>> } else {
> >>> rate->min = rate->max = 48000;
> >>> channels->min = channels->max = 2;
> >>> snd_mask_none(fmt);
> >>> snd_mask_set_format(fmt, pcm_fmt);
> >>> }
> >>>
> >>> Pierre, thoughts?
> >>
> >> thanks Harsha, that looks like what I had in mind, but my earlier question was
> >> why we deal with the rates and formats only in the last case?
> > The speaker codec supported only 16 bit. (Vendor mentioned)
> > For playback on this platform, only 48Khz was used.
>
> ok then, as long as Harsha and Lukasz are aligned I'm fine. Thanks!
>

2020-07-03 12:19:49

[permalink] [raw]

Subject: [PATCH v4] ASoC: Intel: kbl_rt5663_rt5514_max98927: Fix kabylake_ssp_fixup function

Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names,
as current approach, leads to crash while trying to get snd_soc_dpcm with
container_of() macro in kabylake_ssp_fixup().
The crash call path looks as below:
soc_pcm_hw_params()
snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
rtd->dai_link->be_hw_params_fixup(rtd, params)
kabylake_ssp_fixup()
In this case, codec_params is just a copy of an internal structure and is
not embedded into struct snd_soc_dpcm thus we cannot use
container_of() on it.

v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
v3 -> v4:
- Updated code and commit description according to
solution proposed by Harsha

Signed-off-by: Lukasz Majczak <[email protected]>
Signed-off-by: Harsha Priya <[email protected]>
---
.../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++-----------
1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index b34cf6cf11395..df454de40739a 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
{
struct snd_interval *rate = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_RATE);
- struct snd_interval *chan = hw_param_interval(params,
+ struct snd_interval *channels = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);
struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
- params, struct snd_soc_dpcm, hw_params);
- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
+ struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);

/*
* The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
+
+ if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
+ if (params_channels(params) == 2 ||
+ DMIC_CH(dmic_constraints) == 2)
+ channels->min = channels->max = 2;
+ else
+ channels->min = channels->max = 4;
+ } else {
rate->min = rate->max = 48000;
- chan->min = chan->max = 2;
+ channels->min = channels->max = 2;
snd_mask_none(fmt);
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
- } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
- if (params_channels(params) == 2 ||
- DMIC_CH(dmic_constraints) == 2)
- chan->min = chan->max = 2;
- else
- chan->min = chan->max = 4;
}
/*
* The speaker on the SSP0 supports S16_LE and not S24_LE.
* thus changing the mask here
*/
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
+ if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI))
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);

return 0;
--
2.25.1

2020-07-06 15:42:54

[permalink] [raw]

Subject: Re: [PATCH v4] ASoC: Intel: kbl_rt5663_rt5514_max98927: Fix kabylake_ssp_fixup function

On 7/3/20 7:16 AM, Lukasz Majczak wrote:
> Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names,
> as current approach, leads to crash while trying to get snd_soc_dpcm with
> container_of() macro in kabylake_ssp_fixup().
> The crash call path looks as below:
> soc_pcm_hw_params()
> snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
> rtd->dai_link->be_hw_params_fixup(rtd, params)
> kabylake_ssp_fixup()
> In this case, codec_params is just a copy of an internal structure and is
> not embedded into struct snd_soc_dpcm thus we cannot use
> container_of() on it.
>
> v1 -> v2:
> - Extract dmic from SSP0 as every BE should have own fixup function.
> v2 -> v3:
> - Restore naming in the dapm route table to not confuse with other
> drivers
> - Fixed indentations
> v3 -> v4:
> - Updated code and commit description according to
> solution proposed by Harsha

Looks good Lukasz but you need to move the information on changes below
the --- marker (~4 lines below).

>
> Signed-off-by: Lukasz Majczak <[email protected]>
> Signed-off-by: Harsha Priya <[email protected]>
> ---
> .../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++-----------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> index b34cf6cf11395..df454de40739a 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
> {
> struct snd_interval *rate = hw_param_interval(params,
> SNDRV_PCM_HW_PARAM_RATE);
> - struct snd_interval *chan = hw_param_interval(params,
> + struct snd_interval *channels = hw_param_interval(params,
> SNDRV_PCM_HW_PARAM_CHANNELS);
> struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
> - struct snd_soc_dpcm *dpcm = container_of(
> - params, struct snd_soc_dpcm, hw_params);
> - struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
> - struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>
> /*
> * The ADSP will convert the FE rate to 48k, stereo, 24 bit
> */
> - if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
> - !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||
> - !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
> +
> + if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
> + if (params_channels(params) == 2 ||
> + DMIC_CH(dmic_constraints) == 2)
> + channels->min = channels->max = 2;
> + else
> + channels->min = channels->max = 4;
> + } else {
> rate->min = rate->max = 48000;
> - chan->min = chan->max = 2;
> + channels->min = channels->max = 2;
> snd_mask_none(fmt);
> snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
> - } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
> - if (params_channels(params) == 2 ||
> - DMIC_CH(dmic_constraints) == 2)
> - chan->min = chan->max = 2;
> - else
> - chan->min = chan->max = 4;
> }
> /*
> * The speaker on the SSP0 supports S16_LE and not S24_LE.
> * thus changing the mask here
> */
> - if (!strcmp(be_dai_link->name, "SSP0-Codec"))
> + if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI))
> snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
>
> return 0;
>

2020-07-07 08:35:47

[permalink] [raw]

Subject: [PATCH v5] ASoC: Intel: kbl_rt5663_rt5514_max98927: Fix kabylake_ssp_fixup function

Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names,
as current approach, leads to crash while trying to get snd_soc_dpcm with
container_of() macro in kabylake_ssp_fixup().
The crash call path looks as below:
soc_pcm_hw_params()
snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
rtd->dai_link->be_hw_params_fixup(rtd, params)
kabylake_ssp_fixup()
In this case, codec_params is just a copy of an internal structure and is
not embedded into struct snd_soc_dpcm thus we cannot use
container_of() on it.

Signed-off-by: Lukasz Majczak <[email protected]>
Signed-off-by: Harsha Priya <[email protected]>
---
v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
v3 -> v4:
- Updated code and commit description according to
solution proposed by Harsha
---
.../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++-----------
1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index b34cf6cf11395..df454de40739a 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
{
struct snd_interval *rate = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_RATE);
- struct snd_interval *chan = hw_param_interval(params,
+ struct snd_interval *channels = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);
struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
- params, struct snd_soc_dpcm, hw_params);
- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
+ struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);

/*
* The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
+
+ if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
+ if (params_channels(params) == 2 ||
+ DMIC_CH(dmic_constraints) == 2)
+ channels->min = channels->max = 2;
+ else
+ channels->min = channels->max = 4;
+ } else {
rate->min = rate->max = 48000;
- chan->min = chan->max = 2;
+ channels->min = channels->max = 2;
snd_mask_none(fmt);
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
- } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
- if (params_channels(params) == 2 ||
- DMIC_CH(dmic_constraints) == 2)
- chan->min = chan->max = 2;
- else
- chan->min = chan->max = 4;
}
/*
* The speaker on the SSP0 supports S16_LE and not S24_LE.
* thus changing the mask here
*/
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
+ if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI))
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);

return 0;
--
2.25.1

2020-07-07 16:07:05

[permalink] [raw]

Subject: Re: [PATCH v5] ASoC: Intel: kbl_rt5663_rt5514_max98927: Fix kabylake_ssp_fixup function

On 7/7/20 3:34 AM, Lukasz Majczak wrote:
> Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names,
> as current approach, leads to crash while trying to get snd_soc_dpcm with
> container_of() macro in kabylake_ssp_fixup().
> The crash call path looks as below:
> soc_pcm_hw_params()
> snd_soc_dai_hw_params(codec_dai, substream, &codec_params);
> rtd->dai_link->be_hw_params_fixup(rtd, params)
> kabylake_ssp_fixup()
> In this case, codec_params is just a copy of an internal structure and is
> not embedded into struct snd_soc_dpcm thus we cannot use
> container_of() on it.
>
> Signed-off-by: Lukasz Majczak <[email protected]>
> Signed-off-by: Harsha Priya <[email protected]>

Acked-by: Pierre-Louis Bossart <[email protected]>

> ---
> v1 -> v2:
> - Extract dmic from SSP0 as every BE should have own fixup function.
> v2 -> v3:
> - Restore naming in the dapm route table to not confuse with other
> drivers
> - Fixed indentations
> v3 -> v4:
> - Updated code and commit description according to
> solution proposed by Harsha
> ---
> .../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++-----------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> index b34cf6cf11395..df454de40739a 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
> {
> struct snd_interval *rate = hw_param_interval(params,
> SNDRV_PCM_HW_PARAM_RATE);
> - struct snd_interval *chan = hw_param_interval(params,
> + struct snd_interval *channels = hw_param_interval(params,
> SNDRV_PCM_HW_PARAM_CHANNELS);
> struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
> - struct snd_soc_dpcm *dpcm = container_of(
> - params, struct snd_soc_dpcm, hw_params);
> - struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
> - struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
>
> /*
> * The ADSP will convert the FE rate to 48k, stereo, 24 bit
> */
> - if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
> - !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||
> - !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
> +
> + if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
> + if (params_channels(params) == 2 ||
> + DMIC_CH(dmic_constraints) == 2)
> + channels->min = channels->max = 2;
> + else
> + channels->min = channels->max = 4;
> + } else {
> rate->min = rate->max = 48000;
> - chan->min = chan->max = 2;
> + channels->min = channels->max = 2;
> snd_mask_none(fmt);
> snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
> - } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
> - if (params_channels(params) == 2 ||
> - DMIC_CH(dmic_constraints) == 2)
> - chan->min = chan->max = 2;
> - else
> - chan->min = chan->max = 4;
> }
> /*
> * The speaker on the SSP0 supports S16_LE and not S24_LE.
> * thus changing the mask here
> */
> - if (!strcmp(be_dai_link->name, "SSP0-Codec"))
> + if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI))
> snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
>
> return 0;
>

2020-07-16 15:02:01