Hi Pierre-Louis,
On 8/16/21 3:31 PM, Pierre-Louis Bossart wrote:
Hi Hans, I am a bit confused by the use of acpi_dev_add_driver_gpios().
I understand admittedly my solution there is a bit hacky.
You call it from the dailink .init function, and you call acpi_dev_remove_driver_gpios() from the .exit function.
@@ -1172,9 +1250,60 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime) snd_soc_component_set_jack(component, &priv->jack, NULL); }
- if (byt_rt5640_quirk & BYT_RT5640_JD_HP_ELITEP_1000G2) {
ret = snd_soc_card_jack_new(card, "Headset",SND_JACK_HEADSET,&priv->jack, rt5640_pins,ARRAY_SIZE(rt5640_pins));if (ret)return ret;ret = snd_soc_card_jack_new(card, "Headset 2",SND_JACK_HEADSET,&priv->jack2, rt5640_pins2,ARRAY_SIZE(rt5640_pins2));if (ret)return ret;acpi_dev_add_driver_gpios(ACPI_COMPANION(priv->codec_dev),byt_rt5640_hp_elitepad_1000g2_gpios);rt5640_jack_gpio.data = priv;rt5640_jack_gpio.gpiod_dev = priv->codec_dev;rt5640_jack_gpio.jack_status_check = byt_rt5640_hp_elitepad_1000g2_jack1_check;ret = snd_soc_jack_add_gpios(&priv->jack, 1, &rt5640_jack_gpio);if (ret) {acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev));return ret;}rt5640_set_ovcd_params(component);rt5640_jack2_gpio.data = component;rt5640_jack2_gpio.gpiod_dev = priv->codec_dev;rt5640_jack2_gpio.jack_status_check = byt_rt5640_hp_elitepad_1000g2_jack2_check;ret = snd_soc_jack_add_gpios(&priv->jack2, 1, &rt5640_jack2_gpio);if (ret) {snd_soc_jack_free_gpios(&priv->jack, 1, &rt5640_jack_gpio);acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev));return ret;}- }
- return 0;
}
+static void byt_rt5640_exit(struct snd_soc_pcm_runtime *runtime) +{
- struct snd_soc_card *card = runtime->card;
- struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
- if (byt_rt5640_quirk & BYT_RT5640_JD_HP_ELITEP_1000G2) {
snd_soc_jack_free_gpios(&priv->jack2, 1, &rt5640_jack2_gpio);snd_soc_jack_free_gpios(&priv->jack, 1, &rt5640_jack_gpio);acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev));- }
+}
so far so good, but you also add/remove gpios in the machine driver probe
Ack.
@@ -1490,6 +1620,20 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) return -EPROBE_DEFER; priv->codec_dev = get_device(codec_dev);
- if (byt_rt5640_quirk & BYT_RT5640_JD_HP_ELITEP_1000G2) {
acpi_dev_add_driver_gpios(ACPI_COMPANION(priv->codec_dev),byt_rt5640_hp_elitepad_1000g2_gpios);
So this second add here (which runs first, so it really is the first add)
priv->hsmic_detect = devm_fwnode_gpiod_get(&pdev->dev, codec_dev->fwnode,"headset-mic-detect", GPIOD_IN,"headset-mic-detect");
Is to make sure this call can succeed.
acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev));
And this is to not have to add yet an other error-exit path which does this to the probe() function. By simply always removing the lookup here after the gpiod_get() we keep the error-exit paths as they were, rather then making them more complicated.
But I guess things won't be so bad wrt err-exit-path complexity as for them to really be a problem, so if you prefer I can also:
1. Remove the second acpi_dev_add_driver_gpios + acpi_dev_remove_driver_gpios pair from the dai_link .init/.exit. 2. Remove the acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev) call here moving it to snd_byt_rt5640_mc_remove() 3. Introduce a acpi_dev_remove_driver_gpios() remove in the error-exit paths of snd_byt_rt5640_mc_probe where necessary.
if (IS_ERR(priv->hsmic_detect)) {ret_val = PTR_ERR(priv->hsmic_detect);dev_err_probe(&pdev->dev, ret_val, "getting hsmic-detect GPIO\n");goto err_device;}- }
Does this part need to be part of the machine driver probe, or could it be moved in the dailink .init function?
The idea here is that the gpiod_get may fail with -EPROBE_DEFER and then I want to fail as early as possible, so right in the probe function.
This is also why the error is logged with dev_err_probe() which does not log anything for EPROBE_DEFER as retval.
Is this because you wanted to use devm_ function?
No, I did consider adding the gpiod_get() for priv->hsmic_detect to the dai_link .init function and then I would just use a normal get, combined with an explicit _put in the dailink exit. I put this gpiod_get in the platform_driver probe to handle EPROBE_DEFER early on, rather then having it happen deep inside the devm_snd_soc_register_card() call-graph (when it calls the dailink .init).
I would prefer to keep the gpiod_get inside snd_byt_rt5640_mc_probe for this reason, but as mentioned I can removed the second acpi_dev_add_driver_gpios + acpi_dev_remove_driver_gpios call pair from the dai_link .init/.exit.
Please let me know how you want to proceed with this.
Regards,
Hans