| Message ID | 20200717135959.19212-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: wm8962: Do not access WM8962_GPIO_BASE | expand |
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 >
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
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
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
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!
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
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 >
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 :/
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.
--- 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:
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: