Re: [alsa-devel] [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(&rt5677->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(&rt5677->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?