[PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[Peter Ujfalusi]

From: Misael Lopez Cruz <misael.lopez@ti.com>

Output driver has two parameters that can be configured to reduce
pop noise: power-on delay and ramp-up step time. Two new kcontrols
have been added to set these parameters.

Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/codecs/tlv320aic3x.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index f7c2a575a892..70f8b8be9173 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -270,6 +270,15 @@ static const struct soc_enum aic3x_agc_decay_enum[] = {
 	SOC_ENUM_SINGLE(RAGC_CTRL_A, 0, 4, aic3x_agc_decay),
 };
 
+static const char * const aic3x_poweron_time[] = {
+	"0us", "10us", "100us", "1ms", "10ms", "50ms",
+	"100ms", "200ms", "400ms", "800ms", "2s", "4s" };
+static const char * const aic3x_rampup_step[] = { "0ms", "1ms", "2ms", "4ms" };
+static const struct soc_enum aic3x_pop_reduction_enum[] = {
+	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 4, 12, aic3x_poweron_time),
+	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 2, 4, aic3x_rampup_step),
+};
+
 /*
  * DAC digital volumes. From -63.5 to 0 dB in 0.5 dB steps
  */
@@ -399,6 +408,10 @@ static const struct snd_kcontrol_new aic3x_snd_controls[] = {
 	SOC_DOUBLE_R("PGA Capture Switch", LADC_VOL, RADC_VOL, 7, 0x01, 1),
 
 	SOC_ENUM("ADC HPF Cut-off", aic3x_enum[ADC_HPF_ENUM]),
+
+	/* Pop reduction */
+	SOC_ENUM("Output Driver Power-On time", aic3x_pop_reduction_enum[0]),
+	SOC_ENUM("Output Driver Ramp-up step", aic3x_pop_reduction_enum[1]),
 };
 
 static const struct snd_kcontrol_new aic3x_mono_controls[] = {
-- 
2.1.3

[PATCH 2/2] ASoC: tlv320aic3x: Add TDM support

[Peter Ujfalusi]

TDM support is achieved using DSP transfer mode and setting a
programmable offset which specifies where data begins with
respect to the frame sync.

It requires 256-clock mode if CODEC is master (not currently
supported in the driver). No additional dependency if CODEC
is slave.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/codecs/tlv320aic3x.c | 62 ++++++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/tlv320aic3x.h |  1 +
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 70f8b8be9173..510158a5d8ca 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -78,6 +78,8 @@ struct aic3x_priv {
 	struct aic3x_disable_nb disable_nb[AIC3X_NUM_SUPPLIES];
 	struct aic3x_setup_data *setup;
 	unsigned int sysclk;
+	unsigned int dai_fmt;
+	unsigned int tdm_delay;
 	struct list_head list;
 	int master;
 	int gpio_reset;
@@ -1022,6 +1024,25 @@ found:
 	return 0;
 }
 
+static int aic3x_prepare(struct snd_pcm_substream *substream,
+			 struct snd_soc_dai *dai)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
+	int delay = 0;
+
+	/* TDM slot selection only valid in DSP_A/_B mode */
+	if (aic3x->dai_fmt == SND_SOC_DAIFMT_DSP_A)
+		delay += (aic3x->tdm_delay + 1);
+	else if (aic3x->dai_fmt == SND_SOC_DAIFMT_DSP_B)
+		delay += aic3x->tdm_delay;
+
+	/* Configure data delay */
+	snd_soc_write(codec, AIC3X_ASD_INTF_CTRLC, aic3x->tdm_delay);
+
+	return 0;
+}
+
 static int aic3x_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_codec *codec = dai->codec;
@@ -1061,7 +1082,6 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
 	u8 iface_areg, iface_breg;
-	int delay = 0;
 
 	iface_areg = snd_soc_read(codec, AIC3X_ASD_INTF_CTRLA) & 0x3f;
 	iface_breg = snd_soc_read(codec, AIC3X_ASD_INTF_CTRLB) & 0x3f;
@@ -1089,7 +1109,6 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	case (SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF):
 		break;
 	case (SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_IB_NF):
-		delay = 1;
 	case (SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF):
 		iface_breg |= (0x01 << 6);
 		break;
@@ -1103,10 +1122,45 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 		return -EINVAL;
 	}
 
+	aic3x->dai_fmt = fmt & SND_SOC_DAIFMT_FORMAT_MASK;
+
 	/* set iface */
 	snd_soc_write(codec, AIC3X_ASD_INTF_CTRLA, iface_areg);
 	snd_soc_write(codec, AIC3X_ASD_INTF_CTRLB, iface_breg);
-	snd_soc_write(codec, AIC3X_ASD_INTF_CTRLC, delay);
+
+	return 0;
+}
+
+static int aic3x_set_dai_tdm_slot(struct snd_soc_dai *codec_dai,
+				  unsigned int tx_mask, unsigned int rx_mask,
+				  int slots, int slot_width)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
+	unsigned int lsb;
+
+	if (tx_mask != rx_mask) {
+		dev_err(codec->dev, "tx and rx masks must be symmetric\n");
+		return -EINVAL;
+	}
+
+	if (unlikely(!tx_mask)) {
+		dev_err(codec->dev, "tx and rx masks need to be non 0\n");
+		return -EINVAL;
+	}
+
+	/* TDM based on DSP mode requires slots to be adjacent */
+	lsb = __ffs(tx_mask);
+	if ((lsb + 1) != __fls(tx_mask)) {
+		dev_err(codec->dev, "Invalid mask, slots must be adjacent\n");
+		return -EINVAL;
+	}
+
+	aic3x->tdm_delay = lsb * slot_width;
+
+	/* DOUT in high-impedance on inactive bit clocks */
+	snd_soc_update_bits(codec, AIC3X_ASD_INTF_CTRLA,
+			    DOUT_TRISTATE, DOUT_TRISTATE);
 
 	return 0;
 }
@@ -1225,9 +1279,11 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 
 static const struct snd_soc_dai_ops aic3x_dai_ops = {
 	.hw_params	= aic3x_hw_params,
+	.prepare	= aic3x_prepare,
 	.digital_mute	= aic3x_mute,
 	.set_sysclk	= aic3x_set_dai_sysclk,
 	.set_fmt	= aic3x_set_dai_fmt,
+	.set_tdm_slot	= aic3x_set_dai_tdm_slot,
 };
 
 static struct snd_soc_dai_driver aic3x_dai = {
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index e521ac3ddde8..89fa692df206 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -169,6 +169,7 @@
 /* Audio serial data interface control register A bits */
 #define BIT_CLK_MASTER          0x80
 #define WORD_CLK_MASTER         0x40
+#define DOUT_TRISTATE		0x20
 
 /* Codec Datapath setup register 7 */
 #define FSREF_44100		(1 << 7)
-- 
2.1.3

Re: [PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 547 bytes --]

On Mon, Nov 10, 2014 at 12:27:32PM +0200, Peter Ujfalusi wrote:

> +static const struct soc_enum aic3x_pop_reduction_enum[] = {
> +	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 4, 12, aic3x_poweron_time),
> +	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 2, 4, aic3x_rampup_step),
> +};

> +	/* Pop reduction */
> +	SOC_ENUM("Output Driver Power-On time", aic3x_pop_reduction_enum[0]),
> +	SOC_ENUM("Output Driver Ramp-up step", aic3x_pop_reduction_enum[1]),

Don't add arrays of enums with magic number indexes like this, it's hard
to read and hence error prone.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: tlv320aic3x: Add TDM support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 396 bytes --]

On Mon, Nov 10, 2014 at 12:27:33PM +0200, Peter Ujfalusi wrote:
> TDM support is achieved using DSP transfer mode and setting a
> programmable offset which specifies where data begins with
> respect to the frame sync.

Applied, thanks.

> +	if (unlikely(!tx_mask)) {
> +		dev_err(codec->dev, "tx and rx masks need to be non 0\n");

There's no real point in using unlikely() outside of hot paths.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[Peter Ujfalusi]

On 11/10/2014 12:51 PM, Mark Brown wrote:
> On Mon, Nov 10, 2014 at 12:27:32PM +0200, Peter Ujfalusi wrote:
> 
>> +static const struct soc_enum aic3x_pop_reduction_enum[] = {
>> +	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 4, 12, aic3x_poweron_time),
>> +	SOC_ENUM_SINGLE(HPOUT_POP_REDUCTION, 2, 4, aic3x_rampup_step),
>> +};
> 
>> +	/* Pop reduction */
>> +	SOC_ENUM("Output Driver Power-On time", aic3x_pop_reduction_enum[0]),
>> +	SOC_ENUM("Output Driver Ramp-up step", aic3x_pop_reduction_enum[1]),
> 
> Don't add arrays of enums with magic number indexes like this, it's hard
> to read and hence error prone.

I agree on this. I have not changed this since this driver is using enums like
this and I thought it is better to follow the style.

But if I add these to the aic3x_enum[] array with a define for the ID I think
it is going to be a bit better?

-- 
Péter

Re: [PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 790 bytes --]

On Mon, Nov 10, 2014 at 03:23:04PM +0200, Peter Ujfalusi wrote:
> On 11/10/2014 12:51 PM, Mark Brown wrote:

> >> +	/* Pop reduction */
> >> +	SOC_ENUM("Output Driver Power-On time", aic3x_pop_reduction_enum[0]),
> >> +	SOC_ENUM("Output Driver Ramp-up step", aic3x_pop_reduction_enum[1]),

> > Don't add arrays of enums with magic number indexes like this, it's hard
> > to read and hence error prone.

> I agree on this. I have not changed this since this driver is using enums like
> this and I thought it is better to follow the style.

> But if I add these to the aic3x_enum[] array with a define for the ID I think
> it is going to be a bit better?

A bit, though I think I'd still prefer to use individual variables, it's
less to fix when someone does get round to fixing the driver.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ASoC: tlv320aic3x: Add output driver pop reduction controls

[Peter Ujfalusi]

On 11/10/2014 03:27 PM, Mark Brown wrote:
> On Mon, Nov 10, 2014 at 03:23:04PM +0200, Peter Ujfalusi wrote:
>> On 11/10/2014 12:51 PM, Mark Brown wrote:
> 
>>>> +	/* Pop reduction */
>>>> +	SOC_ENUM("Output Driver Power-On time", aic3x_pop_reduction_enum[0]),
>>>> +	SOC_ENUM("Output Driver Ramp-up step", aic3x_pop_reduction_enum[1]),
> 
>>> Don't add arrays of enums with magic number indexes like this, it's hard
>>> to read and hence error prone.
> 
>> I agree on this. I have not changed this since this driver is using enums like
>> this and I thought it is better to follow the style.
> 
>> But if I add these to the aic3x_enum[] array with a define for the ID I think
>> it is going to be a bit better?
> 
> A bit, though I think I'd still prefer to use individual variables, it's
> less to fix when someone does get round to fixing the driver.

Sure, let's do that then.


-- 
Péter