Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi Danny, On Sun, Dec 06, 2015 at 07:53:07PM +0100, Danny Milosavljevic wrote: > Hi, > > this is the fourth version of the patch that adds inputs to sun4i-codec. > > The inputs added are: > - FM-In Left and Right > - Line-In Left and Right > - Mic1-In > - Mic2-In > > Changes compared to v3 are: > - Names of the input are not uppercase anymore. > - bit index constants are now named as in the A20 user manual v1.4 > - added Mic1-In, Mac2-In > - added Mic1 and Mic2 Pre-Amplifiers. > > I successfully tested it using alsamixer, headphones, a radio and my ears. > Note that because of missing capturing support I tested only the mixing. > > Still TODO are: > - sun4i_codec_micin_gain_scale should have an entry for 0 dB at 0. > How? > - maybe the Mic Preamps should be listed in the routes. > It works as it is now, though. > - right now when I press F4 in alsamixer, the Mic1-In Pre-Amplifier Gain > is missing its muting switch. WTF... > How does ALSA (or alsamixer) know which controls are "Capture"? Thanks for your patch. Please send it to the relevant recipients (you can have the list using scripts/get_maintainer.pl) so that the review process can get started. Otherwise, your patch will never be picked. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi Maxime, On Tue, 8 Dec 2015 10:09:50 +0100 Maxime Ripard wrote: > > Now I added an ugly hack to make Mic Gain work on A10 as well - but I don't > > have > > A10 hardware. Let's see what they say. > > That I really prefer having nothing over an "ugly hack" :) I figured you'd say that :) But for these kind of architectural decisions should I send the question to all the people get_maintainers tells me in advance? Or pick one? Or should I include it in the patch E-Mail above the "---"? Like this: PREG1 is the Mic1-In Pre-Amplifier gain control. PREG2 is the Mic2-In Pre-Amplifier gain control. The A10 has PREG1 and PREG2 in the ADC control register. The A20 has PREG1 and PREG2 in the (new) PHONE_CAL control register. Right now my (v5) probe function modifies the global variable "sun4i_codec_widgets" depending on the compatible string. I think it's safe for the time being since the codec is on-die and it's not possible to suddenly need both sun4i widgets and sun7i widgets in the same kernel at the same time. (I don't have A10 hardware. So I wrote it in a way that in the A10 case it doesn't have to do anything and in the A20 case it does the modifications. That way, I could test the modifications (I do have A20 hardware)) The alternatives would be to either 1) split the driver up into kernel modules "sun4i-codec" and "sun7i-codec". Maybe add a common object file to reduce duplicate work. This would be an user-visible change. Is that even allowed? 2) make the "sun4i-codec" kernel module register two different codecs with two different compatibles and two different "sun4i"_codec_widget variables. Is that even possible? 3) make ALSA dynamically add two different widgets at run time depending on the compatible (not added to the "sun4i_codec_widget" array). Is that even possible? Which do we prefer? (All that because someone just had to move those bits. Sigh. The space where they were in A10 is empty in A20, so it's not like they needed the space or anything) Regards, Danny -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi, On Tue, Dec 08, 2015 at 11:51:44AM +0100, Danny Milosavljevic wrote: > Hi Maxime, > > On Tue, 8 Dec 2015 10:09:50 +0100 > Maxime Ripard wrote: > > > > Now I added an ugly hack to make Mic Gain work on A10 as well - but I > > > don't have > > > A10 hardware. Let's see what they say. > > > > That I really prefer having nothing over an "ugly hack" :) > > I figured you'd say that :) > > But for these kind of architectural decisions should I send the question to > all > the people get_maintainers tells me in advance? Or pick one? All of them. > Or should I include it in the patch E-Mail above the "---"? You can do that, but usually there's no strong reason to do this for a single patch, you can just put it on the git send-email command line. > Like this: > > PREG1 is the Mic1-In Pre-Amplifier gain control. > PREG2 is the Mic2-In Pre-Amplifier gain control. > > The A10 has PREG1 and PREG2 in the ADC control register. > The A20 has PREG1 and PREG2 in the (new) PHONE_CAL control register. > > Right now my (v5) probe function modifies the global variable > "sun4i_codec_widgets" depending on the compatible string. > I think it's safe for the time being since the codec is on-die and it's not > possible to suddenly need both sun4i widgets and sun7i widgets in the same > kernel at the same time. > (I don't have A10 hardware. So I wrote it in a way that in the A10 case > it doesn't have to do anything and in the A20 case it does the modifications. > That way, I could test the modifications (I do have A20 hardware)) Yeah, that's not so nice. > The alternatives would be to either > 1) split the driver up into kernel modules "sun4i-codec" and "sun7i-codec". >Maybe add a common object file to reduce duplicate work. >This would be an user-visible change. Is that even allowed? Not really either, if the hardware as similar, you just have the same driver for the two. > 2) make the "sun4i-codec" kernel module register two different codecs >with two different compatibles and two different "sun4i"_codec_widget >variables. Is that even possible? Yes, you have a data field you can associate to the compatible in the of_device_id structure, and that you can later retrieve using of_device_get_match_data. > 3) make ALSA dynamically add two different widgets at run time depending on >the compatible (not added to the "sun4i_codec_widget" array). >Is that even possible? There's no need to have two at the same time, they're mutually exclusive. 4) Copy the structure in probe and modify the copied instance I guess 2 or 4 are the two valid way of doing things. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi Maxime, > There's no need to have two at the same time, they're mutually > exclusive. Hmmm. I don't understand it properly then... Let's say two different compatibles in two different device tree nodes match the same module on the same computer. Will that module's probe() function be entered and left twice in that case? If so, will the pdev differ? (I know that for our current hardware it's not possible for that to happen in the first place, but I mean in general) > 2) make the "sun4i-codec" kernel module register two different codecs >with two different compatibles and two different "sun4i"_codec_widget >variables. > 4) Copy the structure in probe and modify the copied instance > > I guess 2 or 4 are the two valid way of doing things. Yeah, in v6 I did some combination of 2 and 4 (if you squint) which turned out quite nice. Please check whether it's safe that way (if you have the time). Thanks, Danny -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi, On Fri, Dec 11, 2015 at 2:57 AM, Danny Milosavljevic wrote: > Hi Maxime, > >> There's no need to have two at the same time, they're mutually >> exclusive. > > Hmmm. I don't understand it properly then... > Let's say two different compatibles in two different device tree nodes > match the same module on the same computer. > Will that module's probe() function be entered and left twice in that case? > If so, will the pdev differ? In simplified terms, a pdev is created for each device node. For each pdev that has a matching driver, the driver's probe function is entered. So yes, a drivers probe function can be run multiple times, which is often the case for USB hosts, MMC controllers, any thing a system has multiple instances of. (Hence it is not a good idea to use global variables within a driver.) Once a driver successfully probes (probe function returns 0), the driver core considers the device "bound", and will not attempt to match other drivers. > > (I know that for our current hardware it's not possible for that to happen > in the first place, but I mean in general) > >> 2) make the "sun4i-codec" kernel module register two different codecs >>with two different compatibles and two different "sun4i"_codec_widget >>variables. >> 4) Copy the structure in probe and modify the copied instance >> >> I guess 2 or 4 are the two valid way of doing things. > > Yeah, in v6 I did some combination of 2 and 4 (if you squint) which turned > out quite nice. > Please check whether it's safe that way (if you have the time). If everything is known beforehand, having 2 separate read only tables is probably better. No need for maintainers / reviewers to squint. :) Regards ChenYu -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
On Thu, Dec 10, 2015 at 07:57:36PM +0100, Danny Milosavljevic wrote: > Hi Maxime, > > > There's no need to have two at the same time, they're mutually > > exclusive. > > Hmmm. I don't understand it properly then... > Let's say two different compatibles in two different device tree nodes > match the same module on the same computer. My point was that that case won't happen. If you're on an A10 device, all your DT nodes will have the A10 compatible, if you're on an A20, they will all have the A20 compatible, there's no possible mix between them. > Will that module's probe() function be entered and left twice in > that case? Yes. > If so, will the pdev differ? Yes. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi Chen-Yu, On Fri, 11 Dec 2015 11:23:33 +0800 Chen-Yu Tsai wrote: > If everything is known beforehand, having 2 separate read only tables is > probably better. No need for maintainers / reviewers to squint. :) Yeah, that's what v6 already does. Suggestion 4 was to copy it at runtime, so it ended up a bit different from that. Cheers, Danny -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
Hi CK,
Hi Mark,
(I've added Mark - since he committed the original soc-ops line of code below -
and the list)
On Mon, 7 Dec 2015 07:30:07 +0100
Code Kipper wrote:
[...]
> > +static DECLARE_TLV_DB_SCALE(sun4i_codec_linein_output_volume_scale, -150,
> > 150, 0);
> These aren't really volume...that is handled by the Pre-Amp and I'd
> argue playback .
I agree that these are playback controls.
>I would call this loopback gain.
[...]
> > + SOC_SINGLE_TLV("Line-In Playback Volume",
> ditto
[... sun4i_codec_linein_output_volume_scale]
Just to make sure, you mean that, for the user, it should be shown as "Line-In
Loopback Gain"?
I tried to make that work, but by now I had to patch alsa-lib and other parts
of the kernel as well.
I'm still not sure whether I found all the parts that need changing.
So the summary is that this name is both a displaystring AND has meaning to the
system.
If I call it "Line-In Loopback Gain" and not change alsa-lib, then alsamixer
will show it as a boolean control (which it isn't :P).
If I call it "Line-In Loopback Gain" and patch alsa-lib, the control will
vanish from alsamixer (!).
If I call it like that, patch alsa-lib and patch sound/soc/soc-ops.c, then it
will work again.
Btw: If one forgets do to some of the patches and/or not everything is
installed at once on the user's computer, the Line-In control will just vanish
as if it never existed (!).
So if you don't feel strongly about this, I'd rather not do all that and just
call it "Line-In Playback Volume" instead.
(There are possible workarounds like overwriting .info in the struct
snd_kcontrol_new instance in order to prevent it from claiming that it is a
boolean control)
The relevant pseudo-patches (NOT FINISHED - and including some debugging
messages for sanity) are:
To linux-next:
diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index a513a34..20407d9 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -194,7 +194,7 @@ int snd_soc_info_volsw(struct snd_kcontrol *kcontrol,
mc->platform_max = mc->max;
platform_max = mc->platform_max;
- if (platform_max == 1 && !strstr(kcontrol->id.name, " Volume"))
+ if (platform_max == 1 && !strstr(kcontrol->id.name, " Volume") &&
...not gain either)
uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
else
uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
I'm thinking about adding "!kcontrol->tlv.p" as a catch-all... not sure,
though.
@Mark, what do you think about that?
To alsa-lib-1.0.28:
--- orig/alsa-lib-1.0.28/src/mixer/simple_none.c2014-06-17
12:34:19.0 +
+++ alsa-lib-1.0.28/src/mixer/simple_none.c 2015-12-11 21:43:57.314549129
+
@@ -892,10 +892,13 @@
{" Playback Switch", CTL_PLAYBACK_SWITCH},
{" Playback Route", CTL_PLAYBACK_ROUTE},
{" Playback Volume", CTL_PLAYBACK_VOLUME},
+ {" Loopback Switch", CTL_PLAYBACK_SWITCH},
+ {" Loopback Gain", CTL_PLAYBACK_VOLUME},
{" Capture Enum", CTL_CAPTURE_ENUM},
{" Capture Switch", CTL_CAPTURE_SWITCH},
{" Capture Route", CTL_CAPTURE_ROUTE},
{" Capture Volume", CTL_CAPTURE_VOLUME},
+ {" Boost Gain", CTL_CAPTURE_VOLUME},
{" Enum", CTL_GLOBAL_ENUM},
{" Switch", CTL_GLOBAL_SWITCH},
{" Route", CTL_GLOBAL_ROUTE},
@@ -909,6 +912,7 @@
{
const struct suf *p;
size_t nlen = strlen(name);
+ fprintf(stderr, "base_len(%s)\n", name);
p = suffixes;
while (p->suffix) {
size_t slen = strlen(p->suffix);
@@ -918,6 +922,7 @@
if (strncmp(name + l, p->suffix, slen) == 0 &&
(l < 1 || name[l-1] != '-')) { /* 3D Control -
Switch */
*type = p->type;
+ fprintf(stderr, "-> type %u\n", (unsigned)
*type);
return l;
}
}
@@ -1487,6 +1492,7 @@
case CTL_GLOBAL_VOLUME:
case CTL_PLAYBACK_VOLUME:
case CTL_CAPTURE_VOLUME:
+ fprintf(stderr, "control type is playback volume? %u\n",
(unsigned) type == CTL_PLAYBACK_VOLUME);
if (ctype == SND_CTL_ELEM_TYPE_ENUMERATED) {
if (type == CTL_PLAYBACK_VOLUME)
type = CTL_PLAYBACK_ENUM;
@@ -1496,6 +1502,7 @@
type = CTL_GLOBAL_ENUM;
break;
}
+ fprintf(stderr, "ctype is integer? %u; ctype=%u\n", (unsigned)
ctype == SND_CTL_ELEM_TYPE_INTEGER, (unsigned) ctype);
if (ctype != SND_CTL_ELEM_TYPE_INTEGER)
return 0;
break;
Arrrgh...
But we can try your way for the Mic gains, they have larger range and can't be
misrepresented as a boolean. That would still need an alsa-lib patch to make it
recognize it as a capturing contr
Re: [linux-sunxi] [PATCH v4] sun4i-codec: add inputs
On Sat, Dec 12, 2015 at 12:15:09AM +0100, Danny Milosavljevic wrote:
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.
> > > + SOC_SINGLE_TLV("Line-In Playback Volume",
> > ditto
> [... sun4i_codec_linein_output_volume_scale]
> Just to make sure, you mean that, for the user, it should be shown as
> "Line-In Loopback Gain"?
No, absolutely not - all volume controls should end in " Volume". See
ControlNames.txt.
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: PGP signature
