Re: [PATCH 5/8] ASoC: Ux500: Add MSP I2S-driver

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

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

On Fri, Apr 20, 2012 at 11:33:06AM +0200, Ola Lilja wrote:

> Add driver for running I2S with the MSP-block.

This depends on the change for the debug print function.  Otherwise
there's a bunch of relatively minor stuff but overall this looks
generally good.

> +static int ux500_msp_dai_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +	struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(dai->dev);
> +
> +	dev_dbg(dai->dev, "%s: MSP %d (%s): Enter.\n", __func__, dai->id,
> +		snd_soc_stream_str(substream));
> +
> +	/* Don't enable regulator if it's MSP1/3 */

Why not?

> +static void ux500_msp_dai_shutdown(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{

> +	if (drvdata->reg_enabled) {
> +		ret = regulator_disable(drvdata->reg_vape);
> +		if (ret < 0)
> +			dev_err(dai->dev,
> +				"%s: ERROR: Failed to disable regulator (%d)!\n",
> +				__func__, ret);
> +		drvdata->reg_enabled = 0;
> +	}

This looks like the driver is going to get confused with bidirectional
audio - if one direction stops then it'll turn the regulator off.

> +	/* Don't enable regulator if it's MSP1/3 */
> +	if (!drvdata->reg_enabled && (drvdata->msp->id != MSP_1_I2S_CONTROLLER)
> +		&& (drvdata->msp->id != MSP_3_I2S_CONTROLLER)) {

This seems confused, you're enabling in multiple places...

> +static int ux500_msp_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	unsigned int mask, slots_active;
> +	struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(dai->dev);
> +
> +	dev_dbg(dai->dev, "%s: MSP %d (%s): Enter.\n",
> +			__func__, dai->id, snd_soc_stream_str(substream));
> +
> +	switch (drvdata->fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		if (params_channels(params) != 2) {
> +			dev_err(dai->dev,
> +				"%s: Error: I2S requires ch = 2 (ch = %d)!\n",
> +				__func__, params_channels(params));
> +			return -EINVAL;
> +		}

Should really set up constraints for this, though in principle format
can change at runtime (though it rarely does).  Perhaps we should do
something special if the configuration happens on init...

> +		if (params_channels(params) != slots_active) {
> +			dev_err(dai->dev,
> +				"%s: Error: Channels to slots mismatch (ch = %d, slots = %d)!\n",
> +				__func__, params_channels(params),
> +				slots_active);
> +			return -EINVAL;
> +		}

Similarly here.

> +	drvdata->reg_vape = regulator_get(NULL, "v-ape");
> +	if (IS_ERR(drvdata->reg_vape)) {

No, this should be using the struct device.  Regulators should always
be requested in the context of their consumer.  You could use
devm_regulator_get() too (there's a clock one too, but only in -next).

> +	ret = ux500_msp_i2s_init_msp(pdev, &drvdata->msp, platform_data);
> +	if (!drvdata->msp) {

Should be no need to cast away from void.

> +
> +	ux500_msp_i2s_cleanup_msp(pdev, drvdata->msp);
> +	devm_kfree(&pdev->dev, drvdata);

You're missing the point of devm_ here! :)

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

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