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 delay before* 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.
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 as lpass_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 bit clk: %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?
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. I presume that your prepare() function isn't reentrant and can't be called at the same time as your shutdown (right?).
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