[alsa-devel] ASoC: TLV320AIC3x: Adding additional functionality for 3106 with [Patch] for discuss
Peter Ujfalusi
peter.ujfalusi at ti.com
Thu Mar 31 14:58:33 CEST 2016
Timur,
On 03/23/16 17:21, Timur Karaldin wrote:
> Hi Peter!
> Here is my patch v2. It works only for aic3106, but I don't have any others.
> Also patch contains some debug output, I would clean it. I like behaviour of
> this improvements and I didn't know any issue right now for these add-ons
> except debug output and that it's correct only for aic3106 (didn't check for
> others even on datasheet level). Thank you for all your help!
sorry for the delay, I was dragged away for a bit.
Can you send these as separated patches per new functionality? Please use
git format-patch to create the patches and try to clean them up so it is going
to be easier to understand them.
Can you check your mail client to wrap lines correctly?
You have very long lines, for example:
http://www.spinics.net/lists/alsa-devel/msg47793.html
http://www.spinics.net/lists/alsa-devel/msg48199.html
While they should be something like:
http://www.spinics.net/lists/alsa-devel/msg47797.html
Couple of comments, but it would be more beneficial to see a series...
> -----------------------------------------------------------------------
> diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
> index d7349bc..2c06e21 100644
> --- a/sound/soc/codecs/tlv320aic3x.c
> +++ b/sound/soc/codecs/tlv320aic3x.c
> @@ -89,6 +89,7 @@ struct aic3x_priv {
>
> /* Selects the micbias voltage */
> enum aic3x_micbias_voltage micbias_vg;
> + unsigned char cached_gain[LINE1L_2_RADC_CTRL-MIC3LR_2_LADC_CTRL+1];
spaces around '+' '-' '='
See: Documentation/CodingStyle
Also, make sure that tabs are not replaced by spaces and check the patch for
long lines also.
Run scripts/checkpatch.pl --strict on the patches.
> };
>
> static const struct reg_default aic3x_reg[] = {
> @@ -134,16 +135,175 @@ static const struct regmap_config aic3x_regmap = {
>
> #define SOC_DAPM_SINGLE_AIC3X(xname, reg, shift, mask, invert) \
> SOC_SINGLE_EXT(xname, reg, shift, mask, invert, \
> - snd_soc_dapm_get_volsw, snd_soc_dapm_put_volsw_aic3x)
> + snd_soc_dapm_get_volsw_aic3x, snd_soc_dapm_put_volsw_aic3x)
> +
> +#define SOC_DOUBLE_R_AIC3X_TLV(xname, reg, rreg, shift, mask, invert) \
> + SOC_DOUBLE_R_EXT_TLV(xname, reg, rreg, shift, mask, invert, \
> + snd_soc_get_gain_aic3x, snd_soc_put_gain_aic3x, gain_stage_tlv)
> +
> +#define SOC_SINGLE_AIC3X_TLV(xname, reg, shift, mask, invert) \
> + SOC_SINGLE_EXT_TLV(xname, reg, shift, mask, invert, \
> + snd_soc_get_gain_aic3x, snd_soc_put_gain_aic3x, gain_stage_tlv)
> +
> +#define SOC_SINGLE_EXT_VOL(xname, reg, shift, mask, invert) \
> + SOC_SINGLE_EXT(xname, reg, shift, mask, invert, \
> + snd_soc_get_volsw_uncached, snd_soc_put_volsw)
> +
> +/*
> + * Headset detect flag, button press detect flag and headset type flag is
> read only register,
> + * but we could not declare these regs as volatile because other bits are RW,
> so we use
> + * unchached get hanler for these regs.
> +*/
> +static int snd_soc_get_volsw_uncached( struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value * ucontrol)
> +{
> + int ret;
> + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
> +
> + regcache_cache_bypass(aic3x->regmap, true);
> + ret = snd_soc_get_volsw(kcontrol, ucontrol);
> + regcache_cache_bypass(aic3x->regmap, false);
> + return ret;
> +}
> +
> +/*
> + * All input lines have additional gain controls which can be controled by
> volume control.
> + * 0x0 - means 0dB and 0x8 - means -12 dB. It's a litlle bit tricky because
> value 0xF means mute.
> + * value 0x9-0xE reserverd, so in case of switch muted we should store value
> in cache and should
> + * not set up it to register
> +*/
> +static int snd_soc_put_gain_aic3x(struct snd_kcontrol* kcontrol,
> + struct snd_ctl_elem_value* ucontrol)
Please keep the parameter indentation uniform.
> +{
> + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
> + struct soc_mixer_control *mc = (struct soc_mixer_control
> *)kcontrol->private_value;
> + unsigned int reg;
> + unsigned int shift = mc->shift;
> + int max = mc->max;
> + unsigned int mask = (1<<fls(max))-1;
> + unsigned int invert = mc->invert;
> +
> + u8 regdata, newval, regval;
> + u8 loop = 0;
> +
> + //First of all we need to cache value. Then we need to test each value
> on mute in register
> + //If not muted, we should setup new value
> + reg = mc->reg;
> + newval = ( ucontrol->value.integer.value[0] & mask );
> + printk(KERN_INFO "set gain control reg:%d val:%0X mask:%0X shift:%0X
> invert %d\n", reg,newval, mask, shift,invert);
> + do
> + {
It would be better to do this w/o do {}while loop, see snd_soc_put_volsw() for
example.
> + if(invert)
> + {
> + newval = ( 8 - newval ) & mask;
> + printk(KERN_INFO "inverted val %0X\n", newval);
> + }
> + //read, test and update if first reg if needs
> + regdata = snd_soc_read(codec, reg);
> + regval = ( regdata >> shift) & mask;
> +
> + if( reg >= MIC3LR_2_LADC_CTRL && reg <= LINE1L_2_RADC_CTRL )
> + {
> + printk(KERN_INFO "cached_gain before
> %0X\n",aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]);
> + aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]&=~(mask<<shift);
> + aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]|=(newval<<shift);
> + printk(KERN_INFO "cached_gain after
> %0X\n",aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]);
> + }
> +
> + if ( regval!=0xf && regval!=newval )
> + snd_soc_update_bits( codec, reg, mask<<shift,
> newval<<shift );
> +
> + if(snd_soc_volsw_is_stereo(mc))
> + {
> + loop=!loop; //switch loop, so for first time loop
> will be true after and for second time it will be false, so we will finish loop
> + if(loop) //if first time
> + {
> + reg=mc->rreg;
> + newval = (ucontrol->value.integer.value[1] &
> mask);
> + printk(KERN_INFO "control is stereo second
> reg:%d val:%0X\n", reg,newval);
> + continue;
> + }
> + }
> +
> + }while(loop);
> + return 0;
> +}
> +
> +/*
> + * All input lines have additional gain controls which can be controled by
> volume control.
> + * 0x0 - means 0dB and 0x8 - means -12 dB. It's a litlle bit tricky because
> value 0xF means mute.
> + * value 0x9-0xE reserverd, so in case of switch muted we should read value
> from cache in aic3x_priv
> + * and should not read it from regcache or register
> +*/
> +static int snd_soc_get_gain_aic3x( struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
> + struct soc_mixer_control *mc = (struct soc_mixer_control
> *)kcontrol->private_value;
> + unsigned int reg;
> + unsigned int shift = mc->shift;
> + int max = mc->max;
> + unsigned int mask = (1<<fls(max))-1;
> + unsigned int invert = mc->invert;
> +
> + u8 regdata, regval;
> + u8 loop = 0;
> +
> + //First of all we need to cache value. Then we need to test each value
> on mute in register
> + //If not muted, we should set up new value
> + reg = mc->reg;
> +
> + printk(KERN_INFO "get gain control reg:%d mask:%0X shift:%0X invert
> %d\n", reg, mask, shift,invert);
> + do
> + {
same here, please remove the loop.
> + regdata = snd_soc_read(codec, reg);
> + regval = ( regdata >> shift) & mask;
> + printk(KERN_INFO "regval:%0X\n", regval);
> +
> + //check if register is muted then return cached value
> + if( regval == 0xf )
> + {
> + if( reg >=MIC3LR_2_LADC_CTRL && reg <=
> LINE1L_2_RADC_CTRL )
> + regval =
> (aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]>>shift) & mask;
> + printk(KERN_INFO "regval from cache:%0X\n", regval);
> +
> + }
> + if( invert )
> + {
> + regval = (8 - regval) & mask;
> + printk(KERN_INFO "regval after invert:%0X\n", regval);
> + }
> + ucontrol->value.integer.value[loop] = regval;
> +
> + if(snd_soc_volsw_is_stereo(mc))
> + {
> + loop=loop?0:1; //switch loop, so for first time loop
> will be 1 after and for second time it will be 0, so we will exit from while-loop
> + if(loop) //if first time
> + {
> + reg=mc->rreg;
> + printk(KERN_INFO "control is stereo second
> reg:%d\n", reg);
> + continue;
> + }
> + }
> +
> + }while(loop);
> + return 0;
> +}
>
> /*
> * All input lines are connected when !0xf and disconnected with 0xf bit field,
> - * so we have to use specific dapm_put call for input mixer
> + * so we have to use specific dapm_put call for input mixer. In case of unmute
> + * we should set up register value from cached values from aic3x_priv struct.
> */
> static int snd_soc_dapm_put_volsw_aic3x(struct snd_kcontrol *kcontrol,
> struct snd_ctl_elem_value *ucontrol)
> {
> struct snd_soc_codec *codec = snd_soc_dapm_kcontrol_codec(kcontrol);
> + struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
> +
> struct soc_mixer_control *mc =
> (struct soc_mixer_control *)kcontrol->private_value;
> unsigned int reg = mc->reg;
> @@ -151,20 +311,32 @@ static int snd_soc_dapm_put_volsw_aic3x(struct
> snd_kcontrol *kcontrol,
> int max = mc->max;
> unsigned int mask = (1 << fls(max)) - 1;
> unsigned int invert = mc->invert;
> - unsigned short val;
> + unsigned int val;
> struct snd_soc_dapm_update update;
> int connect, change;
>
> val = (ucontrol->value.integer.value[0] & mask);
> + printk(KERN_INFO "dapm put volsw val:%X reg:%X mask:%X shift:%X
> invert:%X\n", val, reg, mask, shift, invert);
>
> mask = 0xf;
> if (val)
> + {
> val = mask;
> -
> + printk(KERN_INFO "new val:%X\n", val);
> + }
> connect = !!val;
>
> if (invert)
> + {
> val = mask - val;
> + printk(KERN_INFO "val after invert:%X\n", val);
> + }
> + if (!val)
> + {
> + if( reg>=MIC3LR_2_LADC_CTRL && reg<=LINE1L_2_RADC_CTRL )
> + val =
> (aic3x->cached_gain[reg-MIC3LR_2_LADC_CTRL]>>shift) & mask;
> + printk(KERN_INFO "cached val:%X\n", val);
> + }
this generally looks good, but please clean up the patch.
>
> mask <<= shift;
> val <<= shift;
> @@ -183,6 +355,36 @@ static int snd_soc_dapm_put_volsw_aic3x(struct
> snd_kcontrol *kcontrol,
> return change;
> }
>
> +/*
> + * Based on standart handler snd_soc_dapm_get_volsw, but changing mask to 0xF
> + *
> + */
> +static int snd_soc_dapm_get_volsw_aic3x(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct soc_mixer_control *mc =
> + (struct soc_mixer_control *)kcontrol->private_value;
> + unsigned int reg = mc->reg;
> + unsigned int shift = mc->shift;
> + int max = mc->max;
> + unsigned int mask = (1 << fls(max)) - 1;
> + int ret;
> +
> + //register contains 4 bits, so we change max temoprally to read
> register by original handler, then return it back
> + mc->max = 15;
> + ret = snd_soc_dapm_get_volsw( kcontrol, ucontrol);
> + printk(KERN_INFO "soc_dapm_get_volsw_aic3x:%ld reg:%X shift:%X max:%X
> newmax:%X\n", ucontrol->value.integer.value[0], reg, shift, max, mc->max );
> + mc->max = max;
> +
> + //all gains except mute (0xf) after invertion is not equl to 0, so we
> need to set 1(on)
> + //for all values except 0. 0 is mute, so we do not need to change it
> + if(ucontrol->value.integer.value[0]!=0)
> + ucontrol->value.integer.value[0]=1;
> + printk(KERN_INFO "new value:%ld\n", ucontrol->value.integer.value[0]);
> +
> + return ret;
> +}
> +
> /*
> * mic bias power on/off share the same register bits with
> * output voltage of mic bias. when power on mic bias, we
> @@ -270,6 +472,14 @@ static const struct soc_enum aic3x_agc_decay_enum[] = {
> SOC_ENUM_SINGLE(RAGC_CTRL_A, 0, 4, aic3x_agc_decay),
> };
>
> +static const char *aic3x_headset_debounce[] = { "16ms", "32ms", "64ms",
> "128ms", "256ms", "512ms" };
> +static const struct soc_enum aic3x_headset_debounce_enum =
> +SOC_ENUM_SINGLE( AIC3X_HEADSET_DETECT_CTRL_A, 2, 6, aic3x_headset_debounce);
> +
> +static const char *aic3x_button_debounce[] = { "0ms", "8ms", "16ms", "32ms" };
> +static const struct soc_enum aic3x_button_debounce_enum =
> +SOC_ENUM_SINGLE( AIC3X_HEADSET_DETECT_CTRL_A, 0, 4, aic3x_button_debounce);
> +
> /*
> * DAC digital volumes. From -63.5 to 0 dB in 0.5 dB steps
> */
> @@ -287,6 +497,9 @@ static DECLARE_TLV_DB_SCALE(adc_tlv, 0, 50, 0);
> */
> static DECLARE_TLV_DB_SCALE(output_stage_tlv, -5900, 50, 1);
>
> +static DECLARE_TLV_DB_SCALE(gain_stage_tlv, -1200, 150, 0);
> +
> +
> static const struct snd_kcontrol_new aic3x_snd_controls[] = {
> /* Output */
> SOC_DOUBLE_R_TLV("PCM Playback Volume",
> @@ -399,6 +612,24 @@ 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]),
> + /* Additional controls */
> + SOC_DOUBLE_R_AIC3X_TLV("Mic3L Volume", MIC3LR_2_LADC_CTRL,
> MIC3LR_2_RADC_CTRL, 4, 8, 1),
> + SOC_DOUBLE_R_AIC3X_TLV("Mic3R Volume", MIC3LR_2_LADC_CTRL,
> MIC3LR_2_RADC_CTRL, 0, 8, 1),
> + SOC_DOUBLE_R_AIC3X_TLV("Line1L Volume", LINE1L_2_LADC_CTRL,
> LINE1L_2_RADC_CTRL, 3, 8, 1),
> + SOC_DOUBLE_R_AIC3X_TLV("Line1R Volume", LINE1R_2_LADC_CTRL,
> LINE1R_2_RADC_CTRL, 3, 8, 1),
> + SOC_SINGLE_AIC3X_TLV("Line2L Volume", LINE2L_2_LADC_CTRL, 3, 8, 1),
> + SOC_SINGLE_AIC3X_TLV("Line2R Volume", LINE2R_2_RADC_CTRL, 3, 8, 1),
> + SOC_ENUM("Headset Jack Debounce", aic3x_headset_debounce_enum ),
> + SOC_ENUM("Button Press Debounce", aic3x_button_debounce_enum ),
> + SOC_SINGLE("Headset Detect Enable", AIC3X_HEADSET_DETECT_CTRL_A, 7, 1,
> 0),
> + SOC_SINGLE_EXT_VOL("Headset Detect Type", AIC3X_HEADSET_DETECT_CTRL_A,
> + AIC3X_HEADSET_DETECT_A_SHIFT, AIC3X_HEADSET_DETECT_A_MASK, 0),
> + SOC_SINGLE_EXT_VOL("Button Detect Flag", AIC3X_HEADSET_DETECT_CTRL_B,
> + AIC3X_BUTTON_DETECT_SHIFT, AIC3X_BUTTON_DETECT_MASK, 0),
> + SOC_SINGLE_EXT_VOL("Headset Detect Flag", AIC3X_HEADSET_DETECT_CTRL_B,
> + AIC3X_HEADSET_DETECT_B_SHIFT, AIC3X_HEADSET_DETECT_B_MASK, 0),
> + SOC_SINGLE("High power output Ac-coupled",
> AIC3X_HEADSET_DETECT_CTRL_B, 7, 1, 0),
> + SOC_SINGLE("Stereo pseudodifferential output",
> AIC3X_HEADSET_DETECT_CTRL_B, 3, 1, 0),
> };
>
> static const struct snd_kcontrol_new aic3x_mono_controls[] = {
> --------------------------------------
> diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
> index e521ac3..bcac87f 100644
> --- a/sound/soc/codecs/tlv320aic3x.h
> +++ b/sound/soc/codecs/tlv320aic3x.h
> @@ -271,9 +271,20 @@ enum {
> AIC3X_BUTTON_DEBOUNCE_32MS = 3
> };
>
> +typedef struct {
> + struct platform_device* pdev;
> + struct proc_dir_entry *proc_value;
> + struct snd_soc_codec *codec;
> +} aic3106_detect_t;
> +
> #define AIC3X_HEADSET_DETECT_ENABLED 0x80
> -#define AIC3X_HEADSET_DETECT_SHIFT 5
> -#define AIC3X_HEADSET_DETECT_MASK 3
> +#define AIC3X_HEADSET_DETECT_A_SHIFT 5
> +#define AIC3X_HEADSET_DETECT_A_MASK 3
> +
> +#define AIC3X_BUTTON_DETECT_SHIFT 5
> +#define AIC3X_BUTTON_DETECT_MASK 1
> +#define AIC3X_HEADSET_DETECT_B_SHIFT 4
> +#define AIC3X_HEADSET_DETECT_B_MASK 1
> #define AIC3X_HEADSET_DEBOUNCE_SHIFT 2
> #define AIC3X_HEADSET_DEBOUNCE_MASK 7
> #define AIC3X_BUTTON_DEBOUNCE_SHIFT 0
> --------------------------------------
It would be great if you could clean this up, have separate patches in order
to be able to review the changes and also to be able to test it.
It looks promising ;)
--
Péter
More information about the Alsa-devel
mailing list