| Message ID | 20210815154935.101178-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel/rt5640: Add support for HP Elite Pad 1000G2 jack-detect | expand |
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?
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
> 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. that sounds good to me, it's probably better to do things once with a bit of additional error handling. >>> + 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. ok with the suggestion above. Thanks Hans!
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index fecccff76caf..369642c07a5d 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -18,6 +18,8 @@ #include <linux/clk.h> #include <linux/device.h> #include <linux/dmi.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> #include <linux/input.h> #include <linux/slab.h> #include <sound/pcm.h> @@ -76,6 +78,7 @@ enum { #define BYT_RT5640_LINEOUT BIT(25) #define BYT_RT5640_LINEOUT_AS_HP2 BIT(26) #define BYT_RT5640_HSMIC2_ON_IN1 BIT(27) +#define BYT_RT5640_JD_HP_ELITEP_1000G2 BIT(28) #define BYTCR_INPUT_DEFAULTS \ (BYT_RT5640_IN3_MAP | \ @@ -89,6 +92,8 @@ enum { struct byt_rt5640_private { struct snd_soc_jack jack; + struct snd_soc_jack jack2; + struct gpio_desc *hsmic_detect; struct clk *mclk; struct device *codec_dev; }; @@ -141,6 +146,8 @@ static void log_quirks(struct device *dev) } if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV) dev_info(dev, "quirk JD_NOT_INV enabled\n"); + if (byt_rt5640_quirk & BYT_RT5640_JD_HP_ELITEP_1000G2) + dev_info(dev, "quirk JD_HP_ELITEPAD_1000G2 enabled\n"); if (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) dev_info(dev, "quirk MONO_SPEAKER enabled\n"); if (byt_rt5640_quirk & BYT_RT5640_NO_SPEAKERS) @@ -438,6 +445,76 @@ static struct snd_soc_jack_pin rt5640_pins[] = { }, }; +static struct snd_soc_jack_pin rt5640_pins2[] = { + { + /* The 2nd headset jack uses lineout with an external HP-amp */ + .pin = "Line Out", + .mask = SND_JACK_HEADPHONE, + }, + { + /* BYT_RT5640_HSMIC2_ON_IN1 uses byt_rt5640_intmic_in1_map */ + .pin = "Internal Mic", + .mask = SND_JACK_MICROPHONE, + }, +}; + +struct snd_soc_jack_gpio rt5640_jack_gpio = { + .name = "hp-detect", + .report = SND_JACK_HEADSET, + .invert = true, + .debounce_time = 200, +}; + +struct snd_soc_jack_gpio rt5640_jack2_gpio = { + .name = "hp2-detect", + .report = SND_JACK_HEADSET, + .invert = true, + .debounce_time = 200, +}; + +static const struct acpi_gpio_params acpi_gpio0 = { 0, 0, false }; +static const struct acpi_gpio_params acpi_gpio1 = { 1, 0, false }; +static const struct acpi_gpio_params acpi_gpio2 = { 2, 0, false }; + +static const struct acpi_gpio_mapping byt_rt5640_hp_elitepad_1000g2_gpios[] = { + { "hp-detect-gpios", &acpi_gpio0, 1, }, + { "headset-mic-detect-gpios", &acpi_gpio1, 1, }, + { "hp2-detect-gpios", &acpi_gpio2, 1, }, + { }, +}; + +int byt_rt5640_hp_elitepad_1000g2_jack1_check(void *data) +{ + struct byt_rt5640_private *priv = data; + int jack_status, mic_status; + + jack_status = gpiod_get_value_cansleep(rt5640_jack_gpio.desc); + if (jack_status) + return 0; + + mic_status = gpiod_get_value_cansleep(priv->hsmic_detect); + if (mic_status) + return SND_JACK_HEADPHONE; + else + return SND_JACK_HEADSET; +} + +int byt_rt5640_hp_elitepad_1000g2_jack2_check(void *data) +{ + struct snd_soc_component *component = data; + int jack_status, report; + + jack_status = gpiod_get_value_cansleep(rt5640_jack2_gpio.desc); + if (jack_status) + return 0; + + rt5640_enable_micbias1_for_ovcd(component); + report = rt5640_detect_headset(component, rt5640_jack2_gpio.desc); + rt5640_disable_micbias1_for_ovcd(component); + + return report; +} + static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -649,7 +726,8 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = { BYT_RT5640_MCLK_EN | BYT_RT5640_LINEOUT | BYT_RT5640_LINEOUT_AS_HP2 | - BYT_RT5640_HSMIC2_ON_IN1), + BYT_RT5640_HSMIC2_ON_IN1 | + BYT_RT5640_JD_HP_ELITEP_1000G2), }, { /* HP Pavilion x2 10-k0XX, 10-n0XX */ .matches = { @@ -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)); + } +} + static int byt_rt5640_codec_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params) { @@ -1287,6 +1416,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { .dpcm_playback = 1, .dpcm_capture = 1, .init = byt_rt5640_init, + .exit = byt_rt5640_exit, .ops = &byt_rt5640_be_ssp2_ops, SND_SOC_DAILINK_REG(ssp2_port, ssp2_codec, platform), }, @@ -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; + } + } + /* Must be called before register_card, also see declaration comment. */ ret_val = byt_rt5640_add_codec_device_props(codec_dev, priv); if (ret_val)
The HP Elitepad 1000 G2 tablet has 2 headset jacks: 1. on the dock which uses the output of the codecs built-in HP-amp + the standard IN2 input which is always used with the headset-jack. 2. on the tablet itself, this uses the line-out of the codec + an external HP-amp, which gets enabled by the ALC5642 codec's GPIO1 pin; and IN1 for the headset-mic. The codec's GPIO1 is also its only IRQ output pin, so this means that the codec's IRQ cannot be used on this tablet. Instead the jack-detect is connected directly to GPIOs on the main SoC. The dock has a helper chip which also detects if a headset-mic is present or not, so there are 2 GPIOs for the jack-detect status of the dock. The tablet jack uses a single GPIO which indicates if a jack is present or not. Differentiating between between headphones vs a headset on the tablet jack is done by using the usual mic-bias over-current-detection mechanism. Add support for this unique setup, this support gets enabled on this tablet through a new BYT_RT5640_JD_HP_ELITEP_1000G2 quirk. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213415 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/intel/boards/bytcr_rt5640.c | 146 +++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 1 deletion(-)