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