Re: [PATCH] ASoC: Intel: Add Merrifield machine driver

[Vinod Koul <vinod.koul@intel.com>]

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

On Fri, Oct 31, 2014 at 05:11:27PM +0000, Mark Brown wrote:
> On Fri, Oct 31, 2014 at 03:41:38PM +0530, Vinod Koul wrote:
> 
> > @@ -26,6 +26,7 @@ snd-soc-sst-haswell-objs := haswell.o
> >  snd-soc-sst-byt-rt5640-mach-objs := byt-rt5640.o
> >  snd-soc-sst-byt-max98090-mach-objs := byt-max98090.o
> >  snd-soc-sst-broadwell-objs := broadwell.o
> > +snd-merr-dpcm-wm8958-objs := merr_dpcm_wm8958.o
> 
> Notice the difference in naming style for the module here...
shouldnt be, will amke it consistent

> 
> > +	switch (level) {
> > +	case SND_SOC_BIAS_STANDBY:
> > +		/*
> > +		 * We are in standby down so */
> > +		/* Switch to 32KHz MCLK2 input clock for codec
> > +		 */
> 
> Interesting coding style!
mea culpa, looks like fixing this cause issue :(
> 
> > +static const struct wm8958_micd_rate micdet_rates[] = {
> > +	{ 32768,       true,  1, 4 },
> > +	{ 32768,       false, 1, 1 },
> > +	{ 44100 * 256, true,  7, 10 },
> > +	{ 44100 * 256, false, 7, 10 },
> > +};
> 
> This isn't referenced anywhere.
Yup, this should come in subsequent jack detect patch not here
> 
> > +static int mrfld_8958_init(struct snd_soc_pcm_runtime *runtime)
> > +{
> > +	int ret;
> > +	struct snd_soc_card *card = runtime->card;
> > +	struct mrfld_8958_mc_private *ctx = snd_soc_card_get_drvdata(card);
> > +	struct snd_soc_dai *aif1_dai = card->rtd[CODEC_BE].codec_dai;
> > +	struct snd_soc_codec *codec = aif1_dai->codec;
> > +
> > +	if (!aif1_dai)
> > +		return -ENODEV;
> > +
> > +	ret = snd_soc_dai_set_tdm_slot(aif1_dai, 0xf0, 0xf0, 4, 24);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "can't set codec pcm format %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	card->dapm.idle_bias_off = true;
> > +
> > +	if (!codec) {
> > +		dev_err(card->dev, "%s: we didnt find the codec pointer!\n", __func__);
> > +		return 0;
> > +	}
> 
> This should be an error if it's even worth checking for (which it
> shouldn't be).
yes..

> 
> > +	snd_jack_set_key(ctx->jack.jack, SND_JACK_BTN_1, KEY_MEDIA);
> > +	snd_jack_set_key(ctx->jack.jack, SND_JACK_BTN_0, KEY_MEDIA);
> 
> Two buttons both reporting the same key?  Might be worth a comment.
Okay loooking back I am not sure why we added two here, let me check and
clean this up.

> 
> > +	ret_val = snd_soc_register_card(&snd_soc_card_mrfld);
> 
> devm_snd_soc_register_card() and you can loose the remove function.
Sure

-- 
~Vinod


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

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