| Message ID | 20210117160555.78376-9-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/12] mfd: arizona: Drop arizona-extcon cells | expand |
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? > if (IS_ERR(info->micvdd)) { > ret = PTR_ERR(info->micvdd); > dev_err(arizona->dev, "Failed to get MICVDD: %d\n", ret); Side note: at some point perhaps consider to use dev_err_probe() with functions which may return deferred probe error code. ... > + info->edev = devm_extcon_dev_allocate(dev, arizona_cable); > if (IS_ERR(info->edev)) { > - dev_err(&pdev->dev, "failed to allocate extcon device\n"); > + dev_err(arizona->dev, "failed to allocate extcon device\n"); Ditto about dev. > return -ENOMEM; > } ... > + ret = devm_gpio_request_one(dev, arizona->pdata.hpdet_id_gpio, > GPIOF_OUT_INIT_LOW, > "HPDET"); > if (ret != 0) { > dev_err(arizona->dev, "Failed to request GPIO%d: %d\n", > arizona->pdata.hpdet_id_gpio, ret); > - goto err_gpio; > + gpiod_put(info->micd_pol_gpio); Perhaps move before dev_err() ? Side comment: Do we need dev_err_probe() here? > + return ret; > }
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
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: > >>> > >>> 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
Hi, On 1/22/21 12:26 PM, Charles Keepax wrote: > 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: >>>>> >>>>> 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. We can keep the devm as long as we pass the codec child-device as dev parameter, this will introduce the mentioned debugfs error getting logged, but other then the logging of that error being a bit ugly it is harmless . But see below. >> 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. Ack, but again see below. >> 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. Ok. >> 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. Ah I did not know that, since the regulator_allow_bypass function takes a bool rather then having enable/disable variants I thought it would directly set the bypass, but you are right. So this is not a problem, good. So this has made me look at the problem again and I believe that a much better solution is to simply re-use the MICVDD regulator-reference which has been regulator_get-ed by the dapm code when instantiating the: SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), widget. So I plan to have a new patch in v3 of the series which replaces the devm_regulator_get with something like this: /* * There is a DAPM widget for the MICVDD regulator, since * the button-press detection has special requirements wrt * the regulator bypass settings we cannot directly * use snd_soc_component_force_enable_pin("MICVDD") / * snd_soc_component_disable_pin("MICVDD"). * * Instead we lookup the widget's regulator reference here * and use that to directly control the regulator. * Both the regulator's enable and bypass settings are * ref-counted so this will not interfere with the DAPM use * of the regulator. */ for_each_card_widgets(dapm->card, w) { if (!strcmp(w->name, "MICVDD")) info->micvdd_regulator = w->regulator; break; } } (note I've not tested this yet, but I expect this to work fine). Regards, Hans
On Fri, Jan 22, 2021 at 01:23:44PM +0100, Hans de Goede wrote: > On 1/22/21 12:26 PM, Charles Keepax wrote: > > On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote: > >> 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: > >> 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. > > Ah I did not know that, since the regulator_allow_bypass function > takes a bool rather then having enable/disable variants I thought > it would directly set the bypass, but you are right. So this is not > a problem, good. > > So this has made me look at the problem again and I believe that > a much better solution is to simply re-use the MICVDD regulator-reference > which has been regulator_get-ed by the dapm code when instantiating the: > > SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), > > widget. So I plan to have a new patch in v3 of the series which replaces > the devm_regulator_get with something like this: > > /* > * There is a DAPM widget for the MICVDD regulator, since > * the button-press detection has special requirements wrt > * the regulator bypass settings we cannot directly > * use snd_soc_component_force_enable_pin("MICVDD") / > * snd_soc_component_disable_pin("MICVDD"). > * > * Instead we lookup the widget's regulator reference here > * and use that to directly control the regulator. > * Both the regulator's enable and bypass settings are > * ref-counted so this will not interfere with the DAPM use > * of the regulator. > */ > for_each_card_widgets(dapm->card, w) { > if (!strcmp(w->name, "MICVDD")) > info->micvdd_regulator = w->regulator; > break; > } > } > > (note I've not tested this yet, but I expect this to work fine). > Alas this won't work either. When I say reference count that isn't quite a totally accurate reflection of the usage of the function. When you call allow_bypass you are saying as this consumer of the regulator I don't mind if it goes into bypass. Then if all consumers agree the regulator will be put into bypass. So it is comparing the reference count to the number of consumers the regulator has to make a decision. If you call allow_bypass independently from the jack detection code and the ASoC framework on the same consumer, as you describe here you will get bad effects. For example the regulator has two consumers, our CODEC driver and some other device. If our codec driver calls allow_bypass twice, then the regulator would go into bypass without the other consumer having approved it would could be fatal to that device. Thanks, Charles
On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote: > 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: > 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). Thinking about this more, I seem to remember this is something that has been discussed before, having the need to have situations where a driver and the framework are both managing the regulator at once on the same device. I wonder if this commit was related to that: commit ff268b56ce8c ("regulator: core: Don't spew backtraces on duplicate sysfs") Apologies I don't have as much time as I normally would to look into such issues at the moment, due to various internal company things going on. I do suspect that this option is the way to go though and if there are issues of duplicates being created by the regulator core those probably need to be resolved in there. But that can probably be done separate from this series. Thanks, Charles
Hi, On 1/22/21 2:04 PM, Charles Keepax wrote: > On Fri, Jan 22, 2021 at 01:23:44PM +0100, Hans de Goede wrote: >> On 1/22/21 12:26 PM, Charles Keepax wrote: >>> On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote: >>>> 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: >>>> 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. >> >> Ah I did not know that, since the regulator_allow_bypass function >> takes a bool rather then having enable/disable variants I thought >> it would directly set the bypass, but you are right. So this is not >> a problem, good. >> >> So this has made me look at the problem again and I believe that >> a much better solution is to simply re-use the MICVDD regulator-reference >> which has been regulator_get-ed by the dapm code when instantiating the: >> >> SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), >> >> widget. So I plan to have a new patch in v3 of the series which replaces >> the devm_regulator_get with something like this: >> >> /* >> * There is a DAPM widget for the MICVDD regulator, since >> * the button-press detection has special requirements wrt >> * the regulator bypass settings we cannot directly >> * use snd_soc_component_force_enable_pin("MICVDD") / >> * snd_soc_component_disable_pin("MICVDD"). >> * >> * Instead we lookup the widget's regulator reference here >> * and use that to directly control the regulator. >> * Both the regulator's enable and bypass settings are >> * ref-counted so this will not interfere with the DAPM use >> * of the regulator. >> */ >> for_each_card_widgets(dapm->card, w) { >> if (!strcmp(w->name, "MICVDD")) >> info->micvdd_regulator = w->regulator; >> break; >> } >> } >> >> (note I've not tested this yet, but I expect this to work fine). >> > <note replying in a singe email to 2 strongly related replies from Charles on this> > Alas this won't work either. When I say reference count that > isn't quite a totally accurate reflection of the usage of the > function. When you call allow_bypass you are saying as this > consumer of the regulator I don't mind if it goes into bypass. > Then if all consumers agree the regulator will be put into > bypass. So it is comparing the reference count to the number of > consumers the regulator has to make a decision. > > If you call allow_bypass independently from the jack detection > code and the ASoC framework on the same consumer, as you > describe here you will get bad effects. For example the > regulator has two consumers, our CODEC driver and some other > device. If our codec driver calls allow_bypass twice, then > the regulator would go into bypass without the other consumer > having approved it would could be fatal to that device. So I just double checked the regulator core code and you are right that the bypass thing is per consumer. So we will indeed need 2 calls to regulator_get, one for the dapm use and one for the jack-det use since those 2 are independent. Note your example does not work as you think it will though: int regulator_allow_bypass(struct regulator *regulator, bool enable) { ... if (enable && !regulator->bypass) { rdev->bypass_count++; ... } else if (!enable && regulator->bypass) { rdev->bypass_count--; ... } if (ret == 0) regulator->bypass = enable; } So a second call to allow_bypass(..., true) from the same consumer will be a no-op. Sharing the same struct regulator result between the dapm widget and the jack-det code would still be an issue though since it will introduce the race which I was worried about earlier. >> 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). > > I wonder if this commit was related to that: > > commit ff268b56ce8c ("regulator: core: Don't spew backtraces on duplicate sysfs") > > Apologies I don't have as much time as I normally would to look > into such issues at the moment, due to various internal company > things going on. Actually you are being super helpful, thank you. I believe that with your latest email this is fully resolved. > I do suspect that this option is the way to go though and if > there are issues of duplicates being created by the regulator > core those probably need to be resolved in there. But that can > probably be done separate from this series. Good catch, thanks. This means that having multiple consumers / regulator_get calls from the same consumer-dev is supposed to work and the debugfs error needs to be silenced somehow. I will look into silencing the error (as a patch separate from this series). Regards, Hans
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index d277f0366e09..2e976cfaa862 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -43,7 +43,7 @@ snd-soc-ak4642-objs := ak4642.o snd-soc-ak4671-objs := ak4671.o snd-soc-ak5386-objs := ak5386.o snd-soc-ak5558-objs := ak5558.o -snd-soc-arizona-objs := arizona.o +snd-soc-arizona-objs := arizona.o arizona-jack.o snd-soc-bd28623-objs := bd28623.o snd-soc-bt-sco-objs := bt-sco.o snd-soc-cpcap-objs := cpcap.o diff --git a/sound/soc/codecs/arizona-jack.c b/sound/soc/codecs/arizona-jack.c index a6e8071f84ab..eb46087f3ab7 100644 --- a/sound/soc/codecs/arizona-jack.c +++ b/sound/soc/codecs/arizona-jack.c @@ -7,14 +7,12 @@ #include <linux/kernel.h> #include <linux/module.h> -#include <linux/i2c.h> #include <linux/slab.h> #include <linux/interrupt.h> #include <linux/err.h> #include <linux/gpio/consumer.h> #include <linux/gpio.h> #include <linux/input.h> -#include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/property.h> #include <linux/regulator/consumer.h> @@ -1337,27 +1335,16 @@ static int arizona_extcon_device_get_pdata(struct device *dev, return 0; } -static int arizona_extcon_probe(struct platform_device *pdev) +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; - struct arizona_priv *info; - unsigned int val; - unsigned int clamp_mode; - int jack_irq_fall, jack_irq_rise; - int ret, mode, i, j; - - if (!arizona->dapm || !arizona->dapm->card) - return -EPROBE_DEFER; - - info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); - if (!info) - return -ENOMEM; + 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"); if (IS_ERR(info->micvdd)) { ret = PTR_ERR(info->micvdd); dev_err(arizona->dev, "Failed to get MICVDD: %d\n", ret); @@ -1365,12 +1352,10 @@ static int arizona_extcon_probe(struct platform_device *pdev) } mutex_init(&info->lock); - info->arizona = arizona; info->last_jackdet = ~(ARIZONA_MICD_CLAMP_STS | ARIZONA_JD1_STS); INIT_DELAYED_WORK(&info->hpdet_work, arizona_hpdet_work); INIT_DELAYED_WORK(&info->micd_detect_work, arizona_micd_detect); INIT_DELAYED_WORK(&info->micd_timeout_work, arizona_micd_timeout_work); - platform_set_drvdata(pdev, info); switch (arizona->type) { case WM5102: @@ -1404,20 +1389,20 @@ static int arizona_extcon_probe(struct platform_device *pdev) break; } - info->edev = devm_extcon_dev_allocate(&pdev->dev, arizona_cable); + info->edev = devm_extcon_dev_allocate(dev, arizona_cable); if (IS_ERR(info->edev)) { - dev_err(&pdev->dev, "failed to allocate extcon device\n"); + dev_err(arizona->dev, "failed to allocate extcon device\n"); return -ENOMEM; } - ret = devm_extcon_dev_register(&pdev->dev, info->edev); + ret = devm_extcon_dev_register(dev, info->edev); if (ret < 0) { dev_err(arizona->dev, "extcon_dev_register() failed: %d\n", ret); return ret; } - info->input = devm_input_allocate_device(&pdev->dev); + info->input = devm_input_allocate_device(dev); if (!info->input) { dev_err(arizona->dev, "Can't allocate input dev\n"); ret = -ENOMEM; @@ -1448,7 +1433,7 @@ static int arizona_extcon_probe(struct platform_device *pdev) else mode = GPIOF_OUT_INIT_LOW; - ret = devm_gpio_request_one(&pdev->dev, pdata->micd_pol_gpio, + ret = devm_gpio_request_one(dev, pdata->micd_pol_gpio, mode, "MICD polarity"); if (ret != 0) { dev_err(arizona->dev, "Failed to request GPIO%d: %d\n", @@ -1481,17 +1466,38 @@ static int arizona_extcon_probe(struct platform_device *pdev) } if (arizona->pdata.hpdet_id_gpio > 0) { - ret = devm_gpio_request_one(&pdev->dev, - arizona->pdata.hpdet_id_gpio, + ret = devm_gpio_request_one(dev, arizona->pdata.hpdet_id_gpio, GPIOF_OUT_INIT_LOW, "HPDET"); if (ret != 0) { dev_err(arizona->dev, "Failed to request GPIO%d: %d\n", arizona->pdata.hpdet_id_gpio, ret); - goto err_gpio; + gpiod_put(info->micd_pol_gpio); + return ret; } } + return 0; +} +EXPORT_SYMBOL_GPL(arizona_jack_codec_dev_probe); + +int arizona_jack_codec_dev_remove(struct arizona_priv *info) +{ + gpiod_put(info->micd_pol_gpio); + return 0; +} +EXPORT_SYMBOL_GPL(arizona_jack_codec_dev_remove); + +static int arizona_jack_enable_jack_detect(struct arizona_priv *info, + struct snd_soc_jack *jack) +{ + struct arizona *arizona = info->arizona; + struct arizona_pdata *pdata = &arizona->pdata; + unsigned int val; + unsigned int clamp_mode; + int jack_irq_fall, jack_irq_rise; + int ret, i, j; + if (arizona->pdata.micd_bias_start_time) regmap_update_bits(arizona->regmap, ARIZONA_MIC_DETECT_1, ARIZONA_MICD_BIAS_STARTTIME_MASK, @@ -1532,16 +1538,15 @@ static int arizona_extcon_probe(struct platform_device *pdev) if (arizona->pdata.num_micd_ranges > ARIZONA_MAX_MICD_RANGE) { dev_err(arizona->dev, "Too many MICD ranges: %d\n", arizona->pdata.num_micd_ranges); + return -EINVAL; } if (info->num_micd_ranges > 1) { for (i = 1; i < info->num_micd_ranges; i++) { if (info->micd_ranges[i - 1].max > info->micd_ranges[i].max) { - dev_err(arizona->dev, - "MICD ranges must be sorted\n"); - ret = -EINVAL; - goto err_gpio; + dev_err(arizona->dev, "MICD ranges must be sorted\n"); + return -EINVAL; } } } @@ -1559,8 +1564,7 @@ static int arizona_extcon_probe(struct platform_device *pdev) if (j == ARIZONA_NUM_MICD_BUTTON_LEVELS) { dev_err(arizona->dev, "Unsupported MICD level %d\n", info->micd_ranges[i].max); - ret = -EINVAL; - goto err_gpio; + return -EINVAL; } dev_dbg(arizona->dev, "%d ohms for MICD threshold %d\n", @@ -1629,43 +1633,40 @@ static int arizona_extcon_probe(struct platform_device *pdev) ret = arizona_request_irq(arizona, jack_irq_rise, "JACKDET rise", arizona_jackdet, info); if (ret != 0) { - dev_err(&pdev->dev, "Failed to get JACKDET rise IRQ: %d\n", - ret); + dev_err(arizona->dev, "Failed to get JACKDET rise IRQ: %d\n", ret); goto err_pm; } ret = arizona_set_irq_wake(arizona, jack_irq_rise, 1); if (ret != 0) { - dev_err(&pdev->dev, "Failed to set JD rise IRQ wake: %d\n", - ret); + dev_err(arizona->dev, "Failed to set JD rise IRQ wake: %d\n", ret); goto err_rise; } ret = arizona_request_irq(arizona, jack_irq_fall, "JACKDET fall", arizona_jackdet, info); if (ret != 0) { - dev_err(&pdev->dev, "Failed to get JD fall IRQ: %d\n", ret); + dev_err(arizona->dev, "Failed to get JD fall IRQ: %d\n", ret); goto err_rise_wake; } ret = arizona_set_irq_wake(arizona, jack_irq_fall, 1); if (ret != 0) { - dev_err(&pdev->dev, "Failed to set JD fall IRQ wake: %d\n", - ret); + dev_err(arizona->dev, "Failed to set JD fall IRQ wake: %d\n", ret); goto err_fall; } ret = arizona_request_irq(arizona, ARIZONA_IRQ_MICDET, "MICDET", arizona_micdet, info); if (ret != 0) { - dev_err(&pdev->dev, "Failed to get MICDET IRQ: %d\n", ret); + dev_err(arizona->dev, "Failed to get MICDET IRQ: %d\n", ret); goto err_fall_wake; } ret = arizona_request_irq(arizona, ARIZONA_IRQ_HPDET, "HPDET", arizona_hpdet_irq, info); if (ret != 0) { - dev_err(&pdev->dev, "Failed to get HPDET IRQ: %d\n", ret); + dev_err(arizona->dev, "Failed to get HPDET IRQ: %d\n", ret); goto err_micdet; } @@ -1677,12 +1678,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) ret = regulator_allow_bypass(info->micvdd, true); if (ret != 0) - dev_warn(arizona->dev, "Failed to set MICVDD to bypass: %d\n", - ret); + dev_warn(arizona->dev, "Failed to set MICVDD to bypass: %d\n", ret); ret = input_register_device(info->input); if (ret) { - dev_err(&pdev->dev, "Can't register input device: %d\n", ret); + dev_err(arizona->dev, "Can't register input device: %d\n", ret); goto err_hpdet; } @@ -1704,14 +1704,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) arizona_free_irq(arizona, jack_irq_rise, info); err_pm: pm_runtime_put(arizona->dev); -err_gpio: - gpiod_put(info->micd_pol_gpio); return ret; } -static int arizona_extcon_remove(struct platform_device *pdev) +static int arizona_jack_disable_jack_detect(struct arizona_priv *info) { - struct arizona_priv *info = platform_get_drvdata(pdev); struct arizona *arizona = info->arizona; int jack_irq_rise, jack_irq_fall; bool change; @@ -1739,8 +1736,7 @@ static int arizona_extcon_remove(struct platform_device *pdev) ARIZONA_MICD_ENA, 0, &change); if (ret < 0) { - dev_err(&pdev->dev, "Failed to disable micd on remove: %d\n", - ret); + dev_err(arizona->dev, "Failed to disable micd on remove: %d\n", ret); } else if (change) { regulator_disable(info->micvdd); pm_runtime_put(arizona->dev); @@ -1753,22 +1749,17 @@ static int arizona_extcon_remove(struct platform_device *pdev) ARIZONA_JD1_ENA, 0); arizona_clk32k_disable(arizona); - gpiod_put(info->micd_pol_gpio); - return 0; } -static struct platform_driver arizona_extcon_driver = { - .driver = { - .name = "arizona-extcon", - }, - .probe = arizona_extcon_probe, - .remove = arizona_extcon_remove, -}; - -module_platform_driver(arizona_extcon_driver); +int arizona_jack_set_jack(struct snd_soc_component *component, + struct snd_soc_jack *jack, void *data) +{ + struct arizona_priv *info = snd_soc_component_get_drvdata(component); -MODULE_DESCRIPTION("Arizona Extcon driver"); -MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>"); -MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:extcon-arizona"); + if (jack) + return arizona_jack_enable_jack_detect(info, jack); + else + return arizona_jack_disable_jack_detect(info); +} +EXPORT_SYMBOL_GPL(arizona_jack_set_jack); diff --git a/sound/soc/codecs/arizona.h b/sound/soc/codecs/arizona.h index 7132dbc3c840..fc515845a3e6 100644 --- a/sound/soc/codecs/arizona.h +++ b/sound/soc/codecs/arizona.h @@ -386,4 +386,10 @@ static inline int arizona_unregister_notifier(struct snd_soc_component *componen int arizona_of_get_audio_pdata(struct arizona *arizona); +int arizona_jack_codec_dev_probe(struct arizona_priv *info, struct device *dev); +int arizona_jack_codec_dev_remove(struct arizona_priv *info); + +int arizona_jack_set_jack(struct snd_soc_component *component, + struct snd_soc_jack *jack, void *data); + #endif
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(). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/codecs/Makefile | 2 +- sound/soc/codecs/arizona-jack.c | 125 +++++++++++++++----------------- sound/soc/codecs/arizona.h | 6 ++ 3 files changed, 65 insertions(+), 68 deletions(-)