On Fri, Jun 4, 2021 at 6:41 AM Doug Anderson dianders@chromium.org wrote:
Judy,
On Thu, Jun 3, 2021 at 8:08 AM Judy Hsiao judyhsiao@chromium.org wrote:
@@ -315,12 +353,54 @@ static int lpass_cpu_daiops_trigger(struct
snd_pcm_substream *substream,
return ret;}
+static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);struct lpaif_i2sctl *i2sctl = drvdata->i2sctl;unsigned int id = dai->driver->id;int ret;/** Ensure lpass BCLK/LRCLK is enabled bit before playback/capture* data flow starts. This allows other codec to have some delaybefore
* the data flow.* (ex: to drop start up pop noise before capture starts).*/nit: there's usually a blank line between the variable declarations and the first line of code, even if the first line of code is a comment. Thanks, noted.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)ret = regmap_fields_write(i2sctl->spken, id,LPAIF_I2SCTL_SPKEN_ENABLE);
elseret = regmap_fields_write(i2sctl->micen, id,LPAIF_I2SCTL_MICEN_ENABLE);
if (ret) {dev_err(dai->dev, "error writing to i2sctl reg: %d\n",ret);
return ret;}/** Check mi2s_was_prepared before enabling BCLK aslpass_cpu_daiops_prepare can
* be called multiple times. It's paired with the clk_disable in* lpass_cpu_daiops_shutdown.*/if (!drvdata->mi2s_was_prepared[dai->driver->id]) {drvdata->mi2s_was_prepared[dai->driver->id] = true;ret = clk_enable(drvdata->mi2s_bit_clk[id]);if (ret) {dev_err(dai->dev, "error in enabling mi2s bitclk: %d\n", ret);
clk_disable(drvdata->mi2s_osr_clk[id]);Can you explain why this clk_disable() is here? Your function didn't turn this clock on, so why should it be turning it off in the error case?
The OSR CLK is disabled in the error case, not the BCLK.
drvdata->mi2s_was_prepared[dai->driver->id] =false;
return ret;}Why not put the `drvdata->mi2s_was_prepared[dai->driver->id] = true;` _after_ you check for errors. Then you don't need to undo it in the error case.
Noted, thanks.
I presume that your prepare() function isn't reentrant and can't be called at the same time as your shutdown (right?).
https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-pcm.c#L658 https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-pcm.c#L825 I think yes, snd_soc_pcm_dai_prepare and snd_soc_dai_shutdown are both guard by mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
Other than that, I don't have any objections to this patch anymore. I probably won't add a formal "Reviewed-by", though, since I _really_ don't know anything about the issue at hand or the code. I just stumbled upon this because I was getting the clock splat at bootup. If someone feels like this needs me to spin up enough to understand / really review this patch then please yell.
-Doug