| Message ID | 20220214195716.1096265-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: cs4265: Fix the duplicated control name | expand |
On Mon, Feb 14, 2022 at 04:57:16PM -0300, Fabio Estevam wrote: > From: Fabio Estevam <festevam@denx.de> > Currently, the following error messages are seen during boot: > > asoc-simple-card sound: control 2:0:0:SPDIF Switch:0 is already present > cs4265 1-004f: ASoC: failed to add widget SPDIF dapm kcontrol SPDIF Switch: -16 > > Quoting Mark Brown: > > "The driver is just plain buggy, it defines both a regular SPIDF Switch > control and a SND_SOC_DAPM_SWITCH() called SPDIF both of which will > create an identically named control, it can never have loaded without > error. One or both of those has to be renamed." > > Rename it from "SPDIF Switch" to "SPDIF" to avoid the name confict. > > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > sound/soc/codecs/cs4265.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c > index c338c9f6cf91..8891e9275cba 100644 > --- a/sound/soc/codecs/cs4265.c > +++ b/sound/soc/codecs/cs4265.c > @@ -150,7 +150,7 @@ static const struct snd_kcontrol_new cs4265_snd_controls[] = { > SOC_SINGLE("E to F Buffer Disable Switch", CS4265_SPDIF_CTL1, > 6, 1, 0), > SOC_ENUM("C Data Access", cam_mode_enum), > - SOC_SINGLE("SPDIF Switch", CS4265_SPDIF_CTL2, 5, 1, 1), > + SOC_SINGLE("SPDIF", CS4265_SPDIF_CTL2, 5, 1, 1), Looking through the code I think its probably better to combine the two controls here. It looks like you would need to set both to enable the SPDIF and I don't really see any reason for them to be different. I think you can just move the register bits onto the DAPM widget and have DAPM control them. This patch also probably needs a fixes tag: Fixes: f853d6b3ba34 ("ASoC: cs4265: Add a S/PDIF enable switch") Apologies for missing this in my review of the original patch. Let me know if you want me to have a bash at combining them. Thanks, Charles
Hi Charles, On Tue, Feb 15, 2022 at 7:20 AM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > Looking through the code I think its probably better to combine > the two controls here. It looks like you would need to set both > to enable the SPDIF and I don't really see any reason for them to > be different. I think you can just move the register bits onto > the DAPM widget and have DAPM control them. > > This patch also probably needs a fixes tag: > > Fixes: f853d6b3ba34 ("ASoC: cs4265: Add a S/PDIF enable switch") > > Apologies for missing this in my review of the original patch. > Let me know if you want me to have a bash at combining them. Do you mean something like this? --- a/sound/soc/codecs/cs4265.c +++ b/sound/soc/codecs/cs4265.c @@ -122,7 +122,7 @@ static const struct snd_kcontrol_new loopback_ctl = SOC_DAPM_SINGLE("Switch", CS4265_SIG_SEL, 1, 1, 0); static const struct snd_kcontrol_new spdif_switch = - SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 0, 0); + SOC_DAPM_SINGLE("Switch", CS4265_SPDIF_CTL2, 5, 1, 1); static const struct snd_kcontrol_new dac_switch = SOC_DAPM_SINGLE("Switch", CS4265_PWRCTL, 1, 1, 0); @@ -150,7 +150,6 @@ static const struct snd_kcontrol_new cs4265_snd_controls[] = { SOC_SINGLE("E to F Buffer Disable Switch", CS4265_SPDIF_CTL1, 6, 1, 0), SOC_ENUM("C Data Access", cam_mode_enum), - SOC_SINGLE("SPDIF Switch", CS4265_SPDIF_CTL2, 5, 1, 1), SOC_SINGLE("Validity Bit Control Switch", CS4265_SPDIF_CTL2, 3, 1, 0), SOC_ENUM("SPDIF Mono/Stereo", spdif_mono_stereo_enum), @@ -186,7 +185,7 @@ static const struct snd_soc_dapm_widget cs4265_dapm_widgets[] = { SND_SOC_DAPM_SWITCH("Loopback", SND_SOC_NOPM, 0, 0, &loopback_ctl), - SND_SOC_DAPM_SWITCH("SPDIF", SND_SOC_NOPM, 0, 0, + SND_SOC_DAPM_SWITCH("SPDIF", CS4265_SPDIF_CTL2, 5, 1, &spdif_switch), SND_SOC_DAPM_SWITCH("DAC", CS4265_PWRCTL, 1, 1, &dac_switch), If this is not what you meant, please feel free to submit a proper patch. Thanks
On Tue, Feb 15, 2022 at 07:52:07AM -0300, Fabio Estevam wrote: > On Tue, Feb 15, 2022 at 7:20 AM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > Do you mean something like this? > > --- a/sound/soc/codecs/cs4265.c > +++ b/sound/soc/codecs/cs4265.c > @@ -122,7 +122,7 @@ static const struct snd_kcontrol_new loopback_ctl = > SOC_DAPM_SINGLE("Switch", CS4265_SIG_SEL, 1, 1, 0); > > static const struct snd_kcontrol_new spdif_switch = > - SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 0, 0); > + SOC_DAPM_SINGLE("Switch", CS4265_SPDIF_CTL2, 5, 1, 1); > > static const struct snd_kcontrol_new dac_switch = > SOC_DAPM_SINGLE("Switch", CS4265_PWRCTL, 1, 1, 0); > @@ -150,7 +150,6 @@ static const struct snd_kcontrol_new > cs4265_snd_controls[] = { > SOC_SINGLE("E to F Buffer Disable Switch", CS4265_SPDIF_CTL1, > 6, 1, 0), > SOC_ENUM("C Data Access", cam_mode_enum), > - SOC_SINGLE("SPDIF Switch", CS4265_SPDIF_CTL2, 5, 1, 1), > SOC_SINGLE("Validity Bit Control Switch", CS4265_SPDIF_CTL2, > 3, 1, 0), > SOC_ENUM("SPDIF Mono/Stereo", spdif_mono_stereo_enum), > @@ -186,7 +185,7 @@ static const struct snd_soc_dapm_widget > cs4265_dapm_widgets[] = { > > SND_SOC_DAPM_SWITCH("Loopback", SND_SOC_NOPM, 0, 0, > &loopback_ctl), > - SND_SOC_DAPM_SWITCH("SPDIF", SND_SOC_NOPM, 0, 0, > + SND_SOC_DAPM_SWITCH("SPDIF", CS4265_SPDIF_CTL2, 5, 1, > &spdif_switch), > SND_SOC_DAPM_SWITCH("DAC", CS4265_PWRCTL, 1, 1, > &dac_switch), > > If this is not what you meant, please feel free to submit a proper patch. Yeah that is almost exactly what I meant. Only thing is you don't need to put the register on the both SOC_DAPM_SINGLE and on the SND_SOC_DAPM_SWITCH. Little bit of a judgement call on which of the two to put it on. Putting it on the SOC_DAPM_SINGLE is much closer to the existing code. But putting it on the SND_SOC_DAPM_SWITCH feels more correct to me, ie. only enabling it when the SPDIF is in use. I would suggest this way, as the existing code is clearly not tested so it doesn't feel very valuable to stick to what the existing code does and this seems more correct. Thanks, Charles
diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c index c338c9f6cf91..8891e9275cba 100644 --- a/sound/soc/codecs/cs4265.c +++ b/sound/soc/codecs/cs4265.c @@ -150,7 +150,7 @@ static const struct snd_kcontrol_new cs4265_snd_controls[] = { SOC_SINGLE("E to F Buffer Disable Switch", CS4265_SPDIF_CTL1, 6, 1, 0), SOC_ENUM("C Data Access", cam_mode_enum), - SOC_SINGLE("SPDIF Switch", CS4265_SPDIF_CTL2, 5, 1, 1), + SOC_SINGLE("SPDIF", CS4265_SPDIF_CTL2, 5, 1, 1), SOC_SINGLE("Validity Bit Control Switch", CS4265_SPDIF_CTL2, 3, 1, 0), SOC_ENUM("SPDIF Mono/Stereo", spdif_mono_stereo_enum),