Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
From: Mark Brown Date: Wed, May 8, 2019 at 7:33 PM To: Curtis Malainey Cc: Fletcher Woodruff, Linux Kernel Mailing List, Ben Zhang, Jaroslav Kysela, Liam Girdwood, Oder Chiou, Takashi Iwai, Curtis Malainey, > On Wed, May 08, 2019 at 02:39:32PM -0700, Curtis Malainey wrote: > > > Pixelbooks (Samus Chromebook) are the only devices that use this part. > > Realtek has confirmed this. Therefore we only have to worry about > > breaking ourselves. That being said I agree there is likely a better > > And there are no other parts that are software compatible enough to > share the same driver? > the rt5676 can use this driver, but from my discussions with Realtek, Samus is the only active consumer of this driver. > > way to handle general abstraction here. We will need the explicit irq > > handling since I will be following these patches up with patches that > > enable hotwording on the codec (we will be sending the firmware to > > linux-firmware as well that is needed for the process.) > > OK. Like I said it might also be clearer split into multiple patches, > it was just really difficult to tell what was going on with the diff > there.
Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
On Wed, May 08, 2019 at 02:39:32PM -0700, Curtis Malainey wrote: > Pixelbooks (Samus Chromebook) are the only devices that use this part. > Realtek has confirmed this. Therefore we only have to worry about > breaking ourselves. That being said I agree there is likely a better And there are no other parts that are software compatible enough to share the same driver? > way to handle general abstraction here. We will need the explicit irq > handling since I will be following these patches up with patches that > enable hotwording on the codec (we will be sending the firmware to > linux-firmware as well that is needed for the process.) OK. Like I said it might also be clearer split into multiple patches, it was just really difficult to tell what was going on with the diff there. signature.asc Description: PGP signature
Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
From: Mark Brown Date: Wed, May 8, 2019 at 12:36 AM To: Fletcher Woodruff Cc: , Ben Zhang, Jaroslav Kysela, Liam Girdwood, Oder Chiou, Takashi Iwai, Curtis Malainey, > On Tue, May 07, 2019 at 04:01:13PM -0600, Fletcher Woodruff wrote: > > > This patch does not add polarity flipping support within regmap-irq > > because there is extra work that must be done within the irq handler > > to support hotword detection. On the Chromebook Pixel, the firmware will > > disconnect GPIO1 from the jack detection irq when a hotword is detected > > and trigger the interrupt handler. Inside the handler, we will need to > > detect this, report the hotword event, and re-connect GPIO1 to the jack > > detection irq. > > Please have a conversation with your firmware team about the concept of > abstraction - this is clearly a problematic thing to do as it's causing > the state of the system to change for devices that are mostly managed > from the operating system. It's not clear to me that this shouldn't be > split off somehow so that it doesn't impact other systems using this > hardware. > Pixelbooks (Samus Chromebook) are the only devices that use this part. Realtek has confirmed this. Therefore we only have to worry about breaking ourselves. That being said I agree there is likely a better way to handle general abstraction here. We will need the explicit irq handling since I will be following these patches up with patches that enable hotwording on the codec (we will be sending the firmware to linux-firmware as well that is needed for the process.)
Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
On Tue, May 07, 2019 at 04:01:13PM -0600, Fletcher Woodruff wrote:
> This patch does not add polarity flipping support within regmap-irq
> because there is extra work that must be done within the irq handler
> to support hotword detection. On the Chromebook Pixel, the firmware will
> disconnect GPIO1 from the jack detection irq when a hotword is detected
> and trigger the interrupt handler. Inside the handler, we will need to
> detect this, report the hotword event, and re-connect GPIO1 to the jack
> detection irq.
Please have a conversation with your firmware team about the concept of
abstraction - this is clearly a problematic thing to do as it's causing
the state of the system to change for devices that are mostly managed
from the operating system. It's not clear to me that this shouldn't be
split off somehow so that it doesn't impact other systems using this
hardware.
I'm actually not entirely clear what the code that does the "reconnect
GPIO1 to the jack detection IRQ" bit is, I couldn't find anything
outside of the initial probe.
> - 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);
> - }
> -
There's a lot of refactoring in the patch here which makes it very hard
to follow what the actual change is.
> + }
> + }
> +exit:
> + mutex_unlock(>irq_lock);
> + return IRQ_HANDLED;
> +}
We uncondtionally report the interrupt as handled?
> +static void rt5677_irq_work(struct work_struct *work)
> {
> - int ret;
> - struct rt5677_priv *rt5677 = i2c_get_clientdata(i2c);
> + struct rt5677_priv *rt5677 =
> + container_of(work, struct rt5677_priv, irq_work.work);
>
> - if (!rt5677->pdata.jd1_gpio &&
> - !rt5677->pdata.jd2_gpio &&
> - !rt5677->pdata.jd3_gpio)
> - return 0;
> + rt5677_irq(0, rt5677);
> +}
I couldn't find anything that schedules this. What is it doing, why is
it here (and this is an example of a really complex to review bit of the
change due to refactoring BTW, the diff is really unhelpful)?
> +static void rt5677_irq_bus_sync_unlock(struct irq_data *data)
> +{
> + struct rt5677_priv *rt5677 = irq_data_get_irq_chip_data(data);
> +
> + 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(>irq_lock);
> +}
Is this the bit that reenables the interrupt? Isn't this just a quirk
to rewrite the masks frequently, that'd seem easier than doing so much
open coding?
> + /* 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);
Are other GPIO outputs supported by the chip? How does this interact
with the jdN_gpio settings above?
signature.asc
Description: PGP signature
[PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
From: Ben Zhang
This patch allows headphone plug detect and mic present
detect to be enabled at the same time. This patch implements
an irq_chip with irq_domain directly instead of using
regmap-irq, so that interrupt source line polarities can
be flipped to support irq sharing.
This patch does not add polarity flipping support within regmap-irq
because there is extra work that must be done within the irq handler
to support hotword detection. On the Chromebook Pixel, the firmware will
disconnect GPIO1 from the jack detection irq when a hotword is detected
and trigger the interrupt handler. Inside the handler, we will need to
detect this, report the hotword event, and re-connect GPIO1 to the jack
detection irq.
Signed-off-by: Ben Zhang
Signed-off-by: Fletcher Woodruff
---
sound/soc/codecs/rt5677.c | 256 --
sound/soc/codecs/rt5677.h | 14 ++-
2 files changed, 203 insertions(+), 67 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 9b7a1833d3316c..7d2039f3f67e75 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -23,6 +23,10 @@
#include
#include
#include
+#include
+#include
+#include
+#include
#include
#include
#include
@@ -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 = {
@@ -4716,37 +4719,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(>dsp_cmd_lock);
mutex_init(>dsp_pri_lock);
@@ -5063,65 +5042,210 @@ static void rt5677_read_device_properties(struct
rt5677_priv *rt5677,
>pdata.jd3_gpio);
}
-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 =
