[alsa-devel] [PATCH] ASoC: Intel: Add merrifield machine driver

[Mark Brown]

On Thu, Jul 17, 2014 at 05:43:45PM +0530, Subhransu S. Prusty wrote:

> +	default n

This is the standard Kconfig behaviour - no need to specify it (other
drivers don't...).

> +static int mrfld_8958_hw_params(struct snd_pcm_substream *substream,
> +			   struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> +
> +	if (!strcmp(codec_dai->name, "wm8994-aif1"))
> +		return mrfld_wm8958_set_clk(codec_dai);
> +	return 0;
> +}

It's much clearer to define separate operations for each DAI, there's no
need for the string matching (especially given that there's nothing to
do for other DAIs).

> +	/*
> +	 * Machine uses 2 DMICs, other configs may use more so change below
> +	 * accordingly
> +	 */
> +	{ "DMIC1DAT", NULL, "DMIC" },
> +	{ "DMIC2DAT", NULL, "DMIC" },
> +	/*{ "DMIC3DAT", NULL, "DMIC" },*/
> +	/*{ "DMIC4DAT", NULL, "DMIC" },*/

Remove commented out code.

> +	/* Force enable VMID to avoid cold latency constraints */
> +	snd_soc_dapm_force_enable_pin(&card->dapm, "VMID");
> +	snd_soc_dapm_sync(&card->dapm);

This looks like you're open coding a version of wm8994_vmid_mode().
Though that might need some tuning for systems that really have no
single ended outputs.

> +	/*
> +	 * Micbias1 is always off, so for pm optimizations make sure the micbias1
> +	 * discharge bit is set to floating to avoid discharge in disable state
> +	 */
> +	snd_soc_update_bits(codec, WM8958_MICBIAS1, WM8958_MICB1_DISCH, 0);

Don't write directly to CODEC registers, provide a way of configuring
this in the CODEC either via platform data or some API.  That way
nothing else is writing to the CODEC register map without the driver for
the CODEC knowing about it.

> +static void snd_mrfld_8958_complete(struct device *dev)
> +{
> +	struct snd_soc_card *card = dev_get_drvdata(dev);
> +	struct snd_soc_codec *codec = card->rtd[CODEC_BE].codec;
> +	struct snd_soc_dapm_context *dapm;
> +
> +	pr_debug("In %s\n", __func__);
> +
> +	pr_debug("found codec %s\n", codec->component.name);
> +	dapm = &codec->dapm;
> +
> +	snd_soc_dapm_force_enable_pin(dapm, "VMID");
> +	snd_soc_dapm_sync(dapm);
> +
> +	snd_soc_resume(dev);
> +}

Doing the manual fiddling before rather than after resuming the core
seems...  concerning, I'd expect this to be done either in one of the
ASoC suspend/resume callbacks or at least after calling snd_soc_resume()
- doing it before seems likely to trigger or cause bugs.

> +static int snd_mrfld_8958_poweroff(struct device *dev)
> +{
> +	pr_debug("In %s\n", __func__);
> +	snd_soc_poweroff(dev);
> +	return 0;
> +}

Remove this, just use the core function directly.

> +	platform_set_drvdata(pdev, &snd_soc_card_mrfld);
> +	pr_info("%s successful\n", __func__);

This print has no content, remove it or at the very least downgrade it
to debug level.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140729/3c98f0e7/attachment.sig>