Re: [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Richard Fitzgerald <rf@opensource.cirrus.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Cezary Rojewski <cezary.rojewski@intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Liam Girdwood <liam.r.girdwood@linux.intel.com>,
	Jie Yang <yang.jie@linux.intel.com>,
	Mark Brown <broonie@kernel.org>, <patches@opensource.cirrus.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ALSA Development Mailing List <alsa-devel@alsa-project.org>
Subject: Re: [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers
Date: Fri, 22 Jan 2021 13:04:12 +0000	[thread overview]
Message-ID: <20210122130412.GI106851@ediswmail.ad.cirrus.com> (raw)
In-Reply-To: <4274589c-9a52-7f1a-f937-1c5d60b76559@redhat.com>

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