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! :)