[PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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;
+	}
+
 	switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBS_CFS:
 		ret = regmap_update_bits(pcm512x->regmap,
@@ -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) {
-- 
2.13.6

[PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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;
+	}
+
 	switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBS_CFS:
 		ret = regmap_update_bits(pcm512x->regmap,
@@ -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) {
-- 
2.13.6

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

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

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

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

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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.
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.

As technically keeping checs where they are now doesn't break anything, and is
aligned with ASoC codecs design, I suggest to keep the checks where they are.
Would you agree?

> 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

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
> 

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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.
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.

As technically keeping checs where they are now doesn't break anything, and is
aligned with ASoC codecs design, I suggest to keep the checks where they are.
Would you agree?

> 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

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
> 

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

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.

> 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 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

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

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.

> 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 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

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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
> 

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Kirill Marinushkin]

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
> 

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

Hi Kirill,

On 12/11/2020 9.57, Kirill Marinushkin wrote:
>> 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.

Thank you. I know it is slightly more work.

When you send v3 please do not use --in-reply-to, let it be itself.

Regards,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] ASoC: pcm512x: Add support for data formats RJ and LJ

[Peter Ujfalusi]

Hi Kirill,

On 12/11/2020 9.57, Kirill Marinushkin wrote:
>> 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.

Thank you. I know it is slightly more work.

When you send v3 please do not use --in-reply-to, let it be itself.

Regards,
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2] ASoC: pcm512x: Add support for more data formats

[Kirill Marinushkin]

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 8153d3d01654..e309227649e7 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1167,6 +1167,8 @@ 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 offset = 0;
 	int gpio;
 	int clock_output;
 	int master_mode;
@@ -1195,6 +1197,28 @@ 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;
+	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;
+	}
+
 	switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBS_CFS:
 		ret = regmap_update_bits(pcm512x->regmap,
@@ -1236,6 +1260,20 @@ 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;
+	}
+
+	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;
+	}
+
 	if (pcm512x->pll_out) {
 		ret = regmap_write(pcm512x->regmap, PCM512x_FLEX_A, 0x11);
 		if (ret != 0) {
-- 
2.13.6

[PATCH v2] ASoC: pcm512x: Add support for more data formats

[Kirill Marinushkin]

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 8153d3d01654..e309227649e7 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1167,6 +1167,8 @@ 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 offset = 0;
 	int gpio;
 	int clock_output;
 	int master_mode;
@@ -1195,6 +1197,28 @@ 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;
+	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;
+	}
+
 	switch (pcm512x->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBS_CFS:
 		ret = regmap_update_bits(pcm512x->regmap,
@@ -1236,6 +1260,20 @@ 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;
+	}
+
+	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;
+	}
+
 	if (pcm512x->pll_out) {
 		ret = regmap_write(pcm512x->regmap, PCM512x_FLEX_A, 0x11);
 		if (ret != 0) {
-- 
2.13.6