[alsa-devel] [PATCH 0/3] ASoC: codecs: max98088: bug fixes and changes
From: Jin Park jinyoungp@nvidia.com
Recently, I working on max98088 and changed something in max98088 asoc driver. The MAX98088 driver had some bug and needed modifies. The changes was confirmed by Peter Hsiang who author of max98088 driver and maxim engineer. Please review my changes.
Thanks, Jin Park.
Jin Park (3): ASoC: codecs: max98088: Fixed invalid register definitions in mixer controls ASoC: codecs: max98088: Moved the EX Limiter Mode from dapm widget to control ASoC: codecs: max98088: Added digital mute function in DAI1 and DAI2
sound/soc/codecs/max98088.c | 62 +++++++++++++++++++++++++++++++------------ sound/soc/codecs/max98088.h | 13 +++++++++ 2 files changed, 58 insertions(+), 17 deletions(-)
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
From: Jin Park jinyoungp@nvidia.com
Fixed invalid register definitions in mixer controls such as left speaker mixer, left hp mixer and left rec mixer.
Signed-off-by: Jin Park jinyoungp@nvidia.com --- sound/soc/codecs/max98088.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index 89498f9..3bdaa04 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -808,10 +808,10 @@ static const struct snd_kcontrol_new max98088_snd_controls[] = {
/* Left speaker mixer switch */ static const struct snd_kcontrol_new max98088_left_speaker_mixer_controls[] = { - SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 0, 1, 0), - SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_2B_MIX_SPK_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_2B_MIX_SPK_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 7, 1, 0), + SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_2B_MIX_SPK_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_2B_MIX_SPK_LEFT, 7, 1, 0), SOC_DAPM_SINGLE("MIC1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 5, 1, 0), SOC_DAPM_SINGLE("MIC2 Switch", M98088_REG_2B_MIX_SPK_LEFT, 6, 1, 0), SOC_DAPM_SINGLE("INA1 Switch", M98088_REG_2B_MIX_SPK_LEFT, 1, 1, 0), @@ -836,10 +836,10 @@ static const struct snd_kcontrol_new max98088_right_speaker_mixer_controls[] = {
/* Left headphone mixer switch */ static const struct snd_kcontrol_new max98088_left_hp_mixer_controls[] = { - SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_25_MIX_HP_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_25_MIX_HP_LEFT, 0, 1, 0), - SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_25_MIX_HP_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_25_MIX_HP_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_25_MIX_HP_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_25_MIX_HP_LEFT, 7, 1, 0), + SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_25_MIX_HP_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_25_MIX_HP_LEFT, 7, 1, 0), SOC_DAPM_SINGLE("MIC1 Switch", M98088_REG_25_MIX_HP_LEFT, 5, 1, 0), SOC_DAPM_SINGLE("MIC2 Switch", M98088_REG_25_MIX_HP_LEFT, 6, 1, 0), SOC_DAPM_SINGLE("INA1 Switch", M98088_REG_25_MIX_HP_LEFT, 1, 1, 0), @@ -864,10 +864,10 @@ static const struct snd_kcontrol_new max98088_right_hp_mixer_controls[] = {
/* Left earpiece/receiver mixer switch */ static const struct snd_kcontrol_new max98088_left_rec_mixer_controls[] = { - SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_28_MIX_REC_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_28_MIX_REC_LEFT, 0, 1, 0), - SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_28_MIX_REC_LEFT, 7, 1, 0), - SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_28_MIX_REC_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Left DAC1 Switch", M98088_REG_28_MIX_REC_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC1 Switch", M98088_REG_28_MIX_REC_LEFT, 7, 1, 0), + SOC_DAPM_SINGLE("Left DAC2 Switch", M98088_REG_28_MIX_REC_LEFT, 0, 1, 0), + SOC_DAPM_SINGLE("Right DAC2 Switch", M98088_REG_28_MIX_REC_LEFT, 7, 1, 0), SOC_DAPM_SINGLE("MIC1 Switch", M98088_REG_28_MIX_REC_LEFT, 5, 1, 0), SOC_DAPM_SINGLE("MIC2 Switch", M98088_REG_28_MIX_REC_LEFT, 6, 1, 0), SOC_DAPM_SINGLE("INA1 Switch", M98088_REG_28_MIX_REC_LEFT, 1, 1, 0),
From: Jin Park jinyoungp@nvidia.com
Moved the EX Limiter Mode from dapm widget to control, because it was not required DAPM route.
Signed-off-by: Jin Park jinyoungp@nvidia.com --- sound/soc/codecs/max98088.c | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index 3bdaa04..c520093 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -656,8 +656,6 @@ static const struct soc_enum max98088_exmode_enum = ARRAY_SIZE(max98088_exmode_texts), max98088_exmode_texts, max98088_exmode_values); -static const struct snd_kcontrol_new max98088_exmode_controls = - SOC_DAPM_VALUE_ENUM("Route", max98088_exmode_enum);
static const char *max98088_ex_thresh[] = { /* volts PP */ "0.6", "1.2", "1.8", "2.4", "3.0", "3.6", "4.2", "4.8"}; @@ -783,6 +781,7 @@ static const struct snd_kcontrol_new max98088_snd_controls[] = { SOC_SINGLE("EQ1 Switch", M98088_REG_49_CFG_LEVEL, 0, 1, 0), SOC_SINGLE("EQ2 Switch", M98088_REG_49_CFG_LEVEL, 1, 1, 0),
+ SOC_ENUM("EX Limiter Mode", max98088_exmode_enum), SOC_ENUM("EX Limiter Threshold", max98088_ex_thresh_enum),
SOC_ENUM("DAI1 Filter Mode", max98088_filter_mode_enum), @@ -1094,9 +1093,6 @@ static const struct snd_soc_dapm_widget max98088_dapm_widgets[] = {
SND_SOC_DAPM_MICBIAS("MICBIAS", M98088_REG_4C_PWR_EN_IN, 3, 0),
- SND_SOC_DAPM_MUX("EX Limiter Mode", SND_SOC_NOPM, 0, 0, - &max98088_exmode_controls), - SND_SOC_DAPM_OUTPUT("HPL"), SND_SOC_DAPM_OUTPUT("HPR"), SND_SOC_DAPM_OUTPUT("SPKL"),
From: Jin Park jinyoungp@nvidia.com
Added digital mute function in DAI1 and DAI2.
Signed-off-by: Jin Park jinyoungp@nvidia.com --- sound/soc/codecs/max98088.c | 32 ++++++++++++++++++++++++++++++++ sound/soc/codecs/max98088.h | 13 +++++++++++++ 2 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index c520093..b5ccf37 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1582,6 +1582,36 @@ static int max98088_dai2_set_fmt(struct snd_soc_dai *codec_dai, return 0; }
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{ + struct snd_soc_codec *codec = codec_dai->codec; + int reg; + + if (mute) + reg = M98088_DAI_MUTE; + else + reg = 0; + + snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY, + M98088_DAI_MUTE_MASK, reg); + return 0; +} + +static int max98088_dai2_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{ + struct snd_soc_codec *codec = codec_dai->codec; + int reg; + + if (mute) + reg = M98088_DAI_MUTE; + else + reg = 0; + + snd_soc_update_bits(codec, M98088_REG_31_LVL_DAI2_PLAY, + M98088_DAI_MUTE_MASK, reg); + return 0; +} + static void max98088_sync_cache(struct snd_soc_codec *codec) { u16 *reg_cache = codec->reg_cache; @@ -1643,12 +1673,14 @@ static struct snd_soc_dai_ops max98088_dai1_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai1_set_fmt, .hw_params = max98088_dai1_hw_params, + .digital_mute = max98088_dai1_digital_mute, };
static struct snd_soc_dai_ops max98088_dai2_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai2_set_fmt, .hw_params = max98088_dai2_hw_params, + .digital_mute = max98088_dai2_digital_mute, };
static struct snd_soc_dai_driver max98088_dai[] = { diff --git a/sound/soc/codecs/max98088.h b/sound/soc/codecs/max98088.h index 56554c7..be89a4f 100644 --- a/sound/soc/codecs/max98088.h +++ b/sound/soc/codecs/max98088.h @@ -133,6 +133,19 @@ #define M98088_REC_LINEMODE (1<<7) #define M98088_REC_LINEMODE_MASK (1<<7)
+/* M98088_REG_2D_MIX_SPK_CNTL */ + #define M98088_MIX_SPKR_GAIN_MASK (3<<2) + #define M98088_MIX_SPKR_GAIN_SHIFT 2 + #define M98088_MIX_SPKL_GAIN_MASK (3<<0) + #define M98088_MIX_SPKL_GAIN_SHIFT 0 + +/* M98088_REG_2F_LVL_DAI1_PLAY, M98088_REG_31_LVL_DAI2_PLAY */ + #define M98088_DAI_MUTE (1<<7) + #define M98088_DAI_MUTE_MASK (1<<7) + #define M98088_DAI_VOICE_GAIN_MASK (3<<4) + #define M98088_DAI_ATTENUATION_MASK (0xF<<0) + #define M98088_DAI_ATTENUATION_SHIFT 0 + /* M98088_REG_35_LVL_MIC1, M98088_REG_36_LVL_MIC2 */ #define M98088_MICPRE_MASK (3<<5) #define M98088_MICPRE_SHIFT 5
Hi Mr. Jin Park,
On Thu, May 12, 2011 at 2:58 PM, jinyoungp@nvidia.com wrote:
From: Jin Park jinyoungp@nvidia.com
Added digital mute function in DAI1 and DAI2.
Signed-off-by: Jin Park jinyoungp@nvidia.com
sound/soc/codecs/max98088.c | 32 ++++++++++++++++++++++++++++++++ sound/soc/codecs/max98088.h | 13 +++++++++++++ 2 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index c520093..b5ccf37 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1582,6 +1582,36 @@ static int max98088_dai2_set_fmt(struct snd_soc_dai *codec_dai, return 0; }
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
In this case, ~M98088_DAI_MUTE is more nice then "0" to me.
- snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
IMHO, we need to chance to get errors during update bits like below.
return snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY, M98088_DAI_MUTE_MASK, reg);
+}
+static int max98088_dai2_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
Same.
- snd_soc_update_bits(codec, M98088_REG_31_LVL_DAI2_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
Also.
+}
static void max98088_sync_cache(struct snd_soc_codec *codec) { u16 *reg_cache = codec->reg_cache; @@ -1643,12 +1673,14 @@ static struct snd_soc_dai_ops max98088_dai1_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai1_set_fmt, .hw_params = max98088_dai1_hw_params,
- .digital_mute = max98088_dai1_digital_mute,
};
static struct snd_soc_dai_ops max98088_dai2_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai2_set_fmt, .hw_params = max98088_dai2_hw_params,
- .digital_mute = max98088_dai2_digital_mute,
};
static struct snd_soc_dai_driver max98088_dai[] = { diff --git a/sound/soc/codecs/max98088.h b/sound/soc/codecs/max98088.h index 56554c7..be89a4f 100644 --- a/sound/soc/codecs/max98088.h +++ b/sound/soc/codecs/max98088.h @@ -133,6 +133,19 @@ #define M98088_REC_LINEMODE (1<<7) #define M98088_REC_LINEMODE_MASK (1<<7)
+/* M98088_REG_2D_MIX_SPK_CNTL */
- #define M98088_MIX_SPKR_GAIN_MASK (3<<2)
- #define M98088_MIX_SPKR_GAIN_SHIFT 2
- #define M98088_MIX_SPKL_GAIN_MASK (3<<0)
- #define M98088_MIX_SPKL_GAIN_SHIFT 0
Are these macros really need to add in this patch? Because, there is nothing to use this macros, and also doesn't have any relatives with "digital mute".
+/* M98088_REG_2F_LVL_DAI1_PLAY, M98088_REG_31_LVL_DAI2_PLAY */
- #define M98088_DAI_MUTE (1<<7)
- #define M98088_DAI_MUTE_MASK (1<<7)
- #define M98088_DAI_VOICE_GAIN_MASK (3<<4)
- #define M98088_DAI_ATTENUATION_MASK (0xF<<0)
- #define M98088_DAI_ATTENUATION_SHIFT 0
Same to "M98088_DAI_VOICE_GAIN_MASK", "M98088_DAI_ATTENUATION_MASK" and "M98088_DAI_ATTENUATION_SHIFT".
Hi Seungwhan,
In this case, ~M98088_DAI_MUTE is more nice then "0" to me.
if using "~M98088_DAI_MUTE" instead "0" as you mentioned, it will make wrong value. Below is snd_soc_update_bits().
int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned short reg, unsigned int mask, unsigned int value) { int change; unsigned int old, new;
old = snd_soc_read(codec, reg); new = (old & ~mask) | value; change = old != new; if (change) snd_soc_write(codec, reg, new);
return change; }
M98088_DAI_MUTE is (1<<7), it means 0x80, so ~ M98088_DAI_MUTE is 0x7F. If it call snd_soc_update_bits() with "~ M98088_DAI_MUTE" and old value is 0x00 in target reg(M98088_REG_2F_LVL_DAI1_PLAY), the new value will be 0x7F. And then this invaild value will write into target reg. But I want to clear only bit7, in this case, new value should be 0x00. So your comment is not right.
And also next your comment...
IMHO, we need to chance to get errors during update bits like below.
return snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY, M98088_DAI_MUTE_MASK, reg);
The snd_soc_update_bits() is returned value what means changed(1) or unchanged(0), it is not update success or fail. So this also is not right.
Are these macros really need to add in this patch? Because, there is nothing to use this macros, and also doesn't have any relatives with "digital mute". Same to "M98088_DAI_VOICE_GAIN_MASK", "M98088_DAI_ATTENUATION_MASK" and "M98088_DAI_ATTENUATION_SHIFT".
I have defined all meaningful bits in register, although it does not use any where.
Thanks, Jin Park.
-----Original Message----- From: Seungwhan Youn [mailto:claude.youn@gmail.com] Sent: Friday, May 13, 2011 5:44 AM To: Jinyoung Park Cc: broonie@opensource.wolfsonmicro.com; Peter.Hsiang@maxim-ic.com; alsa-devel@alsa-project.org; lrg@slimlogic.co.uk Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: codecs: max98088: Added digital mute function in DAI1 and DAI2
Hi Mr. Jin Park,
On Thu, May 12, 2011 at 2:58 PM, jinyoungp@nvidia.com wrote:
From: Jin Park jinyoungp@nvidia.com
Added digital mute function in DAI1 and DAI2.
Signed-off-by: Jin Park jinyoungp@nvidia.com
sound/soc/codecs/max98088.c | 32 ++++++++++++++++++++++++++++++++ sound/soc/codecs/max98088.h | 13 +++++++++++++ 2 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index c520093..b5ccf37 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1582,6 +1582,36 @@ static int max98088_dai2_set_fmt(struct snd_soc_dai *codec_dai, return 0; }
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, +int mute) {
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
In this case, ~M98088_DAI_MUTE is more nice then "0" to me.
- snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
IMHO, we need to chance to get errors during update bits like below.
return snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY, M98088_DAI_MUTE_MASK, reg);
+}
+static int max98088_dai2_digital_mute(struct snd_soc_dai *codec_dai, +int mute) {
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
Same.
- snd_soc_update_bits(codec, M98088_REG_31_LVL_DAI2_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
Also.
+}
static void max98088_sync_cache(struct snd_soc_codec *codec) { u16 *reg_cache = codec->reg_cache; @@ -1643,12 +1673,14 @@ static struct snd_soc_dai_ops max98088_dai1_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai1_set_fmt, .hw_params = max98088_dai1_hw_params,
- .digital_mute = max98088_dai1_digital_mute,
};
static struct snd_soc_dai_ops max98088_dai2_ops = { .set_sysclk = max98088_dai_set_sysclk, .set_fmt = max98088_dai2_set_fmt, .hw_params = max98088_dai2_hw_params,
- .digital_mute = max98088_dai2_digital_mute,
};
static struct snd_soc_dai_driver max98088_dai[] = { diff --git a/sound/soc/codecs/max98088.h b/sound/soc/codecs/max98088.h index 56554c7..be89a4f 100644 --- a/sound/soc/codecs/max98088.h +++ b/sound/soc/codecs/max98088.h @@ -133,6 +133,19 @@ #define M98088_REC_LINEMODE (1<<7) #define M98088_REC_LINEMODE_MASK (1<<7)
+/* M98088_REG_2D_MIX_SPK_CNTL */
- #define M98088_MIX_SPKR_GAIN_MASK (3<<2)
- #define M98088_MIX_SPKR_GAIN_SHIFT 2
- #define M98088_MIX_SPKL_GAIN_MASK (3<<0)
- #define M98088_MIX_SPKL_GAIN_SHIFT 0
Are these macros really need to add in this patch? Because, there is nothing to use this macros, and also doesn't have any relatives with "digital mute".
+/* M98088_REG_2F_LVL_DAI1_PLAY, M98088_REG_31_LVL_DAI2_PLAY */
- #define M98088_DAI_MUTE (1<<7)
- #define M98088_DAI_MUTE_MASK (1<<7)
- #define M98088_DAI_VOICE_GAIN_MASK (3<<4)
- #define M98088_DAI_ATTENUATION_MASK (0xF<<0)
- #define M98088_DAI_ATTENUATION_SHIFT 0
Same to "M98088_DAI_VOICE_GAIN_MASK", "M98088_DAI_ATTENUATION_MASK" and "M98088_DAI_ATTENUATION_SHIFT".
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
On Fri, May 13, 2011 at 2:13 AM, Seungwhan Youn claude.youn@gmail.com wrote:
Hi Mr. Jin Park,
On Thu, May 12, 2011 at 2:58 PM, jinyoungp@nvidia.com wrote:
From: Jin Park jinyoungp@nvidia.com
Added digital mute function in DAI1 and DAI2.
Signed-off-by: Jin Park jinyoungp@nvidia.com
sound/soc/codecs/max98088.c | 32 ++++++++++++++++++++++++++++++++ sound/soc/codecs/max98088.h | 13 +++++++++++++ 2 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index c520093..b5ccf37 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1582,6 +1582,36 @@ static int max98088_dai2_set_fmt(struct snd_soc_dai *codec_dai, return 0; }
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
In this case, ~M98088_DAI_MUTE is more nice then "0" to me.
No need. The snd_soc_update_bits takes care of masking. Better still, one could simply init reg to 0 and discard the else clause.
On Thu, May 12, 2011 at 11:28 AM, jinyoungp@nvidia.com wrote:
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
- snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
+}
+static int max98088_dai2_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
- snd_soc_update_bits(codec, M98088_REG_31_LVL_DAI2_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
+}
max98088_dai1_digital_mute max98088_dai2_digital_mute max98088_dai1_set_fmt max98088_dai2_set_fmt max98088_dai1_hw_params max98088_dai2_hw_params
These pairs are essentially same function operating on different registers. Can't we compress them ?
Jassi, I agree with you and it is not difficult. If Peter Hsiang agrees that compress them, I will do that. He is author of max98088 driver.
Peter, How do you think about that?
Thanks, Jinyoung.
-----Original Message----- From: Jassi Brar [mailto:jassisinghbrar@gmail.com] Sent: Friday, May 13, 2011 4:11 PM To: Jinyoung Park Cc: broonie@opensource.wolfsonmicro.com; Peter.Hsiang@maxim-ic.com; alsa-devel@alsa-project.org; lrg@slimlogic.co.uk Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: codecs: max98088: Added digital mute function in DAI1 and DAI2
On Thu, May 12, 2011 at 11:28 AM, jinyoungp@nvidia.com wrote:
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, +int mute) {
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
- snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
+}
+static int max98088_dai2_digital_mute(struct snd_soc_dai *codec_dai, +int mute) {
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
- snd_soc_update_bits(codec, M98088_REG_31_LVL_DAI2_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
+}
max98088_dai1_digital_mute max98088_dai2_digital_mute max98088_dai1_set_fmt max98088_dai2_set_fmt max98088_dai1_hw_params max98088_dai2_hw_params
These pairs are essentially same function operating on different registers. Can't we compress them ?
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
On Fri, May 13, 2011 at 1:55 PM, Jinyoung Park jinyoungp@nvidia.com wrote:
Jassi, I agree with you and it is not difficult. If Peter Hsiang agrees that compress them, I will do that. He is author of max98088 driver.
Ok you can ask. Though in such trivially good cases, it is better to ask for ack with the final work. Also, please avoid top posting.
On Friday, May 13, 2011, jinyoungp@nvidia.com wrote:
Peter, How do you think about that?
On Friday, May 13, 2011, Jassi Brar wrote:
+static int max98088_dai1_digital_mute(struct snd_soc_dai *codec_dai, +int mute) {
- struct snd_soc_codec *codec = codec_dai->codec;
- int reg;
- if (mute)
- reg = M98088_DAI_MUTE;
- else
- reg = 0;
- snd_soc_update_bits(codec, M98088_REG_2F_LVL_DAI1_PLAY,
- M98088_DAI_MUTE_MASK, reg);
- return 0;
+}
max98088_dai1_digital_mute max98088_dai2_digital_mute max98088_dai1_set_fmt max98088_dai2_set_fmt max98088_dai1_hw_params max98088_dai2_hw_params
These pairs are essentially same function operating on different registers. Can't we compress them ?
Hi Jinyoung,
Thank you for looking at this.
The digital mute function is relatively small. It is ok as separate functions.
regval = mute ? M98088_DAI_MUTE : 0;
For hw_params, I had wanted to combine them too initially. However, there are quite a few different registers in there. For only 2 instantiations, the extra conditional code to enable combining them will somewhat diminish the benefit. I am leaning toward keeping them the way they are. But it's ok with me if you think of a good way to do it. The set_fmt is a possibility.
Peter
On Thu, 2011-05-12 at 14:58 +0900, jinyoungp@nvidia.com wrote:
From: Jin Park jinyoungp@nvidia.com
Recently, I working on max98088 and changed something in max98088 asoc driver. The MAX98088 driver had some bug and needed modifies. The changes was confirmed by Peter Hsiang who author of max98088 driver and maxim engineer. Please review my changes.
Thanks, Jin Park.
Jin Park (3): ASoC: codecs: max98088: Fixed invalid register definitions in mixer controls ASoC: codecs: max98088: Moved the EX Limiter Mode from dapm widget to control ASoC: codecs: max98088: Added digital mute function in DAI1 and DAI2
sound/soc/codecs/max98088.c | 62 +++++++++++++++++++++++++++++++------------ sound/soc/codecs/max98088.h | 13 +++++++++ 2 files changed, 58 insertions(+), 17 deletions(-)
All
Acked-by: Liam Girdwood lrg@ti.com
On Thu, May 12, 2011 at 02:58:35PM +0900, jinyoungp@nvidia.com wrote:
Recently, I working on max98088 and changed something in max98088 asoc driver. The MAX98088 driver had some bug and needed modifies. The changes was confirmed by Peter Hsiang who author of max98088 driver and maxim engineer.
Applied all, thanks.
participants (7)
-
Jassi Brar -
Jinyoung Park -
jinyoungp＠nvidia.com
-
Liam Girdwood -
Mark Brown -
Peter Hsiang -
Seungwhan Youn