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

[Ola Lilja <ola.o.lilja@stericsson.com>]

On 04/23/2012 08:29 PM, Mark Brown wrote:

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


Good to hear!

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


Other blocks take this regulator and when the driver needs to go into low-power
mode this other block will disable the regulator. However, for now I think we
can safely remove this special-case and take the regulator even for MSP1/3.

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


Yes, will fix this.

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


Indeed. Will fix.

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


OK, I will try to rewrite these checks to use constraints instead.

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


OK, I'll look into that.

> 
>> +	ret = ux500_msp_i2s_init_msp(pdev, &drvdata->msp, platform_data);
>> +	if (!drvdata->msp) {
> 
> Should be no need to cast away from void.


OK, will change.

> 
>> +
>> +	ux500_msp_i2s_cleanup_msp(pdev, drvdata->msp);
>> +	devm_kfree(&pdev->dev, drvdata);
> 
> You're missing the point of devm_ here! :)


Ah! =)