Hi,
On 1/22/21 2:04 PM, Charles Keepax wrote:
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).
<note replying in a singe email to 2 strongly related replies from Charles on this>
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.
So I just double checked the regulator core code and you are right that the bypass thing is per consumer. So we will indeed need 2 calls to regulator_get, one for the dapm use and one for the jack-det use since those 2 are independent.
Note your example does not work as you think it will though:
int regulator_allow_bypass(struct regulator *regulator, bool enable) { ...
if (enable && !regulator->bypass) { rdev->bypass_count++; ...
} else if (!enable && regulator->bypass) { rdev->bypass_count--; ... }
if (ret == 0) regulator->bypass = enable; }
So a second call to allow_bypass(..., true) from the same consumer will be a no-op.
Sharing the same struct regulator result between the dapm widget and the jack-det code would still be an issue though since it will introduce the race which I was worried about earlier.
- Keep the code as is, live with the debugfs error. This might be
best for now, as I don't want to grow the scope of this series too much. I will go with this for the next version of this series (unless I receive feedback otherwise before I get around to posting the next version).
I wonder if this commit was related to that:
commit ff268b56ce8c ("regulator: core: Don't spew backtraces on duplicate sysfs")
Apologies I don't have as much time as I normally would to look into such issues at the moment, due to various internal company things going on.
Actually you are being super helpful, thank you. I believe that with your latest email this is fully resolved.
I do suspect that this option is the way to go though and if there are issues of duplicates being created by the regulator core those probably need to be resolved in there. But that can probably be done separate from this series.
Good catch, thanks. This means that having multiple consumers / regulator_get calls from the same consumer-dev is supposed to work and the debugfs error needs to be silenced somehow. I will look into silencing the error (as a patch separate from this series).
Regards,
Hans