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

[Hans de Goede <hdegoede@redhat.com>]

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.

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


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:

int dapm_regulator_event(struct snd_soc_dapm_widget *w,
                   struct snd_kcontrol *kcontrol, int event)
{
        int ret;

        soc_dapm_async_complete(w->dapm);

        if (SND_SOC_DAPM_EVENT_ON(event)) {
                if (w->on_val & SND_SOC_DAPM_REGULATOR_BYPASS) {
                        ret = regulator_allow_bypass(w->regulator, false);
                        if (ret != 0)
                                dev_warn(w->dapm->dev,
                                         "ASoC: Failed to unbypass %s: %d\n",
                                         w->name, ret);
                }

                return regulator_enable(w->regulator);
        } else {
		...

Which is good when the MICBIAS# are being used for recording,
or for detecting the type of device being plugged in. But when
just doing button-press detection, then we can use a combination
of bypass=true, enabled=true (Note enabled=false completely disables
MICVDD independent of the bypass setting). This uses less energy
then bypass=false, enabled=true. So ATM the jack/extcon code
does this:

        if (info->detecting) {
                ret = regulator_allow_bypass(info->micvdd, false);
                if (ret != 0) {
                        dev_err(arizona->dev,
                                "Failed to regulate MICVDD: %d\n",
                                ret);
                }
        }

        ret = regulator_enable(info->micvdd);
        if (ret != 0) {
                dev_err(arizona->dev, "Failed to enable MICVDD: %d\n",
                        ret);
        }

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.

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.

Keeping in mind that switching to force_enable fixes the current racy code,
as well as the KISS-ness of this solution, I personally prefer this option
over option 1 as it makes the code cleaner and more correct.
I could easily do this in a next version of this series if people agree
with going this route.


3. Stop using SND_SOC_DAPM_REGULATOR_SUPPLY for MICVDD, instead making
it a custom DAPM source pin, with an event callback and do have 2
ref-counts for the regulator settings, 1 bypass_disable refcount,
where we enable the bypass if this reaches 0 and if either the
jack-detect or DAPM says the bypass must be disabled then we
disable it. and a second refcount for if the regulator itself
needs to be enabled / disabled (which is already present inside
the regulator-core code, so we don't need to duplicate this).

This solution would be the best solution as making bypass_disable
a refcount-like setting would fix the race, while keeping the
power-saving. This is however best done after the jack-detect
code has been moved from being a separate driver to being part
of the codec drivers. So this is best left as a follow-up to
this series IMHO.

Regards,

Hans