| Message ID | 1432925772-117760-3-git-send-email-yang.a.fang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 29, 2015 at 11:56:12AM -0700, yang.a.fang@intel.com wrote: > + if (event & SND_JACK_MICROPHONE) { > + > + pin_status = snd_soc_dapm_get_pin_status(&codec->dapm, "SHDN"); > + if (!pin_status) > + snd_soc_dapm_force_enable_pin(&codec->dapm, "SHDN"); This seems wrong - either we need the pin enabled or we don't. If it's currently enabled for something transient like playback then it might get turned off later so we should still force it on. > + snd_soc_dapm_disable_pin(&codec->dapm, "MICBIAS"); > + snd_soc_dapm_sync(&codec->dapm); > + /** > + * SHDN is max980090 shutdown pin we can not disable > + * it in case we are in the middle of playabck or record > + * we mark it unlock only so dapm will take care of it > + * next time > + */ > + snd_soc_dapm_disable_pin_unlocked(&codec->dapm, "SHDN"); This is wrong, you're mixing locked and unlocked versions of the DAPM operations which can't be right - the difference between locked and unlocked versions of the operations is that the locked versions is if the locks for DAPM are already held and clearly there's no locking code here. This last operation shuld be a normal _disable_pin(). I'd also expect it to be before the sync - there's no telling how long it'll be till the next sync otherwise and no reason to leave the pin forced on if it's not needed. If it is in use due to some other thing then DAPM should ensure that the state doesn't get changed by the disable. Indentation is broken for the comment too.
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Wednesday, June 03, 2015 11:16 AM > To: Fang, Yang A > Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; > dgreid@chromium.org; Nujella, Sathyanarayana; > kevin.strasser@linux.intel.com; Sripathi, Srinivas; Iriawan, Denny; Jain, > Praveen K; Koul, Vinod > Subject: Re: [PATCH v3 3/3] ASoC: Intel: fixed TI button detection > > On Fri, May 29, 2015 at 11:56:12AM -0700, yang.a.fang@intel.com wrote: > > > + if (event & SND_JACK_MICROPHONE) { > > + > > + pin_status = snd_soc_dapm_get_pin_status(&codec->dapm, > "SHDN"); > > + if (!pin_status) > > + snd_soc_dapm_force_enable_pin(&codec->dapm, > "SHDN"); > > This seems wrong - either we need the pin enabled or we don't. If it's > currently enabled for something transient like playback then it might get > turned off later so we should still force it on. Got it. I will remove the condition. > > > + snd_soc_dapm_disable_pin(&codec->dapm, "MICBIAS"); > > + snd_soc_dapm_sync(&codec->dapm); > > + /** > > + * SHDN is max980090 shutdown pin we can not disable > > + * it in case we are in the middle of playabck or record > > + * we mark it unlock only so dapm will take care of it > > + * next time > > + */ > > + snd_soc_dapm_disable_pin_unlocked(&codec->dapm, > "SHDN"); > > This is wrong, you're mixing locked and unlocked versions of the DAPM > operations which can't be right - the difference between locked and > unlocked versions of the operations is that the locked versions is if the locks > for DAPM are already held and clearly there's no locking code here. This last > operation shuld be a normal _disable_pin(). > Thanks Mark. I will use snd_soc_dapm_disable_pin > I'd also expect it to be before the sync - there's no telling how long it'll be till > the next sync otherwise and no reason to leave the pin forced on if it's not > needed. If it is in use due to some other thing then DAPM should ensure > that the state doesn't get changed by the disable. > I was thinking widget will be disabled right after calling snd_soc_dapm_sync I just tried to move SHDN before calling snd_soc_dapm_sync .as you said if it is in use dapm will not turn it off immediately for example if playback is ongoing.It will be turned off few seconds after playback stops. > Indentation is broken for the comment too. I will remove the comments since I will move the disable SHDN pin code before calling snd_soc_dapm_sync
diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c index 1be0794..3e5842d 100644 --- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c +++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c @@ -101,6 +101,43 @@ static int cht_aif1_hw_params(struct snd_pcm_substream *substream, return 0; } +static int cht_ti_jack_event(struct notifier_block *nb, + unsigned long event, void *data) +{ + + struct snd_soc_jack *jack = (struct snd_soc_jack *)data; + struct snd_soc_dai *codec_dai = jack->card->rtd->codec_dai; + struct snd_soc_codec *codec = codec_dai->codec; + int pin_status; + + if (event & SND_JACK_MICROPHONE) { + + pin_status = snd_soc_dapm_get_pin_status(&codec->dapm, "SHDN"); + if (!pin_status) + snd_soc_dapm_force_enable_pin(&codec->dapm, "SHDN"); + + snd_soc_dapm_force_enable_pin(&codec->dapm, "MICBIAS"); + snd_soc_dapm_sync(&codec->dapm); + } else { + + snd_soc_dapm_disable_pin(&codec->dapm, "MICBIAS"); + snd_soc_dapm_sync(&codec->dapm); + /** + * SHDN is max980090 shutdown pin we can not disable + * it in case we are in the middle of playabck or record + * we mark it unlock only so dapm will take care of it + * next time + */ + snd_soc_dapm_disable_pin_unlocked(&codec->dapm, "SHDN"); + } + + return 0; +} + +static struct notifier_block cht_jack_nb = { + .notifier_call = cht_ti_jack_event, +}; + static int cht_codec_init(struct snd_soc_pcm_runtime *runtime) { int ret; @@ -130,6 +167,9 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime) return ret; } + if (ctx->ts3a227e_present) + snd_soc_jack_notifier_register(jack, &cht_jack_nb); + return ret; }