Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754187AbbL0SWJ (ORCPT ); Sun, 27 Dec 2015 13:22:09 -0500 Received: from down.free-electrons.com ([37.187.137.238]:56220 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752815AbbL0SWB (ORCPT ); Sun, 27 Dec 2015 13:22:01 -0500 Date: Sun, 27 Dec 2015 19:21:57 +0100 From: Maxime Ripard To: Danny Milosavljevic Cc: Mark Brown , Chen-Yu Tsai , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, alsa-devel@alsa-project.org, Jaroslav Kysela , Takashi Iwai , Liam Girdwood , linux-sunxi@googlegroups.com Subject: Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line and Mic inputs Message-ID: <20151227182157.GI30359@lukather> References: <20151221123148.1ab6480b@dayas> <20151221123416.51aa938e@dayas> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="XK4NhNJ9GVfup2vH" Content-Disposition: inline In-Reply-To: <20151221123416.51aa938e@dayas> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14184 Lines: 393 --XK4NhNJ9GVfup2vH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Mon, Dec 21, 2015 at 12:34:16PM +0100, Danny Milosavljevic wrote: > This is the second part, actually adding FM, Line and Mic inputs. Again, having a meaningful and standalone commit log would be nice. > Signed-off-by: Danny Milosavljevic > --- > b/sound/soc/sunxi/sun4i-codec.c | 185 +++++++++++++++++++++++++++++++++= ++++++- > 1 file changed, 181 insertions(+), 4 deletions(-) >=20 > diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c > index 6628e6e..f23d8a9 100644 > --- a/sound/soc/sunxi/sun4i-codec.c > +++ b/sound/soc/sunxi/sun4i-codec.c > @@ -59,9 +59,20 @@ > #define SUN4I_CODEC_DAC_ACTL_DACAENR (31) > #define SUN4I_CODEC_DAC_ACTL_DACAENL (30) > #define SUN4I_CODEC_DAC_ACTL_MIXEN (29) > +#define SUN4I_CODEC_DAC_ACTL_LNG (26) > +#define SUN4I_CODEC_DAC_ACTL_FMG (23) > +#define SUN4I_CODEC_DAC_ACTL_MICG (20) > +#define SUN4I_CODEC_DAC_ACTL_LLNS (19) > +#define SUN4I_CODEC_DAC_ACTL_RLNS (18) > +#define SUN4I_CODEC_DAC_ACTL_LFMS (17) > +#define SUN4I_CODEC_DAC_ACTL_RFMS (16) > #define SUN4I_CODEC_DAC_ACTL_LDACLMIXS (15) > #define SUN4I_CODEC_DAC_ACTL_RDACRMIXS (14) > #define SUN4I_CODEC_DAC_ACTL_LDACRMIXS (13) > +#define SUN4I_CODEC_DAC_ACTL_MIC1LS (12) > +#define SUN4I_CODEC_DAC_ACTL_MIC1RS (11) > +#define SUN4I_CODEC_DAC_ACTL_MIC2LS (10) > +#define SUN4I_CODEC_DAC_ACTL_MIC2RS (9) > #define SUN4I_CODEC_DAC_ACTL_DACPAS (8) > #define SUN4I_CODEC_DAC_ACTL_MIXPAS (7) > #define SUN4I_CODEC_DAC_ACTL_PA_MUTE (6) > @@ -87,8 +98,11 @@ > #define SUN4I_CODEC_ADC_ACTL_PREG1EN (29) > #define SUN4I_CODEC_ADC_ACTL_PREG2EN (28) > #define SUN4I_CODEC_ADC_ACTL_VMICEN (27) > -#define SUN4I_CODEC_ADC_ACTL_VADCG (20) > +#define SUN4I_CODEC_ADC_ACTL_PREG1_A10 (25) > +#define SUN4I_CODEC_ADC_ACTL_PREG2_A10 (23) Why the A10 suffix? > +#define SUN4I_CODEC_ADC_ACTL_ADCG (20) > #define SUN4I_CODEC_ADC_ACTL_ADCIS (17) > +#define SUN4I_CODEC_ADC_ACTL_LNRDF (16) > #define SUN4I_CODEC_ADC_ACTL_PA_EN (4) > #define SUN4I_CODEC_ADC_ACTL_DDE (3) > #define SUN4I_CODEC_ADC_DEBUG (0x2c) > @@ -100,6 +114,16 @@ > #define SUN7I_CODEC_AC_DAC_CAL (0x38) > #define SUN7I_CODEC_AC_MIC_PHONE_CAL (0x3c) > =20 > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1 (29) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2 (26) > +/* note: no idea where the output pins for the following are. */ > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTG (5) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTEN (4) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTS3 (3) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTS2 (2) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTS1 (1) > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PHONEOUTS0 (0) Are you using that PHONEOUT stuff anywhere? > + > struct sun4i_codec { > struct device *dev; > struct regmap *regmap; > @@ -509,19 +533,102 @@ static const struct snd_kcontrol_new sun4i_codec_p= a_mute =3D > SUN4I_CODEC_DAC_ACTL_PA_MUTE, 1, 0); > =20 > static DECLARE_TLV_DB_SCALE(sun4i_codec_pa_volume_scale, -6300, 100, 1); > +static DECLARE_TLV_DB_SCALE(sun4i_codec_linein_loopback_gain_scale, > + -150, > + 150, > + 0); > +static DECLARE_TLV_DB_SCALE(sun4i_codec_fmin_loopback_gain_scale, > + -450, > + 150, > + 0); > +static DECLARE_TLV_DB_SCALE(sun4i_codec_micin_loopback_gain_scale, > + -450, > + 150, > + 0); > +static DECLARE_TLV_DB_RANGE(sun4i_codec_micin_preamp_gain_scale_a10, > + 0, 0, TLV_DB_SCALE_ITEM(0, 0, 0), > + 1, 7, TLV_DB_SCALE_ITEM(3500, 300, 0)); > +static DECLARE_TLV_DB_SCALE(sun4i_codec_adc_gain_scale, -450, 150, 0); > +/* Sources: > + * A10 User Manual v1.5 20130820 > + * A20 User Manual v1.4 20150510 > + */ There's no need to give your sources here. > +static const char * const sun4i_codec_capture_source[] =3D { > + "Line-In", > + "FM", > + "Mic1", > + "Mic2", > + "Mic1,Mic2", > + "Mic1+Mic2", > + "Output Mixer", > + "Line-In,Mic1", > +}; > +static SOC_ENUM_SINGLE_DECL(sun4i_codec_enum_capture_source, > + SUN4I_CODEC_ADC_ACTL, > + SUN4I_CODEC_ADC_ACTL_ADCIS, > + sun4i_codec_capture_source); Isn't it possible to expose this as two (shared) muxes with different names to make it clear what will go to the left ADC and what will go to the right? > +static const struct snd_kcontrol_new sun4i_codec_capture_source_controls= =3D > + SOC_DAPM_ENUM("Route", sun4i_codec_enum_capture_source); > =20 > #define SUN4I_COMMON_CODEC_WIDGETS \ > - SOC_SINGLE_TLV("Power Amplifier Volume", SUN4I_CODEC_DAC_ACTL,\ > - SUN4I_CODEC_DAC_ACTL_PA_VOL, 0x3F, 0,\ > - sun4i_codec_pa_volume_scale) > + SOC_SINGLE_TLV("Power Amplifier Playback Volume", SUN4I_CODEC_DAC_ACTL,\ > + SUN4I_CODEC_DAC_ACTL_PA_VOL, 0x3F, 0, \ > + sun4i_codec_pa_volume_scale), \ The power amplifier is only for the playback, so there's no need to differentiate between playback and capture. The current name was fine. > + /* Line-In, FM, Mic1, Mic2 */ \ > + SOC_SINGLE_TLV("Line-In Playback Volume", \ > + SUN4I_CODEC_DAC_ACTL, \ > + SUN4I_CODEC_DAC_ACTL_LNG, \ > + 1, \ > + 0, \ > + sun4i_codec_linein_loopback_gain_scale), \ > + SOC_SINGLE_TLV("FM Playback Volume", \ > + SUN4I_CODEC_DAC_ACTL, \ > + SUN4I_CODEC_DAC_ACTL_FMG, \ > + 3, \ > + 0, \ > + sun4i_codec_fmin_loopback_gain_scale), \ > + SOC_SINGLE_TLV("Mic Playback Volume", \ > + SUN4I_CODEC_DAC_ACTL, \ > + SUN4I_CODEC_DAC_ACTL_MICG, \ > + 7, \ > + 0, \ > + sun4i_codec_micin_loopback_gain_scale), \ Those are not volume it's gain, and it should probably be two different shared controls for mic1 and mic2. > + /* ADC */ \ > + SOC_SINGLE_TLV("Capture Volume", \ > + SUN4I_CODEC_ADC_ACTL, \ > + SUN4I_CODEC_ADC_ACTL_ADCG, \ > + 4, \ > + 0, \ > + sun4i_codec_adc_gain_scale) This one is the ADC Gain. Please name it that way. > static const struct snd_kcontrol_new sun4i_codec_widgets_a10[] =3D { > SUN4I_COMMON_CODEC_WIDGETS, > + SOC_SINGLE_TLV("Mic1 Capture Volume", > + SUN4I_CODEC_ADC_ACTL, > + SUN4I_CODEC_ADC_ACTL_PREG1_A10, > + 3, > + 0, > + sun4i_codec_micin_preamp_gain_scale_a10), > + SOC_SINGLE_TLV("Mic2 Capture Volume", > + SUN4I_CODEC_ADC_ACTL, > + SUN4I_CODEC_ADC_ACTL_PREG2_A10, > + 3, > + 0, > + sun4i_codec_micin_preamp_gain_scale_a10), And those two are not capture volume, it's the pre-amplifier gain. > }; > =20 > static const struct snd_kcontrol_new sun4i_codec_left_mixer_controls[] = =3D { > SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL, > SUN4I_CODEC_DAC_ACTL_LDACLMIXS, 1, 0), > + SOC_DAPM_SINGLE("Left Line-In Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_LLNS, 1, 0), > + SOC_DAPM_SINGLE("Left FM Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_LFMS, 1, 0), > + SOC_DAPM_SINGLE("Mic1 Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_MIC1LS, 1, 0), > + SOC_DAPM_SINGLE("Mic2 Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_MIC2LS, 1, 0), > }; > =20 > static const struct snd_kcontrol_new sun4i_codec_right_mixer_controls[] = =3D { > @@ -529,6 +636,14 @@ static const struct snd_kcontrol_new sun4i_codec_rig= ht_mixer_controls[] =3D { > SUN4I_CODEC_DAC_ACTL_RDACRMIXS, 1, 0), > SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL, > SUN4I_CODEC_DAC_ACTL_LDACRMIXS, 1, 0), > + SOC_DAPM_SINGLE("Right Line-In Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_RLNS, 1, 0), > + SOC_DAPM_SINGLE("Right FM Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_RFMS, 1, 0), > + SOC_DAPM_SINGLE("Mic1 Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_MIC1RS, 1, 0), > + SOC_DAPM_SINGLE("Mic2 Playback Switch", SUN4I_CODEC_DAC_ACTL, > + SUN4I_CODEC_DAC_ACTL_MIC2RS, 1, 0), > }; > =20 > static const struct snd_kcontrol_new sun4i_codec_pa_mixer_controls[] =3D= { > @@ -561,6 +676,10 @@ static const struct snd_soc_dapm_widget sun4i_codec_= codec_dapm_widgets[] =3D { > SND_SOC_DAPM_DAC("Right DAC", "Codec Playback", SUN4I_CODEC_DAC_ACTL, > SUN4I_CODEC_DAC_ACTL_DACAENR, 0), > =20 > + /* MUX */ > + SND_SOC_DAPM_MUX("Capture Source", SND_SOC_NOPM, 0, 0, > + &sun4i_codec_capture_source_controls), > + Please call it "ADC Source" > /* Mixers */ > SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0, > sun4i_codec_left_mixer_controls, > @@ -580,6 +699,8 @@ static const struct snd_soc_dapm_widget sun4i_codec_c= odec_dapm_widgets[] =3D { > /* Mic Pre-Amplifiers */ > SND_SOC_DAPM_PGA("MIC1 Pre-Amplifier", SUN4I_CODEC_ADC_ACTL, > SUN4I_CODEC_ADC_ACTL_PREG1EN, 0, NULL, 0), > + SND_SOC_DAPM_PGA("MIC2 Pre-Amplifier", SUN4I_CODEC_ADC_ACTL, > + SUN4I_CODEC_ADC_ACTL_PREG2EN, 0, NULL, 0), > =20 > /* Power Amplifier */ > SND_SOC_DAPM_MIXER("Power Amplifier", SUN4I_CODEC_ADC_ACTL, > @@ -590,9 +711,15 @@ static const struct snd_soc_dapm_widget sun4i_codec_= codec_dapm_widgets[] =3D { > &sun4i_codec_pa_mute), > =20 > SND_SOC_DAPM_INPUT("Mic1"), > + SND_SOC_DAPM_INPUT("Mic2"), > =20 > SND_SOC_DAPM_OUTPUT("HP Right"), > SND_SOC_DAPM_OUTPUT("HP Left"), > + > + SND_SOC_DAPM_INPUT("Line-In Right"), > + SND_SOC_DAPM_INPUT("Line-In Left"), > + SND_SOC_DAPM_INPUT("FM Right"), > + SND_SOC_DAPM_INPUT("FM Left"), Having all the inputs and all the outputs grouped together would be nice. > }; > =20 > static const struct snd_soc_dapm_route sun4i_codec_codec_dapm_routes[] = =3D { > @@ -629,6 +756,39 @@ static const struct snd_soc_dapm_route sun4i_codec_c= odec_dapm_routes[] =3D { > { "Right ADC", NULL, "MIC1 Pre-Amplifier" }, > { "MIC1 Pre-Amplifier", NULL, "Mic1"}, > { "Mic1", NULL, "VMIC" }, > + { "Right Mixer", "Mic1 Playback Switch", "MIC1 Pre-Amplifier" }, > + { "Left Mixer", "Mic1 Playback Switch", "MIC1 Pre-Amplifier" }, > + The right and left mixer routes should be in the matching sections. > + /* Mic2 Routes */ > + { "Left ADC", NULL, "MIC2 Pre-Amplifier" }, > + { "Right ADC", NULL, "MIC2 Pre-Amplifier" }, > + { "MIC2 Pre-Amplifier", NULL, "Mic2"}, > + { "Mic2", NULL, "VMIC" }, > + { "Right Mixer", "Mic2 Playback Switch", "MIC2 Pre-Amplifier" }, > + { "Left Mixer", "Mic2 Playback Switch", "MIC2 Pre-Amplifier" }, Ditto. > + > + /* Line-In, FM Routes */ > + { "Right Mixer", "Right Line-In Playback Switch", "Line-In Right" }, > + { "Left Mixer", "Left Line-In Playback Switch", "Line-In Left" }, > + { "Right Mixer", "Right FM Playback Switch", "FM Right" }, > + { "Left Mixer", "Left FM Playback Switch", "FM Left" }, Ditto. > + > + /* ADC Capturing Sources */ "ADC Input Routes" > + {"Capture Source", "Line-In", "Line-In Left"}, > + {"Capture Source", "Line-In", "Line-In Right"}, > + {"Capture Source", "FM", "FM Left"}, > + {"Capture Source", "FM", "FM Right"}, > + {"Capture Source", "Mic1", "MIC1 Pre-Amplifier"}, > + {"Capture Source", "Mic2", "MIC2 Pre-Amplifier"}, > + {"Capture Source", "Mic1,Mic2", "MIC1 Pre-Amplifier"}, > + {"Capture Source", "Mic1,Mic2", "MIC2 Pre-Amplifier"}, > + {"Capture Source", "Mic1+Mic2", "MIC1 Pre-Amplifier"}, > + {"Capture Source", "Mic1+Mic2", "MIC2 Pre-Amplifier"}, > + {"Capture Source", "Output Mixer", "Left Mixer"}, > + {"Capture Source", "Output Mixer", "Right Mixer"}, > + {"Capture Source", "Line-In,Mic1", "Line-In Left"}, > + {"Capture Source", "Line-In,Mic1", "Line-In Right"}, /* depends on LNRD= F */ > + {"Capture Source", "Line-In,Mic1", "MIC1 Pre-Amplifier"}, > }; You should also have routes from the ADC Input to the Left and Right ADCs > static struct snd_soc_codec_driver sun4i_codec_codec_a10 =3D { > @@ -757,8 +917,25 @@ static struct snd_soc_card *sun4i_codec_create_card(= struct device *dev) > return card; > }; > =20 > +static DECLARE_TLV_DB_RANGE(sun7i_codec_micin_preamp_gain_scale, > + 0, 0, TLV_DB_SCALE_ITEM(0, 0, 0), > + 1, 7, TLV_DB_SCALE_ITEM(2400, 300, 0) > +); > + > static const struct snd_kcontrol_new sun7i_codec_widgets[] =3D { > SUN4I_COMMON_CODEC_WIDGETS, > + SOC_SINGLE_TLV("Mic1 Capture Volume", > + SUN7I_CODEC_AC_MIC_PHONE_CAL, > + SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1, > + 7, > + 0, > + sun7i_codec_micin_preamp_gain_scale), > + SOC_SINGLE_TLV("Mic2 Capture Volume", > + SUN7I_CODEC_AC_MIC_PHONE_CAL, > + SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2, > + 7, > + 0, > + sun7i_codec_micin_preamp_gain_scale), "Mic1 Pre-Amplifier Gain", "Mic2 Pre-Amplifier Gain". Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --XK4NhNJ9GVfup2vH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWgCxFAAoJEBx+YmzsjxAgIbkP/3+ePKz/l3QdOlkiW6bRTxQo VCHVg4hgUyEFKS1a8EBQZt38qRQUhJkruvNYTJmilLHIAMNKjfF8Kf99osT48LyA e+fcuBenGaO4D/SjqMB8djrlb7RpQbaZlmq1NLEwPLsrCR3NNhlAXbxvgUYnGvSa jsDXxNfeclmVMkUTfnYHLaIdVWyMQAKdmM7CebHSgv2Q3IGTSmHuSUllyCG2vlpM 0OCGGhgZP32AFDiM4qXyfjIZ4e/JiEpFXfPnZdmi/i9+QqbRoe0CF/dTAMaFTzH3 9GTs75H1537Pvj6r9NG13lhfk9S2HOAiqvQuo77gqOqbsGxGeirEpEypICqtvY9D Q1WOkvm3908qUYsv+rzfzXezmisycVLjPU6hiNjaJWh27U/cHiLdrpFtSvP31jZR rGtYnBWXW9a3XXvEboPNZQkyELxQPjepxbSrfBmCl0SvWZNOjiEkyiM/+4EcESBe duNKWgYDUc/pQhl1V02IKKp6rYs2vUNeYLp7QBk4wYtKBoc9R7k8ZS5Y9tmbLsPQ Gv4+tNjGbxkG+Zz/canNwzsDIsFMEEIeJ1M2oZMRwaAcY6O0x1KkFl3QbQRWkVno h3Cn5NKgL93z29U7m7NIjef2Ob3HwKHbOIYXDfmf0FkcIbAOgzcVzfZXXBlrph5/ bLKO8ylML9hbFMdkmSvA =00rM -----END PGP SIGNATURE----- --XK4NhNJ9GVfup2vH-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/