Re: [PATCH 5/5] ASoC: Intel: bytcr_rt5640: Add support for HP Elite Pad 1000G2 jack-detect

[Hans de Goede <hdegoede@redhat.com>]

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