[PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

older
[PATCH v3 0/2] Add documentation...

Fabio Estevam

17 Jul 2020 17 Jul '20

3:59 p.m.

According to the WM8962 datasheet, there is no register at address 0x200.

WM8962_GPIO_BASE is just a base address for the GPIO registers and not a real register, so remove it from wm8962_readable_register().

Also, Register 515 (WM8962_GPIO_BASE + 3) does not exist, so skip its access.

This fixes the following errors:

wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Signed-off-by: Fabio Estevam festevam@gmail.com --- Hi,

There is still one more soc_component_read_no_lock error left on register 48.

I can get rid of it with the below change:

--- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -151,6 +151,7 @@ static const struct reg_default wm8962_reg[] = { { 40, 0x0000 }, /* R40 - SPKOUTL volume */ { 41, 0x0000 }, /* R41 - SPKOUTR volume */

+ { 48, 0x0000 }, /* R48 - Additional control(4) */ { 49, 0x0010 }, /* R49 - Class D Control 1 */ { 51, 0x0003 }, /* R51 - Class D Control 2 */

@@ -841,7 +842,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_SPKOUTL_VOLUME: case WM8962_SPKOUTR_VOLUME: case WM8962_THERMAL_SHUTDOWN_STATUS: - case WM8962_ADDITIONAL_CONTROL_4: case WM8962_CLASS_D_CONTROL_1: case WM8962_CLASS_D_CONTROL_2: case WM8962_CLOCKING_4:

I haven't submitted it yet because I don't know if this is the correct approach.

Please advise.

Thanks

sound/soc/codecs/wm8962.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index df8cdc71357d..8159a3866cde 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -956,7 +956,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_EQ39: case WM8962_EQ40: case WM8962_EQ41: - case WM8962_GPIO_BASE: case WM8962_GPIO_2: case WM8962_GPIO_3: case WM8962_GPIO_5: @@ -3437,7 +3436,13 @@ static int wm8962_probe(struct snd_soc_component *component) /* Save boards having to disable DMIC when not in use */ dmicclk = false; dmicdat = false; - for (i = 0; i < WM8962_MAX_GPIO; i++) { + for (i = 1; i < WM8962_MAX_GPIO; i++) { + /* + * Register 515 (WM8962_GPIO_BASE + 3) does not exist, + * so skip its access + */ + if (i == 3) + continue; switch (snd_soc_component_read(component, WM8962_GPIO_BASE + i) & WM8962_GP2_FN_MASK) { case WM8962_GPIO_FN_DMICCLK:

-- 2.17.1

Show replies by date

Charles Keepax

23 Jul 23 Jul

11:11 a.m.

On Fri, Jul 17, 2020 at 10:59:59AM -0300, Fabio Estevam wrote:

...

According to the WM8962 datasheet, there is no register at address 0x200.

WM8962_GPIO_BASE is just a base address for the GPIO registers and not a real register, so remove it from wm8962_readable_register().

Also, Register 515 (WM8962_GPIO_BASE + 3) does not exist, so skip its access.

This fixes the following errors:

wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Signed-off-by: Fabio Estevam festevam@gmail.com

This doesn't quite smell right admittedly I am not 100% sure for this chip but usually the Wolfson chips just return zero when you read a non-existant register rather than NACKing the transaction. But even if the chip was NACKing the transaction I would probably expect an EIO rather than EBUSY error.

Are we sure we understand the error we are addressing here?

Thanks, Charles

...

Hi,

There is still one more soc_component_read_no_lock error left on register 48.

I can get rid of it with the below change:

--- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -151,6 +151,7 @@ static const struct reg_default wm8962_reg[] = { { 40, 0x0000 }, /* R40 - SPKOUTL volume */ { 41, 0x0000 }, /* R41 - SPKOUTR volume */

{ 48, 0x0000 }, /* R48 - Additional control(4) */ { 49, 0x0010 }, /* R49 - Class D Control 1 */ { 51, 0x0003 }, /* R51 - Class D Control 2 */

@@ -841,7 +842,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_SPKOUTL_VOLUME: case WM8962_SPKOUTR_VOLUME: case WM8962_THERMAL_SHUTDOWN_STATUS:

case WM8962_ADDITIONAL_CONTROL_4: case WM8962_CLASS_D_CONTROL_1: case WM8962_CLASS_D_CONTROL_2: case WM8962_CLOCKING_4:

I haven't submitted it yet because I don't know if this is the correct approach.

Please advise.

Thanks

sound/soc/codecs/wm8962.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index df8cdc71357d..8159a3866cde 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -956,7 +956,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_EQ39: case WM8962_EQ40: case WM8962_EQ41:

case WM8962_GPIO_BASE: case WM8962_GPIO_2: case WM8962_GPIO_3: case WM8962_GPIO_5:

@@ -3437,7 +3436,13 @@ static int wm8962_probe(struct snd_soc_component *component) /* Save boards having to disable DMIC when not in use */ dmicclk = false; dmicdat = false;

for (i = 0; i < WM8962_MAX_GPIO; i++) {
for (i = 1; i < WM8962_MAX_GPIO; i++) {
/*
 * Register 515 (WM8962_GPIO_BASE + 3) does not exist,
 * so skip its access
 */
if (i == 3)
	continue;
switch (snd_soc_component_read(component, WM8962_GPIO_BASE + i) & WM8962_GP2_FN_MASK) { case WM8962_GPIO_FN_DMICCLK:
-- 2.17.1

Charles Keepax

11:21 a.m.

On Fri, Jul 17, 2020 at 10:59:59AM -0300, Fabio Estevam wrote:

...

According to the WM8962 datasheet, there is no register at address 0x200.

WM8962_GPIO_BASE is just a base address for the GPIO registers and not a real register, so remove it from wm8962_readable_register().

Also, Register 515 (WM8962_GPIO_BASE + 3) does not exist, so skip its access.

This fixes the following errors:

wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Ah ok I think I can see what is going on here, you get an EBUSY if the regmap is in cache only and you try to read a register which isn't in the cache. Is that what you are seeing?

...

Signed-off-by: Fabio Estevam festevam@gmail.com

Hi,

There is still one more soc_component_read_no_lock error left on register 48.

I can get rid of it with the below change:

--- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -151,6 +151,7 @@ static const struct reg_default wm8962_reg[] = { { 40, 0x0000 }, /* R40 - SPKOUTL volume */ { 41, 0x0000 }, /* R41 - SPKOUTR volume */

{ 48, 0x0000 }, /* R48 - Additional control(4) */ { 49, 0x0010 }, /* R49 - Class D Control 1 */ { 51, 0x0003 }, /* R51 - Class D Control 2 */

@@ -841,7 +842,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_SPKOUTL_VOLUME: case WM8962_SPKOUTR_VOLUME: case WM8962_THERMAL_SHUTDOWN_STATUS:

case WM8962_ADDITIONAL_CONTROL_4: case WM8962_CLASS_D_CONTROL_1: case WM8962_CLASS_D_CONTROL_2: case WM8962_CLOCKING_4:

I haven't submitted it yet because I don't know if this is the correct approach.

Yeah this one doesn't look like the right fix to me. Is this also a cache issue? Since this register is volatile.

I suspect for all of these it would be edifying to know which reads happen to these registers whilst the cache is set to cache only.

Thanks, Charles

Fabio Estevam

9:59 p.m.

Hi Charles,

On Thu, Jul 23, 2020 at 6:21 AM Charles Keepax ckeepax@opensource.cirrus.com wrote:

...

Ah ok I think I can see what is going on here, you get an EBUSY if the regmap is in cache only and you try to read a register which isn't in the cache. Is that what you are seeing?

After adding some debug info I got:

************ register is 512 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

************ register is 515 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Both register 512 and 515 do not exist as per the WM8962 datasheet, so the driver should not try to access them, right?

This patch avoids reading from these unexisting registers, which makes sense IMHO.

Do you have any other suggestions to avoid these errors?

Thanks

Charles Keepax

27 Jul 27 Jul

3:27 p.m.

On Thu, Jul 23, 2020 at 04:59:24PM -0300, Fabio Estevam wrote:

...

Hi Charles,

On Thu, Jul 23, 2020 at 6:21 AM Charles Keepax ckeepax@opensource.cirrus.com wrote:

...
Ah ok I think I can see what is going on here, you get an EBUSY if the regmap is in cache only and you try to read a register which isn't in the cache. Is that what you are seeing?

After adding some debug info I got:

************ register is 512 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

************ register is 515 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Both register 512 and 515 do not exist as per the WM8962 datasheet, so the driver should not try to access them, right?

This patch avoids reading from these unexisting registers, which makes sense IMHO.

Do you have any other suggestions to avoid these errors?

Alright fair enough, this is a good a fix as any for these two registers. Although I would suggest considering my questions for your additional control 4 issue, since there is a little more to think about there.

Acked-by: Charles Keepax ckeepax@opensource.cirrus.com

Thanks, Charles

Fabio Estevam

3:57 p.m.

Hi Charles,

On Mon, Jul 27, 2020 at 10:27 AM Charles Keepax ckeepax@opensource.cirrus.com wrote:

...

Alright fair enough, this is a good a fix as any for these two registers. Although I would suggest considering my questions for your additional control 4 issue, since there is a little more to think about there.

Yes, I will investigate more the issue related to the control 4 register.

...

Acked-by: Charles Keepax ckeepax@opensource.cirrus.com

Thanks, Charles!

Mark Brown

31 Jul 31 Jul

12:27 a.m.

On Fri, 17 Jul 2020 10:59:59 -0300, Fabio Estevam wrote:

...

According to the WM8962 datasheet, there is no register at address 0x200.

WM8962_GPIO_BASE is just a base address for the GPIO registers and not a real register, so remove it from wm8962_readable_register().

Also, Register 515 (WM8962_GPIO_BASE + 3) does not exist, so skip its access.

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: wm8962: Do not access WM8962_GPIO_BASE commit: 658bb297e3930b90f80c08ddff18b4065b89a132

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks, Mark

Shengjiu Wang

3 Aug 3 Aug

4:07 a.m.

Hi Fabio, Mark

On Fri, Jul 17, 2020 at 10:02 PM Fabio Estevam festevam@gmail.com wrote:

...

According to the WM8962 datasheet, there is no register at address 0x200.

WM8962_GPIO_BASE is just a base address for the GPIO registers and not a real register, so remove it from wm8962_readable_register().

Also, Register 515 (WM8962_GPIO_BASE + 3) does not exist, so skip its access.

This fixes the following errors:

wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16 wm8962 0-001a: ASoC: error at soc_component_read_no_lock on wm8962.0-001a: -16

Signed-off-by: Fabio Estevam festevam@gmail.com

Hi,

There is still one more soc_component_read_no_lock error left on register 48.

I can get rid of it with the below change:

--- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -151,6 +151,7 @@ static const struct reg_default wm8962_reg[] = { { 40, 0x0000 }, /* R40 - SPKOUTL volume */ { 41, 0x0000 }, /* R41 - SPKOUTR volume */
  { 48, 0x0000 },   /* R48    - Additional control(4) */
  { 49, 0x0010 },   /* R49    - Class D Control 1 */
  { 51, 0x0003 },   /* R51    - Class D Control 2 */
@@ -841,7 +842,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_SPKOUTL_VOLUME: case WM8962_SPKOUTR_VOLUME: case WM8962_THERMAL_SHUTDOWN_STATUS:
  case WM8962_ADDITIONAL_CONTROL_4:

WM8962_ADDITIONAL_CONTROL_4 should not be removed from the readable registers, after this change, there is distortion on output sound. but this patch has been merged.

"[PATCH RESEND] ASoC: wm8962: Do not access WM8962_GPIO_BASE" that patch is correct.

best regards wang shengjiu

wang shengjiu

...

    case WM8962_CLASS_D_CONTROL_1:
    case WM8962_CLASS_D_CONTROL_2:
    case WM8962_CLOCKING_4:
I haven't submitted it yet because I don't know if this is the correct approach.

Please advise.

Thanks

sound/soc/codecs/wm8962.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index df8cdc71357d..8159a3866cde 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -956,7 +956,6 @@ static bool wm8962_readable_register(struct device *dev, unsigned int reg) case WM8962_EQ39: case WM8962_EQ40: case WM8962_EQ41:
  case WM8962_GPIO_BASE:
  case WM8962_GPIO_2:
  case WM8962_GPIO_3:
  case WM8962_GPIO_5:
@@ -3437,7 +3436,13 @@ static int wm8962_probe(struct snd_soc_component *component) /* Save boards having to disable DMIC when not in use */ dmicclk = false; dmicdat = false;
  for (i = 0; i < WM8962_MAX_GPIO; i++) {
  for (i = 1; i < WM8962_MAX_GPIO; i++) {
          /*
           * Register 515 (WM8962_GPIO_BASE + 3) does not exist,
           * so skip its access
           */
          if (i == 3)
                  continue;
          switch (snd_soc_component_read(component, WM8962_GPIO_BASE + i)
                  & WM8962_GP2_FN_MASK) {
          case WM8962_GPIO_FN_DMICCLK:
-- 2.17.1

Mark Brown

12:51 p.m.

On Mon, Aug 03, 2020 at 10:07:06AM +0800, Shengjiu Wang wrote:

...

WM8962_ADDITIONAL_CONTROL_4 should not be removed from the readable registers, after this change, there is distortion on output sound. but this patch has been merged.

If there's an issue please submit an incremental patch fixing this.

...

"[PATCH RESEND] ASoC: wm8962: Do not access WM8962_GPIO_BASE" that patch is correct.

If the two patches differ clearly that wasn't a resend :/

Fabio Estevam

1:58 p.m.

Hi Mark,

On Mon, Aug 3, 2020 at 7:51 AM Mark Brown broonie@kernel.org wrote:

...

On Mon, Aug 03, 2020 at 10:07:06AM +0800, Shengjiu Wang wrote:

...
WM8962_ADDITIONAL_CONTROL_4 should not be removed from the readable registers, after this change, there is distortion on output sound. but this patch has been merged.

If there's an issue please submit an incremental patch fixing this.

I have just sent a fix.

The patch I submitted originally did not touch the WM8962_ADDITIONAL_CONTROL_4 register: https://www.spinics.net/lists/alsa-devel/msg112549.html

The part that touched this register was below the --- line and it was just a comment, not supposed to get merged.

Maybe this confused git. Sorry about that.

1509

Age (days ago)

1526

Last active (days ago)

List overview

Download

9 comments

4 participants

tags (0)

participants (4)

Charles Keepax
Fabio Estevam
Mark Brown
Shengjiu Wang