Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4133 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]