On Tue, Jul 14, 2015 at 6:28 PM, Mark Brown broonie@kernel.org wrote:
On Tue, Jul 14, 2015 at 10:09:44AM +0000, Bard Liao wrote:
Thanks for the review. I think what we need is something like
snd_soc_dapm_force_enable_pin(dapm, "ADC L power");snd_soc_dapm_force_enable_pin(dapm, "ADC R power");snd_soc_dapm_sync(dapm);if (!codec->component.card->instantiated) {regmap_update_bits(rt5645->regmap, RT5645_PWR_DIG1,RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT,RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT);}Yes, that's more what I'd expect. You could probably just do the regmap update unconditionally since it shouldn't make any difference but it's a bit neater this way.
rt5645_enable_push_button_irq (where this code is added), is only called from rt5645_jack_detect, where this kind of pattern is currently common:
if (codec->component.card->instantiated) snd_soc_dapm_force_enable_pin(dapm, ...) else regmap_update_bits(...)
Not saying this is right, but if we fix this one we should fix them all.
The problem that I'm trying to solve with this series, is that rt5645->codec might still be null when rt5645_jack_detect and rt5645_enable_push_button_irq are first called, so in some cases we do not have a valid dapm pointer yet, and the test above is modified in 3/3 of the series...
If you look at patch 3/3 of the series, I do something like this, early in the function: + struct snd_soc_dapm_context *dapm = NULL; + + if (rt5645->codec && rt5645->codec->component.card->instantiated) { + dapm = snd_soc_codec_get_dapm(rt5645->codec); + }
and then use this pattern:
if (dapm) snd_soc_dapm_force_enable_pin(dapm, ...) else regmap_update_bits(...)
If guess something like this might be preferable: if (rt5645->codec) { dapm = snd_soc_codec_get_dapm(rt5645->codec); }
and then:
if (dapm) snd_soc_dapm_force_enable_pin(dapm, ...)
regmap_update_bits(...)
Does that make sense?
Is there a better way to communicate my intent in this series? Maybe patch 1/3 should convert everyhing to this pattern: snd_soc_dapm_force_enable_pin(dapm, ...) regmap_update_bits(...)
And then 3/3 would add the if (dapm) tests?
Thanks for the feedback.