On Fri, Aug 13, 2010 at 10:23 PM, Mark Brown broonie@opensource.wolfsonmicro.com 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.