* [PATCH] ASoC: wm_hubs: Fix DC Servo readback @ 2014-08-13 10:47 Nikesh Oswal 2014-08-13 11:08 ` Charles Keepax 0 siblings, 1 reply; 6+ messages in thread From: Nikesh Oswal @ 2014-08-13 10:47 UTC (permalink / raw) To: broonie, lgirdwood; +Cc: perex, tiwai, alsa-devel, linux-kernel, patches wm_hubs is a common driver code shared by WM8958, WM1811, WM8944 and WM8993 all these codecs have the DC Servo Values either in register 57h or 59h. Current code was reading register 58h for WM8958 and WM8994 revisions 2 and 3 but looking at the data sheet the DC Servo Values are stored in register 57h for these codecs. This patch fixes it by reading the correct register for DC Servo Signed-off-by: Nikesh Oswal <nikesh@opensource.wolfsonmicro.com> --- sound/soc/codecs/wm_hubs.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c index 916817f..95c5d71 100644 --- a/sound/soc/codecs/wm_hubs.c +++ b/sound/soc/codecs/wm_hubs.c @@ -210,9 +210,6 @@ static int wm_hubs_read_dc_servo(struct snd_soc_codec *codec, case 2: dcs_reg = WM8994_DC_SERVO_4E; break; - case 1: - dcs_reg = WM8994_DC_SERVO_READBACK; - break; default: dcs_reg = WM8993_DC_SERVO_3; break; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: wm_hubs: Fix DC Servo readback 2014-08-13 10:47 [PATCH] ASoC: wm_hubs: Fix DC Servo readback Nikesh Oswal @ 2014-08-13 11:08 ` Charles Keepax 2014-08-13 11:38 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Charles Keepax @ 2014-08-13 11:08 UTC (permalink / raw) To: Nikesh Oswal Cc: broonie, lgirdwood, tiwai, alsa-devel, patches, linux-kernel, perex On Wed, Aug 13, 2014 at 11:47:20AM +0100, Nikesh Oswal wrote: > wm_hubs is a common driver code shared by WM8958, WM1811, WM8944 > and WM8993 all these codecs have the DC Servo Values either in register > 57h or 59h. Current code was reading register 58h for WM8958 and WM8994 > revisions 2 and 3 but looking at the data sheet the DC Servo Values are > stored in register 57h for these codecs. This patch fixes it by reading > the correct register for DC Servo > > Signed-off-by: Nikesh Oswal <nikesh@opensource.wolfsonmicro.com> > --- > sound/soc/codecs/wm_hubs.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c > index 916817f..95c5d71 100644 > --- a/sound/soc/codecs/wm_hubs.c > +++ b/sound/soc/codecs/wm_hubs.c > @@ -210,9 +210,6 @@ static int wm_hubs_read_dc_servo(struct snd_soc_codec *codec, > case 2: > dcs_reg = WM8994_DC_SERVO_4E; > break; > - case 1: > - dcs_reg = WM8994_DC_SERVO_READBACK; > - break; > default: > dcs_reg = WM8993_DC_SERVO_3; > break; > -- This doesn't look right, firstly if it is the same register for all versions then surely we should change the code that sets dcs_readback_mode rather than setting that to different values but treating them the same. Although obviously if nothing still uses this case we could remove it as well. Also I think the situation is more complex for example on version 4.4 of the datasheet for wm8994 the WR_VAL fields appear to be in register 59h. Which is not consistent with this. Thanks, Charles ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: wm_hubs: Fix DC Servo readback 2014-08-13 11:08 ` Charles Keepax @ 2014-08-13 11:38 ` Mark Brown 2014-08-13 12:16 ` Charles Keepax 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2014-08-13 11:38 UTC (permalink / raw) To: Charles Keepax Cc: Nikesh Oswal, lgirdwood, tiwai, alsa-devel, patches, linux-kernel, perex [-- Attachment #1: Type: text/plain, Size: 1015 bytes --] On Wed, Aug 13, 2014 at 12:08:44PM +0100, Charles Keepax wrote: > On Wed, Aug 13, 2014 at 11:47:20AM +0100, Nikesh Oswal wrote: > > case 2: > > dcs_reg = WM8994_DC_SERVO_4E; > > break; > > - case 1: > > - dcs_reg = WM8994_DC_SERVO_READBACK; > > - break; > > default: > > dcs_reg = WM8993_DC_SERVO_3; > > break; > This doesn't look right, firstly if it is the same register for > all versions then surely we should change the code that sets > dcs_readback_mode rather than setting that to different values > but treating them the same. Although obviously if nothing still > uses this case we could remove it as well. > Also I think the situation is more complex for example on version > 4.4 of the datasheet for wm8994 the WR_VAL fields appear to be in > register 59h. Which is not consistent with this. There was a change in the DC servo between revisions of the WM8994 (at revision E from the look of the code). This isn't documented in the datasheets as they only document current silicon. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: wm_hubs: Fix DC Servo readback 2014-08-13 11:38 ` Mark Brown @ 2014-08-13 12:16 ` Charles Keepax 2014-08-13 12:39 ` Charles Keepax 2014-08-13 12:40 ` Mark Brown 0 siblings, 2 replies; 6+ messages in thread From: Charles Keepax @ 2014-08-13 12:16 UTC (permalink / raw) To: Mark Brown Cc: Nikesh Oswal, lgirdwood, tiwai, alsa-devel, patches, linux-kernel, perex On Wed, Aug 13, 2014 at 12:38:41PM +0100, Mark Brown wrote: > On Wed, Aug 13, 2014 at 12:08:44PM +0100, Charles Keepax wrote: > > On Wed, Aug 13, 2014 at 11:47:20AM +0100, Nikesh Oswal wrote: > > > > case 2: > > > dcs_reg = WM8994_DC_SERVO_4E; > > > break; > > > - case 1: > > > - dcs_reg = WM8994_DC_SERVO_READBACK; > > > - break; > > > default: > > > dcs_reg = WM8993_DC_SERVO_3; > > > break; > > > This doesn't look right, firstly if it is the same register for > > all versions then surely we should change the code that sets > > dcs_readback_mode rather than setting that to different values > > but treating them the same. Although obviously if nothing still > > uses this case we could remove it as well. > > > Also I think the situation is more complex for example on version > > 4.4 of the datasheet for wm8994 the WR_VAL fields appear to be in > > register 59h. Which is not consistent with this. > > There was a change in the DC servo between revisions of the WM8994 (at > revision E from the look of the code). This isn't documented in the > datasheets as they only document current silicon. Indeed, but this patch sets all revs of wm8994 to use register 57h. The lastest version of the datasheet has this register at 59h although some of the older versions of the datasheet (which likely match some older revs of the chip although no way to tell which one just from the datasheet) have it at 57h. I think basically we need to get some clarity from hardware here on which revs use which address and update this patch to match, but either way it looks likely that this patch doesn't address the whole picture. Thanks, Charles ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: wm_hubs: Fix DC Servo readback 2014-08-13 12:16 ` Charles Keepax @ 2014-08-13 12:39 ` Charles Keepax 2014-08-13 12:40 ` Mark Brown 1 sibling, 0 replies; 6+ messages in thread From: Charles Keepax @ 2014-08-13 12:39 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, tiwai, patches, lgirdwood, Nikesh Oswal, linux-kernel, perex On Wed, Aug 13, 2014 at 01:16:37PM +0100, Charles Keepax wrote: > On Wed, Aug 13, 2014 at 12:38:41PM +0100, Mark Brown wrote: > > On Wed, Aug 13, 2014 at 12:08:44PM +0100, Charles Keepax wrote: > > > On Wed, Aug 13, 2014 at 11:47:20AM +0100, Nikesh Oswal wrote: > > > > > > case 2: > > > > dcs_reg = WM8994_DC_SERVO_4E; > > > > break; > > > > - case 1: > > > > - dcs_reg = WM8994_DC_SERVO_READBACK; > > > > - break; > > > > default: > > > > dcs_reg = WM8993_DC_SERVO_3; > > > > break; > > > > > This doesn't look right, firstly if it is the same register for > > > all versions then surely we should change the code that sets > > > dcs_readback_mode rather than setting that to different values > > > but treating them the same. Although obviously if nothing still > > > uses this case we could remove it as well. > > > > > Also I think the situation is more complex for example on version > > > 4.4 of the datasheet for wm8994 the WR_VAL fields appear to be in > > > register 59h. Which is not consistent with this. > > > > There was a change in the DC servo between revisions of the WM8994 (at > > revision E from the look of the code). This isn't documented in the > > datasheets as they only document current silicon. > > Indeed, but this patch sets all revs of wm8994 to use register 57h. > The lastest version of the datasheet has this register at 59h > although some of the older versions of the datasheet (which likely > match some older revs of the chip although no way to tell which one > just from the datasheet) have it at 57h. > > I think basically we need to get some clarity from hardware here > on which revs use which address and update this patch to match, > but either way it looks likely that this patch doesn't address > the whole picture. Sorry no ignore me I am talking non-sense here. It still treats the newer revs correctly, I suspect this might be a nice change though: --- a/sound/soc/codecs/wm_hubs.c +++ b/sound/soc/codecs/wm_hubs.c @@ -209,7 +209,7 @@ static int wm_hubs_read_dc_servo(struct snd_soc_codec *codec, dcs_reg = WM8994_DC_SERVO_4E; break; case 1: - dcs_reg = WM8994_DC_SERVO_READBACK; + dcs_reg = WM8994_DC_SERVO_4; break; Thanks, Charles ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: wm_hubs: Fix DC Servo readback 2014-08-13 12:16 ` Charles Keepax 2014-08-13 12:39 ` Charles Keepax @ 2014-08-13 12:40 ` Mark Brown 1 sibling, 0 replies; 6+ messages in thread From: Mark Brown @ 2014-08-13 12:40 UTC (permalink / raw) To: Charles Keepax Cc: Nikesh Oswal, lgirdwood, tiwai, alsa-devel, patches, linux-kernel, perex [-- Attachment #1: Type: text/plain, Size: 1348 bytes --] On Wed, Aug 13, 2014 at 01:16:37PM +0100, Charles Keepax wrote: > On Wed, Aug 13, 2014 at 12:38:41PM +0100, Mark Brown wrote: > > On Wed, Aug 13, 2014 at 12:08:44PM +0100, Charles Keepax wrote: > > > Also I think the situation is more complex for example on version > > > 4.4 of the datasheet for wm8994 the WR_VAL fields appear to be in > > > register 59h. Which is not consistent with this. > > There was a change in the DC servo between revisions of the WM8994 (at > > revision E from the look of the code). This isn't documented in the > > datasheets as they only document current silicon. > Indeed, but this patch sets all revs of wm8994 to use register 57h. > The lastest version of the datasheet has this register at 59h > although some of the older versions of the datasheet (which likely > match some older revs of the chip although no way to tell which one > just from the datasheet) have it at 57h. Yes, I'm agreeing with you - I'm pointing out what Nikesh has missed (you can see this from the changelogs as well). The patch will break anything using older devices. > I think basically we need to get some clarity from hardware here > on which revs use which address and update this patch to match, > but either way it looks likely that this patch doesn't address > the whole picture. This code got rather a lot of attention... [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-13 12:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-13 10:47 [PATCH] ASoC: wm_hubs: Fix DC Servo readback Nikesh Oswal 2014-08-13 11:08 ` Charles Keepax 2014-08-13 11:38 ` Mark Brown 2014-08-13 12:16 ` Charles Keepax 2014-08-13 12:39 ` Charles Keepax 2014-08-13 12:40 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.