[PATCH 0/5] ASoC: Intel/rt5640: Add support for HP Elite Pad 1000G2 jack-detect
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.
Regards,
Hans
Hans de Goede (5): ASoC: rt5640: Move rt5640_disable_jack_detect() up in the rt5640.c file ASoC: rt5640: Delay requesting IRQ until the machine-drv calls set_jack ASoC: rt5640: Add optional hp_det_gpio parameter to rt5640_detect_headset() ASoC: rt5640: Add rt5640_set_ovcd_params() helper ASoC: Intel: bytcr_rt5640: Add support for HP Elite Pad 1000G2 jack-detect
sound/soc/codecs/rt5640.c | 133 ++++++++++++----------- sound/soc/codecs/rt5640.h | 6 ++ sound/soc/intel/boards/bytcr_rt5640.c | 146 +++++++++++++++++++++++++- 3 files changed, 225 insertions(+), 60 deletions(-)
Move rt5640_disable_jack_detect() to above rt5640_enable_jack_detect(). This is a preparation patch for reworking how the IRQ gets requested.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5640.c | 46 +++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 9523f4b5c800..a425e6b1687d 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2362,6 +2362,29 @@ static void rt5640_cancel_work(void *data) cancel_delayed_work_sync(&rt5640->bp_work); }
+static void rt5640_disable_jack_detect(struct snd_soc_component *component) +{ + struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + + /* + * soc_remove_component() force-disables jack and thus rt5640->jack + * could be NULL at the time of driver's module unloading. + */ + if (!rt5640->jack) + return; + + disable_irq(rt5640->irq); + rt5640_cancel_work(rt5640); + + if (rt5640->jack->status & SND_JACK_MICROPHONE) { + rt5640_disable_micbias1_ovcd_irq(component); + rt5640_disable_micbias1_for_ovcd(component); + snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0); + } + + rt5640->jack = NULL; +} + static void rt5640_enable_jack_detect(struct snd_soc_component *component, struct snd_soc_jack *jack) { @@ -2428,29 +2451,6 @@ static void rt5640_enable_jack_detect(struct snd_soc_component *component, queue_work(system_long_wq, &rt5640->jack_work); }
-static void rt5640_disable_jack_detect(struct snd_soc_component *component) -{ - struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); - - /* - * soc_remove_component() force-disables jack and thus rt5640->jack - * could be NULL at the time of driver's module unloading. - */ - if (!rt5640->jack) - return; - - disable_irq(rt5640->irq); - rt5640_cancel_work(rt5640); - - if (rt5640->jack->status & SND_JACK_MICROPHONE) { - rt5640_disable_micbias1_ovcd_irq(component); - rt5640_disable_micbias1_for_ovcd(component); - snd_soc_jack_report(rt5640->jack, 0, SND_JACK_BTN_0); - } - - rt5640->jack = NULL; -} - static int rt5640_set_jack(struct snd_soc_component *component, struct snd_soc_jack *jack, void *data) {
On Sun, Aug 15, 2021 at 05:49:31PM +0200, Hans de Goede wrote:
Move rt5640_disable_jack_detect() to above rt5640_enable_jack_detect(). This is a preparation patch for reworking how the IRQ gets requested.
This doesn't apply against current code, please check and resend.
Delay requesting the IRQ until the machine-drv calls set_jack.
The main reason for this is that the codec's IRQ is unused on some boards, in which case we really should not call request_irq at all.
On some boards there is an IRQ listed at index 0 for the codec, but this is not connected to the codec, but rather is directly connected to the jack's jack-detect pin. These special setups will be handled by the machine-driver, but the machine driver can only request the IRQ if it is not first requested by the codec driver. Moving the request_irq to the set_jack callback (which will not get called in this case) avoids the codec-driver clobbering the IRQ.
Moving the request_irq also removes the need to disable the IRQ immediately after requesting it, avoiding a small race (this could also have been fixed by using the new IRQF_NO_AUTOEN flag when requesting the IRQ).
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5640.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index a425e6b1687d..d32e9d69231c 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2373,7 +2373,7 @@ static void rt5640_disable_jack_detect(struct snd_soc_component *component) if (!rt5640->jack) return;
- disable_irq(rt5640->irq); + free_irq(rt5640->irq, rt5640); rt5640_cancel_work(rt5640);
if (rt5640->jack->status & SND_JACK_MICROPHONE) { @@ -2389,6 +2389,7 @@ static void rt5640_enable_jack_detect(struct snd_soc_component *component, struct snd_soc_jack *jack) { struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + int ret;
/* Select JD-source */ snd_soc_component_update_bits(component, RT5640_JD_CTRL, @@ -2446,7 +2447,17 @@ static void rt5640_enable_jack_detect(struct snd_soc_component *component, rt5640_enable_micbias1_ovcd_irq(component); }
- enable_irq(rt5640->irq); + ret = request_irq(rt5640->irq, rt5640_irq, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "rt5640", rt5640); + if (ret) { + dev_warn(component->dev, "Failed to reguest IRQ %d: %d\n", rt5640->irq, ret); + rt5640->irq = -ENXIO; + /* Undo above settings */ + rt5640_disable_jack_detect(component); + return; + } + /* sync initial jack state */ queue_work(system_long_wq, &rt5640->jack_work); } @@ -2836,18 +2847,6 @@ static int rt5640_i2c_probe(struct i2c_client *i2c, if (ret) return ret;
- ret = devm_request_irq(&i2c->dev, rt5640->irq, rt5640_irq, - IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING - | IRQF_ONESHOT, "rt5640", rt5640); - if (ret == 0) { - /* Gets re-enabled by rt5640_set_jack() */ - disable_irq(rt5640->irq); - } else { - dev_warn(&i2c->dev, "Failed to reguest IRQ %d: %d\n", - rt5640->irq, ret); - rt5640->irq = -ENXIO; - } - return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5640, rt5640_dai, ARRAY_SIZE(rt5640_dai));
Some devices don't use the builtin jack-detect but can still benefit from the mic-bias-current over-current-detection headphones vs headset detection done by rt5640_detect_headset().
In this case the jack-inserted check done by rt5640_detect_headset() needs to be done through a GPIO rather then by using the codec's builtin jack-detect. Add an optional hp_det_gpio parameter and export rt5640_detect_headset() for use on machines where jack-detect is handled outside of the codec.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5640.c | 14 ++++++++++---- sound/soc/codecs/rt5640.h | 2 ++ 2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index d32e9d69231c..04820af03ae8 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2241,7 +2241,7 @@ static void rt5640_button_press_work(struct work_struct *work) schedule_delayed_work(&rt5640->bp_work, msecs_to_jiffies(BP_POLL_TIME)); }
-static int rt5640_detect_headset(struct snd_soc_component *component) +int rt5640_detect_headset(struct snd_soc_component *component, struct gpio_desc *hp_det_gpio) { int i, headset_count = 0, headphone_count = 0;
@@ -2259,8 +2259,13 @@ static int rt5640_detect_headset(struct snd_soc_component *component) msleep(JACK_SETTLE_TIME);
/* Check the jack is still connected before checking ovcd */ - if (!rt5640_jack_inserted(component)) - return 0; + if (hp_det_gpio) { + if (gpiod_get_value_cansleep(hp_det_gpio)) + return 0; + } else { + if (!rt5640_jack_inserted(component)) + return 0; + }
if (rt5640_micbias1_ovcd(component)) { /* @@ -2285,6 +2290,7 @@ static int rt5640_detect_headset(struct snd_soc_component *component) dev_err(component->dev, "Error detecting headset vs headphones, bad contact?, assuming headphones\n"); return SND_JACK_HEADPHONE; } +EXPORT_SYMBOL_GPL(rt5640_detect_headset);
static void rt5640_jack_work(struct work_struct *work) { @@ -2309,7 +2315,7 @@ static void rt5640_jack_work(struct work_struct *work) /* Jack inserted */ WARN_ON(rt5640->ovcd_irq_enabled); rt5640_enable_micbias1_for_ovcd(component); - status = rt5640_detect_headset(component); + status = rt5640_detect_headset(component, NULL); if (status == SND_JACK_HEADSET) { /* Enable ovcd IRQ for button press detect. */ rt5640_enable_micbias1_ovcd_irq(component); diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h index 4fd47f2b936b..4d19997dd684 100644 --- a/sound/soc/codecs/rt5640.h +++ b/sound/soc/codecs/rt5640.h @@ -10,6 +10,7 @@ #define _RT5640_H
#include <linux/clk.h> +#include <linux/gpio/consumer.h> #include <linux/workqueue.h> #include <dt-bindings/sound/rt5640.h>
@@ -2156,5 +2157,6 @@ int rt5640_dmic_enable(struct snd_soc_component *component, bool dmic1_data_pin, bool dmic2_data_pin); int rt5640_sel_asrc_clk_src(struct snd_soc_component *component, unsigned int filter_mask, unsigned int clk_src); +int rt5640_detect_headset(struct snd_soc_component *component, struct gpio_desc *hp_det_gpio);
#endif
Some devices don't use the builtin jack-detect but can still benefit from the mic-bias-current over-current-detection to differentiate between headphones vs a headset.
Move the ovcd init code from rt5640_enable_jack_detect() into a new rt5640_set_ovcd_params() helper and export this helper as well as a couple of related ovcd functions.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5640.c | 50 +++++++++++++++++++++++---------------- sound/soc/codecs/rt5640.h | 4 ++++ 2 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 04820af03ae8..cd1db5caabad 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -2093,7 +2093,7 @@ int rt5640_sel_asrc_clk_src(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(rt5640_sel_asrc_clk_src);
-static void rt5640_enable_micbias1_for_ovcd(struct snd_soc_component *component) +void rt5640_enable_micbias1_for_ovcd(struct snd_soc_component *component) { struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
@@ -2105,8 +2105,9 @@ static void rt5640_enable_micbias1_for_ovcd(struct snd_soc_component *component) snd_soc_dapm_sync_unlocked(dapm); snd_soc_dapm_mutex_unlock(dapm); } +EXPORT_SYMBOL_GPL(rt5640_enable_micbias1_for_ovcd);
-static void rt5640_disable_micbias1_for_ovcd(struct snd_soc_component *component) +void rt5640_disable_micbias1_for_ovcd(struct snd_soc_component *component) { struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
@@ -2117,6 +2118,7 @@ static void rt5640_disable_micbias1_for_ovcd(struct snd_soc_component *component snd_soc_dapm_sync_unlocked(dapm); snd_soc_dapm_mutex_unlock(dapm); } +EXPORT_SYMBOL_GPL(rt5640_disable_micbias1_for_ovcd);
static void rt5640_enable_micbias1_ovcd_irq(struct snd_soc_component *component) { @@ -2368,6 +2370,31 @@ static void rt5640_cancel_work(void *data) cancel_delayed_work_sync(&rt5640->bp_work); }
+void rt5640_set_ovcd_params(struct snd_soc_component *component) +{ + struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + + snd_soc_component_write(component, RT5640_PR_BASE + RT5640_BIAS_CUR4, + 0xa800 | rt5640->ovcd_sf); + + snd_soc_component_update_bits(component, RT5640_MICBIAS, + RT5640_MIC1_OVTH_MASK | RT5640_MIC1_OVCD_MASK, + rt5640->ovcd_th | RT5640_MIC1_OVCD_EN); + + /* + * The over-current-detect is only reliable in detecting the absence + * of over-current, when the mic-contact in the jack is short-circuited, + * the hardware periodically retries if it can apply the bias-current + * leading to the ovcd status flip-flopping 1-0-1 with it being 0 about + * 10% of the time, as we poll the ovcd status bit we might hit that + * 10%, so we enable sticky mode and when checking OVCD we clear the + * status, msleep() a bit and then check to get a reliable reading. + */ + snd_soc_component_update_bits(component, RT5640_IRQ_CTRL2, + RT5640_MB1_OC_STKY_MASK, RT5640_MB1_OC_STKY_EN); +} +EXPORT_SYMBOL_GPL(rt5640_set_ovcd_params); + static void rt5640_disable_jack_detect(struct snd_soc_component *component) { struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); @@ -2415,24 +2442,7 @@ static void rt5640_enable_jack_detect(struct snd_soc_component *component, /* Enabling jd2 in general control 2 */ snd_soc_component_write(component, RT5640_DUMMY2, 0x4001);
- snd_soc_component_write(component, RT5640_PR_BASE + RT5640_BIAS_CUR4, - 0xa800 | rt5640->ovcd_sf); - - snd_soc_component_update_bits(component, RT5640_MICBIAS, - RT5640_MIC1_OVTH_MASK | RT5640_MIC1_OVCD_MASK, - rt5640->ovcd_th | RT5640_MIC1_OVCD_EN); - - /* - * The over-current-detect is only reliable in detecting the absence - * of over-current, when the mic-contact in the jack is short-circuited, - * the hardware periodically retries if it can apply the bias-current - * leading to the ovcd status flip-flopping 1-0-1 with it being 0 about - * 10% of the time, as we poll the ovcd status bit we might hit that - * 10%, so we enable sticky mode and when checking OVCD we clear the - * status, msleep() a bit and then check to get a reliable reading. - */ - snd_soc_component_update_bits(component, RT5640_IRQ_CTRL2, - RT5640_MB1_OC_STKY_MASK, RT5640_MB1_OC_STKY_EN); + rt5640_set_ovcd_params(component);
/* * All IRQs get or-ed together, so we need the jack IRQ to report 0 diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h index 4d19997dd684..2c28f83e338a 100644 --- a/sound/soc/codecs/rt5640.h +++ b/sound/soc/codecs/rt5640.h @@ -2157,6 +2157,10 @@ int rt5640_dmic_enable(struct snd_soc_component *component, bool dmic1_data_pin, bool dmic2_data_pin); int rt5640_sel_asrc_clk_src(struct snd_soc_component *component, unsigned int filter_mask, unsigned int clk_src); + +void rt5640_set_ovcd_params(struct snd_soc_component *component); +void rt5640_enable_micbias1_for_ovcd(struct snd_soc_component *component); +void rt5640_disable_micbias1_for_ovcd(struct snd_soc_component *component); int rt5640_detect_headset(struct snd_soc_component *component, struct gpio_desc *hp_det_gpio);
#endif
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(-)
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)
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:
- 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!
On Sun, 15 Aug 2021 17:49:30 +0200, Hans de Goede wrote:
The HP Elitepad 1000 G2 tablet has 2 headset jacks:
- 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.
- 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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: rt5640: Move rt5640_disable_jack_detect() up in the rt5640.c file commit: 5caab9f48b96f6998fb23d38a7b57fca91ef1653 [2/5] ASoC: rt5640: Delay requesting IRQ until the machine-drv calls set_jack commit: 15d54840ecf6f00061d03180394a0a21ff8ffa48 [3/5] ASoC: rt5640: Add optional hp_det_gpio parameter to rt5640_detect_headset() commit: d21213b4503ea66777313e4345e116cc8a5366bf [4/5] ASoC: rt5640: Add rt5640_set_ovcd_params() helper commit: e3f2a6603a982467601e0831d706786ed1ade833 [5/5] ASoC: Intel: bytcr_rt5640: Add support for HP Elite Pad 1000G2 jack-detect commit: 9ba00856686ade106afee2884b5e8ac1e09d137a
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart