On Thu, Jan 08, 2009 at 10:07:04PM +0100, Robert Jarzmik wrote:
Mark Brown broonie@sirena.org.uk writes:
It'd probably be easier to split this up still further, with one patch providing a standard ASoC machine and another adding the scenario stuff after that.
Wish for 2 patches instead of that one, one for core support and one for scenarii ? As this patch has already been reviewed and include into your tree, I had hopped I shouldn't inject any more work that what had already be done ...
Exactly the same sort of scrutiny is being applied to the v2 core as it moves into mainline - it's being broken up into smaller patches and merged gradually too, often in not quite the same form as it went into the out of tree branch.
Also note that there's a two stage thing I'm saying here: as well as "should we do scenarios at all?" there's also the question of how it's implemented. This is a generic problem and most of your implementation looks like it's not so far away from something that could be used by other boards too so it deserves to be hoisted out into something that those other systems can use, rather than having people reinvent the wheel or cut'n'paste.
Right now my feeling is that we probably want to do at least some scenario stuff in kernel space and that what's sensible for a given machine will depend on both the capabilities of the machine (having both GSM and WiFi causes the number of use cases to get much larger, for example) and if there's hardware restrictions that need to be enforced (like not being able to use both speaker and headphones at the same time, which is fairly common but easily enforced without scenarios as such).
I'd like to recall what happened to one of the mioa701 users here, when scenarii were handled in userspace. The wm9713 chip overheated, and destroyed the battery underneath. That was the reason to put that stuff into the kernel : protection. This was also already discussed when this was _previously_ submitted.
If the system is overheating that's something that it's worth preventing in kernel space. Do you know what they did to make that happen - is it a case of enabling more outputs than can comfortably be driven, for example?
And I know Liam's API, I tried it. Let me tell you my point of view : the mioa701 users have a standard API (alsa) to control the sound of the mioa701. I could force them to migrate towards Liam's library. But I can't have a guarantee of maintainance over that library. I don't even know if it will be the final solution.
The idea is not to replace ALSA for regular applications but rather to provide an adjunct to it which a central management process can use to do setup depending on the current system state. There is no desire at all to replace ALSA, only to configure it.
At the minute the majority of deployments seem to use alsactl and state files managed by a central daemon to do this job - though obviously a dedicated scenario API could just be dropped into that daemon.
+#define ARRAY_AND_SIZE(x) (x), ARRAY_SIZE(x)
This is also defined somewhere or other in the PXA or ARM architecture
There was a discussion to include it in kernel.h. ATM, it is defined in
Yeah, I know - it was pretty strongly NACKed so I'd be shocked if it ever went in.
arch/arm/mach-pxa/generic.h, which is not easily reachable, hence the definition.
#include <mach/generic.h>? (not tested at all)
Moreover, if you look at that file, you'll see the definition is the same, and I think it is perfectly correct.
Given the strong NACKs, though... I do believe it's OK but it'd be better to just include the PXA file (the driver is board-specific after all).
memset(mname, 0, 44);strncpy(mname, mixes[pos].mixname, 43);
It's bikesheding but strcpy plus termination would do the job, wouldn't it?
I don't get that ...
The memset() is mostly redundant and the above should be equivalent to strncpy() plus an assignment to make sure the string is terminated.
kctl->get(kctl, &ucontrol);ucontrol.value.enumerated.item[0] = mixes[pos].val;kctl->put(kctl, &ucontrol);}
Does an open application (eg, alsamixer) notice these changes without the driver telling it? snd_ctl_notify() is the API call, IIRC though there's a reasonable chance I am misremembering. Though if you do mask them from user space it shoudn't matter anyway, I expect.
It does. At least when I change the mode, all the muxers the drivers touches change state. I don't know how, but it works ...
So the UI of interactive applications update immediately? In that case it's fine.
<nitpick>mixers and muxes</nitpick>
+static int phone_stream_start(struct snd_soc_codec *codec) +{
snd_soc_dapm_stream_event(codec, "AC97 HiFi",SND_SOC_DAPM_STREAM_START);return 0;+}
Hrm. What are these needed for? I'd have expected that DAPM would power active bypass paths up without this (I test that case frequently) and this isn't coordinated with what the core is doing at all.
Without this, in asoc-v2, the dapm is not powering any of the elements in the phone audio path without this. Will check in asoc-v1, but I see no reason this
Is the phone using the I2S interface for digital audio out from the GSM chipset by any chance? Some comments at the head of the driver describing the wiring might be useful.
has changed. I don't quite understand what you mean by "active bypass path" so I will make the test myself.
A bypass path is a path through the codec which doesn't use any DACs and ADCs but is analogue only.