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@xxxxxxxxxx> >>>> Cc: Mark Brown <broonie@xxxxxxxxxx> >>>> Cc: Takashi Iwai <tiwai@xxxxxxxx> >>>> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx> >>>> Cc: Matthias Reichl <hias@xxxxxxxxx> >>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> >>>> Cc: Peter Ujfalusi <peter.ujfalusi@xxxxxx> >>>> Cc: alsa-devel@xxxxxxxxxxxxxxxx >>>> Cc: linux-kernel@xxxxxxxxxxxxxxx >>>> --- >>>> 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 >