AM43xx-ePOS-EVM audio support with TLV320AIC31XX driver

+- ai31xx-micbias-vg - MicBias Voltage required.
+        MICBIAS_OFF - MICBIAS output it not powered

Quoted 2 lines · Jyri Sarha

+- ai31xx-micbias-vg - MicBias Voltage required.
+        MICBIAS_OFF - MICBIAS output it not powered

Same comment as last time - why is this something which can be selected in the binding?

Documentation/devicetree/bindings/sound/tlv320aic31xx.txt line 25

Quoted 3 lines

Documentation/devicetree/bindings/sound/tlv320aic31xx.txt new file +3 -0

+        MICBIAS_2_0V - MICBIAS output is powered to 2.0V
+        MICBIAS_2_5V - MICBIAS output is powered to 2.5V
+        MICBIAS_AVDD - MICBIAS output is connected to AVDD

The numbers still need to be defined in the binding, the point with the defines is to make both code and DTs more readable but we need to know what is actually going to go into the binary.

sound/soc/codecs/tlv320aic31xx.c line 566

Quoted 3 lines

sound/soc/codecs/tlv320aic31xx.c new file +3 -0

+	/* Mic Bias */
+	SND_SOC_DAPM_SUPPLY("Mic Bias", SND_SOC_NOPM, 0, 0, mic_bias_event,
+			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),

The idiomatic thing would be to use the pin name.

sound/soc/codecs/tlv320aic31xx.c line 615

Quoted 2 lines

sound/soc/codecs/tlv320aic31xx.c new file +2 -0

+	/* Make ADC turn on when recording even if not mixed from any inputs */
+	{"ADC", NULL, "Internal ADC Source"},

Don't do this (or the equivalent from the DACs) - we don't do this for any other drivers, we shouldn't do it for this one. If we're going to do something like this it should be generic not per driver hacks.

sound/soc/codecs/tlv320aic31xx.c line 1076

Quoted 7 lines

sound/soc/codecs/tlv320aic31xx.c new file +7 -0

+	case SND_SOC_BIAS_STANDBY:
+		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)
+			aic31xx_set_power(codec, 1);
+		break;
+	case SND_SOC_BIAS_OFF:
+		aic31xx_set_power(codec, 0);
+		break;

Just inline the set power function, or at the very least split it into separate on and off functions - there is zero shared code between the power on and off paths.

Quoted 5 lines · Mark Brown

sound/soc/codecs/tlv320aic31xx.c line 566

Quoted 3 lines

sound/soc/codecs/tlv320aic31xx.c new file +3 -0

+	/* Mic Bias */
+	SND_SOC_DAPM_SUPPLY("Mic Bias", SND_SOC_NOPM, 0, 0, mic_bias_event,
+			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),

The idiomatic thing would be to use the pin name.

In this case the mic bias comes out of the chip trough a separate pin and it is up to board designer to connect it with any (or all) of the three mic pins.

Quoted 7 lines · Jyri Sarha

Quoted 5 lines · Mark Brown

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* Mic Bias */
+	SND_SOC_DAPM_SUPPLY("Mic Bias", SND_SOC_NOPM, 0, 0, mic_bias_event,
+			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),

The idiomatic thing would be to use the pin name.

Quoted 3 lines

In this case the mic bias comes out of the chip trough a separate pin and it is up to board designer to connect it with any (or all) of the three mic pins.

Yes, exactly - you should be using the name of the pin that supplies the bias. This is how essentially all microphone biases work.

The tlv320aic32x4 related files were wrongly placed after tlv320aic3x files.

Quoted 1 line · Jyri Sarha

The tlv320aic32x4 related files were wrongly placed after tlv320aic3x files.

This doesn't apply against current code, please check and resend.

sound/soc/davinci/davinci-evm.c +15 -0

+	struct device_node *np = codec->card->dev->of_node;
+	int ret;
+
+	snd_soc_dapm_new_controls(dapm, aic31xx_dapm_widgets,
+				  ARRAY_SIZE(aic31xx_dapm_widgets));
+
+	if (np) {
+		ret = snd_soc_of_parse_audio_routing(codec->card,
+						     "ti,audio-routing");
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

sound/soc/davinci/davinci-evm.c static int evm_aic3x_init(struct snd_soc_pcm_runtime *rtd) · line 141

Quoted 12 lines

sound/soc/davinci/davinci-evm.c +11 -0

+static const struct snd_soc_dapm_widget aic31xx_dapm_widgets[] = {
+    SND_SOC_DAPM_HP("Headphone Jack", NULL),
+    SND_SOC_DAPM_SPK("Speaker", NULL),
+    SND_SOC_DAPM_MIC("Mic Jack", NULL),
+};
+
+/* Logic for EVMs with an aic31xx */
+static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd)
+{
+    struct snd_soc_codec *codec = rtd->codec;
+    struct snd_soc_dapm_context *dapm = &codec->dapm;

Machine level DAPM elements should go into the card's DAPM context, not the CODEC context. Also use rtd->card to get a pointer to the card instead of rtd->codec->card.

Add machine driver support for AM43xx-ePOS-EVM and update associated device tree binding document.

sound/soc/davinci/davinci-evm.c +8 -0

+/* Logic for EVMs with an aic31xx */
+static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_card *card = rtd->card;
+	struct snd_soc_dapm_context *dapm = &card->dapm;
+	struct device_node *np = card->dev->of_node;
+	int ret;

Quoted 2 lines · Jyri Sarha

Add machine driver support for AM43xx-ePOS-EVM and update associated device tree binding document.

Don't send new patches in replies to the middle of threads, that is just confusing and hard to follow. Send a new series.

Quoted 8 lines

sound/soc/davinci/davinci-evm.c +8 -0

+/* Logic for EVMs with an aic31xx */
+static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_card *card = rtd->card;
+	struct snd_soc_dapm_context *dapm = &card->dapm;
+	struct device_node *np = card->dev->of_node;
+	int ret;

sound/soc/davinci/davinci-evm.c static int evm_aic3x_init(struct snd_soc_pcm_runtime *rtd) · line 154

Quoted 10 lines

sound/soc/davinci/davinci-evm.c +10 -0

+
+	snd_soc_dapm_new_controls(dapm, aic31xx_dapm_widgets,
+				  ARRAY_SIZE(aic31xx_dapm_widgets));
+
+	if (np) {
+		ret = snd_soc_of_parse_audio_routing(codec->card,
+						     "ti,audio-routing");
+		if (ret)
+			return ret;
+	}

Why not add the DAPM widget table to...

sound/soc/davinci/davinci-evm.c static struct snd_soc_dai_link evm_dai_tlv320aic3x = { · line 375

Quoted 4 lines

sound/soc/davinci/davinci-evm.c +4 -0

+	{
+		.compatible = "ti,am43xx-epos-evm-audio",
+		.data = &evm_dai_tlv320aic3111,
+	},

...the data here?

Or alternatively should support for the widgets binding that was recently added by added to the kernel - that way the binding becomes more general and doesnn't need individual boards adding? Indeed ideally the simple-card binding (which that was added for) could just be used for these boards.

However, I think there is a problem with simple-card. It does not invert bitclock-master and frame-master values when converting them to CB[SM] and CF[SM] for cpu_dai as I think it should. I can get around it by setting the both cpu-dai and the codec as bclk and frame masters in the sound node. I volunteer to fix this if you agree.

Quoted 6 lines · Jyri Sarha

Quoted 6 lines

However, I think there is a problem with simple-card. It does not invert bitclock-master and frame-master values when converting them to CB[SM] and CF[SM] for cpu_dai as I think it should. I can get around it by setting the both cpu-dai and the codec as bclk and frame masters in the sound node. I volunteer to fix this if you agree.

No, it shouldn't do any inversion. If inversion is required one of the drivers is buggy, they are CODEC bit master and CPU bit master so for a CPU driver the sense should be inverted when parsing.

Quoted 3 lines · Mark Brown

No, it shouldn't do any inversion. If inversion is required one of the drivers is buggy, they are CODEC bit master and CPU bit master so for a CPU driver the sense should be inverted when parsing.

Yes, that is the problem. The same code in simple-card parses the codec node and cpu-dai node and they produce the same SND_SOC_DAIFMT_C??_C?? flags for both codec and cpu-dai drivers.

Quoted 5 lines · Jyri Sarha

Quoted 3 lines · Mark Brown

No, it shouldn't do any inversion. If inversion is required one of the drivers is buggy, they are CODEC bit master and CPU bit master so for a CPU driver the sense should be inverted when parsing.

Quoted 3 lines

Yes, that is the problem. The same code in simple-card parses the codec node and cpu-dai node and they produce the same SND_SOC_DAIFMT_C??_C?? flags for both codec and cpu-dai drivers.

Sorry, thinko above - the master flags are specified in terms of the CODEC. Anything interpreting them that isn't a CODEC needs to be inverting the sense.

Exactly, CBM_CFM means that the codec is the master of both clocks. Codec is configured as master and the cpu side is configured as slave.

But the issue is that with simple card (when you want to have the codec as master for both clocks):

simple-audio-card,codec {
	sound-dai = <&aic3106>;
	bitclock-master;
	frame-master;
};

simple-audio-card,cpu {
	sound-dai = <&mcasp1 0>;
};

The codec will get CBM_CFM, however the cpu_dai will end up having CBS_CFS (since *-master is not specified in the dts).

So when the simple card parses the master/slave configuration it has to invert the cpu_dai settings it got back from snd_soc_of_parse_daifmt() to get it right.