LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Fletcher Woodruff <fletcherw@chromium.org>
Cc: linux-kernel@vger.kernel.org, Ben Zhang <benzh@chromium.org>,
	Jaroslav Kysela <perex@perex.cz>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Oder Chiou <oder_chiou@realtek.com>,
	Takashi Iwai <tiwai@suse.com>,
	Curtis Malainey <cujomalainey@chromium.org>,
	alsa-devel@alsa-project.org
Subject: Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources
Date: Wed, 8 May 2019 16:36:23 +0900	[thread overview]
Message-ID: <20190508073623.GT14916@sirena.org.uk> (raw)
In-Reply-To: <20190507220115.90395-2-fletcherw@chromium.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 --]

  reply	other threads:[~2019-05-08  7:36 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-07 22:01 [PATCH v5 0/3] Fix jack detection for Chromebook Pixel Fletcher Woodruff
2019-05-07 22:01 ` [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources Fletcher Woodruff
2019-05-08  7:36   ` Mark Brown [this message]
2019-05-08 21:39     ` Curtis Malainey
2019-05-09  2:32       ` Mark Brown
2019-05-09 21:25         ` Curtis Malainey
2019-05-07 22:01 ` [PATCH v5 2/3] ASoC: rt5677: handle concurrent interrupts Fletcher Woodruff
2019-05-07 22:01 ` [PATCH v5 3/3] ASoC: rt5677: fall back to DT prop names on error Fletcher Woodruff
2019-05-08  9:04   ` Mark Brown
2019-06-05 22:24 ` [PATCH v6 0/4] Fix jack detection for Chromebook Pixel Fletcher Woodruff
2019-06-05 22:24   ` [PATCH v6 1/4] ASoC: rt5677: fall back to DT prop names on error Fletcher Woodruff
2019-06-05 22:24   ` [PATCH v6 2/4] ASoC: rt5677: move jack-detect init to i2c probe Fletcher Woodruff
2019-06-05 22:24   ` [PATCH v6 3/4] ASoC: rt5677: clear interrupts by polarity flip Fletcher Woodruff
2019-06-05 22:24   ` [PATCH v6 4/4] ASoC: rt5677: handle concurrent interrupts Fletcher Woodruff

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190508073623.GT14916@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=benzh@chromium.org \
    --cc=cujomalainey@chromium.org \
    --cc=fletcherw@chromium.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oder_chiou@realtek.com \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v5 1/3] ASoC: rt5677: allow multiple interrupt sources' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).