Re: [PATCH 10/11] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4350 bytes --]

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.

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

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

> +	/* LineIn */
> +	{"LINL Mute", NULL, "LineIn Left"},
> +	{"LINR Enable", NULL, "LineIn Right"},
> +
> +	{"LINL Enable", NULL, "LINL Mute"},
> +	{"LINR Enable", NULL, "LineIn Right"},

This looks *really* odd, especially with the lack of symmetry.

> +	{"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?

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

> +static struct snd_kcontrol_new ab8500_ctrls[] = {
> +	/* Charge pump */
> +	SOC_ENUM("Charge Pump High Threshold For Low Voltage",
> +		soc_enum_envdeththre),
> +	SOC_ENUM("Charge Pump Low Threshold For Low Voltage",
> +		soc_enum_envdetlthre),
> +	SOC_ENUM("Charge Pump Envelope Detection", soc_enum_envdetcpen),
> +	SOC_ENUM("Charge Pump Envelope Detection Decay Time",
> +		soc_enum_envdettime),

Again, why is this all exposed to userspace?

> +	/* 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".

> +	SOC_ENUM("Digital Interface 0 Bit-clock Switch", soc_enum_fsbitclk0),
> +	SOC_ENUM("Digital Interface 1 Bit-clock Switch", soc_enum_fsbitclk1),

Hrm?

> +	/* Digital interface - Burst FIFO */
> +	SOC_ENUM("Digital Interface 0 FIFO Enable Switch", soc_enum_if0fifoen),

If this is a "...Switch" it shouldn't be an enum.

> +	/* 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.

> +int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
> +			unsigned int fmt,
> +			unsigned int wl,
> +			unsigned int delay)

Why is this not static?

> +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
> +
> +	return 0;

Remove empty functions.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]