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 13:23:44 +0100	[thread overview]
Message-ID: <4274589c-9a52-7f1a-f937-1c5d60b76559@redhat.com> (raw)
In-Reply-To: <20210122112607.GH106851@ediswmail.ad.cirrus.com>

Hi,

On 1/22/21 12:26 PM, Charles Keepax wrote:
> On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> 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:
>>>>>
>>>>> Convert the arizona extcon driver into a helper library for direct use
>>>>> from the arizona codec-drivers, rather then being bound to a separate
>>>>> MFD cell.
>>>>>
>>>>> Note the probe (and remove) sequence is split into 2 parts:
>>>>>
>>>>> 1. The arizona_jack_codec_dev_probe() function inits a bunch of
>>>>> jack-detect specific variables in struct arizona_priv and tries to get
>>>>> a number of resources where getting them may fail with -EPROBE_DEFER.
>>>>>
>>>>> 2. Then once the machine driver has create a snd_sock_jack through
>>>>> snd_soc_card_jack_new() it calls snd_soc_component_set_jack() on
>>>>> the codec component, which will call the new arizona_jack_set_jack(),
>>>>> which sets up jack-detection and requests the IRQs.
>>>>>
>>>>> This split is necessary, because the IRQ handlers need access to the
>>>>> arizona->dapm pointer and the snd_sock_jack which are not available
>>>>> when the codec-driver's probe function runs.
>>>>>
>>>>> Note this requires that machine-drivers for codecs which are converted
>>>>> to use the new helper functions from arizona-jack.c are modified to
>>>>> create a snd_soc_jack through snd_soc_card_jack_new() and register
>>>>> this jack with the codec through snd_soc_component_set_jack().
>>>>
>>>> ...
>>>>
>>>>> +int arizona_jack_codec_dev_probe(struct arizona_priv *info, struct device *dev)
>>>>>   {
>>>>> -       struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
>>>>> +       struct arizona *arizona = info->arizona;
>>>>>          struct arizona_pdata *pdata = &arizona->pdata;
>>>>
>>>>> +       int ret, mode;
>>>>>
>>>>>          if (!dev_get_platdata(arizona->dev))
>>>>> -               arizona_extcon_device_get_pdata(&pdev->dev, arizona);
>>>>> +               arizona_extcon_device_get_pdata(dev, arizona);
>>>>>
>>>>> -       info->micvdd = devm_regulator_get(&pdev->dev, "MICVDD");
>>>>> +       info->micvdd = devm_regulator_get(arizona->dev, "MICVDD");
>>>>
>>>> I'm wondering if arizona->dev == dev here. if no, can this function
>>>> get a comment / kernel-doc explaining what dev is?
>>>>
>>>
>>> pdev->dev would be *this* driver.
>>> arizona->dev should be the MFD parent driver.
>>>
>>> I think these gets should be against the dev passed in as argument
>>> (I assume that is the caller's pdev->dev). So they are owned by this
>>> driver, not its parent.
>>
>> Right, this is all correct.
>>
>> The reason why I used arizona->dev instead of dev for the devm_regulator_get()
>> is because the codec code already does a regulator_get for MICVDD through:
>>
>> SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
>>
>> And doing it again leads to an error being logged about trying to
>> create a file in debugs with a name which already exists, because now
>> we do a regulator_get("MICVDD") with the same consumer twice.
>>
>> But I now see that I overlooked the devm part, turning my "fix" from
>> a cute hack to just being outright wrong.
>>
> 
> Aye we should definitely drop the devm here.

We can keep the devm as long as we pass the codec child-device as dev
parameter, this will introduce the mentioned debugfs error getting
logged, but other then the logging of that error being a bit
ugly it is harmless .

But see below.

>> So there are a number of solutions here:
>>
>>
>> 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).
>>
> 
> Not ideal but as you say might be the best thing for now.

Ack, but again see below.

>> 2. Switch the arizona-jack code from directly poking the regulator
>> to using snd_soc_component_force_enable_pin("MICVDD") and
>> snd_soc_component_disable_pin("MICVDD"). I like this, but there is
>> one downside, the dapm code assumes that when the regulator is
>> enabled the bypass must be disabled:
>>
> ...
>>
>> When enabling MIC-current / button-press IRQs.
>>
>> If we switch to using snd_soc_component_force_enable_pin("MICVDD") and
>> snd_soc_component_disable_pin("MICVDD") we loose the power-saving
>> of using the bypass when we only need MICVDD for button-press
>> detection.
>>
> 
> Yeah we really don't want to force the micbias's to be regulated
> during button detect, so I think this option has to go.

Ok.

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

Regards,

Hans