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