| Message ID | 1397701446-11977-1-git-send-email-bardliao@realtek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8bfc6d2d1b6266e8da2a7cf89e8d05e2ea8b09e5 |
| Headers | show |
On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote: > From: Bard Liao <bardliao@realtek.com> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Signed-off-by: Bard Liao <bardliao@realtek.com> Jarkko, are you OK with this (and the change in general)?
On 04/17/2014 10:01 PM, Mark Brown wrote: > On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote: > >> From: Bard Liao <bardliao@realtek.com> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> Signed-off-by: Bard Liao <bardliao@realtek.com> > Jarkko, are you OK with this (and the change in general)? Yes, it's effectively same as my patch with Bard's actual device ID bits description in reset register.
On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote: > From: Bard Liao <bardliao@realtek.com> > > We have been using rt5640.c codec driver with RT5642 codec chip before commit > 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using > device ID reading in reset register for adding device specific controls and > routes runtime. Applied, thanks. Now I look again and remember that it exists I see there's no update to the DT portions of the code - please submit a followup patch adding the device to the DT bindings and to the OF match table. rt5639 is also missing there.
2014-04-22 19:53 GMT+08:00 Mark Brown <broonie@kernel.org>: > On Thu, Apr 17, 2014 at 10:24:06AM +0800, bardliao@realtek.com wrote: >> From: Bard Liao <bardliao@realtek.com> >> >> We have been using rt5640.c codec driver with RT5642 codec chip before commit >> 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using >> device ID reading in reset register for adding device specific controls and >> routes runtime. > > Applied, thanks. Now I look again and remember that it exists I see > there's no update to the DT portions of the code - please submit a > followup patch adding the device to the DT bindings and to the OF match > table. rt5639 is also missing there. We've sent the patch of binding document to "/Documentation/devicetree/bindings/sound/rt5640.txt" for rt5639. If it is not satisfied, please kindly tell us what we need to do, thank you very much.
On Tue, Apr 22, 2014 at 10:14:47PM +0800, Oder wrote: > > Applied, thanks. Now I look again and remember that it exists I see > > there's no update to the DT portions of the code - please submit a > > followup patch adding the device to the DT bindings and to the OF match > > table. rt5639 is also missing there. > We've sent the patch of binding document to > "/Documentation/devicetree/bindings/sound/rt5640.txt" for rt5639. > If it is not satisfied, please kindly tell us what we need to do, > thank you very much. It's not in the match table in the driver as well, just the binding document isn't enough.
On 04/16/2014 08:24 PM, bardliao@realtek.com wrote: > From: Bard Liao <bardliao@realtek.com> > > We have been using rt5640.c codec driver with RT5642 codec chip before commit > 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using > device ID reading in reset register for adding device specific controls and > routes runtime. > > Now since device ID appears to be different between RT5640 and RT5642 the > driver doesn't add those controls and routes that are valid also on RT5642. > > Fix this by adding a device ID found by debugging and minimal code for > supporting RT5642. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Is this derived from Jarkko's patch? If so, shouldn't he be listed as the author, not you? If it wasn't, then presumably his S-o-b line shouldn't be in the patch description. > Signed-off-by: Bard Liao <bardliao@realtek.com> This patch causes problems for me. I see an enormous amount of spew during kernel boot along the lines of: > [ 2.285515] rt5640 0-001c: ASoC: no source widget found for OUT MIXL > [ 2.291899] rt5640 0-001c: ASoC: Failed to add route OUT MIXL -> OUT MIXL Switch -> RECMIXL > [ 2.300306] rt5640 0-001c: ASoC: no source widget found for OUT MIXR > [ 2.306662] rt5640 0-001c: ASoC: Failed to add route OUT MIXR -> OUT MIXR Switch -> RECMIXR > [ 2.315662] rt5640 0-001c: ASoC: no sink widget found for Stereo DAC MIXL (but repeated for about 28 widget/route pairs) Perhaps it's related to: > diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h > -#define RT5639_RESET_ID 0x0008 > -#define RT5640_RESET_ID 0x000c These values cover at least bits 3:2. > +/* SW Reset & Device ID (0x00) */ > +#define RT5640_ID_MASK (0x3 << 1) > +#define RT5640_ID_5639 (0x0 << 1) > +#define RT5640_ID_5640 (0x1 << 1) > +#define RT5640_ID_5642 (0x3 << 1) whereas these values cover bits 2:1. Should the shift be 2? Even with that fixed, the old 5639 value was 2 << 2 but the new value here is 0 << 2. Similarly, the old 5640 value was 3 <<2 whereas the new value is 1 << 2. There's obviously quite some confusion here. The schematic of my board says I have an RT5640, everything worked before this commit, and register 0 (where these values are stored) reads as 0xc which matches the RT5640 value before this commit but not after. I can see why this patch causes the driver to support the wrong chip. However, I can't imagine why that causes all the log spew at startup. Perhaps the driver is just broken on RT5639 at present (although I don't recall seeing any issues when booting on a board that actually had one...) Is part of the driver keying off this now incorrect ID register read, yet some other part of the driver registering widgets/routes based on which entry matched in struct i2c_device_id rt5640_i2c_id, hence they're falling out of sync due to this change? If so, that seems like another bug that needs fixing.
On Fri, Apr 25, 2014 at 04:08:18PM -0600, Stephen Warren wrote: > On 04/16/2014 08:24 PM, bardliao@realtek.com wrote: > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Is this derived from Jarkko's patch? If so, shouldn't he be listed as > the author, not you? If it wasn't, then presumably his S-o-b line > shouldn't be in the patch description. It was derived but edited; I did check with Jarkko that he was OK before applying. > I can see why this patch causes the driver to support the wrong chip. > However, I can't imagine why that causes all the log spew at startup. > Perhaps the driver is just broken on RT5639 at present (although I don't > recall seeing any issues when booting on a board that actually had > one...) Is part of the driver keying off this now incorrect ID register > read, yet some other part of the driver registering widgets/routes based > on which entry matched in struct i2c_device_id rt5640_i2c_id, hence > they're falling out of sync due to this change? If so, that seems like > another bug that needs fixing. I'd be surprised if it weren't the latter. Either way it needs fixing.
> On 04/16/2014 08:24 PM, bardliao@realtek.com wrote: > > From: Bard Liao <bardliao@realtek.com> > > > > We have been using rt5640.c codec driver with RT5642 codec chip before commit > > 022d21f004c1 ("ASoC: rt5640: add rt5639 support"). That commits starts using > > device ID reading in reset register for adding device specific controls and > > routes runtime. > > > > Now since device ID appears to be different between RT5640 and RT5642 the > > driver doesn't add those controls and routes that are valid also on RT5642. > > > > Fix this by adding a device ID found by debugging and minimal code for > > supporting RT5642. > > > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > > Is this derived from Jarkko's patch? If so, shouldn't he be listed as > the author, not you? If it wasn't, then presumably his S-o-b line > shouldn't be in the patch description. I am sorry. I don't know the meaning of S-o-b's order. > > > Signed-off-by: Bard Liao <bardliao@realtek.com> > > This patch causes problems for me. I see an enormous amount of spew > during kernel boot along the lines of: > > > [ 2.285515] rt5640 0-001c: ASoC: no source widget found for OUT MIXL > > [ 2.291899] rt5640 0-001c: ASoC: Failed to add route OUT MIXL -> OUT MIXL Switch -> RECMIXL > > [ 2.300306] rt5640 0-001c: ASoC: no source widget found for OUT MIXR > > [ 2.306662] rt5640 0-001c: ASoC: Failed to add route OUT MIXR -> OUT MIXR Switch -> RECMIXR > > [ 2.315662] rt5640 0-001c: ASoC: no sink widget found for Stereo DAC MIXL > > (but repeated for about 28 widget/route pairs) > > Perhaps it's related to: > > > diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h > > > -#define RT5639_RESET_ID 0x0008 > > -#define RT5640_RESET_ID 0x000c > > These values cover at least bits 3:2. Actually, only bits 2:1 is related to device ID. Others are for other informations. > > > +/* SW Reset & Device ID (0x00) */ > > +#define RT5640_ID_MASK (0x3 << 1) > > +#define RT5640_ID_5639 (0x0 << 1) > > +#define RT5640_ID_5640 (0x1 << 1) > > +#define RT5640_ID_5642 (0x3 << 1) It's my fault. 5640's ID should be (0x2 << 1) instead (0x1 << 1). > > whereas these values cover bits 2:1. Should the shift be 2? Even with > that fixed, the old 5639 value was 2 << 2 but the new value here is 0 << > 2. Similarly, the old 5640 value was 3 <<2 whereas the new value is 1 << > 2. There's obviously quite some confusion here. > > The schematic of my board says I have an RT5640, everything worked > before this commit, and register 0 (where these values are stored) reads > as 0xc which matches the RT5640 value before this commit but not after. > > I can see why this patch causes the driver to support the wrong chip. > However, I can't imagine why that causes all the log spew at startup. > Perhaps the driver is just broken on RT5639 at present (although I don't > recall seeing any issues when booting on a board that actually had > one...) Is part of the driver keying off this now incorrect ID register > read, yet some other part of the driver registering widgets/routes based > on which entry matched in struct i2c_device_id rt5640_i2c_id, hence > they're falling out of sync due to this change? If so, that seems like > another bug that needs fixing. We will check and fix it. Thanks. > > ------Please consider the environment before printing this e-mail.
On 04/26/2014 03:22 AM, Mark Brown wrote: > On Fri, Apr 25, 2014 at 04:08:18PM -0600, Stephen Warren wrote: >> On 04/16/2014 08:24 PM, bardliao@realtek.com wrote: >>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> Is this derived from Jarkko's patch? If so, shouldn't he be listed as >> the author, not you? If it wasn't, then presumably his S-o-b line >> shouldn't be in the patch description. > It was derived but edited; I did check with Jarkko that he was OK before > applying. It was ok to me since derivate had more information (device ID bit descriptions) than my patch.
On Sun, Apr 27, 2014 at 02:57:24PM +0000, Bard Liao wrote: > > On 04/16/2014 08:24 PM, bardliao@realtek.com wrote: > > Is this derived from Jarkko's patch? If so, shouldn't he be listed as > > the author, not you? If it wasn't, then presumably his S-o-b line > > shouldn't be in the patch description. > I am sorry. I don't know the meaning of S-o-b's order. It's not just the ordering, it's also the authorship information - signed off by has a specific meaning about who wrote the code and handing off responsibility for it which is important to licensing. Adding a signoff for someone who didn't write the code is therefore particularly problematic.
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 6674372..79635ee 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -1997,8 +1997,9 @@ static int rt5640_probe(struct snd_soc_codec *codec) snd_soc_update_bits(codec, RT5640_MICBIAS, 0x0030, 0x0030); snd_soc_update_bits(codec, RT5640_DSP_PATH2, 0xfc00, 0x0c00); - switch (snd_soc_read(codec, RT5640_RESET)) { - case RT5640_RESET_ID: + switch (snd_soc_read(codec, RT5640_RESET) & RT5640_ID_MASK) { + case RT5640_ID_5640: + case RT5640_ID_5642: snd_soc_add_codec_controls(codec, rt5640_specific_snd_controls, ARRAY_SIZE(rt5640_specific_snd_controls)); @@ -2009,7 +2010,7 @@ static int rt5640_probe(struct snd_soc_codec *codec) rt5640_specific_dapm_routes, ARRAY_SIZE(rt5640_specific_dapm_routes)); break; - case RT5639_RESET_ID: + case RT5640_ID_5639: snd_soc_dapm_new_controls(&codec->dapm, rt5639_specific_dapm_widgets, ARRAY_SIZE(rt5639_specific_dapm_widgets)); @@ -2149,6 +2150,7 @@ static const struct regmap_config rt5640_regmap = { static const struct i2c_device_id rt5640_i2c_id[] = { { "rt5640", 0 }, { "rt5639", 0 }, + { "rt5642", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, rt5640_i2c_id); diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h index 3b50459..ded2059 100644 --- a/sound/soc/codecs/rt5640.h +++ b/sound/soc/codecs/rt5640.h @@ -14,9 +14,6 @@ #include <sound/rt5640.h> -#define RT5639_RESET_ID 0x0008 -#define RT5640_RESET_ID 0x000c - /* Info */ #define RT5640_RESET 0x00 #define RT5640_VENDOR_ID 0xfd @@ -195,6 +192,13 @@ #define RT5640_R_VOL_MASK (0x3f) #define RT5640_R_VOL_SFT 0 +/* SW Reset & Device ID (0x00) */ +#define RT5640_ID_MASK (0x3 << 1) +#define RT5640_ID_5639 (0x0 << 1) +#define RT5640_ID_5640 (0x1 << 1) +#define RT5640_ID_5642 (0x3 << 1) + + /* IN1 and IN2 Control (0x0d) */ /* IN3 and IN4 Control (0x0e) */ #define RT5640_BST_SFT1 12