Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303098 > unrolled thread

Re: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line and Mic inputs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line  and Mic inputs Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-01-06 23:10 +0100
    Re: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM,  Line and Mic inputs Danny Milosavljevic <dannym@scratchpost.org> - 2016-01-09 16:50 +0100

#1303098 — Re: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line and Mic inputs

[Multipart message — attachments visible in raw view] — view raw

On Mon, Dec 28, 2015 at 04:06:49AM +0100, Danny Milosavljevic wrote:
> Hi Maxime,
> 
> On Sun, 27 Dec 2015 19:21:57 +0100
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > 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.
> 
> Okay, will elaborate some more in v9.
> 
> > > +#define SUN4I_CODEC_ADC_ACTL_PREG1_A10			(25)
> > > +#define SUN4I_CODEC_ADC_ACTL_PREG2_A10			(23)
> > 
> > Why the A10 suffix?
> 
> 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 
> there sometimes is no bit (or, worse, the wrong bit).
> 
> It's intended to be used like this:
> 
> 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[] = {
> > > +	"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?
> 
> I don't know how to do that. I'll try to find out.
> 
> (It would be best if someone who knows how that should act did the alsamixer 
> testing later too, though)
> 
> 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 
> 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.
> 
> "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", \
> ...
> > 
> > Those are not volume it's gain, 
> 
> We tried to call the things ..." Gain" before and it was difficult to do, 
> with some breakage along the way, see below.
> Also, Mark said they should be named ..." Volume" (see 
> <https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg15126.html>).

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 mic2.
> 
> I'll try...
> 
> >> "Capture Source"
> > This one is the ADC Gain. Please name it that way.
> 
> Unfortunately, the strings have meaning to asoc and alsa-lib and you can't go 
> renaming them like that without breaking things. The names were carefully 
> chosen to make it actually work properly without having to patch alsa-lib and 
> parts of ASoC core (which I did before and have since reverted).
> 
> In this case, there's a special case in upstream alsa-lib:
> 
> alsa-lib-1.0.28:
> ./src/mixer/simple_none.c:	if (strcmp(name, "Capture Source") == 0) {
> ...
> 
> I'm not totally against naming them like you suggested - but know that you 
> are requiring changes in alsa-lib as well then - or presumably breaking the 
> user's workflow.
> 
> For example the (upstream) "Capture Source" special case in alsa-lib adds 
> radio-button-like things to the respective elements. 
> You can switch to one of the inputs by pressing Space while its gain element 
> is selected.
> In the mic case, it's the mic preamplier gain that you press Space on - if it's 
> indeed shown as a Capture control...

No, you're totally right, I just entirely missed that ControlNames
files you pointed me to.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1305249 — Re: [linux-sunxi] Re: [PATCH v8 2/2] ASoc: sun4i-codec: Add FM, Line and Mic inputs

Hi Maxime,
Hi Mark,

On Wed, 6 Jan 2016 23:09:08 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> 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.

Okay. I will remove the "_a10" and "_A10" suffixes for v9. 
There are not many places it's used anyway.

> > > 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?

That is possible and is working correctly so far. It will probably be in v9.

> Yeah, the case where two widgets share the same bits is handled iirc
> but sharing the controls.

Yeah. The struct kcontrol_new address (of a dapm control) is compared in order 
to find out whether to share the control or not. 

> Judging from ControlNames.txt, the Power Amplifier Volume should
> probably called Headphone Playback Volume then.

Yeah. I'll rename it to "Headphone Playback Volume" for v9.

> > >and it should probably be two different shared controls for mic1 and mic2

Once a control is shared there's just one control to the user.

I now have Mic Playback Volume shared between Left Mixer and Right Mixer 
(Note: was not in v8).
So far so good. (the name will be neither "Left Mixer Mic Playback 
Volume" nor "Right Mixer Mic Playback Volume" but rather "Mic Playback 
Volume" - because of the sharing)

But I can't get having a control for Mic2 Playback Volume that does the same 
as Mic1 Playback Volume (and updates the same register bitrange) to work 
[yet?] instead.

Sharing uses the same control for two different mixers, muxes etc.

The other direction (making two controls which both do the same and update 
each other's value) doesn't seem to be easily available. 
Should I write a _put handler that does it manually?

(Or should that be handled by alsa-lib mixer modules instead?)

Or, come to think of it, did you mean something else?

And something related but different:

Is it possible to merge the "Left Mixer" control and respective 
"Right Mixer" control into one selem in alsamixer?

Because alsamixer actually has support for one-sided muting.

For example:

Left Mixer Left FM Playback Switch and Right Mixer Right FM Playback Switch and 
FM Playback Volume could be one selem "FM Playback". Muting the left mixer left 
fm playback would be done by pressing "," and the right mixer right fm playback 
would be done by pressing ".".

Right now they are 3 different selem for that. So it would help reduce a lot of 
clutter.

(Right Mixer Left DAC Playback Switch - not a typo - would be the odd duck 
 and still be an extra selem)

For Mic there's:
- Left Mixer Mic1 Playback Switch
- Left Mixer Mic2 Playback Switch
- Right Mixer Mic1 Playback Switch
- Right Mixer Mic2 Playback Switch
- Mic Playback Volume

If possible, it could also then be collected together as:
- Mic1 Playback:
    Left Mixer Mic1 Playback Switch, 
    Right Mixer Mic1 Playback Switch, 
    Mic Playback Volume
- Mic2 Playback:
    Left Mixer Mic2 Playback Switch, 
    Right Mixer Mic2 Playback Switch, 
    Mic Playback Volume [note: there it is again]

Is it possible?

Regards,
    Danny

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web