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. -----------------------------------------------------------------------------------