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

From: Hans de Goede <hdegoede@redhat.com>
To: Charles Keepax <ckeepax@opensource.cirrus.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 14:36:01 +0100	[thread overview]
Message-ID: <5436af42-e6d1-b0ed-7f16-60658b590919@redhat.com> (raw)
In-Reply-To: <20210122130412.GI106851@ediswmail.ad.cirrus.com>

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.

>> 1. 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