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
next prev parent reply other threads:[~2021-01-22 13:37 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-17 16:05 [PATCH v2 00/12] MFD/extcon/ASoC: Rework arizona codec jack-detect support Hans de Goede
2021-01-17 16:05 ` [PATCH v2 01/12] mfd: arizona: Drop arizona-extcon cells Hans de Goede
2021-01-17 16:05 ` [PATCH v2 02/12] ASoC: arizona-jack: Add arizona-jack.c Hans de Goede
2021-01-18 11:57 ` Andy Shevchenko
2021-01-17 16:05 ` [PATCH v2 03/12] ASoC: arizona-jack: Fix some issues when HPDET IRQ fires after the jack has been unplugged Hans de Goede
2021-01-18 12:47 ` Mark Brown
2021-01-21 15:58 ` Hans de Goede
2021-01-22 11:07 ` Charles Keepax
2021-01-17 16:05 ` [PATCH v2 04/12] ASoC: arizona-jack: Fix various races on driver unbind Hans de Goede
2021-01-17 16:05 ` [PATCH v2 05/12] ASoC: arizona-jack: Fix flags parameter to the gpiod_get("wlf,micd-pol") call Hans de Goede
2021-01-17 16:05 ` [PATCH v2 06/12] ASoC: arizona-jack: Move jack-detect variables to struct arizona_priv Hans de Goede
2021-01-18 12:01 ` Andy Shevchenko
2021-01-22 11:12 ` Charles Keepax
2021-01-17 16:05 ` [PATCH v2 07/12] ASoC: arizona-jack: Use arizona->dev for runtime-pm Hans de Goede
2021-01-18 12:02 ` Andy Shevchenko
2021-01-22 0:03 ` Hans de Goede
2021-01-22 9:38 ` Andy Shevchenko
2021-01-22 13:56 ` Hans de Goede
2021-01-17 16:05 ` [PATCH v2 08/12] ASoC: arizona-jack: convert into a helper library for codec drivers Hans de Goede
2021-01-18 17:24 ` Andy Shevchenko
2021-01-19 9:51 ` Richard Fitzgerald
2021-01-21 16:55 ` Hans de Goede
2021-01-22 11:26 ` Charles Keepax
2021-01-22 12:23 ` Hans de Goede
2021-01-22 13:04 ` Charles Keepax
2021-01-22 13:36 ` Hans de Goede [this message]
2021-01-22 13:21 ` Charles Keepax
2021-01-17 16:05 ` [PATCH v2 09/12] ASoC: arizona-jack: Use snd_soc_jack to report jack events Hans de Goede
2021-01-17 16:05 ` [PATCH v2 10/12] ASoC: arizona: Make the wm5102, wm5110, wm8997 and wm8998 drivers use the new jack library Hans de Goede
2021-01-17 16:05 ` [PATCH v2 11/12] ASoC: Intel: bytcr_wm5102: Add jack detect support Hans de Goede
2021-01-17 16:05 ` [PATCH v2 12/12] extcon: arizona: Drop the arizona extcon driver Hans de Goede
2021-01-18 12:13 ` Andy Shevchenko
2021-01-18 9:55 ` [PATCH v2 00/12] MFD/extcon/ASoC: Rework arizona codec jack-detect support Lee Jones
2021-01-18 10:28 ` Hans de Goede
2021-01-18 10:47 ` Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5436af42-e6d1-b0ed-7f16-60658b590919@redhat.com \
--to=hdegoede@redhat.com \
--cc=alsa-devel@alsa-project.org \
--cc=andy.shevchenko@gmail.com \
--cc=broonie@kernel.org \
--cc=cezary.rojewski@intel.com \
--cc=ckeepax@opensource.cirrus.com \
--cc=lee.jones@linaro.org \
--cc=liam.r.girdwood@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rf@opensource.cirrus.com \
--cc=yang.jie@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).