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
