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

[N, Harshapriya]

> 
> 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 <harshapriya.n@xxxxxxxxx>
> napisaÅ(a):
> >
> > > -----Original Message-----
> > > From: Åukasz Majczak <lma@xxxxxxxxxxxx>
> > > Sent: Monday, June 29, 2020 4:11 AM
> > > To: N, Harshapriya <harshapriya.n@xxxxxxxxx>
> > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Pierre-Louis Bossart <pierre-
> > > louis.bossart@xxxxxxxxxxxxxxx>; Jie Yang <yang.jie@xxxxxxxxxxxxxxx>;
> > > Radoslaw Biernacki <rad@xxxxxxxxxxxx>; Ross Zwisler
> > > <zwisler@xxxxxxxxxx>; linux- kernel@xxxxxxxxxxxxxxx; Liam Girdwood
> > > <liam.r.girdwood@xxxxxxxxxxxxxxx>;
> > > Bob Brandt <brndt@xxxxxxxxxx>; Marcin Wojtas <mw@xxxxxxxxxxxx>;
> Alex
> > > Levin <levinale@xxxxxxxxxxxx>
> > > 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-
> > > louis.bossart@xxxxxxxxxxxxxxx> 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
> > > > > <pierre-louis.bossart@xxxxxxxxxxxxxxx> 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.