[alsa-devel] [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver

[Mark Brown]

On Wed, Jun 05, 2013 at 04:21:41PM +0800, Nicolin Chen wrote:

Looks pretty good, a few comments but they're all fairly minor.

> +  WM8962 pins:
> +  * HPOUTL
> +  * HPOUTR
> +  * SPKOUTL
> +  * SPKOUTR
> +  * MICBIAS
> +  * IN3R
> +  * DMIC
> +  * DMICDAT

No need to list all these, just reference the CODEC (and add them to the
CODEC binding documentation if they're not there which they probably
aren't).  This is generally good practice if there's a family of devices
sharing a driver and means that you don't have to worry about missing
all the other input pins as you've done here.  :)

> +- hp-det-gpios : The gpio port of Headphone detection.
> +- mic-det-gpios: The gpio port of Micphone detection.

I'd say a bit more about these - obviously the CODEC has its own
accessory detection here, and I rather suspect that in fact the HP
detection you have there is really jack detection.

> +static int check_hw_params(struct snd_pcm_substream *substream,
> +		snd_pcm_format_t sample_format, unsigned int sample_rate)
> +{
> +	struct imx_priv *priv = &card_priv;
> +	struct device *dev = substream->pcm->card->dev;
> +
> +	substream->runtime->sample_bits =
> +		snd_pcm_format_physical_width(sample_format);
> +	substream->runtime->rate = sample_rate;
> +	substream->runtime->format = sample_format;
> +
> +	if (!priv->first_stream) {
> +		priv->first_stream = substream;
> +	} else {
> +		priv->second_stream = substream;
> +
> +		if (priv->first_stream->runtime->rate !=
> +				priv->second_stream->runtime->rate) {
> +			dev_err(dev, "KEEP THE SAME SAMPLE RATE: %d!\n",
> +					priv->first_stream->runtime->rate);
> +			return -EINVAL;
> +		}
> +		if (priv->first_stream->runtime->sample_bits !=
> +				priv->second_stream->runtime->sample_bits) {
> +			snd_pcm_format_t first_format =
> +				priv->first_stream->runtime->format;
> +
> +			dev_err(dev, "KEEP THE SAME FORMAT: %s!\n",
> +					snd_pcm_format_name(first_format));
> +			return -EINVAL;
> +		}
> +	}

The sample rate checking can be done with the symmmetric_rates feature
in the core.  The sample_bits check can't be but it's something that a
lot of systems probably ought to have so it ought to be factored out
into the core too rather than open coded in the driver.

> +		/*
> +		 * SYSCLK of wm8962's not disabled yet. If we wanna close FLL,

No '.

> +		ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK,
> +				data->clk_frequency, SND_SOC_CLOCK_IN);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set SYSCLK: %d\n", ret);
> +			return ret;
> +		}
> +
> +		/* Disable FLL and let codec do pm_runtime_put() */
> +		ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, 0, 0, 0);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to disable FLL: %d\n", ret);
> +			return ret;

You're enabling and disabling the CODEC only while there's an audio
stream active.  This means that it's not possible to support analogue
bypass paths (which the device can do) - you should probably also
support enabling the FLL via set_bias_level() and use set_bias_level()
to turn it off (which works in all cases).

> +	data->codec_clk = clk_get(&codec_dev->dev, NULL);
> +	if (IS_ERR(data->codec_clk)) {

devm_clk_get().

> +		/* assuming clock enabled by default */
> +		data->codec_clk = NULL;
> +		ret = of_property_read_u32(codec_np, "clock-frequency",
> +					&data->clk_frequency);
> +		if (ret) {
> +			dev_err(&codec_dev->dev,
> +				"clock-frequency missing or invalid\n");
> +			goto fail;
> +		}

Since it's easy to define a fixed rate clock (there's a generic driver
for that) I'd just require the user to provide a clock API clock and fix
the rate using that.  This is going to be less error prone and makes the
code simpler.

> +	} else {
> +		data->clk_frequency = clk_get_rate(data->codec_clk);
> +		clk_prepare_enable(data->codec_clk);

Not needed immediately but if there is actually a clock we have control
of we might want to vary the frequency...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20130605/e9259888/attachment.sig>