| Message ID | 20220609133541.3984886-12-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: codecs: Series of fixes for realtek codecs used on RVPs | expand |
On Thu, Jun 09, 2022 at 03:35:41PM +0200, Amadeusz Sławiński wrote: > On our RVP platforms using rt298 with combojack we've seen issues with > controls being in incorrect state after suspend/resume cycle. This is > caused by codec driver not setting pins to correct state and causing > codec suspend method to not be called. Which on resume caused codec > registers to be in undefined state. Fix this by setting pins correctly > in jack detect function. Again fixes should go before cleanups. Could you be more specific about what was wrong with the existing code and how this fixes it? > static int rt298_mic_detect(struct snd_soc_component *component, > struct snd_soc_jack *jack, void *data) > { > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); > struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component); > - struct snd_soc_dapm_context *dapm; > - bool hp = false; > - bool mic = false; > - int status = 0; > > rt298->jack = jack; > > - /* If jack in NULL, disable HS jack */ > - if (!jack) { > + if (jack) { > + /* enable IRQ */ > + if (rt298->jack->status & SND_JACK_HEADPHONE) > + snd_soc_dapm_force_enable_pin(dapm, "LDO1"); > + if (rt298->jack->status & SND_JACK_MICROPHONE) { > + snd_soc_dapm_force_enable_pin(dapm, "HV"); > + snd_soc_dapm_force_enable_pin(dapm, "VREF"); > + } > + regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2); > + enable_irq(rt298->i2c->irq); > + snd_soc_jack_report(rt298->jack, rt298->jack->status, > + SND_JACK_MICROPHONE | SND_JACK_HEADPHONE); It looks rt298_jack_detect() already forces the pins on? It's not clear to me what the relationship between this code and the existing code is.
On 6/9/2022 4:55 PM, Mark Brown wrote: > On Thu, Jun 09, 2022 at 03:35:41PM +0200, Amadeusz Sławiński wrote: >> On our RVP platforms using rt298 with combojack we've seen issues with >> controls being in incorrect state after suspend/resume cycle. This is >> caused by codec driver not setting pins to correct state and causing >> codec suspend method to not be called. Which on resume caused codec >> registers to be in undefined state. Fix this by setting pins correctly >> in jack detect function. > > Again fixes should go before cleanups. Could you be more specific about > what was wrong with the existing code and how this fixes it? > >> static int rt298_mic_detect(struct snd_soc_component *component, >> struct snd_soc_jack *jack, void *data) >> { >> + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); >> struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component); >> - struct snd_soc_dapm_context *dapm; >> - bool hp = false; >> - bool mic = false; >> - int status = 0; >> >> rt298->jack = jack; >> >> - /* If jack in NULL, disable HS jack */ >> - if (!jack) { >> + if (jack) { >> + /* enable IRQ */ >> + if (rt298->jack->status & SND_JACK_HEADPHONE) >> + snd_soc_dapm_force_enable_pin(dapm, "LDO1"); >> + if (rt298->jack->status & SND_JACK_MICROPHONE) { >> + snd_soc_dapm_force_enable_pin(dapm, "HV"); >> + snd_soc_dapm_force_enable_pin(dapm, "VREF"); >> + } >> + regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2); >> + enable_irq(rt298->i2c->irq); >> + snd_soc_jack_report(rt298->jack, rt298->jack->status, >> + SND_JACK_MICROPHONE | SND_JACK_HEADPHONE); > > It looks rt298_jack_detect() already forces the pins on? It's not clear > to me what the relationship between this code and the existing code is. This aligns the code to be similar to other two rt2xx drivers and fixes a problem on our side. Original code doesn't reach rt298_jack_detect() when jack == NULL, so it never disables those pins in this case. I could probably fix this by moving rt298_jack_detect() call, but as drivers for rt2xx codecs are quite similar to each other I opted to fix the issue by minimizing the differences between them.
On Fri, Jun 10, 2022 at 11:46:19AM +0200, Amadeusz Sławiński wrote: > On 6/9/2022 4:55 PM, Mark Brown wrote: > > It looks rt298_jack_detect() already forces the pins on? It's not clear > > to me what the relationship between this code and the existing code is. > This aligns the code to be similar to other two rt2xx drivers and fixes a > problem on our side. > Original code doesn't reach rt298_jack_detect() when jack == NULL, so it > never disables those pins in this case. > I could probably fix this by moving rt298_jack_detect() call, but as drivers > for rt2xx codecs are quite similar to each other I opted to fix the issue by > minimizing the differences between them. It would be good to at least clarify what's going on in the changelog - it's not really clear what exactly the problem is or how this fixes it.
diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c index 2af037536bc9..f1c3dfdea5e7 100644 --- a/sound/soc/codecs/rt298.c +++ b/sound/soc/codecs/rt298.c @@ -330,36 +330,31 @@ static void rt298_jack_detect_work(struct work_struct *work) static int rt298_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *jack, void *data) { + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component); - struct snd_soc_dapm_context *dapm; - bool hp = false; - bool mic = false; - int status = 0; rt298->jack = jack; - /* If jack in NULL, disable HS jack */ - if (!jack) { + if (jack) { + /* enable IRQ */ + if (rt298->jack->status & SND_JACK_HEADPHONE) + snd_soc_dapm_force_enable_pin(dapm, "LDO1"); + if (rt298->jack->status & SND_JACK_MICROPHONE) { + snd_soc_dapm_force_enable_pin(dapm, "HV"); + snd_soc_dapm_force_enable_pin(dapm, "VREF"); + } + regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2); + enable_irq(rt298->i2c->irq); + snd_soc_jack_report(rt298->jack, rt298->jack->status, + SND_JACK_MICROPHONE | SND_JACK_HEADPHONE); + } else { disable_irq(rt298->i2c->irq); regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x0); - dapm = snd_soc_component_get_dapm(component); + snd_soc_dapm_disable_pin(dapm, "HV"); + snd_soc_dapm_disable_pin(dapm, "VREF"); snd_soc_dapm_disable_pin(dapm, "LDO1"); - snd_soc_dapm_sync(dapm); - return 0; } - - regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2); - enable_irq(rt298->i2c->irq); - - rt298_jack_detect(rt298, &hp, &mic); - if (hp) - status |= SND_JACK_HEADPHONE; - - if (mic) - status |= SND_JACK_MICROPHONE; - - snd_soc_jack_report(rt298->jack, status, - SND_JACK_MICROPHONE | SND_JACK_HEADPHONE); + snd_soc_dapm_sync(dapm); return 0; }