[alsa-devel] [PATCH v7 0/4] Fix jack detection for Chromebook Pixel
Headphone/mic jack detection doesn't work on the Chromebook Pixel 2015.
This patch changes the irq implementation to support polarity flipping and fixes the configuration code so that correct GPIO pins are read from ACPI.
With this series, plugging and unplugging the headphone jack switches between headphones and speakers automatically, and headset microphones are also detected.
v7: - Rebase onto for-next branch of broonie/sound.git
v6: - Move refactoring into its own patch - Reorder patches so that DT property names patch is first - Clarify commit message for patch which implements irq handler - Remove unused work struct - Make IRQ function return IRQ_HANDLED only if IRQs actually fire
v5: - Fix void* parameter to devm_request_threaded_irq
v4: - Fix incorrect void* cast in rt5677_irq()
v3: - Update commit message for patch 1/3 to clarify why we implement our own irq_chip.
v2: - Split IRQ change into two patches: adding and fixing potential race - Change config reading code to try both DT and ACPI style names
Ben Zhang (2): ASoC: rt5677: clear interrupts by polarity flip ASoC: rt5677: handle concurrent interrupts
Fletcher Woodruff (2): ASoC: rt5677: fall back to DT prop names on error ASoC: rt5677: move jack-detect init to i2c probe
sound/soc/codecs/rt5677.c | 319 ++++++++++++++++++++++++++------------ sound/soc/codecs/rt5677.h | 13 +- 2 files changed, 236 insertions(+), 96 deletions(-)
The rt5677 driver uses ACPI-style property names to read from the device API. However, these do not match the property names in _DSD used on the Chromebook Pixel 2015, which are closer to the Device Tree style. Unify the two functions for reading from the device API so that they try ACPI-style names first and fall back to the DT names on error.
With this patch, plugging and unplugging the headphone jack switches between headphones and speakers automatically.
Signed-off-by: Fletcher Woodruff fletcherw@chromium.org --- sound/soc/codecs/rt5677.c | 74 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index c49b5c21866663..fe000f30b9ad5f 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -5020,48 +5020,50 @@ static const struct acpi_device_id rt5677_acpi_match[] = { }; MODULE_DEVICE_TABLE(acpi, rt5677_acpi_match);
-static void rt5677_read_acpi_properties(struct rt5677_priv *rt5677, +static void rt5677_read_device_properties(struct rt5677_priv *rt5677, struct device *dev) { u32 val;
- if (!device_property_read_u32(dev, "DCLK", &val)) - rt5677->pdata.dmic2_clk_pin = val; + rt5677->pdata.in1_diff = + device_property_read_bool(dev, "IN1") || + device_property_read_bool(dev, "realtek,in1-differential");
- rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1"); - rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2"); - rt5677->pdata.lout1_diff = device_property_read_bool(dev, "OUT1"); - rt5677->pdata.lout2_diff = device_property_read_bool(dev, "OUT2"); - rt5677->pdata.lout3_diff = device_property_read_bool(dev, "OUT3"); + rt5677->pdata.in2_diff = + device_property_read_bool(dev, "IN2") || + device_property_read_bool(dev, "realtek,in2-differential");
- device_property_read_u32(dev, "JD1", &rt5677->pdata.jd1_gpio); - device_property_read_u32(dev, "JD2", &rt5677->pdata.jd2_gpio); - device_property_read_u32(dev, "JD3", &rt5677->pdata.jd3_gpio); -} + rt5677->pdata.lout1_diff = + device_property_read_bool(dev, "OUT1") || + device_property_read_bool(dev, "realtek,lout1-differential");
-static void rt5677_read_device_properties(struct rt5677_priv *rt5677, - struct device *dev) -{ - rt5677->pdata.in1_diff = device_property_read_bool(dev, - "realtek,in1-differential"); - rt5677->pdata.in2_diff = device_property_read_bool(dev, - "realtek,in2-differential"); - rt5677->pdata.lout1_diff = device_property_read_bool(dev, - "realtek,lout1-differential"); - rt5677->pdata.lout2_diff = device_property_read_bool(dev, - "realtek,lout2-differential"); - rt5677->pdata.lout3_diff = device_property_read_bool(dev, - "realtek,lout3-differential"); + rt5677->pdata.lout2_diff = + device_property_read_bool(dev, "OUT2") || + device_property_read_bool(dev, "realtek,lout2-differential"); + + rt5677->pdata.lout3_diff = + device_property_read_bool(dev, "OUT3") || + device_property_read_bool(dev, "realtek,lout3-differential");
device_property_read_u8_array(dev, "realtek,gpio-config", - rt5677->pdata.gpio_config, RT5677_GPIO_NUM); - - device_property_read_u32(dev, "realtek,jd1-gpio", - &rt5677->pdata.jd1_gpio); - device_property_read_u32(dev, "realtek,jd2-gpio", - &rt5677->pdata.jd2_gpio); - device_property_read_u32(dev, "realtek,jd3-gpio", - &rt5677->pdata.jd3_gpio); + rt5677->pdata.gpio_config, + RT5677_GPIO_NUM); + + if (!device_property_read_u32(dev, "DCLK", &val) || + !device_property_read_u32(dev, "realtek,dmic2_clk_pin", &val)) + rt5677->pdata.dmic2_clk_pin = val; + + if (!device_property_read_u32(dev, "JD1", &val) || + !device_property_read_u32(dev, "realtek,jd1-gpio", &val)) + rt5677->pdata.jd1_gpio = val; + + if (!device_property_read_u32(dev, "JD2", &val) || + !device_property_read_u32(dev, "realtek,jd2-gpio", &val)) + rt5677->pdata.jd2_gpio = val; + + if (!device_property_read_u32(dev, "JD3", &val) || + !device_property_read_u32(dev, "realtek,jd3-gpio", &val)) + rt5677->pdata.jd3_gpio = val; }
static struct regmap_irq rt5677_irqs[] = { @@ -5144,20 +5146,18 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) match_id = of_match_device(rt5677_of_match, &i2c->dev); if (match_id) rt5677->type = (enum rt5677_type)match_id->data; - - rt5677_read_device_properties(rt5677, &i2c->dev); } else if (ACPI_HANDLE(&i2c->dev)) { const struct acpi_device_id *acpi_id;
acpi_id = acpi_match_device(rt5677_acpi_match, &i2c->dev); if (acpi_id) rt5677->type = (enum rt5677_type)acpi_id->driver_data; - - rt5677_read_acpi_properties(rt5677, &i2c->dev); } else { return -EINVAL; }
+ rt5677_read_device_properties(rt5677, &i2c->dev); + /* pow-ldo2 and reset are optional. The codec pins may be statically * connected on the board without gpios. If the gpio device property * isn't specified, devm_gpiod_get_optional returns NULL.
The patch
ASoC: rt5677: fall back to DT prop names on error
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3
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
From 8893cba2fa6994ac8434cbc616eeddcde211ec45 Mon Sep 17 00:00:00 2001
From: Fletcher Woodruff fletcherw@chromium.org Date: Fri, 14 Jun 2019 13:48:51 -0600 Subject: [PATCH] ASoC: rt5677: fall back to DT prop names on error
The rt5677 driver uses ACPI-style property names to read from the device API. However, these do not match the property names in _DSD used on the Chromebook Pixel 2015, which are closer to the Device Tree style. Unify the two functions for reading from the device API so that they try ACPI-style names first and fall back to the DT names on error.
With this patch, plugging and unplugging the headphone jack switches between headphones and speakers automatically.
Signed-off-by: Fletcher Woodruff fletcherw@chromium.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/rt5677.c | 74 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index c49b5c218666..fe000f30b9ad 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -5020,48 +5020,50 @@ static const struct acpi_device_id rt5677_acpi_match[] = { }; MODULE_DEVICE_TABLE(acpi, rt5677_acpi_match);
-static void rt5677_read_acpi_properties(struct rt5677_priv *rt5677, +static void rt5677_read_device_properties(struct rt5677_priv *rt5677, struct device *dev) { u32 val;
- if (!device_property_read_u32(dev, "DCLK", &val)) - rt5677->pdata.dmic2_clk_pin = val; + rt5677->pdata.in1_diff = + device_property_read_bool(dev, "IN1") || + device_property_read_bool(dev, "realtek,in1-differential");
- rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1"); - rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2"); - rt5677->pdata.lout1_diff = device_property_read_bool(dev, "OUT1"); - rt5677->pdata.lout2_diff = device_property_read_bool(dev, "OUT2"); - rt5677->pdata.lout3_diff = device_property_read_bool(dev, "OUT3"); + rt5677->pdata.in2_diff = + device_property_read_bool(dev, "IN2") || + device_property_read_bool(dev, "realtek,in2-differential");
- device_property_read_u32(dev, "JD1", &rt5677->pdata.jd1_gpio); - device_property_read_u32(dev, "JD2", &rt5677->pdata.jd2_gpio); - device_property_read_u32(dev, "JD3", &rt5677->pdata.jd3_gpio); -} + rt5677->pdata.lout1_diff = + device_property_read_bool(dev, "OUT1") || + device_property_read_bool(dev, "realtek,lout1-differential");
-static void rt5677_read_device_properties(struct rt5677_priv *rt5677, - struct device *dev) -{ - rt5677->pdata.in1_diff = device_property_read_bool(dev, - "realtek,in1-differential"); - rt5677->pdata.in2_diff = device_property_read_bool(dev, - "realtek,in2-differential"); - rt5677->pdata.lout1_diff = device_property_read_bool(dev, - "realtek,lout1-differential"); - rt5677->pdata.lout2_diff = device_property_read_bool(dev, - "realtek,lout2-differential"); - rt5677->pdata.lout3_diff = device_property_read_bool(dev, - "realtek,lout3-differential"); + rt5677->pdata.lout2_diff = + device_property_read_bool(dev, "OUT2") || + device_property_read_bool(dev, "realtek,lout2-differential"); + + rt5677->pdata.lout3_diff = + device_property_read_bool(dev, "OUT3") || + device_property_read_bool(dev, "realtek,lout3-differential");
device_property_read_u8_array(dev, "realtek,gpio-config", - rt5677->pdata.gpio_config, RT5677_GPIO_NUM); - - device_property_read_u32(dev, "realtek,jd1-gpio", - &rt5677->pdata.jd1_gpio); - device_property_read_u32(dev, "realtek,jd2-gpio", - &rt5677->pdata.jd2_gpio); - device_property_read_u32(dev, "realtek,jd3-gpio", - &rt5677->pdata.jd3_gpio); + rt5677->pdata.gpio_config, + RT5677_GPIO_NUM); + + if (!device_property_read_u32(dev, "DCLK", &val) || + !device_property_read_u32(dev, "realtek,dmic2_clk_pin", &val)) + rt5677->pdata.dmic2_clk_pin = val; + + if (!device_property_read_u32(dev, "JD1", &val) || + !device_property_read_u32(dev, "realtek,jd1-gpio", &val)) + rt5677->pdata.jd1_gpio = val; + + if (!device_property_read_u32(dev, "JD2", &val) || + !device_property_read_u32(dev, "realtek,jd2-gpio", &val)) + rt5677->pdata.jd2_gpio = val; + + if (!device_property_read_u32(dev, "JD3", &val) || + !device_property_read_u32(dev, "realtek,jd3-gpio", &val)) + rt5677->pdata.jd3_gpio = val; }
static struct regmap_irq rt5677_irqs[] = { @@ -5144,20 +5146,18 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) match_id = of_match_device(rt5677_of_match, &i2c->dev); if (match_id) rt5677->type = (enum rt5677_type)match_id->data; - - rt5677_read_device_properties(rt5677, &i2c->dev); } else if (ACPI_HANDLE(&i2c->dev)) { const struct acpi_device_id *acpi_id;
acpi_id = acpi_match_device(rt5677_acpi_match, &i2c->dev); if (acpi_id) rt5677->type = (enum rt5677_type)acpi_id->driver_data; - - rt5677_read_acpi_properties(rt5677, &i2c->dev); } else { return -EINVAL; }
+ rt5677_read_device_properties(rt5677, &i2c->dev); + /* pow-ldo2 and reset are optional. The codec pins may be statically * connected on the board without gpios. If the gpio device property * isn't specified, devm_gpiod_get_optional returns NULL.
This patch moves the code to select the gpios for jack detection from rt5677_probe to rt5677_init_irq (called from rt5677_i2c_probe).
It also sets some registers to fix bugs related to jack detection, and adds some constants and comments to make it easier to understand what certain register settings are controlling.
Signed-off-by: Ben Zhang benzh@chromium.org Signed-off-by: Fletcher Woodruff fletcherw@chromium.org --- sound/soc/codecs/rt5677.c | 60 ++++++++++++++++++++++----------------- sound/soc/codecs/rt5677.h | 6 ++++ 2 files changed, 40 insertions(+), 26 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index fe000f30b9ad5f..87a92ba0d040b7 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -4716,37 +4716,13 @@ static int rt5677_probe(struct snd_soc_component *component)
snd_soc_component_force_bias_level(component, SND_SOC_BIAS_OFF);
- regmap_write(rt5677->regmap, RT5677_DIG_MISC, 0x0020); + regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, + ~RT5677_IRQ_DEBOUNCE_SEL_MASK, 0x0020); regmap_write(rt5677->regmap, RT5677_PWR_DSP2, 0x0c00);
for (i = 0; i < RT5677_GPIO_NUM; i++) rt5677_gpio_config(rt5677, i, rt5677->pdata.gpio_config[i]);
- if (rt5677->irq_data) { - regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, 0x8000, - 0x8000); - regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, 0x0018, - 0x0008); - - if (rt5677->pdata.jd1_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD1_MASK, - rt5677->pdata.jd1_gpio << - RT5677_SEL_GPIO_JD1_SFT); - - if (rt5677->pdata.jd2_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD2_MASK, - rt5677->pdata.jd2_gpio << - RT5677_SEL_GPIO_JD2_SFT); - - if (rt5677->pdata.jd3_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD3_MASK, - rt5677->pdata.jd3_gpio << - RT5677_SEL_GPIO_JD3_SFT); - } - mutex_init(&rt5677->dsp_cmd_lock); mutex_init(&rt5677->dsp_pri_lock);
@@ -5096,6 +5072,7 @@ static int rt5677_init_irq(struct i2c_client *i2c) { int ret; struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c); + unsigned int jd_mask = 0, jd_val = 0;
if (!rt5677->pdata.jd1_gpio && !rt5677->pdata.jd2_gpio && @@ -5107,6 +5084,37 @@ static int rt5677_init_irq(struct i2c_client *i2c) return -EINVAL; }
+ /* + * Select RC as the debounce clock so that GPIO works even when + * MCLK is gated which happens when there is no audio stream + * (SND_SOC_BIAS_OFF). + */ + regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, + RT5677_IRQ_DEBOUNCE_SEL_MASK, + RT5677_IRQ_DEBOUNCE_SEL_RC); + + /* Enable auto power on RC when GPIO states are changed */ + regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, 0xff, 0xff); + + /* Select and enable jack detection sources per platform data */ + if (rt5677->pdata.jd1_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD1_MASK; + jd_val |= rt5677->pdata.jd1_gpio << RT5677_SEL_GPIO_JD1_SFT; + } + if (rt5677->pdata.jd2_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD2_MASK; + jd_val |= rt5677->pdata.jd2_gpio << RT5677_SEL_GPIO_JD2_SFT; + } + if (rt5677->pdata.jd3_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD3_MASK; + jd_val |= rt5677->pdata.jd3_gpio << RT5677_SEL_GPIO_JD3_SFT; + } + regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, jd_mask, jd_val); + + /* Set GPIO1 pin to be IRQ output */ + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, + RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ); + ret = regmap_add_irq_chip(rt5677->regmap, i2c->irq, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, 0, &rt5677_irq_chip, &rt5677->irq_data); diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index 076e5161d8da30..c26edd387e340b 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1664,6 +1664,12 @@ #define RT5677_GPIO6_P_NOR (0x0 << 0) #define RT5677_GPIO6_P_INV (0x1 << 0)
+/* General Control (0xfa) */ +#define RT5677_IRQ_DEBOUNCE_SEL_MASK (0x3 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_MCLK (0x0 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_RC (0x1 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_SLIM (0x2 << 3) + /* Virtual DSP Mixer Control (0xf7 0xf8 0xf9) */ #define RT5677_DSP_IB_01_H (0x1 << 15) #define RT5677_DSP_IB_01_H_SFT 15
The patch
ASoC: rt5677: move jack-detect init to i2c probe
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3
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
From 241800642ea3482ab3f80a2a3662e9f2e6a82208 Mon Sep 17 00:00:00 2001
From: Fletcher Woodruff fletcherw@chromium.org Date: Fri, 14 Jun 2019 13:48:52 -0600 Subject: [PATCH] ASoC: rt5677: move jack-detect init to i2c probe
This patch moves the code to select the gpios for jack detection from rt5677_probe to rt5677_init_irq (called from rt5677_i2c_probe).
It also sets some registers to fix bugs related to jack detection, and adds some constants and comments to make it easier to understand what certain register settings are controlling.
Signed-off-by: Ben Zhang benzh@chromium.org Signed-off-by: Fletcher Woodruff fletcherw@chromium.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/rt5677.c | 60 ++++++++++++++++++++++----------------- sound/soc/codecs/rt5677.h | 6 ++++ 2 files changed, 40 insertions(+), 26 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index fe000f30b9ad..87a92ba0d040 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -4716,37 +4716,13 @@ static int rt5677_probe(struct snd_soc_component *component)
snd_soc_component_force_bias_level(component, SND_SOC_BIAS_OFF);
- regmap_write(rt5677->regmap, RT5677_DIG_MISC, 0x0020); + regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, + ~RT5677_IRQ_DEBOUNCE_SEL_MASK, 0x0020); regmap_write(rt5677->regmap, RT5677_PWR_DSP2, 0x0c00);
for (i = 0; i < RT5677_GPIO_NUM; i++) rt5677_gpio_config(rt5677, i, rt5677->pdata.gpio_config[i]);
- if (rt5677->irq_data) { - regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, 0x8000, - 0x8000); - regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, 0x0018, - 0x0008); - - if (rt5677->pdata.jd1_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD1_MASK, - rt5677->pdata.jd1_gpio << - RT5677_SEL_GPIO_JD1_SFT); - - if (rt5677->pdata.jd2_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD2_MASK, - rt5677->pdata.jd2_gpio << - RT5677_SEL_GPIO_JD2_SFT); - - if (rt5677->pdata.jd3_gpio) - regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, - RT5677_SEL_GPIO_JD3_MASK, - rt5677->pdata.jd3_gpio << - RT5677_SEL_GPIO_JD3_SFT); - } - mutex_init(&rt5677->dsp_cmd_lock); mutex_init(&rt5677->dsp_pri_lock);
@@ -5096,6 +5072,7 @@ static int rt5677_init_irq(struct i2c_client *i2c) { int ret; struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c); + unsigned int jd_mask = 0, jd_val = 0;
if (!rt5677->pdata.jd1_gpio && !rt5677->pdata.jd2_gpio && @@ -5107,6 +5084,37 @@ static int rt5677_init_irq(struct i2c_client *i2c) return -EINVAL; }
+ /* + * Select RC as the debounce clock so that GPIO works even when + * MCLK is gated which happens when there is no audio stream + * (SND_SOC_BIAS_OFF). + */ + regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, + RT5677_IRQ_DEBOUNCE_SEL_MASK, + RT5677_IRQ_DEBOUNCE_SEL_RC); + + /* Enable auto power on RC when GPIO states are changed */ + regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, 0xff, 0xff); + + /* Select and enable jack detection sources per platform data */ + if (rt5677->pdata.jd1_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD1_MASK; + jd_val |= rt5677->pdata.jd1_gpio << RT5677_SEL_GPIO_JD1_SFT; + } + if (rt5677->pdata.jd2_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD2_MASK; + jd_val |= rt5677->pdata.jd2_gpio << RT5677_SEL_GPIO_JD2_SFT; + } + if (rt5677->pdata.jd3_gpio) { + jd_mask |= RT5677_SEL_GPIO_JD3_MASK; + jd_val |= rt5677->pdata.jd3_gpio << RT5677_SEL_GPIO_JD3_SFT; + } + regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1, jd_mask, jd_val); + + /* Set GPIO1 pin to be IRQ output */ + regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, + RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ); + ret = regmap_add_irq_chip(rt5677->regmap, i2c->irq, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, 0, &rt5677_irq_chip, &rt5677->irq_data); diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index 076e5161d8da..c26edd387e34 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1664,6 +1664,12 @@ #define RT5677_GPIO6_P_NOR (0x0 << 0) #define RT5677_GPIO6_P_INV (0x1 << 0)
+/* General Control (0xfa) */ +#define RT5677_IRQ_DEBOUNCE_SEL_MASK (0x3 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_MCLK (0x0 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_RC (0x1 << 3) +#define RT5677_IRQ_DEBOUNCE_SEL_SLIM (0x2 << 3) + /* Virtual DSP Mixer Control (0xf7 0xf8 0xf9) */ #define RT5677_DSP_IB_01_H (0x1 << 15) #define RT5677_DSP_IB_01_H_SFT 15
From: Ben Zhang benzh@chromium.org
The rt5677 jack detection function has a requirement that the polarity of an interrupt be flipped after it fires in order to clear the interrupt.
This patch implements an irq_chip with irq_domain directly instead of using regmap-irq, so that interrupt source line polarities can be flipped in the irq handler.
The reason that this patch does not add this feature within regmap-irq is that future patches will add hotword detection support to this irq handler. Those patches will require adding additional logic that would not make sense to have in regmap-irq.
Signed-off-by: Ben Zhang benzh@chromium.org Signed-off-by: Fletcher Woodruff fletcherw@chromium.org --- sound/soc/codecs/rt5677.c | 170 ++++++++++++++++++++++++++++++-------- sound/soc/codecs/rt5677.h | 7 +- 2 files changed, 143 insertions(+), 34 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index 87a92ba0d040b7..87466ee222ee59 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -23,6 +23,10 @@ #include <linux/firmware.h> #include <linux/of_device.h> #include <linux/property.h> +#include <linux/irq.h> +#include <linux/interrupt.h> +#include <linux/irqdomain.h> +#include <linux/workqueue.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -4620,7 +4624,6 @@ static void rt5677_gpio_config(struct rt5677_priv *rt5677, unsigned offset, static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) { struct rt5677_priv *rt5677 = gpiochip_get_data(chip); - struct regmap_irq_chip_data *data = rt5677->irq_data; int irq;
if ((rt5677->pdata.jd1_gpio == 1 && offset == RT5677_GPIO1) || @@ -4646,7 +4649,7 @@ static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) return -ENXIO; }
- return regmap_irq_get_virq(data, irq); + return irq_create_mapping(rt5677->domain, irq); }
static const struct gpio_chip rt5677_template_chip = { @@ -5042,30 +5045,130 @@ static void rt5677_read_device_properties(struct rt5677_priv *rt5677, rt5677->pdata.jd3_gpio = val; }
-static struct regmap_irq rt5677_irqs[] = { +struct rt5677_irq_desc { + unsigned int enable_mask; + unsigned int status_mask; + unsigned int polarity_mask; +}; + +static const struct rt5677_irq_desc rt5677_irq_descs[] = { [RT5677_IRQ_JD1] = { - .reg_offset = 0, - .mask = RT5677_EN_IRQ_GPIO_JD1, + .enable_mask = RT5677_EN_IRQ_GPIO_JD1, + .status_mask = RT5677_STA_GPIO_JD1, + .polarity_mask = RT5677_INV_GPIO_JD1, }, [RT5677_IRQ_JD2] = { - .reg_offset = 0, - .mask = RT5677_EN_IRQ_GPIO_JD2, + .enable_mask = RT5677_EN_IRQ_GPIO_JD2, + .status_mask = RT5677_STA_GPIO_JD2, + .polarity_mask = RT5677_INV_GPIO_JD2, }, [RT5677_IRQ_JD3] = { - .reg_offset = 0, - .mask = RT5677_EN_IRQ_GPIO_JD3, + .enable_mask = RT5677_EN_IRQ_GPIO_JD3, + .status_mask = RT5677_STA_GPIO_JD3, + .polarity_mask = RT5677_INV_GPIO_JD3, }, };
-static struct regmap_irq_chip rt5677_irq_chip = { - .name = RT5677_DRV_NAME, - .irqs = rt5677_irqs, - .num_irqs = ARRAY_SIZE(rt5677_irqs), +static irqreturn_t rt5677_irq(int unused, void *data) +{ + struct rt5677_priv *rt5677 = data; + int ret = 0, i, reg_irq, virq; + bool irq_fired = false; + + mutex_lock(&rt5677->irq_lock); + /* Read interrupt status */ + ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq); + if (ret) { + pr_err("rt5677: failed reading IRQ status: %d\n", ret); + goto exit; + } + + for (i = 0; i < RT5677_IRQ_NUM; i++) { + if (reg_irq & rt5677_irq_descs[i].status_mask) { + irq_fired = true; + virq = irq_find_mapping(rt5677->domain, i); + if (virq) + handle_nested_irq(virq); + + /* Clear the interrupt by flipping the polarity of the + * interrupt source line that fired + */ + reg_irq ^= rt5677_irq_descs[i].polarity_mask; + } + } + + if (!irq_fired) + goto exit; + + ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq); + if (ret) { + pr_err("rt5677: failed updating IRQ status: %d\n", ret); + goto exit; + } +exit: + mutex_unlock(&rt5677->irq_lock); + if (irq_fired) + return IRQ_HANDLED; + else + return IRQ_NONE; +}
- .num_regs = 1, - .status_base = RT5677_IRQ_CTRL1, - .mask_base = RT5677_IRQ_CTRL1, - .mask_invert = 1, +static void rt5677_irq_bus_lock(struct irq_data *data) +{ + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); + + mutex_lock(&rt5677->irq_lock); +} + +static void rt5677_irq_bus_sync_unlock(struct irq_data *data) +{ + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); + + // Set the enable/disable bits for the jack detect IRQs. + regmap_update_bits(rt5677->regmap, RT5677_IRQ_CTRL1, + RT5677_EN_IRQ_GPIO_JD1 | RT5677_EN_IRQ_GPIO_JD2 | + RT5677_EN_IRQ_GPIO_JD3, rt5677->irq_en); + mutex_unlock(&rt5677->irq_lock); +} + +static void rt5677_irq_enable(struct irq_data *data) +{ + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); + + rt5677->irq_en |= rt5677_irq_descs[data->hwirq].enable_mask; +} + +static void rt5677_irq_disable(struct irq_data *data) +{ + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data); + + rt5677->irq_en &= ~rt5677_irq_descs[data->hwirq].enable_mask; +} + +static struct irq_chip rt5677_irq_chip = { + .name = "rt5677_irq_chip", + .irq_bus_lock = rt5677_irq_bus_lock, + .irq_bus_sync_unlock = rt5677_irq_bus_sync_unlock, + .irq_disable = rt5677_irq_disable, + .irq_enable = rt5677_irq_enable, +}; + +static int rt5677_irq_map(struct irq_domain *h, unsigned int virq, + irq_hw_number_t hw) +{ + struct rt5677_priv *rt5677 = h->host_data; + + irq_set_chip_data(virq, rt5677); + irq_set_chip(virq, &rt5677_irq_chip); + irq_set_nested_thread(virq, 1); + irq_set_noprobe(virq); + return 0; +} + + +static const struct irq_domain_ops rt5677_domain_ops = { + .map = rt5677_irq_map, + .xlate = irq_domain_xlate_twocell, };
static int rt5677_init_irq(struct i2c_client *i2c) @@ -5084,6 +5187,8 @@ static int rt5677_init_irq(struct i2c_client *i2c) return -EINVAL; }
+ mutex_init(&rt5677->irq_lock); + /* * Select RC as the debounce clock so that GPIO works even when * MCLK is gated which happens when there is no audio stream @@ -5092,7 +5197,6 @@ static int rt5677_init_irq(struct i2c_client *i2c) regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, RT5677_IRQ_DEBOUNCE_SEL_MASK, RT5677_IRQ_DEBOUNCE_SEL_RC); - /* Enable auto power on RC when GPIO states are changed */ regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, 0xff, 0xff);
@@ -5115,26 +5219,25 @@ static int rt5677_init_irq(struct i2c_client *i2c) regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ);
- ret = regmap_add_irq_chip(rt5677->regmap, i2c->irq, - IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, 0, - &rt5677_irq_chip, &rt5677->irq_data); + /* Ready to listen for interrupts */ + rt5677->domain = irq_domain_add_linear(i2c->dev.of_node, + RT5677_IRQ_NUM, &rt5677_domain_ops, rt5677); + if (!rt5677->domain) { + dev_err(&i2c->dev, "Failed to create IRQ domain\n"); + return -ENOMEM; + }
- if (ret != 0) { - dev_err(&i2c->dev, "Failed to register IRQ chip: %d\n", ret); + ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, rt5677_irq, + IRQF_TRIGGER_RISING | IRQF_ONESHOT, + "rt5677", rt5677); + if (ret) { + dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret); return ret; }
return 0; }
-static void rt5677_free_irq(struct i2c_client *i2c) -{ - struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c); - - if (rt5677->irq_data) - regmap_del_irq_chip(i2c->irq, rt5677->irq_data); -} - static int rt5677_i2c_probe(struct i2c_client *i2c) { struct rt5677_priv *rt5677; @@ -5259,7 +5362,9 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) RT5677_MICBIAS1_CTRL_VDD_3_3V);
rt5677_init_gpio(i2c); - rt5677_init_irq(i2c); + ret = rt5677_init_irq(i2c); + if (ret) + dev_err(&i2c->dev, "Failed to initialize irq: %d\n", ret);
return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5677, @@ -5268,7 +5373,6 @@ static int rt5677_i2c_probe(struct i2c_client *i2c)
static int rt5677_i2c_remove(struct i2c_client *i2c) { - rt5677_free_irq(i2c); rt5677_free_gpio(i2c);
return 0; diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index c26edd387e340b..d0ac26e562eb5f 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1749,6 +1749,7 @@ enum { RT5677_IRQ_JD1, RT5677_IRQ_JD2, RT5677_IRQ_JD3, + RT5677_IRQ_NUM, };
enum rt5677_type { @@ -1847,9 +1848,13 @@ struct rt5677_priv { struct gpio_chip gpio_chip; #endif bool dsp_vad_en; - struct regmap_irq_chip_data *irq_data; bool is_dsp_mode; bool is_vref_slow; + + /* Interrupt handling */ + struct irq_domain *domain; + struct mutex irq_lock; + unsigned int irq_en; };
int rt5677_sel_asrc_clk_src(struct snd_soc_component *component,
On 2019-06-14 21:48, Fletcher Woodruff wrote:
From: Ben Zhang benzh@chromium.org
The rt5677 jack detection function has a requirement that the polarity of an interrupt be flipped after it fires in order to clear the interrupt.
This patch implements an irq_chip with irq_domain directly instead of using regmap-irq, so that interrupt source line polarities can be flipped in the irq handler.
The reason that this patch does not add this feature within regmap-irq is that future patches will add hotword detection support to this irq handler. Those patches will require adding additional logic that would not make sense to have in regmap-irq.
Signed-off-by: Ben Zhang benzh@chromium.org Signed-off-by: Fletcher Woodruff fletcherw@chromium.org
sound/soc/codecs/rt5677.c | 170 ++++++++++++++++++++++++++++++-------- sound/soc/codecs/rt5677.h | 7 +- 2 files changed, 143 insertions(+), 34 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index 87a92ba0d040b7..87466ee222ee59 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -23,6 +23,10 @@ #include <linux/firmware.h> #include <linux/of_device.h> #include <linux/property.h> +#include <linux/irq.h> +#include <linux/interrupt.h> +#include <linux/irqdomain.h> +#include <linux/workqueue.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -4620,7 +4624,6 @@ static void rt5677_gpio_config(struct rt5677_priv *rt5677, unsigned offset, static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) { struct rt5677_priv *rt5677 = gpiochip_get_data(chip);
struct regmap_irq_chip_data *data = rt5677->irq_data; int irq;
if ((rt5677->pdata.jd1_gpio == 1 && offset == RT5677_GPIO1) ||
@@ -4646,7 +4649,7 @@ static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset) return -ENXIO; }
- return regmap_irq_get_virq(data, irq);
return irq_create_mapping(rt5677->domain, irq); }
static const struct gpio_chip rt5677_template_chip = {
@@ -5042,30 +5045,130 @@ static void rt5677_read_device_properties(struct rt5677_priv *rt5677, rt5677->pdata.jd3_gpio = val; }
-static struct regmap_irq rt5677_irqs[] = { +struct rt5677_irq_desc {
- unsigned int enable_mask;
- unsigned int status_mask;
- unsigned int polarity_mask;
+};
+static const struct rt5677_irq_desc rt5677_irq_descs[] = { [RT5677_IRQ_JD1] = {
.reg_offset = 0,
.mask = RT5677_EN_IRQ_GPIO_JD1,
.enable_mask = RT5677_EN_IRQ_GPIO_JD1,
.status_mask = RT5677_STA_GPIO_JD1,
}, [RT5677_IRQ_JD2] = {.polarity_mask = RT5677_INV_GPIO_JD1,
.reg_offset = 0,
.mask = RT5677_EN_IRQ_GPIO_JD2,
.enable_mask = RT5677_EN_IRQ_GPIO_JD2,
.status_mask = RT5677_STA_GPIO_JD2,
}, [RT5677_IRQ_JD3] = {.polarity_mask = RT5677_INV_GPIO_JD2,
.reg_offset = 0,
.mask = RT5677_EN_IRQ_GPIO_JD3,
.enable_mask = RT5677_EN_IRQ_GPIO_JD3,
.status_mask = RT5677_STA_GPIO_JD3,
}, };.polarity_mask = RT5677_INV_GPIO_JD3,
-static struct regmap_irq_chip rt5677_irq_chip = {
- .name = RT5677_DRV_NAME,
- .irqs = rt5677_irqs,
- .num_irqs = ARRAY_SIZE(rt5677_irqs),
+static irqreturn_t rt5677_irq(int unused, void *data) +{
- struct rt5677_priv *rt5677 = data;
- int ret = 0, i, reg_irq, virq;
- bool irq_fired = false;
- mutex_lock(&rt5677->irq_lock);
- /* Read interrupt status */
- ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
- if (ret) {
pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
goto exit;
- }
- for (i = 0; i < RT5677_IRQ_NUM; i++) {
if (reg_irq & rt5677_irq_descs[i].status_mask) {
irq_fired = true;
virq = irq_find_mapping(rt5677->domain, i);
if (virq)
handle_nested_irq(virq);
/* Clear the interrupt by flipping the polarity of the
* interrupt source line that fired
*/
reg_irq ^= rt5677_irq_descs[i].polarity_mask;
}
- }
- if (!irq_fired)
goto exit;
- ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq);
- if (ret) {
pr_err("rt5677: failed updating IRQ status: %d\n", ret);
Same here.
goto exit;
- }
+exit:
- mutex_unlock(&rt5677->irq_lock);
- if (irq_fired)
return IRQ_HANDLED;
- else
return IRQ_NONE;
+}
- .num_regs = 1,
- .status_base = RT5677_IRQ_CTRL1,
- .mask_base = RT5677_IRQ_CTRL1,
- .mask_invert = 1,
+static void rt5677_irq_bus_lock(struct irq_data *data) +{
- struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data);
- mutex_lock(&rt5677->irq_lock);
+}
+static void rt5677_irq_bus_sync_unlock(struct irq_data *data) +{
- struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data);
- // Set the enable/disable bits for the jack detect IRQs.
- regmap_update_bits(rt5677->regmap, RT5677_IRQ_CTRL1,
RT5677_EN_IRQ_GPIO_JD1 | RT5677_EN_IRQ_GPIO_JD2 |
RT5677_EN_IRQ_GPIO_JD3, rt5677->irq_en);
- mutex_unlock(&rt5677->irq_lock);
+}
+static void rt5677_irq_enable(struct irq_data *data) +{
- struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data);
- rt5677->irq_en |= rt5677_irq_descs[data->hwirq].enable_mask;
+}
+static void rt5677_irq_disable(struct irq_data *data) +{
- struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data);
- rt5677->irq_en &= ~rt5677_irq_descs[data->hwirq].enable_mask;
+}
+static struct irq_chip rt5677_irq_chip = {
- .name = "rt5677_irq_chip",
- .irq_bus_lock = rt5677_irq_bus_lock,
- .irq_bus_sync_unlock = rt5677_irq_bus_sync_unlock,
- .irq_disable = rt5677_irq_disable,
- .irq_enable = rt5677_irq_enable,
+};
+static int rt5677_irq_map(struct irq_domain *h, unsigned int virq,
irq_hw_number_t hw)
+{
- struct rt5677_priv *rt5677 = h->host_data;
- irq_set_chip_data(virq, rt5677);
- irq_set_chip(virq, &rt5677_irq_chip);
- irq_set_nested_thread(virq, 1);
- irq_set_noprobe(virq);
- return 0;
+}
+static const struct irq_domain_ops rt5677_domain_ops = {
.map = rt5677_irq_map,
.xlate = irq_domain_xlate_twocell, };
static int rt5677_init_irq(struct i2c_client *i2c)
@@ -5084,6 +5187,8 @@ static int rt5677_init_irq(struct i2c_client *i2c) return -EINVAL; }
- mutex_init(&rt5677->irq_lock);
- /*
- Select RC as the debounce clock so that GPIO works even when
- MCLK is gated which happens when there is no audio stream
@@ -5092,7 +5197,6 @@ static int rt5677_init_irq(struct i2c_client *i2c) regmap_update_bits(rt5677->regmap, RT5677_DIG_MISC, RT5677_IRQ_DEBOUNCE_SEL_MASK, RT5677_IRQ_DEBOUNCE_SEL_RC);
- /* Enable auto power on RC when GPIO states are changed */ regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, 0xff, 0xff);
@@ -5115,26 +5219,25 @@ static int rt5677_init_irq(struct i2c_client *i2c) regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1, RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ);
- ret = regmap_add_irq_chip(rt5677->regmap, i2c->irq,
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, 0,
&rt5677_irq_chip, &rt5677->irq_data);
- /* Ready to listen for interrupts */
- rt5677->domain = irq_domain_add_linear(i2c->dev.of_node,
RT5677_IRQ_NUM, &rt5677_domain_ops, rt5677);
- if (!rt5677->domain) {
dev_err(&i2c->dev, "Failed to create IRQ domain\n");
return -ENOMEM;
- }
- if (ret != 0) {
dev_err(&i2c->dev, "Failed to register IRQ chip: %d\n", ret);
- ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, rt5677_irq,
IRQF_TRIGGER_RISING | IRQF_ONESHOT,
"rt5677", rt5677);
- if (ret) {
return ret;dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret);
You may want to simplify this, similarly to how's it done in your rt5677_i2c_probe - leave message alone and return "ret" explicitly at the end.
}
return 0; }
-static void rt5677_free_irq(struct i2c_client *i2c) -{
- struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c);
- if (rt5677->irq_data)
regmap_del_irq_chip(i2c->irq, rt5677->irq_data);
-}
- static int rt5677_i2c_probe(struct i2c_client *i2c) { struct rt5677_priv *rt5677;
@@ -5259,7 +5362,9 @@ static int rt5677_i2c_probe(struct i2c_client *i2c) RT5677_MICBIAS1_CTRL_VDD_3_3V);
rt5677_init_gpio(i2c);
- rt5677_init_irq(i2c);
ret = rt5677_init_irq(i2c);
if (ret)
dev_err(&i2c->dev, "Failed to initialize irq: %d\n", ret);
return devm_snd_soc_register_component(&i2c->dev, &soc_component_dev_rt5677,
@@ -5268,7 +5373,6 @@ static int rt5677_i2c_probe(struct i2c_client *i2c)
static int rt5677_i2c_remove(struct i2c_client *i2c) {
rt5677_free_irq(i2c); rt5677_free_gpio(i2c);
return 0;
diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h index c26edd387e340b..d0ac26e562eb5f 100644 --- a/sound/soc/codecs/rt5677.h +++ b/sound/soc/codecs/rt5677.h @@ -1749,6 +1749,7 @@ enum { RT5677_IRQ_JD1, RT5677_IRQ_JD2, RT5677_IRQ_JD3,
RT5677_IRQ_NUM, };
enum rt5677_type {
@@ -1847,9 +1848,13 @@ struct rt5677_priv { struct gpio_chip gpio_chip; #endif bool dsp_vad_en;
- struct regmap_irq_chip_data *irq_data; bool is_dsp_mode; bool is_vref_slow;
/* Interrupt handling */
struct irq_domain *domain;
struct mutex irq_lock;
unsigned int irq_en; };
int rt5677_sel_asrc_clk_src(struct snd_soc_component *component,
On Sun, Jun 16, 2019 at 10:56 AM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2019-06-14 21:48, Fletcher Woodruff wrote:
+static irqreturn_t rt5677_irq(int unused, void *data) +{
struct rt5677_priv *rt5677 = data;
int ret = 0, i, reg_irq, virq;
bool irq_fired = false;
mutex_lock(&rt5677->irq_lock);
/* Read interrupt status */
ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
if (ret) {
pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
I was using dev_XXX, but I believe Curtis found that 'component' was sometimes uninitialized when this function was called, so I switched back to pr_XXX. I may be misremembering though, so I'll let Curtis comment as well.
if (ret) {
dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret); return ret;
You may want to simplify this, similarly to how's it done in your rt5677_i2c_probe - leave message alone and return "ret" explicitly at the end.
Good suggestion. I'll update that for the next patch.
On Tue, Jun 18, 2019 at 11:01 AM Fletcher Woodruff fletcherw@chromium.org wrote:
On Sun, Jun 16, 2019 at 10:56 AM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2019-06-14 21:48, Fletcher Woodruff wrote:
+static irqreturn_t rt5677_irq(int unused, void *data) +{
struct rt5677_priv *rt5677 = data;
int ret = 0, i, reg_irq, virq;
bool irq_fired = false;
mutex_lock(&rt5677->irq_lock);
/* Read interrupt status */
ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
if (ret) {
pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
I was using dev_XXX, but I believe Curtis found that 'component' was sometimes uninitialized when this function was called, so I switched back to pr_XXX. I may be misremembering though, so I'll let Curtis comment as well.
The issue here is that the IRQ is setup in the i2c probe and the component is setup in the codec probe. In theory if the hardware is in a bad state you can get an interrupt between the probes and have the interrupt called with the component being NULL. It might be worth considering migrating the IRQ setup to the codec probe so this condition cannot happen and then we can avoid using pr_XXX.
if (ret) {
dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret); return ret;
You may want to simplify this, similarly to how's it done in your rt5677_i2c_probe - leave message alone and return "ret" explicitly at the end.
Good suggestion. I'll update that for the next patch.
On Tue, Jun 18, 2019 at 11:12:58AM -0700, Curtis Malainey wrote:
On Tue, Jun 18, 2019 at 11:01 AM Fletcher Woodruff
On Sun, Jun 16, 2019 at 10:56 AM Cezary Rojewski
On 2019-06-14 21:48, Fletcher Woodruff wrote:
ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
if (ret) {
pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
I was using dev_XXX, but I believe Curtis found that 'component' was sometimes uninitialized when this function was called, so I switched back to pr_XXX. I may be misremembering though, so I'll let Curtis comment as well.
The issue here is that the IRQ is setup in the i2c probe and the component is setup in the codec probe. In theory if the hardware is in
The component is not needed for a struct device, you must have a struct device if you have a regmap or have probed at all.
On Tue, Jun 18, 2019 at 11:47 AM Mark Brown broonie@kernel.org wrote:
On Tue, Jun 18, 2019 at 11:12:58AM -0700, Curtis Malainey wrote:
On Tue, Jun 18, 2019 at 11:01 AM Fletcher Woodruff
On Sun, Jun 16, 2019 at 10:56 AM Cezary Rojewski
On 2019-06-14 21:48, Fletcher Woodruff wrote:
ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
if (ret) {
pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very function. Consider reusing "component" field which is already part of struct rt5677_priv and removing pr_XXX.
I was using dev_XXX, but I believe Curtis found that 'component' was sometimes uninitialized when this function was called, so I switched back to pr_XXX. I may be misremembering though, so I'll let Curtis comment as well.
The issue here is that the IRQ is setup in the i2c probe and the component is setup in the codec probe. In theory if the hardware is in
The component is not needed for a struct device, you must have a struct device if you have a regmap or have probed at all.
Ah yes, we could modify the struct and store the i2c device and get the device out of that as well. That will likely be simpler. Ok lets do that.
From: Ben Zhang benzh@chromium.org
The rt5677 driver writes to the IRQ control register within the IRQ handler in order to flip the polarity of the interrupts that have been signalled. If an interrupt fires in the interval between the regmap_read and the regmap_write, it will not trigger a new call to rt5677_irq.
Add a bounded loop to rt5677_irq that keeps checking interrupts until none are seen, so that any interrupts that are signalled in that interval are correctly handled.
Signed-off-by: Ben Zhang benzh@chromium.org Signed-off-by: Fletcher Woodruff fletcherw@chromium.org --- sound/soc/codecs/rt5677.c | 67 ++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index 87466ee222ee59..e88766e34ddb1d 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -5072,38 +5072,55 @@ static const struct rt5677_irq_desc rt5677_irq_descs[] = { static irqreturn_t rt5677_irq(int unused, void *data) { struct rt5677_priv *rt5677 = data; - int ret = 0, i, reg_irq, virq; + int ret = 0, loop, i, reg_irq, virq; bool irq_fired = false;
mutex_lock(&rt5677->irq_lock); - /* Read interrupt status */ - ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq); - if (ret) { - pr_err("rt5677: failed reading IRQ status: %d\n", ret); - goto exit; - }
- for (i = 0; i < RT5677_IRQ_NUM; i++) { - if (reg_irq & rt5677_irq_descs[i].status_mask) { - irq_fired = true; - virq = irq_find_mapping(rt5677->domain, i); - if (virq) - handle_nested_irq(virq); - - /* Clear the interrupt by flipping the polarity of the - * interrupt source line that fired - */ - reg_irq ^= rt5677_irq_descs[i].polarity_mask; + /* + * Loop to handle interrupts until the last i2c read shows no pending + * irqs. The interrupt line is shared by multiple interrupt sources. + * After the regmap_read() below, a new interrupt source line may + * become high before the regmap_write() finishes, so there isn't a + * rising edge on the shared interrupt line for the new interrupt. Thus, + * the loop is needed to avoid missing irqs. + * + * A safeguard of 20 loops is used to avoid hanging in the irq handler + * if there is something wrong with the interrupt status update. The + * interrupt sources here are audio jack plug/unplug events which + * shouldn't happen at a high frequency for a long period of time. + * Empirically, more than 3 loops have never been seen. + */ + for (loop = 0; loop < 20; loop++) { + /* Read interrupt status */ + ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq); + if (ret) { + pr_err("rt5677: failed reading IRQ status: %d\n", ret); + goto exit; } - }
- if (!irq_fired) - goto exit; + irq_fired = false; + for (i = 0; i < RT5677_IRQ_NUM; i++) { + if (reg_irq & rt5677_irq_descs[i].status_mask) { + irq_fired = true; + virq = irq_find_mapping(rt5677->domain, i); + if (virq) + handle_nested_irq(virq); + + /* Clear the interrupt by flipping the polarity + * of the interrupt source line that fired + */ + reg_irq ^= rt5677_irq_descs[i].polarity_mask; + } + } + if (!irq_fired) + goto exit;
- ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq); - if (ret) { - pr_err("rt5677: failed updating IRQ status: %d\n", ret); - goto exit; + ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq); + if (ret) { + pr_err("rt5677: failed updating IRQ status: %d\n", ret); + goto exit; + } } exit: mutex_unlock(&rt5677->irq_lock);
participants (4)
-
Cezary Rojewski
-
Curtis Malainey
-
Fletcher Woodruff
-
Mark Brown