[PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Fabio Estevam]

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Charles Keepax]

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
> 

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Charles Keepax]

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Fabio Estevam]

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Charles Keepax]

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Fabio Estevam]

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!

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Mark Brown]

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Shengjiu Wang]

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
>

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Mark Brown]

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

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 :/

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

Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE

[Fabio Estevam]

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.