On Wed, Mar 27, 2013 at 04:50:21PM -0600, Stephen Warren wrote:
On 03/26/2013 07:15 PM, Mark Brown wrote:
On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
+/* IN1/IN2 Input Type */ +static const char * const rt5640_input_mode[] = { + "Single ended", "Differential"};
This looks like platform data.
I thought the idea was to expose all the CODEC's configuration/routing through DAPM, and then let everything get set up through controls at
Given that changing between single ended and differential generally involves a modification of the circuit this is something we know at build time in the same way we know how the microphone biases are wired up. Changing it would generally need to be coordinated with some external circuit (eg, flipping an analouge switch to reroute the signals) and hence done from the machine driver.
run-time? Looking at the WM8903 driver, it seems like it has the exact same kind of option.
Older drivers aren't always good examples of this stuff.
+/* DMIC */ +static const char * const rt5640_dmic_mode[] = {"Disable", "DMIC1", "DMIC2"}; + +static const SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
DAPM?
I'm not sure what change you're asking for here; isn't this defining a control that influences the DAPM routing?
It looks like just a plain old enum to me.
Perhaps the issue is that the enum above feeds into a snd_kcontrol, rather than being an snd_soc_dapm_route that's conditional upon one of the values in that list?
Probably, or a mux or something.
I'm not really sure what MIC1/MIC2 are meant to represent...
I think they're supposed to represent the microphones someone might connect to the inputs but that should be done in the machine driver.
Shouldn't the DMIC CLK widget be handling the clock enables/muxes from the DMIC event above?
Oh right, so you mean:
- Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".
No need for that, it can just be handled as a supply within the device since it's an integral part of the DMIC interface.
- Add routing table entries where "DMIC1 *" are each fed from "DMIC
CLK", with "conditions" based on which DMIC is "on":
{"DMIC L1" "DMIC1" "DMIC CLK"}, {"DMIC R1" "DMIC1" "DMIC CLK"}, {"DMIC L2" "DMIC2" "DMIC CLK"}, {"DMIC R2" "DMIC2" "DMIC CLK"},
or something like that?
Pretty much.
+static int get_sdp_info(struct snd_soc_codec *codec, int dai_id)
- bclk_ms = frame_size > 32 ? 1 : 0;
Grumble.
Sorry, I don't understand the issue here. (through lack of familiarity with the issue; I'm not saying there isn't one).
I don't like the ternery operator where it's not especially idiomatic, if I wanted to read line noise I'd be working with perl.
+static int rt5640_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_codec *codec = rtd->codec; + struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); + + rt5640->aif_pu = dai->id; + return 0; +}
This looks like only one DAI can be active at once, shouldn't there be some sort of checking for busy here?
I haven't read the spec through in explicit detail, but I didn't see anything to indicate that only one DAI could be active at once. In fact the diagrams in the documentation make it look at least somewhat cross-bar-like.
It looks like rt5640->aif_pu is used solely by set_dmic_clk() to somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is actually active. I assume "which DAI is actually active" should be implemented as "which DAI is routed from the DMICs". I guess this should be implemented in a very different way then?
Probably, yes. This code really doesn't look like it'll work properly if the two interfaces are running at once, especially if they are started simultaneously.
I wonder if this all shouldn't be explicitly represented by DAPM widgets/routes, but the diagrams and text in the documentation aren't particularly clear on what this third thing is that the code seems to support:-(
Yes, putting it all into DAPM seems like the way forward.
+static int rt5640_resume(struct snd_soc_codec *codec) +{ + int ret = 0 ; + + codec->cache_sync = 1; + ret = snd_soc_cache_sync(codec); + if (ret) { + dev_err(codec->dev, "Failed to sync cache: %d\n", ret); + return ret; + } + rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
We also sync the cache in the bias management code.
In other words, I should just remove the call to snd_soc_cache_sync(codec) here, since it's redundant?
Or remove it from the bias management since there's no regulator or other code I can see which cuts the power or otherwise resets the device so restoring the cache outside of suspend is probably a waste of time at the minute.