[PATCH v3 0/4] ASoC: pcm512x: Patch series to set fmt from `set_fmt()`
Set format from `set_fmt()` func instead of `hw_params()`, plus supportive commits
Kirill Marinushkin (4): ASoC: pcm512x: Fix not setting word length if DAIFMT_CBS_CFS ASoC: pcm512x: Rearrange operations in `hw_params()` ASoC: pcm512x: Move format check into `set_fmt()` ASoC: pcm512x: Add support for more data formats
sound/soc/codecs/pcm512x.c | 134 ++++++++++++++++++++++++++++----------------- 1 file changed, 84 insertions(+), 50 deletions(-)
In `pcm512x_hw_params()`, the following switch-case:
~~~~ switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: ~~~~
returns 0, which was preventing word length from being written into codecs register.
Fixed by writing it into register before checking `SND_SOC_DAIFMT_MASTER_MASK`. Tested with Raspberry Pi + sound card `hifiberry_dacplus` in CBS_CFS format
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 | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index 8153d3d01654..db3dc6a40feb 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1195,6 +1195,13 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
+ ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1, + PCM512x_ALEN, alen); + if (ret != 0) { + dev_err(component->dev, "Failed to set frame size: %d\n", ret); + return ret; + } + switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: ret = regmap_update_bits(pcm512x->regmap, @@ -1229,13 +1236,6 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1, - PCM512x_ALEN, alen); - if (ret != 0) { - dev_err(component->dev, "Failed to set frame size: %d\n", ret); - return ret; - } - if (pcm512x->pll_out) { ret = regmap_write(pcm512x->regmap, PCM512x_FLEX_A, 0x11); if (ret != 0) {
This commit is a preparation for the next patch in the series. It's goal is to make format check easy-to-move-out. Theoretically, more butifications are possile in `hw_params()` func, but my intention in this commit is to keep behaviour unchanged.
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 | 49 +++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 29 deletions(-)
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index db3dc6a40feb..aa55a477a28f 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1204,16 +1204,8 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: - ret = regmap_update_bits(pcm512x->regmap, - PCM512x_BCLK_LRCLK_CFG, - PCM512x_BCKP - | PCM512x_BCKO | PCM512x_LRKO, - 0); - if (ret != 0) { - dev_err(component->dev, - "Failed to enable slave mode: %d\n", ret); - return ret; - } + clock_output = 0; + master_mode = 0;
ret = regmap_update_bits(pcm512x->regmap, PCM512x_ERROR_DETECT, PCM512x_DCAS, 0); @@ -1223,7 +1215,7 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, ret); return ret; } - return 0; + goto skip_pll; case SND_SOC_DAIFMT_CBM_CFM: clock_output = PCM512x_BCKO | PCM512x_LRKO; master_mode = PCM512x_RLRK | PCM512x_RBCK; @@ -1316,25 +1308,7 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, dev_err(component->dev, "Failed to enable pll: %d\n", ret); return ret; } - } - - ret = regmap_update_bits(pcm512x->regmap, PCM512x_BCLK_LRCLK_CFG, - PCM512x_BCKP | PCM512x_BCKO | PCM512x_LRKO, - clock_output); - if (ret != 0) { - dev_err(component->dev, "Failed to enable clock output: %d\n", ret); - return ret; - }
- ret = regmap_update_bits(pcm512x->regmap, PCM512x_MASTER_MODE, - PCM512x_RLRK | PCM512x_RBCK, - master_mode); - if (ret != 0) { - dev_err(component->dev, "Failed to enable master mode: %d\n", ret); - return ret; - } - - if (pcm512x->pll_out) { gpio = PCM512x_G1OE << (pcm512x->pll_out - 1); ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN, gpio, gpio); @@ -1368,6 +1342,23 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return ret; }
+skip_pll: + ret = regmap_update_bits(pcm512x->regmap, PCM512x_BCLK_LRCLK_CFG, + PCM512x_BCKP | PCM512x_BCKO | PCM512x_LRKO, + clock_output); + if (ret != 0) { + dev_err(component->dev, "Failed to enable clock output: %d\n", ret); + return ret; + } + + ret = regmap_update_bits(pcm512x->regmap, PCM512x_MASTER_MODE, + PCM512x_RLRK | PCM512x_RBCK, + master_mode); + if (ret != 0) { + dev_err(component->dev, "Failed to enable master mode: %d\n", ret); + return ret; + } + return 0; }
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;
Currently, pcm512x driver supports only I2S data format. This commit adds RJ, LJ, DSP_A and DSP_B as well.
I don't expect regression WRT existing sound cards, because:
* default value in corresponding register of pcm512x codec is 0 == I2S * existing in-tree sound cards with pcm512x codec are configured for I2S * i don't see how existing off-tree sound cards with pcm512x codec could be configured differently - it would not work * tested explicitly, that there is no regression with Raspberry Pi + sound card `sound/soc/bcm/hifiberry_dacplus.c`
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 | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index 22ef77955a28..4dc844f3c1fc 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1335,6 +1335,8 @@ 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 afmt; + int offset = 0; int clock_output; int master_mode; int ret; @@ -1372,6 +1374,42 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) return ret; }
+ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + afmt = PCM512x_AFMT_I2S; + break; + case SND_SOC_DAIFMT_RIGHT_J: + afmt = PCM512x_AFMT_RTJ; + break; + case SND_SOC_DAIFMT_LEFT_J: + afmt = PCM512x_AFMT_LTJ; + break; + case SND_SOC_DAIFMT_DSP_A: + offset = 1; + fallthrough; + case SND_SOC_DAIFMT_DSP_B: + afmt = PCM512x_AFMT_DSP; + break; + default: + dev_err(component->dev, "unsupported DAI format: 0x%x\n", + pcm512x->fmt); + return -EINVAL; + } + + ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1, + PCM512x_AFMT, afmt); + if (ret != 0) { + dev_err(component->dev, "Failed to set data format: %d\n", ret); + return ret; + } + + ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_2, + 0xFF, offset); + if (ret != 0) { + dev_err(component->dev, "Failed to set data offset: %d\n", ret); + return ret; + } + pcm512x->fmt = fmt;
return 0;
Kirill,
On 15/11/2020 14.23, Kirill Marinushkin wrote:
Set format from `set_fmt()` func instead of `hw_params()`, plus supportive commits
Thank you for the clean series!
Reviewed-by: Peter Ujfalusi peter.ujfalusi@ti.com
Kirill Marinushkin (4): ASoC: pcm512x: Fix not setting word length if DAIFMT_CBS_CFS ASoC: pcm512x: Rearrange operations in `hw_params()` ASoC: pcm512x: Move format check into `set_fmt()` ASoC: pcm512x: Add support for more data formats
sound/soc/codecs/pcm512x.c | 134 ++++++++++++++++++++++++++++----------------- 1 file changed, 84 insertions(+), 50 deletions(-)
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Sun, 15 Nov 2020 13:23:02 +0100, Kirill Marinushkin wrote:
Set format from `set_fmt()` func instead of `hw_params()`, plus supportive commits
Kirill Marinushkin (4): ASoC: pcm512x: Fix not setting word length if DAIFMT_CBS_CFS ASoC: pcm512x: Rearrange operations in `hw_params()` ASoC: pcm512x: Move format check into `set_fmt()` ASoC: pcm512x: Add support for more data formats
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: pcm512x: Fix not setting word length if DAIFMT_CBS_CFS commit: 6feaaa7c19bde25595e03bf883953f85711e4ac8 [2/4] ASoC: pcm512x: Rearrange operations in `hw_params()` commit: 798714b6121d833c8abe4161761a94fdd1e73a90 [3/4] ASoC: pcm512x: Move format check into `set_fmt()` commit: 26b97d95a05d0346e1ad6096deedac3f24a4607b [4/4] ASoC: pcm512x: Add support for more data formats commit: 25d27c4f68d2040c4772d586be3e02ee99eb71af
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Kirill Marinushkin -
Mark Brown -
Peter Ujfalusi