Hello Peter,
On 11/12/2020 08:41 AM, Peter Ujfalusi wrote:
Hi Kirill,
On 11/11/2020 9.54, Kirill Marinushkin wrote:
Hello Peter,
than you for your review!
The bus format and
switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
case SND_SOC_DAIFMT_CBS_CFS: ret = regmap_update_bits(pcm512x->regmap,
the clock generation role should be set in pcm512x_set_fmt(), in that way you can deny specific setups earlier.
I think we could move both checks for`SND_SOC_DAIFMT_FORMAT_MASK` and `SND_SOC_DAIFMT_MASTER_MASK` into `pcm512x_set_fmt()`. But it would be a different scope, and I didn't intend to do that level of refactoring.
Right, I was just saying what would make sense.
Looking at other codecs in kernel, I would say, that doing those checks in `pcm512x_hw_params()`, as they are done currently, is an equally valid approach.
The exception proves the rule
As technically keeping checs where they are now doesn't break anything
They are just in a wrong place.
and is aligned with ASoC codecs design, I suggest to keep the checks where they are.
The set_fmt callback is there to set the bus format, it has nothing to do (in most cases) with the sample format (hw_params). Bus coding, clock source has nothing to do with hw_params.
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.
Those are good arguments
Would you agree?
I don't have a device to test, I'm just trying to point out what is the right thing to do.
I have a device to test. I will move format checks into `pcm512x_set_fmt()`, ensure that it works properly, and submit as patch v3.
I don't buy the argument that the sequencing is important here for the register writes. The fmt is set only once and those registers will be only written once.
I would also add DSP_A and DSP_B modes at the same time, DSP_A would need a write of 1 to register 41 (PCM512x_I2S_2, offset = 1), other formats should set the offset to 0.
That's a good idea, than you for technical details! I just didn't know how to use DSP_A and DSP_B. I will add them, and submit as patch v2
Great! Thanks
- Péter
Best regards, Kirill
On 11/10/2020 07:59 AM, Peter Ujfalusi wrote:
On 09/11/2020 23.21, Kirill Marinushkin wrote:
Currently, pcm512x driver supports only I2S data format. This commit adds RJ and LJ 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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index 8153d3d01654..c6e975fb4a43 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1167,6 +1167,7 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, struct snd_soc_component *component = dai->component; struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component); int alen;
- int afmt; int gpio; int clock_output; int master_mode;
@@ -1195,6 +1196,22 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- switch (pcm512x->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;- default:
dev_err(component->dev, "unsupported DAI format: 0x%x\n",pcm512x->fmt);return -EINVAL;- }
The bus format and
switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
case SND_SOC_DAIFMT_CBS_CFS: ret = regmap_update_bits(pcm512x->regmap,
the clock generation role should be set in pcm512x_set_fmt(), in that way you can deny specific setups earlier.
I would also add DSP_A and DSP_B modes at the same time, DSP_A would need a write of 1 to register 41 (PCM512x_I2S_2, offset = 1), other formats should set the offset to 0.
@@ -1236,6 +1253,13 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream, return ret; }
- 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;- }
- if (pcm512x->pll_out) { ret = regmap_write(pcm512x->regmap, PCM512x_FLEX_A, 0x11); if (ret != 0) {
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki