[PATCH 0/2] *** wm8962 regmap related fix ***

[PATCH 0/2] *** wm8962 regmap related fix ***

[Jiada Wang]

This patch set aims to fix issues in wm8962 codec driver related to regmap,
currently any attempt to read from ALC Coefficient register will fail
when wm8962 is in suspend mode. As ALC2 register is volatile register,
it can't be read when cache_only flag is set.

Another issue is, if wm8962's regulator is set to 'regulator-always-on'
mode, then after wm8962 is resumed from suspend, wm8962 codec is reset,
but cache_dirty flag isn't set, this cause difference between actual wm8962
HW and regmap cache.

Jiada Wang (2):
  ASoC: WM8962: mark cache_dirty flag after software reset in pm_resume
  ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

 sound/soc/codecs/wm8962.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.4.5

[PATCH 1/2] ASoC: WM8962: mark cache_dirty flag after software reset in pm_resume

[Jiada Wang]

By doing software reset of wm8962 in pm_resume, all registers which
have already been set will be reset to default value without regmap
interface be involved, thus driver need to mark cache_dirty flag,
to let regcache can be updated by regcache_sync().

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 sound/soc/codecs/wm8962.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 293e47a..319ee38 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -3805,6 +3805,13 @@ static int wm8962_runtime_resume(struct device *dev)
 
 	wm8962_reset(wm8962);
 
+	/* All registers have been reset to default value without calling
+	 * to regmap interface, even if reset fails, some registers
+	 * maybe in intermediate status, so we need to mark regmap
+	 * cache_dirty flag.
+	 */
+	regcache_mark_dirty(wm8962->regmap);
+
 	/* SYSCLK defaults to on; make sure it is off so we can safely
 	 * write to registers if the device is declocked.
 	 */
-- 
2.4.5

Re: [PATCH 1/2] ASoC: WM8962: mark cache_dirty flag after software reset in pm_resume

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

On Tue, Oct 06, 2015 at 04:06:54PM +0900, Jiada Wang wrote:

> +	/* All registers have been reset to default value without calling
> +	 * to regmap interface, even if reset fails, some registers
> +	 * maybe in intermediate status, so we need to mark regmap
> +	 * cache_dirty flag.
> +	 */
> +	regcache_mark_dirty(wm8962->regmap);

This is a standard thing that applies to almost all devices, there
should be no need for such an extensive comment (which would normally
flag up that there's something weird and surprising going on).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] ASoC: WM8962: mark cache_dirty flag after software reset in pm_resume

[Jiada Wang]

Hi

On 10/06/2015 07:59 PM, Mark Brown wrote:
> On Tue, Oct 06, 2015 at 04:06:54PM +0900, Jiada Wang wrote:
>
>> +	/* All registers have been reset to default value without calling
>> +	 * to regmap interface, even if reset fails, some registers
>> +	 * maybe in intermediate status, so we need to mark regmap
>> +	 * cache_dirty flag.
>> +	 */
>> +	regcache_mark_dirty(wm8962->regmap);
>
> This is a standard thing that applies to almost all devices, there
> should be no need for such an extensive comment (which would normally
> flag up that there's something weird and surprising going on).
>

Will remove the comment in next update

[PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Jiada Wang]

As ALC2 register is volatile, declare it as one of ALC Coefficients
register together with other non-volatile registers will cause issue,
in case wm8962 has enter suspend mode, and cache_only flag is set,
any attempt to read from ALC2 will fail.

Instead of declaring one ALC Coefficients register which contains
ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
so that regmap can handle these registers differently based on their
classification.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 sound/soc/codecs/wm8962.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 319ee38..1201efd 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -1782,8 +1782,11 @@ SND_SOC_BYTES("HD Bass Coefficients", WM8962_HDBASS_AI_1, 30),
 
 SOC_DOUBLE("ALC Switch", WM8962_ALC1, WM8962_ALCL_ENA_SHIFT,
 		WM8962_ALCR_ENA_SHIFT, 1, 0),
-SND_SOC_BYTES_MASK("ALC Coefficients", WM8962_ALC1, 4,
+SND_SOC_BYTES_MASK("ALC1", WM8962_ALC1, 1,
 		WM8962_ALCL_ENA_MASK | WM8962_ALCR_ENA_MASK),
+SND_SOC_BYTES("ALC2", WM8962_ALC2, 1),
+SND_SOC_BYTES("ALC3", WM8962_ALC3, 1),
+SND_SOC_BYTES("Noise Gate", WM8962_NOISE_GATE, 1),
 };
 
 static const struct snd_kcontrol_new wm8962_spk_mono_controls[] = {
-- 
2.4.5

Re: [PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Tue, Oct 06, 2015 at 04:06:55PM +0900, Jiada Wang wrote:

> As ALC2 register is volatile, declare it as one of ALC Coefficients
> register together with other non-volatile registers will cause issue,
> in case wm8962 has enter suspend mode, and cache_only flag is set,
> any attempt to read from ALC2 will fail.

> Instead of declaring one ALC Coefficients register which contains
> ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
> so that regmap can handle these registers differently based on their
> classification.

I don't understand this commit log.  Why does regmap care how these
registers are presented to userspace, and how does splitting the
controls up address the problem with one of the registers being volatile?
Surely that register still has the same problem?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Jiada Wang]

Hi


On 10/06/2015 08:01 PM, Mark Brown wrote:
> On Tue, Oct 06, 2015 at 04:06:55PM +0900, Jiada Wang wrote:
>
>> As ALC2 register is volatile, declare it as one of ALC Coefficients
>> register together with other non-volatile registers will cause issue,
>> in case wm8962 has enter suspend mode, and cache_only flag is set,
>> any attempt to read from ALC2 will fail.
>
>> Instead of declaring one ALC Coefficients register which contains
>> ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
>> so that regmap can handle these registers differently based on their
>> classification.
>
> I don't understand this commit log.  Why does regmap care how these
> registers are presented to userspace, and how does splitting the
> controls up address the problem with one of the registers being volatile?
> Surely that register still has the same problem?
>
.get callback function will call regmap_raw_read() to read register
value from these registers, when these 4 regsters are declared as one
"ALC coefficient" register, condition check of regmap_volatile_range()
will return false, thus regmap will go word by word for the cache from
each register of "ALC Coefficient", the failure scenario is, when
wm8962 is in suspend mode (cache_only flag is set), as ALC2 doesn't
have cached value, then any attempt to read from it fails,

By splitting these registers, regmap can handle ALC2 as a single
volatile register, and always read from HW

Thanks,
Jiada

[PATCH v2 0/2] *** wm8962 regmap related fix ***

[Jiada Wang]

This patch set aims to fix issues in wm8962 codec driver related to regmap,
currently any attempt to read from ALC Coefficient register will fail
when wm8962 is in suspend mode. As ALC2 register is volatile register,
it can't be read when cache_only flag is set.

Another issue is, if wm8962's regulator is set to 'regulator-always-on'
mode, then after wm8962 is resumed from suspend, wm8962 codec is reset,
but cache_dirty flag isn't set, this cause difference between actual wm8962
HW and regmap cache.


Changeset:
--------------
v1 -> v2
* removed comment before regcache_mark_dirty 

Jiada Wang (2):
  ASoC: WM8962: mark cache_dirty flag after software reset in pm_resume
  ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

 sound/soc/codecs/wm8962.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.4.5

[PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Jiada Wang]

As ALC2 register is volatile, declare it as one of ALC Coefficients
register together with other non-volatile registers will cause issue,
in case wm8962 has enter suspend mode, and cache_only flag is set,
any attempt to read from ALC2 will fail.

Instead of declaring one ALC Coefficients register which contains
ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
so that regmap can handle these registers differently based on their
classification.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 sound/soc/codecs/wm8962.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index a3d7778..157530c 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -1782,8 +1782,11 @@ SND_SOC_BYTES("HD Bass Coefficients", WM8962_HDBASS_AI_1, 30),
 
 SOC_DOUBLE("ALC Switch", WM8962_ALC1, WM8962_ALCL_ENA_SHIFT,
 		WM8962_ALCR_ENA_SHIFT, 1, 0),
-SND_SOC_BYTES_MASK("ALC Coefficients", WM8962_ALC1, 4,
+SND_SOC_BYTES_MASK("ALC1", WM8962_ALC1, 1,
 		WM8962_ALCL_ENA_MASK | WM8962_ALCR_ENA_MASK),
+SND_SOC_BYTES("ALC2", WM8962_ALC2, 1),
+SND_SOC_BYTES("ALC3", WM8962_ALC3, 1),
+SND_SOC_BYTES("Noise Gate", WM8962_NOISE_GATE, 1),
 };
 
 static const struct snd_kcontrol_new wm8962_spk_mono_controls[] = {
-- 
2.4.5

Re: [PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Charles Keepax]

On Tue, Oct 20, 2015 at 11:47:12AM +0900, Jiada Wang wrote:
> As ALC2 register is volatile, declare it as one of ALC Coefficients
> register together with other non-volatile registers will cause issue,
> in case wm8962 has enter suspend mode, and cache_only flag is set,
> any attempt to read from ALC2 will fail.
> 
> Instead of declaring one ALC Coefficients register which contains
> ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
> so that regmap can handle these registers differently based on their
> classification.
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  sound/soc/codecs/wm8962.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
> index a3d7778..157530c 100644
> --- a/sound/soc/codecs/wm8962.c
> +++ b/sound/soc/codecs/wm8962.c
> @@ -1782,8 +1782,11 @@ SND_SOC_BYTES("HD Bass Coefficients", WM8962_HDBASS_AI_1, 30),
>  
>  SOC_DOUBLE("ALC Switch", WM8962_ALC1, WM8962_ALCL_ENA_SHIFT,
>  		WM8962_ALCR_ENA_SHIFT, 1, 0),
> -SND_SOC_BYTES_MASK("ALC Coefficients", WM8962_ALC1, 4,
> +SND_SOC_BYTES_MASK("ALC1", WM8962_ALC1, 1,
>  		WM8962_ALCL_ENA_MASK | WM8962_ALCR_ENA_MASK),
> +SND_SOC_BYTES("ALC2", WM8962_ALC2, 1),
> +SND_SOC_BYTES("ALC3", WM8962_ALC3, 1),
> +SND_SOC_BYTES("Noise Gate", WM8962_NOISE_GATE, 1),

This doesn't really seem ideal to be changing the interface at
this point in the drivers life.

Looking through the datasheet/driver there are 5 status bits in
the ALC2 register but we don't use them anywhere in the driver
and they don't look like they are likely to be useful to the end
user. I wonder if an easier solution might just be to have the
register be non-volatile?

Thanks,
Charles

Re: [PATCH 2/2] ASoC: Codec: wm8962: declare ALC Coefficients as 4 separate registers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Tue, Oct 20, 2015 at 11:47:12AM +0900, Jiada Wang wrote:
> As ALC2 register is volatile, declare it as one of ALC Coefficients
> register together with other non-volatile registers will cause issue,
> in case wm8962 has enter suspend mode, and cache_only flag is set,
> any attempt to read from ALC2 will fail.
> 
> Instead of declaring one ALC Coefficients register which contains
> ALC1 ~ ALC3 and Noise Gate, this patch declares 4 separate registers,
> so that regmap can handle these registers differently based on their
> classification.

In addition to the issue with the ABI change that Charles raised this
also doesn't entirely fix the issue in that we still have a control that
is going to give an error in suspend mode.  Charles' suggestion seems
like it's more likely to be helpful to users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]