diff mbox

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

Message ID 1415615253-28919-1-git-send-email-peter.ujfalusi@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Ujfalusi Nov. 10, 2014, 10:27 a.m. UTC 
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(+)

Comments

Mark Brown Nov. 10, 2014, 10:51 a.m. UTC | #1 
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.
Peter Ujfalusi Nov. 10, 2014, 1:23 p.m. UTC | #2 
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?
Mark Brown Nov. 10, 2014, 1:27 p.m. UTC | #3 
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.
Peter Ujfalusi Nov. 11, 2014, 8:01 a.m. UTC | #4 
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.
diff mbox

Patch

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[] = {