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

[Charles Keepax]

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@xxxxxxxxxx> 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.

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

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

> 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