On 05/08/2012 08:27 PM, Mark Brown wrote:
On Tue, May 08, 2012 at 03:57:30PM +0200, Ola Lilja wrote:
+static void show_regulator_status(struct snd_soc_dapm_context *dapm) +{
- struct device *dev = dapm->dev;
- dev_dbg(dev, "%s: Regulator-status:\n", __func__);
- dev_dbg(dev, "%s: V-AUD: %s\n", __func__,
(snd_soc_dapm_get_regulator_status(dapm, "V-AUD") > 0) ?"On" : "Off");So, you were adding this just for debug... Let's not do that. There's already diagnostic infrastructure in the regulator API and at the DAPM level too. If we need this sort of stuff it's probably not device specific so we should probably improve the core if it's not easy enough to figure out what's going on already.
Well, I want this in our driver and you told me to use the regulator-widget, so now the only way for us to have this information, which I find very useful when we debug, is to let the core have some means to provide the information to our driver. I don't want to be forced to enable debug prints in a lot of other places than our driver. That just makes it harder.
+static void show_clock_status(struct snd_soc_dapm_context *dapm) +{
- struct device *dev = dapm->dev;
Similarly here (though the clock API diagnostics are probably a bit weaker).
Same answer as above. I want to have this debug-information in our driver!
- SND_SOC_DAPM_ADC("ADC", "ab8500_0c", SND_SOC_NOPM, 0, 0),
- SND_SOC_DAPM_DAC("DAC", "ab8500_0p", SND_SOC_NOPM, 0, 0),
Please convert all these to use DAPM to hook up the streams to their audio interfaces rather than having a stream attached to the widget.
- {"Mic 1a or 1b Select", "Mic 1a", "MIC1A V-AMICx Enable"},
- {"Mic 1a or 1b Select", "Mic 1b", "MIC1B V-AMICx Enable"},
This also looks very odd... is this the micbias stuff again?
I'll rename them to "MIC1A Enable" and "MIC1B Enable". They are connected connected to the correct regulator-supply from the machine-driver.
+static int mclk_input_control_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
- struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(codec->dev);
- ucontrol->value.enumerated.item[0] = drvdata->mclk_sel;
- return 0;
+}
Same as last time this should be configured by the machine driver.
I also told you last time that I have a hard time doing this from the machine-driver. The switch between these clocks comes from a component in user-space getting its information from a DSP running its own life. If we just run the ASoC-driver normally without Android this will never change, but our whole system-design form Android needs to be able to do this clock-switch in the way we do it. I have no means of getting this information inside the linux-kernel.
- /* Mic 1, Mic 2, LineIn */
- SOC_DOUBLE_R_TLV("Mic Master Gain",
AB8500_ADDIGGAIN3, AB8500_ADDIGGAIN4,0, AB8500_ADDIGGAINX_ADXGAIN_MAX, 1, adx_dig_gain_tlv),All volume controls should be "...Volume".
So what you are saying is that "Gain" is not an accepted term for a volume?!
- SOC_ENUM("Digital Interface 0 Bit-clock Switch", soc_enum_fsbitclk0),
- SOC_ENUM("Digital Interface 1 Bit-clock Switch", soc_enum_fsbitclk1),
Hrm?
In our current Android-design this is also needed to be controlled for the same reasons as the switching of clocks.
- /* Attach regulators to AMic DAPM-paths */
- dev_dbg(codec->dev, "%s: Mic 1a regulator: %s\n", __func__,
amic_micbias_str(amics->mic1a_micbias));- route = &ab8500_dapm_routes_mic1a_vamicx[amics->mic1a_micbias];
- status = snd_soc_dapm_add_routes(&codec->dapm, route, 1);
- dev_dbg(codec->dev, "%s: Mic 1b regulator: %s\n", __func__,
amic_micbias_str(amics->mic1b_micbias));- route = &ab8500_dapm_routes_mic1b_vamicx[amics->mic1b_micbias];
- status |= snd_soc_dapm_add_routes(&codec->dapm, route, 1);
- dev_dbg(codec->dev, "%s: Mic 2 regulator: %s\n", __func__,
amic_micbias_str(amics->mic2_micbias));- route = &ab8500_dapm_routes_mic2_vamicx[amics->mic2_micbias];
- status |= snd_soc_dapm_add_routes(&codec->dapm, route, 1);
This is fairly impenetrable and would usually be done in hte machine driver. Machines might not use the chip biases for some or all of the mics but it looks like this code assumes they do.
OK, so it will be fine if I just move the code to the machine-driver?
+int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
unsigned int fmt,unsigned int wl,unsigned int delay)Why is this not static?
Because it is called from the machine-driver.