On Fri, Jan 22, 2021 at 01:23:44PM +0100, Hans de Goede wrote:
On 1/22/21 12:26 PM, Charles Keepax wrote:
On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
On 1/19/21 10:51 AM, Richard Fitzgerald wrote:
On 18/01/2021 17:24, Andy Shevchenko wrote:
On Sun, Jan 17, 2021 at 6:06 PM Hans de Goede hdegoede@redhat.com wrote:
Note there is a pretty big issue with the original code here, if the MICVDD DAPM pin is on for an internal-mic and then we run through the jack-detect mic-detect sequence, we end up setting bypass=true causing the micbias for the internal-mic to no longer be what was configured. IOW poking the bypass setting underneath the DAPM code is racy.
The regulator bypass code keeps an internal reference count. All the users of the regulator need to allow bypass for it to be placed into bypass mode, so I believe this can't happen.
Ah I did not know that, since the regulator_allow_bypass function takes a bool rather then having enable/disable variants I thought it would directly set the bypass, but you are right. So this is not a problem, good.
So this has made me look at the problem again and I believe that a much better solution is to simply re-use the MICVDD regulator-reference which has been regulator_get-ed by the dapm code when instantiating the:
SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
widget. So I plan to have a new patch in v3 of the series which replaces the devm_regulator_get with something like this:
/* * There is a DAPM widget for the MICVDD regulator, since * the button-press detection has special requirements wrt * the regulator bypass settings we cannot directly * use snd_soc_component_force_enable_pin("MICVDD") / * snd_soc_component_disable_pin("MICVDD"). * * Instead we lookup the widget's regulator reference here * and use that to directly control the regulator. * Both the regulator's enable and bypass settings are * ref-counted so this will not interfere with the DAPM use * of the regulator. */ for_each_card_widgets(dapm->card, w) { if (!strcmp(w->name, "MICVDD")) info->micvdd_regulator = w->regulator; break; } }
(note I've not tested this yet, but I expect this to work fine).
Alas this won't work either. When I say reference count that isn't quite a totally accurate reflection of the usage of the function. When you call allow_bypass you are saying as this consumer of the regulator I don't mind if it goes into bypass. Then if all consumers agree the regulator will be put into bypass. So it is comparing the reference count to the number of consumers the regulator has to make a decision.
If you call allow_bypass independently from the jack detection code and the ASoC framework on the same consumer, as you describe here you will get bad effects. For example the regulator has two consumers, our CODEC driver and some other device. If our codec driver calls allow_bypass twice, then the regulator would go into bypass without the other consumer having approved it would could be fatal to that device.
Thanks, Charles