* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-17 13:59 [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE Fabio Estevam
@ 2020-07-23 9:11 ` Charles Keepax
2020-07-23 9:21 ` Charles Keepax
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Charles Keepax @ 2020-07-23 9:11 UTC (permalink / raw)
To: Fabio Estevam; +Cc: shengjiu.wang, broonie, alsa-devel, kuninori.morimoto.gx
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-17 13:59 [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE Fabio Estevam
2020-07-23 9:11 ` Charles Keepax
@ 2020-07-23 9:21 ` Charles Keepax
2020-07-23 19:59 ` Fabio Estevam
2020-07-30 22:27 ` Mark Brown
2020-08-03 2:07 ` Shengjiu Wang
3 siblings, 1 reply; 10+ messages in thread
From: Charles Keepax @ 2020-07-23 9:21 UTC (permalink / raw)
To: Fabio Estevam; +Cc: shengjiu.wang, broonie, alsa-devel, kuninori.morimoto.gx
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-23 9:21 ` Charles Keepax
@ 2020-07-23 19:59 ` Fabio Estevam
2020-07-27 13:27 ` Charles Keepax
0 siblings, 1 reply; 10+ messages in thread
From: Fabio Estevam @ 2020-07-23 19:59 UTC (permalink / raw)
To: Charles Keepax; +Cc: S.j. Wang, Mark Brown, Linux-ALSA, Kuninori Morimoto
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-23 19:59 ` Fabio Estevam
@ 2020-07-27 13:27 ` Charles Keepax
2020-07-27 13:57 ` Fabio Estevam
0 siblings, 1 reply; 10+ messages in thread
From: Charles Keepax @ 2020-07-27 13:27 UTC (permalink / raw)
To: Fabio Estevam; +Cc: S.j. Wang, Mark Brown, Linux-ALSA, Kuninori Morimoto
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-27 13:27 ` Charles Keepax
@ 2020-07-27 13:57 ` Fabio Estevam
0 siblings, 0 replies; 10+ messages in thread
From: Fabio Estevam @ 2020-07-27 13:57 UTC (permalink / raw)
To: Charles Keepax; +Cc: S.j. Wang, Mark Brown, Linux-ALSA, Kuninori Morimoto
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-17 13:59 [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE Fabio Estevam
2020-07-23 9:11 ` Charles Keepax
2020-07-23 9:21 ` Charles Keepax
@ 2020-07-30 22:27 ` Mark Brown
2020-08-03 2:07 ` Shengjiu Wang
3 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2020-07-30 22:27 UTC (permalink / raw)
To: Fabio Estevam; +Cc: alsa-devel, ckeepax, shengjiu.wang, kuninori.morimoto.gx
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-07-17 13:59 [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE Fabio Estevam
` (2 preceding siblings ...)
2020-07-30 22:27 ` Mark Brown
@ 2020-08-03 2:07 ` Shengjiu Wang
2020-08-03 10:51 ` Mark Brown
3 siblings, 1 reply; 10+ messages in thread
From: Shengjiu Wang @ 2020-08-03 2:07 UTC (permalink / raw)
To: Fabio Estevam
Cc: Shengjiu Wang, Mark Brown, Charles Keepax, Linux-ALSA, Kuninori Morimoto
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-08-03 2:07 ` Shengjiu Wang
@ 2020-08-03 10:51 ` Mark Brown
2020-08-03 11:58 ` Fabio Estevam
0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2020-08-03 10:51 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Charles Keepax, Shengjiu Wang, Fabio Estevam, Linux-ALSA,
Kuninori Morimoto
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: wm8962: Do not access WM8962_GPIO_BASE
2020-08-03 10:51 ` Mark Brown
@ 2020-08-03 11:58 ` Fabio Estevam
0 siblings, 0 replies; 10+ messages in thread
From: Fabio Estevam @ 2020-08-03 11:58 UTC (permalink / raw)
To: Mark Brown
Cc: Charles Keepax, Shengjiu Wang, Shengjiu Wang, Linux-ALSA,
Kuninori Morimoto
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.
^ permalink raw reply [flat|nested] 10+ messages in thread