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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

Hi Hans,
I am a bit confused by the use of acpi_dev_add_driver_gpios(). 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

> @@ -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);
> +		priv->hsmic_detect = devm_fwnode_gpiod_get(&pdev->dev, codec_dev->fwnode,
> +							   "headset-mic-detect", GPIOD_IN,
> +							   "headset-mic-detect");
> +		acpi_dev_remove_driver_gpios(ACPI_COMPANION(priv->codec_dev));
> +		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? Is this because you wanted to
use devm_ function?