[PATCH 0/3] ASoC: wm8741: Varios enhancements

[PATCH 0/3] ASoC: wm8741: Varios enhancements

[Sergej Sawazki]

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(-)

-- 
2.7.4

[PATCH 1/3] ASoC: wm8741: Add digital mute callback

[Sergej Sawazki]

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 = {
-- 
2.7.4

[PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[Sergej Sawazki]

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;
 }
 
-- 
2.7.4

[PATCH 3/3] ASoC: wm8741: Set SR mode in hw_params()

[Sergej Sawazki]

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;
-- 
2.7.4

Re: [PATCH 1/3] ASoC: wm8741: Add digital mute callback

[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

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

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

> +
>  	/* 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

Re: [PATCH 3/3] ASoC: wm8741: Set SR mode in hw_params()

[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?

> 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

Re: [PATCH 1/3] ASoC: wm8741: Add digital mute callback

[Sergej Sawazki]

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
> 

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[Sergej Sawazki]

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

Re: [PATCH 3/3] ASoC: wm8741: Set SR mode in hw_params()

[Sergej Sawazki]

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

Re: [PATCH 1/3] ASoC: wm8741: Add digital mute callback

[Charles Keepax]

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

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[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:
> > > +	/* 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

Re: [PATCH 3/3] ASoC: wm8741: Set SR mode in hw_params()

[Charles Keepax]

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

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[Sergej Sawazki]

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

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[Charles Keepax]

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

Re: [PATCH 2/3] ASoC: wm8741: Set OSR mode in hw_params()

[Sergej Sawazki]

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