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:
- 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.
Aye we should definitely drop the devm here.
So there are a number of solutions here:
- 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.
- 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.
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.
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.
It is pretty problematic to loose the power benefits of the button detect, for the sake of making the code a little cleaner.
Thanks, Charles