From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752199AbcAFWJN (ORCPT ); Wed, 6 Jan 2016 17:09:13 -0500 Received: from down.free-electrons.com ([37.187.137.238]:59979 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751905AbcAFWJL (ORCPT ); Wed, 6 Jan 2016 17:09:11 -0500 Date: Wed, 6 Jan 2016 23:09:08 +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: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line and Mic inputs Message-ID: <20160106220908.GE9631@lukather> References: <20151221123148.1ab6480b@dayas> <20151221123416.51aa938e@dayas> <20151227182157.GI30359@lukather> <20151228040649.475a742f@dayas> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="vv4Sf/kQfcwinyKX" Content-Disposition: inline In-Reply-To: <20151228040649.475a742f@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 --vv4Sf/kQfcwinyKX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 28, 2015 at 04:06:49AM +0100, Danny Milosavljevic wrote: > Hi Maxime, >=20 > On Sun, 27 Dec 2015 19:21:57 +0100 > Maxime Ripard wrote: >=20 > > 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. > >=20 > > Again, having a meaningful and standalone commit log would be nice. >=20 > Okay, will elaborate some more in v9. >=20 > > > +#define SUN4I_CODEC_ADC_ACTL_PREG1_A10 (25) > > > +#define SUN4I_CODEC_ADC_ACTL_PREG2_A10 (23) > >=20 > > Why the A10 suffix? >=20 > The sun4i*_a10 names are for things that work on A10 but not on A20. > This way whoever touches the driver later can know for which things he has > to consider multiple cases. > Otherwise he will have no indication that he is using a bit index where= =20 > there sometimes is no bit (or, worse, the wrong bit). >=20 > It's intended to be used like this: >=20 > sun4i_foo_a10 > foo is sun4i_foo_a10 on A10 (only). > sun4i_foo > foo is sun4i_foo on A10 and also on future chips (like A20). > sun7i_foo > foo is sun7i_foo on A20 and (hopefully) also on future chips. I find the sun4i_*_a10 and sun4i highly redundant. If there the same define for sun7i, then you know that it's not meant to be used for the A20, and that's it. My point is also that it will just be an ever growing list of suffixes when we will support more SoCs, for example for symbols that might be in the A10, not the A20, but the A31. > > > +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); > >=20 > > 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? >=20 > I don't know how to do that. I'll try to find out. >=20 > (It would be best if someone who knows how that should act did the alsami= xer=20 > testing later too, though) >=20 > Can two muxes use the same bit in the hardware without problems? > Or do you mean reuse the same mux instance? (I think _new1 always creates= =20 > a new instance from each struct kcontrol_new automatically). Yeah, the case where two widgets share the same bits is handled iirc but sharing the controls. > > The power amplifier is only for the playback, so there's no need to > > differentiate between playback and capture. The current name was fine. >=20 > "Power Amplifier Volume" shows up as Capture control in alsamixer as well. > It isn't supposed to be a Capture control. > > > + /* Line-In, FM, Mic1, Mic2 */ \ > > > + SOC_SINGLE_TLV("Line-In Playback Volume", \ > ... > > > + SOC_SINGLE_TLV("FM Playback Volume", \ > ... > > > + SOC_SINGLE_TLV("Mic Playback Volume", \ > ... > >=20 > > Those are not volume it's gain,=20 >=20 > We tried to call the things ..." Gain" before and it was difficult to do,= =20 > with some breakage along the way, see below. > Also, Mark said they should be named ..." Volume" (see=20 > = ). Ah, my bad. Judging from ControlNames.txt, the Power Amplifier Volume should probably called Headphone Playback Volume then. > >and it should probably be two different shared controls for mic1 and mic= 2. >=20 > I'll try... >=20 > >> "Capture Source" > > This one is the ADC Gain. Please name it that way. >=20 > Unfortunately, the strings have meaning to asoc and alsa-lib and you can'= t go=20 > renaming them like that without breaking things. The names were carefully= =20 > chosen to make it actually work properly without having to patch alsa-lib= and=20 > parts of ASoC core (which I did before and have since reverted). >=20 > In this case, there's a special case in upstream alsa-lib: >=20 > alsa-lib-1.0.28: > ./src/mixer/simple_none.c: if (strcmp(name, "Capture Source") =3D=3D 0) { > ... >=20 > I'm not totally against naming them like you suggested - but know that yo= u=20 > are requiring changes in alsa-lib as well then - or presumably breaking t= he=20 > user's workflow. >=20 > For example the (upstream) "Capture Source" special case in alsa-lib adds= =20 > radio-button-like things to the respective elements.=20 > You can switch to one of the inputs by pressing Space while its gain elem= ent=20 > is selected. > In the mic case, it's the mic preamplier gain that you press Space on - i= f it's=20 > indeed shown as a Capture control... No, you're totally right, I just entirely missed that ControlNames files you pointed me to. Thanks, Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --vv4Sf/kQfcwinyKX Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWjZCEAAoJEBx+YmzsjxAg2GoP/0asYzeXIoWiqCg9agU19/ay fxawCA7WqV1/U3OVG9SDaL+0MCGSxlRrqrwOoG1yjAMLKH6+R6YJtGESRH9bHL6L 3O2WmqbgiaSLZecsvghWQY0JZwje188Yw+GrChig8Q0Zh9xrhkXEKTWHTIgPLrHU prXNmQryY5w4mE2CxYc3sH5Stv0cNqo7yJt9XMlRvC1fdpAbWq5nJ5YhGCISJkjx M/qypzkb3+UAYJcSbE1zB90cxhD8ZQLK99NJBaCbDCRrvHjjQioVXmjDcvea5UBn rJatIThnwgBrCSVBhytFKBLSR7XDZwy6juAXyzKTypjHQJJmmTLAWsFC8OtgqDKR JemajdSHVUeNXBtpcMAviVjLoQJp67ZRKLGRAW8vrIP0++AyWCBQ3OY4GFfBojEM KPRzWzKrRYIIbkakgnKguUb82KDP7TwedxCmTVHgq5XAVAhKskNksm87HupTusIy 48Iefl9c+dLuLEG/OdkTG2sdEoWmgqehuuJwTnp4A3wMAEwxxD79lXH+DlIxA7Fl 2D+ox5/mZuelvnWMKu9DjuK3iBq7Y6WiMAi3QR5QsGFMcEjX0J4zKyH7x48Q30Vu 3hFFnaGmvYXh1vdkTKZwmNcHhxk7AvRB55tdz45Q0WTNKUl9mLY0MsJ6sWIpKjaR eTeoORWXxSwsohdAO3v6 =dfoA -----END PGP SIGNATURE----- --vv4Sf/kQfcwinyKX--