[alsa-devel] [PATCH 0/3] ASoC: wm8741: Varios enhancements
This patch series contains the following 3 patches for the WM8741 codec driver.
1) Adds digital mute callback. 2) Sets oversampling rate mode (OSR), required for sampling rates > 48kHz. 3) Sets MCLK/LRCLK sampling rate ratio (SR), avoids relying on auto-detection.
Sergej Sawazki (3): ASoC: wm8741: Add digital mute callback ASoC: wm8741: Set OSR mode in hw_params() ASoC: wm8741: Set SR mode in hw_params()
sound/soc/codecs/wm8741.c | 49 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 8 deletions(-)
Signed-off-by: Sergej Sawazki sergej@taudac.com --- sound/soc/codecs/wm8741.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index b8c1940..d552a8a 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -351,6 +351,17 @@ static int wm8741_set_dai_fmt(struct snd_soc_dai *codec_dai, return 0; }
+int wm8741_mute(struct snd_soc_dai *codec_dai, int mute) +{ + struct snd_soc_codec *codec = codec_dai->codec; + + dev_dbg(codec->dev, "%s: mute = %d\n", __func__, mute); + + snd_soc_update_bits(codec, WM8741_VOLUME_CONTROL, + WM8741_SOFT_MASK, !!mute << WM8741_SOFT_SHIFT); + return 0; +} + #define WM8741_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | \ SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 | \ @@ -364,6 +375,7 @@ static const struct snd_soc_dai_ops wm8741_dai_ops = { .hw_params = wm8741_hw_params, .set_sysclk = wm8741_set_dai_sysclk, .set_fmt = wm8741_set_dai_fmt, + .digital_mute = wm8741_mute, };
static struct snd_soc_dai_driver wm8741_dai = {
On Mon, Sep 04, 2017 at 09:34:11PM +0200, Sergej Sawazki wrote:
Signed-off-by: Sergej Sawazki sergej@taudac.com
sound/soc/codecs/wm8741.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index b8c1940..d552a8a 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -351,6 +351,17 @@ static int wm8741_set_dai_fmt(struct snd_soc_dai *codec_dai, return 0; }
+int wm8741_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- dev_dbg(codec->dev, "%s: mute = %d\n", __func__, mute);
Its not really normal style to use __func__ in this sort of print, just make it descriptive of what it does. Although I would also say you could just drop it as well if you fancied it is a little verbose even for debug output.
Otherwise this patch looks ok to me.
Thanks, Charles
Thanks Charles,
agreed about the __func__. But, I would like to keep the debug message, in order to be consistent with the other callbacks. They all print debug messages.
Would it be ok for you?
Regards, Sergej
Am 05.09.2017 um 15:33 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:11PM +0200, Sergej Sawazki wrote:
Signed-off-by: Sergej Sawazki sergej@taudac.com
sound/soc/codecs/wm8741.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index b8c1940..d552a8a 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -351,6 +351,17 @@ static int wm8741_set_dai_fmt(struct snd_soc_dai *codec_dai, return 0; }
+int wm8741_mute(struct snd_soc_dai *codec_dai, int mute) +{
- struct snd_soc_codec *codec = codec_dai->codec;
- dev_dbg(codec->dev, "%s: mute = %d\n", __func__, mute);
Its not really normal style to use __func__ in this sort of print, just make it descriptive of what it does. Although I would also say you could just drop it as well if you fancied it is a little verbose even for debug output.
Otherwise this patch looks ok to me.
Thanks, Charles
On Tue, Sep 05, 2017 at 08:15:05PM +0200, Sergej Sawazki wrote:
Thanks Charles,
agreed about the __func__. But, I would like to keep the debug message, in order to be consistent with the other callbacks. They all print debug messages.
Would it be ok for you?
I am ok with leaving the print, if we update the message.
Thanks, Charles
For correct operation of the digital filtering and other processing on the WM8741, the user must ensure the correct value of OSR[1:0] is set at all times.[1] Hence, depending the selected sampling rate, set the OSR (over- sampling rate) mode in hw_params().
References: [1] https://d3uzseaevmutz1.cloudfront.net/pubs/proDatasheet/WM8741_v4.3.pdf "WM8741 Data Sheet"
Signed-off-by: Sergej Sawazki sergej@taudac.com --- sound/soc/codecs/wm8741.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index d552a8a..7e8a7fe 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -197,6 +197,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, struct snd_soc_codec *codec = dai->codec; struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec); u16 iface = snd_soc_read(codec, WM8741_FORMAT_CONTROL) & 0x1FC; + u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183; int i;
/* The set of sample rates that can be supported depends on the @@ -220,6 +221,12 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
+ /* oversampling rate */ + if (params_rate(params) > 96000) + mode |= 0x0040; + else if (params_rate(params) > 48000) + mode |= 0x0020; + /* bit size */ switch (params_width(params)) { case 16: @@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params));
snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface); + snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode); return 0; }
On Mon, Sep 04, 2017 at 09:34:12PM +0200, Sergej Sawazki wrote:
- /* oversampling rate */
- if (params_rate(params) > 96000)
mode |= 0x0040;- else if (params_rate(params) > 48000)
mode |= 0x0020;
Should this not have a case for <= 48k as well? To reset the mode if it was set on a previous playback.
- /* bit size */ switch (params_width(params)) { case 16:
@@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params));
snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface);
- snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode);
Better to use an update_bits rather than basically open-code it with read and then this write.
Thanks, Charles
Am 05.09.2017 um 18:12 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:12PM +0200, Sergej Sawazki wrote:
- /* oversampling rate */
- if (params_rate(params) > 96000)
mode |= 0x0040;- else if (params_rate(params) > 48000)
mode |= 0x0020;Should this not have a case for <= 48k as well? To reset the mode if it was set on a previous playback.
It is reset in here:
u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183;
For <= 48k we simply leave it 0. I though about an empty else statement, but it is kind of strange, too.
- /* bit size */ switch (params_width(params)) { case 16:
@@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params));
snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface);
- snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode);
Better to use an update_bits rather than basically open-code it with read and then this write.
I have used write() and not update_bits() here to be consistent with the way the WM8741_FORMAT_CONTROL register is written. If we change it to update_bits(), we should refactor the WM8741_FORMAT_CONTROL write to use update_bits() too, right?
Thanks, Sergej
On Tue, Sep 05, 2017 at 08:36:24PM +0200, Sergej Sawazki wrote:
Am 05.09.2017 um 18:12 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:12PM +0200, Sergej Sawazki wrote:
- /* oversampling rate */
- if (params_rate(params) > 96000)
mode |= 0x0040;- else if (params_rate(params) > 48000)
mode |= 0x0020;Should this not have a case for <= 48k as well? To reset the mode if it was set on a previous playback.
It is reset in here:
u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183;For <= 48k we simply leave it 0. I though about an empty else statement, but it is kind of strange, too.
Oops.. sorry yes that is fine.
- /* bit size */ switch (params_width(params)) { case 16:
@@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params)); snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface);
- snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode);
Better to use an update_bits rather than basically open-code it with read and then this write.
I have used write() and not update_bits() here to be consistent with the way the WM8741_FORMAT_CONTROL register is written. If we change it to update_bits(), we should refactor the WM8741_FORMAT_CONTROL write to use update_bits() too, right?
Yeah that should probably be an update bits too by the looks of it.
Thanks, Charles
Am 06.09.2017 um 10:20 schrieb Charles Keepax:
On Tue, Sep 05, 2017 at 08:36:24PM +0200, Sergej Sawazki wrote:
Am 05.09.2017 um 18:12 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:12PM +0200, Sergej Sawazki wrote:
[...]
@@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params)); snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface);
- snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode);
Better to use an update_bits rather than basically open-code it with read and then this write.
I have used write() and not update_bits() here to be consistent with the way the WM8741_FORMAT_CONTROL register is written. If we change it to update_bits(), we should refactor the WM8741_FORMAT_CONTROL write to use update_bits() too, right?
Yeah that should probably be an update bits too by the looks of it.
Charles,
I think we should split "adding the OSR mode configuration" and "refactoring the code to use update_bits()" in at least two commits.
For consistency, I would keep the write() in this patch, and provide the update_bits() refactoring later. What do you think?
Thanks, Sergej
On Mon, Sep 18, 2017 at 09:30:36PM +0200, Sergej Sawazki wrote:
Am 06.09.2017 um 10:20 schrieb Charles Keepax:
On Tue, Sep 05, 2017 at 08:36:24PM +0200, Sergej Sawazki wrote:
Am 05.09.2017 um 18:12 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:12PM +0200, Sergej Sawazki wrote:
[...]
@@ -243,6 +250,7 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, params_width(params), params_rate(params)); snd_soc_write(codec, WM8741_FORMAT_CONTROL, iface);
- snd_soc_write(codec, WM8741_MODE_CONTROL_1, mode);
Better to use an update_bits rather than basically open-code it with read and then this write.
I have used write() and not update_bits() here to be consistent with the way the WM8741_FORMAT_CONTROL register is written. If we change it to update_bits(), we should refactor the WM8741_FORMAT_CONTROL write to use update_bits() too, right?
Yeah that should probably be an update bits too by the looks of it.
Charles,
I think we should split "adding the OSR mode configuration" and "refactoring the code to use update_bits()" in at least two commits.
For consistency, I would keep the write() in this patch, and provide the update_bits() refactoring later. What do you think?
Two commits seems sensible although its only a very small refactoring so feels like we might as well get it done whilst changing the code. I am happy to do the initial refactoring patch, if that helps but I don't presently have hardware to test this CODEC.
Thanks, Charles
On Tue, 19 Sep 2017 10:47:56 +0100, Charles Keepax wrote:
On Mon, Sep 18, 2017 at 09:30:36PM +0200, Sergej Sawazki wrote:
Am 06.09.2017 um 10:20 schrieb Charles Keepax:
...
I think we should split "adding the OSR mode configuration" and "refactoring the code to use update_bits()" in at least two commits.
For consistency, I would keep the write() in this patch, and provide the update_bits() refactoring later. What do you think?
Two commits seems sensible although its only a very small refactoring so feels like we might as well get it done whilst changing the code. I am happy to do the initial refactoring patch, if that helps but I don't presently have hardware to test this CODEC.
Thanks Charles,
I have the hardware and could make the tests if you provide initial refactoring patch.
Regards Sergej
Set the ratio of MCLK/LRCLK in hw_params() instead of relying on the auto-detection.
The ratio of MCLK/LRCLK is known to the driver, there is no need to let the device to detect it.
Signed-off-by: Sergej Sawazki sergej@taudac.com --- sound/soc/codecs/wm8741.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index 7e8a7fe..534741b 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -198,7 +198,6 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec); u16 iface = snd_soc_read(codec, WM8741_FORMAT_CONTROL) & 0x1FC; u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183; - int i;
/* The set of sample rates that can be supported depends on the * MCLK supplied to the CODEC - enforce this. @@ -209,13 +208,27 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- /* Find a supported LRCLK rate */ - for (i = 0; i < wm8741->sysclk_constraints->count; i++) { - if (wm8741->sysclk_constraints->list[i] == params_rate(params)) - break; - } - - if (i == wm8741->sysclk_constraints->count) { + /* MCLK to LRCLK sampling rate ratio */ + switch (wm8741->sysclk / params_rate(params)) { + case 128: + mode |= 0x0004; + break; + case 192: + mode |= 0x0008; + break; + case 256: + mode |= 0x000C; + break; + case 384: + mode |= 0x0010; + break; + case 512: + mode |= 0x0014; + break; + case 768: + mode |= 0x0018; + break; + default: dev_err(codec->dev, "LRCLK %d unsupported with MCLK %d\n", params_rate(params), wm8741->sysclk); return -EINVAL;
On Mon, Sep 04, 2017 at 09:34:13PM +0200, Sergej Sawazki wrote:
Set the ratio of MCLK/LRCLK in hw_params() instead of relying on the auto-detection.
The ratio of MCLK/LRCLK is known to the driver, there is no need to let the device to detect it.
But also no reason to not let it do so, are there some problems with the auto-detect?
Signed-off-by: Sergej Sawazki sergej@taudac.com
sound/soc/codecs/wm8741.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index 7e8a7fe..534741b 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -198,7 +198,6 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec); u16 iface = snd_soc_read(codec, WM8741_FORMAT_CONTROL) & 0x1FC; u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183;
int i;
/* The set of sample rates that can be supported depends on the
- MCLK supplied to the CODEC - enforce this.
@@ -209,13 +208,27 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- /* Find a supported LRCLK rate */
- for (i = 0; i < wm8741->sysclk_constraints->count; i++) {
if (wm8741->sysclk_constraints->list[i] == params_rate(params))break;- }
This looks like it should be in a separate patch, is removing this part of this patch? It feels like the constraints should have already been applied by startup so we should never be able to fail here in hw_params, is that why you are removing it?
Thanks, Charles
Am 05.09.2017 um 18:23 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:13PM +0200, Sergej Sawazki wrote:
Set the ratio of MCLK/LRCLK in hw_params() instead of relying on the auto-detection.
The ratio of MCLK/LRCLK is known to the driver, there is no need to let the device to detect it.
But also no reason to not let it do so, are there some problems with the auto-detect?
I haven't noticed any problems. But, as we know the ratio and the device provides an interface to set it, why don't we just do it? Setting it feels right somehow, even if it is not absolutely necessary.
Signed-off-by: Sergej Sawazki sergej@taudac.com
sound/soc/codecs/wm8741.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c index 7e8a7fe..534741b 100644 --- a/sound/soc/codecs/wm8741.c +++ b/sound/soc/codecs/wm8741.c @@ -198,7 +198,6 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec); u16 iface = snd_soc_read(codec, WM8741_FORMAT_CONTROL) & 0x1FC; u16 mode = snd_soc_read(codec, WM8741_MODE_CONTROL_1) & 0x183;
int i;
/* The set of sample rates that can be supported depends on the
- MCLK supplied to the CODEC - enforce this.
@@ -209,13 +208,27 @@ static int wm8741_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- /* Find a supported LRCLK rate */
- for (i = 0; i < wm8741->sysclk_constraints->count; i++) {
if (wm8741->sysclk_constraints->list[i] == params_rate(params))break;- }
This looks like it should be in a separate patch, is removing this part of this patch? It feels like the constraints should have already been applied by startup so we should never be able to fail here in hw_params, is that why you are removing it?
This for loop has been replaced by a switch statement. The switch is now checking if the LRCLK rate is supported with the supplied MCLK and sets the MCLK/LRCLK ratio if it is.
Thanks, Sergej
On Tue, Sep 05, 2017 at 09:05:27PM +0200, Sergej Sawazki wrote:
Am 05.09.2017 um 18:23 schrieb Charles Keepax:
On Mon, Sep 04, 2017 at 09:34:13PM +0200, Sergej Sawazki wrote:
Set the ratio of MCLK/LRCLK in hw_params() instead of relying on the auto-detection.
The ratio of MCLK/LRCLK is known to the driver, there is no need to let the device to detect it.
But also no reason to not let it do so, are there some problems with the auto-detect?
I haven't noticed any problems. But, as we know the ratio and the device provides an interface to set it, why don't we just do it? Setting it feels right somehow, even if it is not absolutely necessary.
I would lean more towards if the auto-detect works we might as well use it and not add code we don't need.
Thanks, Charles
participants (3)
-
Charles Keepax -
Sergej Sawazki -
Sergej Sawazki