On Fri, Aug 13, 2010 at 10:23 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Aug 13, 2010 at 09:55:44PM +0800, Haojian Zhuang wrote:
>
>> +static int snd_soc_put_equalizer(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> + struct pm860x_priv *pm860x = snd_soc_codec_get_drvdata(codec);
>> +
>> + if (ucontrol->value.integer.value[0] >= FILTER_MAX)
>> + return -EINVAL;
>> + pm860x->filter = ucontrol->value.integer.value[0];
>> + switch (pm860x->filter) {
>> + case FILTER_BYPASS:
>> + snd_soc_update_bits(codec, PM860X_I2S_IFACE_4, I2S_EQU_BYP,
>> + I2S_EQU_BYP);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N0_1, 0);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N0_2, 0);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N1_1, 0);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N1_2, 0);
>> + snd_soc_write(codec, PM860X_EQUALIZER_D1_1, 0);
>> + snd_soc_write(codec, PM860X_EQUALIZER_D1_2, 0);
>> + snd_soc_update_bits(codec, PM860X_EAR_CTRL_2, RSYNC_CHANGE,
>> + RSYNC_CHANGE);
>> + return 0;
>
> Rather than hard coding every possible set of EQ configurations into the
> driver it would be much better to allow the user to supply these via
> platform data. That way if users want to supply their own EQ
> coefficients they will be able to. Several Wolfson CODEC drivers have
> examples of doing this.
>
> It's fine to provide a default set of configurations in case the user
> doesn't specify anything in platform data.
>
OK.
>> + case FILTER_HIGH_PASS_3:
>> + snd_soc_write(codec, PM860X_EQUALIZER_N0_1, 0x55);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N0_2, 0x39);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N1_1, 0x5a);
>> + snd_soc_write(codec, PM860X_EQUALIZER_N1_2, 0xf3);
>> + snd_soc_write(codec, PM860X_EQUALIZER_D1_1, 0x7e);
>> + snd_soc_write(codec, PM860X_EQUALIZER_D1_2, 0x53);
>> + break;
>> + }
>
> You're also missing a default.
>
Bypass is default. After audio part is reset, EQ is in bypass state.
>> +/* DAPM Widget Events */
>> +static int pm860x_rsync_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_codec *codec = w->codec;
>> + struct pm860x_priv *pm860x = snd_soc_codec_get_drvdata(codec);
>> +
>> + /* unmute DAC */
>> + if (pm860x->automute) {
>> + snd_soc_update_bits(codec, PM860X_DAC_OFFSET, DAC_MUTE, 0);
>> + pm860x->automute = 0;
>> + }
>> + snd_soc_update_bits(codec, PM860X_EAR_CTRL_2,
>> + RSYNC_CHANGE, RSYNC_CHANGE);
>> + return 0;
>> +}
>
> What's this doing? There's also some similar code in the DAC widget
> event - I think some comments explaining what's being done here are
> required at the very least.
>
A lot of registers belong to RSYNC domain. It means that those
changing won't be valid until RSYNC bit is set. So I'll set RSYNC
again in a lot of areas.
>> +static int pm860x_adc_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_codec *codec = w->codec;
>> + unsigned int en1 = 0, en2 = 0;
>> +
>> + if (!strcmp(w->name, "Left ADC")) {
>> + en1 = ADC_MOD_LEFT;
>> + en2 = ADC_LEFT;
>> + }
>> + if (!strcmp(w->name, "Right ADC")) {
>> + en1 = ADC_MOD_RIGHT;
>> + en2 = ADC_RIGHT;
>> + }
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + snd_soc_update_bits(codec, PM860X_ADC_EN_1, en1, en1);
>> + snd_soc_update_bits(codec, PM860X_ADC_EN_2, en2, en2);
>
> Can you do this more simply by using a supply widget for the en1
> register bits?
>
When I enable this, I need to touch two different registers at the
same time. So I have to implement the adc_event().
>> +static int pm860x_spk_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_codec *codec = w->codec;
>> +
>> + dev_dbg(codec->dev, "event:%x\n", event);
>> + return 0;
>> +}
>
> Remove this, it's purely for debug.
>
OK.
>> +static const struct soc_enum pm860x_enum[] = {
>> + SOC_ENUM_SINGLE(PM860X_HS1_CTRL, 5, 4, pm860x_opamp_texts),
>
> Don't put your enums in an array, it is error prone and hard to read.
> Just define variables for each enum.
>
OK.
>> +/* Headset 1 Mux / Mux15 */
>> +static const char *in_text[] = {
>> + "DIGITAL", "ANALOG",
>
> Why ALL CAPS?
>
Sure. I'll change to lowcases.
>> + data = snd_soc_read(codec, PM860X_PCM_IFACE_2) & ~mask;
>> + data |= inf;
>> + snd_soc_write(codec, PM860X_PCM_IFACE_2, data);
>
> snd_soc_update_bits().
>
>> +static int pm860x_pcm_hw_free(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> +
>> + /* disable PCM interface */
>> + snd_soc_update_bits(codec, PM860X_ADC_EN_2, 1, 0);
>> + snd_soc_update_bits(codec, PM860X_EAR_CTRL_2,
>> + RSYNC_CHANGE, RSYNC_CHANGE);
>> + return 0;
>> +}
>
> This looks like it should be done by DAPM.
>
But I don't want to export this to amixer.
>> +static int pm860x_i2s_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> + unsigned char inf;
>> + int data;
>> +
>> + /*
>> + * Enable LDO15 for VDD_CODEC, audio charger pump for VDDSTP/VDDSTN
>> + * and reset audio module.
>> + */
>> + data = LDO15_EN | CPUMP_EN | AUDIO_EN;
>> + snd_soc_update_bits(codec, PM860X_AUDIO_SUPPLIES_2, data, data);
>
> These look like they should all be handled via DAPM, probably supply
> widgets.
>
>> +static int pm860x_i2s_hw_free(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>
> This also looks like DAPM stuff.
>
>> + /* Enable Mic1 Bias & MICDET, HSDET */
>> + pm860x_set_bits(codec->control_data, REG_ADC_ANA_1,
>> + MIC1BIAS_MASK, MIC1BIAS_MASK);
>> + pm860x_set_bits(codec->control_data, REG_MIC_DET,
>> + MICDET_MASK, MICDET_MASK);
>> + pm860x_set_bits(codec->control_data, REG_HS_DET,
>> + EN_HS_DET, EN_HS_DET);
>
> The microphone bias should be handled via DAPM.
>
This one is different. Without this, mic/headset detection can't be
finished. This bit have to be set in initialization.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
