| Message ID | 1423053895-24342-1-git-send-email-yang.jie@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 04, 2015 at 08:44:55PM +0800, Jie Yang wrote: > Some platforms, e.g. WSB, don't need irq handler and jack detection when > system is in Suspend, for power save reason. > > Here add irq enable/disable APIs and export them, also disable LDO1, > which is used for jack detection when headphone is plugged in. > +int rt286_enable_irq(struct snd_soc_codec *codec) > +{ This isn't a great idiom for the API - the machine driver isn't working with the interrupt, it's working with the jack detection so it should do the disable and reenable in terms of the jack detection too. Otherwise it's not clear that this won't interfere with other interrupt functions the device has and that it'll disable the jack detection hardware in the device rather than just ignoring the interrupt (which is presumably desired and appears to be actually happening in the code).
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Wednesday, February 04, 2015 8:59 PM > To: Jie, Yang > Cc: alsa-devel@alsa-project.org; bardliao@realtek.com; Girdwood, Liam R > Subject: Re: [PATCH] ASoC: rt286: add irq enable/disable APIs and export > > On Wed, Feb 04, 2015 at 08:44:55PM +0800, Jie Yang wrote: > > Some platforms, e.g. WSB, don't need irq handler and jack detection > > when system is in Suspend, for power save reason. > > > > Here add irq enable/disable APIs and export them, also disable LDO1, > > which is used for jack detection when headphone is plugged in. > > > +int rt286_enable_irq(struct snd_soc_codec *codec) { > > This isn't a great idiom for the API - the machine driver isn't working with the > interrupt, it's working with the jack detection so it should do the disable and > reenable in terms of the jack detection too. Otherwise it's not clear that this > won't interfere with other interrupt functions the device has and that it'll > disable the jack detection hardware in the device rather than just ignoring the > interrupt (which is presumably desired and appears to be actually happening in > the code). [Keyon] thanks for review, Mark, do you mean we should change the function names to rt286_enable/disable_jack_detection()?
On Wed, Feb 04, 2015 at 01:12:25PM +0000, Jie, Yang wrote: > [Keyon] thanks for review, Mark, do you mean we should change the > function names to rt286_enable/disable_jack_detection()? Yes, that should be fine.
diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c index 8104d22..66afb9b 100644 --- a/sound/soc/codecs/rt286.c +++ b/sound/soc/codecs/rt286.c @@ -400,6 +400,30 @@ int rt286_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack) } EXPORT_SYMBOL_GPL(rt286_mic_detect); +int rt286_enable_irq(struct snd_soc_codec *codec) +{ + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec); + + if (rt286->jack->status | SND_JACK_HEADPHONE) + snd_soc_dapm_force_enable_pin(&rt286->codec->dapm, "LDO1"); + snd_soc_dapm_sync(&rt286->codec->dapm); + enable_irq(rt286->i2c->irq); + return 0; +} +EXPORT_SYMBOL_GPL(rt286_enable_irq); + +int rt286_disable_irq(struct snd_soc_codec *codec) +{ + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec); + + disable_irq(rt286->i2c->irq); + + snd_soc_dapm_disable_pin(&rt286->codec->dapm, "LDO1"); + snd_soc_dapm_sync(&rt286->codec->dapm); + return 0; +} +EXPORT_SYMBOL_GPL(rt286_disable_irq); + static int is_mclk_mode(struct snd_soc_dapm_widget *source, struct snd_soc_dapm_widget *sink) { diff --git a/sound/soc/codecs/rt286.h b/sound/soc/codecs/rt286.h index b539b73..fb18331 100644 --- a/sound/soc/codecs/rt286.h +++ b/sound/soc/codecs/rt286.h @@ -193,6 +193,8 @@ enum { }; int rt286_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack); +int rt286_enable_irq(struct snd_soc_codec *codec); +int rt286_disable_irq(struct snd_soc_codec *codec); #endif /* __RT286_H__ */