I would like to describe the reasoning by quoting Peter Ujfalusi peter.ujfalusi@ti.com from his review of this patch series v1 [1]:
When you bind a link you will use set_fmt for the two sides to see if they can agree, that both can support what has been asked.
The pcm512x driver just saves the fmt and say back to that card: whatever, I'm fine with it. But runtime during hw_params it can fail due to unsupported bus format, which it actually acked to be ok.
This is the difference.
Sure, some device have constraint based on the fmt towards the hw_params and it is perfectly OK to do such a checks and rejections or build rules/constraints based on fmt, but failing hw_params just because set_fmt did not checked that the bus format is not even supported is not a nice thing to do.
[1] https://patchwork.kernel.org/project/alsa-devel/patch/ 20201109212133.25869-1-kmarinushkin@birdec.com/
Signed-off-by: Kirill Marinushkin kmarinushkin@birdec.com Cc: Mark Brown broonie@kernel.org Cc: Takashi Iwai tiwai@suse.com Cc: Liam Girdwood lgirdwood@gmail.com Cc: Matthias Reichl hias@horus.com Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Peter Ujfalusi peter.ujfalusi@ti.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org --- sound/soc/codecs/pcm512x.c | 55 +++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index aa55a477a28f..22ef77955a28 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1168,8 +1168,6 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component); int alen; int gpio; - int clock_output; - int master_mode; int ret;
dev_dbg(component->dev, "hw_params %u Hz, %u channels\n", @@ -1202,11 +1200,8 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return ret; }
- switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) { - case SND_SOC_DAIFMT_CBS_CFS: - clock_output = 0; - master_mode = 0; - + if ((pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) == + SND_SOC_DAIFMT_CBS_CFS) { ret = regmap_update_bits(pcm512x->regmap, PCM512x_ERROR_DETECT, PCM512x_DCAS, 0); if (ret != 0) { @@ -1216,16 +1211,6 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return ret; } goto skip_pll; - case SND_SOC_DAIFMT_CBM_CFM: - clock_output = PCM512x_BCKO | PCM512x_LRKO; - master_mode = PCM512x_RLRK | PCM512x_RBCK; - break; - case SND_SOC_DAIFMT_CBM_CFS: - clock_output = PCM512x_BCKO; - master_mode = PCM512x_RBCK; - break; - default: - return -EINVAL; }
if (pcm512x->pll_out) { @@ -1343,6 +1328,34 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, }
skip_pll: + return 0; +} + +static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{ + struct snd_soc_component *component = dai->component; + struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component); + int clock_output; + int master_mode; + int ret; + + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + clock_output = 0; + master_mode = 0; + break; + case SND_SOC_DAIFMT_CBM_CFM: + clock_output = PCM512x_BCKO | PCM512x_LRKO; + master_mode = PCM512x_RLRK | PCM512x_RBCK; + break; + case SND_SOC_DAIFMT_CBM_CFS: + clock_output = PCM512x_BCKO; + master_mode = PCM512x_RBCK; + break; + default: + return -EINVAL; + } + ret = regmap_update_bits(pcm512x->regmap, PCM512x_BCLK_LRCLK_CFG, PCM512x_BCKP | PCM512x_BCKO | PCM512x_LRKO, clock_output); @@ -1359,14 +1372,6 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return ret; }
- return 0; -} - -static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) -{ - struct snd_soc_component *component = dai->component; - struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component); - pcm512x->fmt = fmt;
return 0;