[PATCH 1/3] ASoC: rt5677: Include gpio driver header

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH 1/3] ASoC: rt5677: Include gpio driver header
@ 2014-10-03 21:37 Ben Zhang
  2014-10-03 21:37 ` [PATCH 2/3] ASoC: rt5677: Add ACPI device probing Ben Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ben Zhang @ 2014-10-03 21:37 UTC (permalink / raw)
  To: alsa-devel; +Cc: Bard Liao, Oder Chiou, Mark Brown, linux-kernel, Ben Zhang

The header file is needed because struct gpio_chip is
placed in struct rt5677_priv.

Signed-off-by: Ben Zhang <benzh@chromium.org>
---
 sound/soc/codecs/rt5677.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h
index d4eb6d5..99fd023 100644
--- a/sound/soc/codecs/rt5677.h
+++ b/sound/soc/codecs/rt5677.h
@@ -13,6 +13,7 @@
 #define __RT5677_H__
 
 #include <sound/rt5677.h>
+#include <linux/gpio/driver.h>
 
 /* Info */
 #define RT5677_RESET				0x00
-- 
2.1.0.rc2.206.gedb03e5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] ASoC: rt5677: Add ACPI device probing
  2014-10-03 21:37 [PATCH 1/3] ASoC: rt5677: Include gpio driver header Ben Zhang
@ 2014-10-03 21:37 ` Ben Zhang
  2014-10-07 19:12   ` Mark Brown
  2014-10-03 21:37 ` [PATCH 3/3] ASoC: rt5677: Add jack detection support Ben Zhang
  2014-10-07 19:36 ` [PATCH 1/3] ASoC: rt5677: Include gpio driver header Mark Brown
  2 siblings, 1 reply; 6+ messages in thread
From: Ben Zhang @ 2014-10-03 21:37 UTC (permalink / raw)
  To: alsa-devel; +Cc: Bard Liao, Oder Chiou, Mark Brown, linux-kernel, Ben Zhang

The rt5677 codec driver looks for ACPI device ID "RT5677CE",
which is specified in coreboot.

Signed-off-by: Ben Zhang <benzh@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/207622
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: AndyX C Huang <andyx.c.huang@intel.com>
---
 sound/soc/codecs/rt5677.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index cecbdb1..19dbb8a 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
+#include <linux/acpi.h>
 #include <linux/gpio.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -3680,10 +3681,21 @@ static int rt5677_i2c_remove(struct i2c_client *i2c)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rt5677_acpi_id[] = {
+	{ "RT5677CE", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, rt5677_acpi_id);
+#endif
+
 static struct i2c_driver rt5677_i2c_driver = {
 	.driver = {
 		.name = "rt5677",
 		.owner = THIS_MODULE,
+#ifdef CONFIG_ACPI
+		.acpi_match_table = ACPI_PTR(rt5677_acpi_id),
+#endif
 	},
 	.probe = rt5677_i2c_probe,
 	.remove   = rt5677_i2c_remove,
-- 
2.1.0.rc2.206.gedb03e5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] ASoC: rt5677: Add ACPI device probing
  2014-10-03 21:37 ` [PATCH 2/3] ASoC: rt5677: Add ACPI device probing Ben Zhang
@ 2014-10-07 19:12   ` Mark Brown
  0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2014-10-07 19:12 UTC (permalink / raw)
  To: Ben Zhang; +Cc: alsa-devel, Bard Liao, Oder Chiou, linux-kernel

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

On Fri, Oct 03, 2014 at 02:37:25PM -0700, Ben Zhang wrote:

> +#ifdef CONFIG_ACPI
> +		.acpi_match_table = ACPI_PTR(rt5677_acpi_id),
> +#endif

Why do you need this ifdef - surely the whole point of ACPI_PTR() is to
avoid needing it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] ASoC: rt5677: Add jack detection support
  2014-10-03 21:37 [PATCH 1/3] ASoC: rt5677: Include gpio driver header Ben Zhang
  2014-10-03 21:37 ` [PATCH 2/3] ASoC: rt5677: Add ACPI device probing Ben Zhang
@ 2014-10-03 21:37 ` Ben Zhang
  2014-10-07 19:35   ` Mark Brown
  2014-10-07 19:36 ` [PATCH 1/3] ASoC: rt5677: Include gpio driver header Mark Brown
  2 siblings, 1 reply; 6+ messages in thread
From: Ben Zhang @ 2014-10-03 21:37 UTC (permalink / raw)
  To: alsa-devel; +Cc: Bard Liao, Oder Chiou, Mark Brown, linux-kernel, Ben Zhang

This patch adds support for jack detection using GPIOs on the codec.
Plug detect signal and mic present signal can be routed from the physical
audio jack to GPIOs on the codec. The codec is configured so that upon
signal change, an IRQ(GPIO1) is fired. The codec interrupt handler reads
related status registers, figures out what has been plugged/unplugged from
the audio jack, and report jack states via snd_soc_jack_report().

ASoC machine driver should register audio jacks for detection with the
codec driver using rt5677_register_jack_detect().

Board setup code should assign GPIOs receiving plug detect/mic present
signal to the codec device itself, via Device Tree, ACPI or platform data.
The corresponding GPIO indexes are:
RT5677_GPIO_PLUG_DET - 0
RT5677_GPIO_MIC_PRESENT_L - 1

Signed-off-by: Ben Zhang <benzh@chromium.org>
---
 sound/soc/codecs/rt5677.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt5677.h |  92 ++++++++++++++++++++++++
 2 files changed, 271 insertions(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 19dbb8a..99f4d1a 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -29,6 +29,7 @@
 #include <sound/soc-dapm.h>
 #include <sound/initval.h>
 #include <sound/tlv.h>
+#include <sound/jack.h>
 
 #include "rl6231.h"
 #include "rt5677.h"
@@ -3341,6 +3342,25 @@ static void rt5677_free_gpio(struct i2c_client *i2c)
 
 	gpiochip_remove(&rt5677->gpio_chip);
 }
+
+static void rt5677_request_codec_gpios(struct rt5677_priv *rt5677,
+		struct i2c_client *i2c)
+{
+	struct gpio_desc *desc;
+
+	desc = devm_gpiod_get_index(&i2c->dev, "RT5677_GPIO",
+			RT5677_GPIO_PLUG_DET);
+	if (!IS_ERR(desc) && !gpiod_direction_input(desc))
+		rt5677->gpio_plug_det = desc_to_gpio(desc)
+				- rt5677->gpio_chip.base + 1;
+
+	desc = devm_gpiod_get_index(&i2c->dev, "RT5677_GPIO",
+			RT5677_GPIO_MIC_PRESENT_L);
+	if (!IS_ERR(desc) && !gpiod_direction_input(desc))
+		rt5677->gpio_mic_present_l = desc_to_gpio(desc)
+				- rt5677->gpio_chip.base + 1;
+}
+
 #else
 static void rt5677_init_gpio(struct i2c_client *i2c)
 {
@@ -3349,8 +3369,155 @@ static void rt5677_init_gpio(struct i2c_client *i2c)
 static void rt5677_free_gpio(struct i2c_client *i2c)
 {
 }
+static void rt5677_request_codec_gpios(struct rt5677_priv *rt5677,
+		struct i2c_client *i2c)
+{
+}
 #endif
 
+static void rt5677_report_jack_status(struct rt5677_priv *rt5677, int reg_irq)
+{
+	/* reg_irq: IRQ Control register MX-BDh */
+	int polarity;
+	if (reg_irq & RT5677_JD2_STATUS_MASK) {
+		polarity = reg_irq & RT5677_JD2_POLARITY_MASK;
+		if (rt5677->headphone_jack) {
+			snd_soc_jack_report(rt5677->headphone_jack,
+				polarity ? SND_JACK_HEADPHONE : 0,
+				SND_JACK_HEADPHONE);
+		}
+	}
+
+	if (reg_irq & RT5677_JD3_STATUS_MASK) {
+		polarity = reg_irq & RT5677_JD3_POLARITY_MASK;
+		if (rt5677->mic_jack) {
+			snd_soc_jack_report(rt5677->mic_jack,
+				polarity ? 0 : SND_JACK_MICROPHONE,
+				SND_JACK_MICROPHONE);
+		}
+	}
+}
+
+static irqreturn_t rt5677_irq(int unused, void *data)
+{
+	struct rt5677_priv *rt5677 = data;
+	int ret = 0, i;
+	bool irq_fired;
+	int reg_irq;
+
+	/*
+	 * Loop to handle interrupts until the last i2c read shows no pending
+	 * irqs with a safeguard of 20 loops
+	 */
+	for (i = 0; i < 20; i++) {
+		/* Read interrupt status */
+		ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, &reg_irq);
+		if (ret)
+			break;
+
+		/* Flip polarity for interrupts that just fired  */
+		irq_fired = false;
+		if (reg_irq & RT5677_JD2_STATUS_MASK) {
+			reg_irq ^= RT5677_JD2_POLARITY_MASK;
+			irq_fired = true;
+		}
+
+		if (reg_irq & RT5677_JD3_STATUS_MASK) {
+			reg_irq ^= RT5677_JD3_POLARITY_MASK;
+			irq_fired = true;
+		}
+
+		if (!irq_fired)
+			break;
+
+		/* Clear interrupts */
+		ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq);
+		if (ret)
+			break;
+
+		/* Process interrupts */
+		rt5677_report_jack_status(rt5677, reg_irq);
+	}
+	return IRQ_HANDLED;
+}
+
+int rt5677_register_jack_detect(struct snd_soc_codec *codec,
+	struct snd_soc_jack *headphone_jack, struct snd_soc_jack *mic_jack)
+{
+	struct rt5677_priv *rt5677 = snd_soc_codec_get_drvdata(codec);
+
+	rt5677->headphone_jack = headphone_jack;
+	rt5677->mic_jack = mic_jack;
+
+	/* Report initial jack status */
+	rt5677_irq(0, rt5677);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rt5677_register_jack_detect);
+
+static void rt5677_setup_jack_detect(struct snd_soc_codec *codec,
+		struct rt5677_priv *rt5677)
+{
+	unsigned int val;
+
+	/* Skip jack detect setup if there are no assigned GPIOs */
+	if (!rt5677->gpio_plug_det && !rt5677->gpio_mic_present_l)
+		return;
+	/* GPIO range check */
+	if (rt5677->gpio_plug_det < 4 || rt5677->gpio_plug_det > 6) {
+		dev_err(codec->dev, "PLUG_DET can only use GPIO 4~6, given %d\n",
+				rt5677->gpio_plug_det);
+	}
+	if (rt5677->gpio_mic_present_l < 4 || rt5677->gpio_mic_present_l > 6) {
+		dev_err(codec->dev, "MIC_PRESENT_L can only use GPIO 4~6, given %d\n",
+				rt5677->gpio_mic_present_l);
+	}
+
+	/*
+	 * 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 */
+	val = RT5677_AUTO_RC_ON_GPIO_CHANGE_MASK;
+	if (rt5677->gpio_plug_det)
+		val |= 1 << (rt5677->gpio_plug_det - 1);
+	if (rt5677->gpio_mic_present_l)
+		val |= 1 << (rt5677->gpio_mic_present_l - 1);
+	regmap_update_bits(rt5677->regmap, RT5677_GEN_CTRL1, val, val);
+
+	/*
+	 * Select jack detection source
+	 * JD2: PLUG_DET
+	 * JD3: !MIC_PRESENT_L
+	 */
+	val = 0;
+	if (rt5677->gpio_plug_det)
+		val |= (rt5677->gpio_plug_det - 3)
+			<< RT5677_JD2_SRC_SEL_SFT;
+	if (rt5677->gpio_mic_present_l)
+		val |= (rt5677->gpio_mic_present_l - 3)
+			<< RT5677_JD3_SRC_SEL_SFT;
+	regmap_update_bits(rt5677->regmap, RT5677_JD_CTRL1,
+			RT5677_JD2_SRC_SEL_MASK | RT5677_JD3_SRC_SEL_MASK,
+			val);
+
+	/* Enable JD2 and/or JD3 as IRQ source */
+	val = 0;
+	if (rt5677->gpio_plug_det)
+		val |= RT5677_JD2_IRQ_EN_MASK;
+	if (rt5677->gpio_mic_present_l)
+		val |= RT5677_JD3_IRQ_EN_MASK | RT5677_JD3_POLARITY_INV;
+	regmap_update_bits(rt5677->regmap, RT5677_IRQ_CTRL1, val, val);
+
+	/* Set GPIO1 to be IRQ */
+	regmap_update_bits(rt5677->regmap, RT5677_GPIO_CTRL1,
+			RT5677_GPIO1_PIN_MASK, RT5677_GPIO1_PIN_IRQ);
+}
+
 static int rt5677_probe(struct snd_soc_codec *codec)
 {
 	struct rt5677_priv *rt5677 = snd_soc_codec_get_drvdata(codec);
@@ -3372,6 +3539,7 @@ static int rt5677_probe(struct snd_soc_codec *codec)
 	regmap_write(rt5677->regmap, RT5677_DIG_MISC, 0x0020);
 	regmap_write(rt5677->regmap, RT5677_PWR_DSP2, 0x0c00);
 
+	rt5677_setup_jack_detect(codec, rt5677);
 	return 0;
 }
 
@@ -3668,6 +3836,17 @@ static int rt5677_i2c_probe(struct i2c_client *i2c,
 	}
 
 	rt5677_init_gpio(i2c);
+	rt5677_request_codec_gpios(rt5677, i2c);
+
+	if (i2c->irq) {
+		ret = request_threaded_irq(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 snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5677,
 				      rt5677_dai, ARRAY_SIZE(rt5677_dai));
diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h
index 99fd023..2ec0844 100644
--- a/sound/soc/codecs/rt5677.h
+++ b/sound/soc/codecs/rt5677.h
@@ -1368,6 +1368,47 @@
 #define RT5677_SEL_SRC_IB01			(0x1 << 0)
 #define RT5677_SEL_SRC_IB01_SFT			0
 
+/* Jack Detection Control 1 (0xb5) */
+#define RT5677_JD1_SRC_SEL_MASK			(0x3 << 14)
+#define RT5677_JD1_SRC_GPIO1			(0x1 << 14)
+#define RT5677_JD1_SRC_GPIO2			(0x2 << 14)
+#define RT5677_JD1_SRC_GPIO3			(0x3 << 14)
+#define RT5677_JD2_SRC_SEL_MASK			(0x3 << 12)
+#define RT5677_JD2_SRC_SEL_SFT			12
+#define RT5677_JD2_SRC_GPIO4			(0x1 << 12)
+#define RT5677_JD2_SRC_GPIO5			(0x2 << 12)
+#define RT5677_JD2_SRC_GPIO6			(0x3 << 12)
+#define RT5677_JD3_SRC_SEL_MASK			(0x3 << 10)
+#define RT5677_JD3_SRC_SEL_SFT			10
+#define RT5677_JD3_SRC_GPIO4			(0x1 << 10)
+#define RT5677_JD3_SRC_GPIO5			(0x2 << 10)
+#define RT5677_JD3_SRC_GPIO6			(0x3 << 10)
+
+/* IRQ Control 1 (0xbd) */
+#define RT5677_JD1_STATUS_MASK			(0x1 << 15)
+#define RT5677_JD1_STATUS_SFT			15
+#define RT5677_JD1_IRQ_EN_MASK			(0x1 << 14)
+#define RT5677_JD1_IRQ_EN_SFT			14
+#define RT5677_JD1_POLARITY_MASK		(0x1 << 12)
+#define RT5677_JD1_POLARITY_NOR			(0x0 << 12)
+#define RT5677_JD1_POLARITY_INV			(0x1 << 12)
+
+#define RT5677_JD2_STATUS_MASK			(0x1 << 11)
+#define RT5677_JD2_STATUS_SFT			11
+#define RT5677_JD2_IRQ_EN_MASK			(0x1 << 10)
+#define RT5677_JD2_IRQ_EN_SFT			10
+#define RT5677_JD2_POLARITY_MASK		(0x1 << 8)
+#define RT5677_JD2_POLARITY_NOR			(0x0 << 8)
+#define RT5677_JD2_POLARITY_INV			(0x1 << 8)
+
+#define RT5677_JD3_STATUS_MASK			(0x1 << 3)
+#define RT5677_JD3_STATUS_SFT			3
+#define RT5677_JD3_IRQ_EN_MASK			(0x1 << 2)
+#define RT5677_JD3_IRQ_EN_SFT			2
+#define RT5677_JD3_POLARITY_MASK		(0x1 << 0)
+#define RT5677_JD3_POLARITY_NOR			(0x0 << 0)
+#define RT5677_JD3_POLARITY_INV			(0x1 << 0)
+
 /* GPIO status (0xbf) */
 #define RT5677_GPIO6_STATUS_MASK		(0x1 << 5)
 #define RT5677_GPIO6_STATUS_SFT			5
@@ -1472,6 +1513,28 @@
 #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)
+
+/* General Control (0xfb) */
+#define RT5677_AUTO_RC_ON_GPIO_CHANGE_MASK	(0x1 << 7)
+#define RT5677_AUTO_RC_ON_GPIO_CHANGE_SFT	7
+#define RT5677_AUTO_RC_ON_GPIO6_MASK		(0x1 << 5)
+#define RT5677_AUTO_RC_ON_GPIO6_SFT		5
+#define RT5677_AUTO_RC_ON_GPIO5_MASK		(0x1 << 4)
+#define RT5677_AUTO_RC_ON_GPIO5_SFT		4
+#define RT5677_AUTO_RC_ON_GPIO4_MASK		(0x1 << 3)
+#define RT5677_AUTO_RC_ON_GPIO4_SFT		3
+#define RT5677_AUTO_RC_ON_GPIO3_MASK		(0x1 << 2)
+#define RT5677_AUTO_RC_ON_GPIO3_SFT		2
+#define RT5677_AUTO_RC_ON_GPIO2_MASK		(0x1 << 1)
+#define RT5677_AUTO_RC_ON_GPIO2_SFT		1
+#define RT5677_AUTO_RC_ON_GPIO1_MASK		(0x1 << 0)
+#define RT5677_AUTO_RC_ON_GPIO1_SFT		0
+
 /* Virtual DSP Mixer Control (0xf7 0xf8 0xf9) */
 #define RT5677_DSP_IB_01_H			(0x1 << 15)
 #define RT5677_DSP_IB_01_H_SFT			15
@@ -1542,10 +1605,20 @@ enum {
 	RT5677_GPIO_NUM,
 };
 
+enum {
+	RT5677_GPIO_PLUG_DET,
+	RT5677_GPIO_MIC_PRESENT_L,
+	RT5677_GPIO_HOTWORD_DET_L,
+	RT5677_GPIO_DSP_INT,
+	RT5677_GPIO_HP_AMP_SHDN_L,
+};
+
 struct rt5677_priv {
 	struct snd_soc_codec *codec;
 	struct rt5677_platform_data pdata;
 	struct regmap *regmap;
+	struct snd_soc_jack *headphone_jack;
+	struct snd_soc_jack *mic_jack;
 
 	int sysclk;
 	int sysclk_src;
@@ -1559,6 +1632,25 @@ struct rt5677_priv {
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip gpio_chip;
 #endif
+	/*
+	 * Predefined common codec GPIO tasks. Codec GPIO number is
+	 * 1-based, 0 means no GPIO is assigned to the corresponding task.
+	 *
+	 * gpio_plug_det: A codec GPIO used for headphone jack detect
+	 * Low - headphone is not present
+	 * High - headphone is plugged in
+	 * Supported GPIO: 4,5,6
+	 *
+	 * gpio_mic_present_l: A codec GPIO used for mic jack detect
+	 * Low - mic is plugged in
+	 * High - mic is not present
+	 * Supported GPIO: 4,5,6
+	 */
+	int gpio_plug_det;
+	int gpio_mic_present_l;
 };
 
+int rt5677_register_jack_detect(struct snd_soc_codec *codec,
+	struct snd_soc_jack *headphone_jack, struct snd_soc_jack *mic_jack);
+
 #endif /* __RT5677_H__ */
-- 
2.1.0.rc2.206.gedb03e5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] ASoC: rt5677: Add jack detection support
  2014-10-03 21:37 ` [PATCH 3/3] ASoC: rt5677: Add jack detection support Ben Zhang
@ 2014-10-07 19:35   ` Mark Brown
  0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2014-10-07 19:35 UTC (permalink / raw)
  To: Ben Zhang; +Cc: alsa-devel, Bard Liao, Oder Chiou, linux-kernel

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

On Fri, Oct 03, 2014 at 02:37:26PM -0700, Ben Zhang wrote:

> This patch adds support for jack detection using GPIOs on the codec.
> Plug detect signal and mic present signal can be routed from the physical
> audio jack to GPIOs on the codec. The codec is configured so that upon
> signal change, an IRQ(GPIO1) is fired. The codec interrupt handler reads

This then assumes that GPIO1 was configured as the IRQ - what happens if
it was connected with some other function on the board and some other
GPIO was assigned as IRQ?

> +	desc = devm_gpiod_get_index(&i2c->dev, "RT5677_GPIO",
> +			RT5677_GPIO_PLUG_DET);
> +	if (!IS_ERR(desc) && !gpiod_direction_input(desc))
> +		rt5677->gpio_plug_det = desc_to_gpio(desc)
> +				- rt5677->gpio_chip.base + 1;

I'm having trouble reading this, I think other reasonable users would
too.  It's not entirely clear to me what it's supposed to be doing or
why we are mixing the descriptor and number based GPIO APIs.

> +       /*
> +        * Loop to handle interrupts until the last i2c read shows no pending
> +        * irqs with a safeguard of 20 loops
> +        */
> +       for (i = 0; i < 20; i++) {

Eh?  Why are we doing this and why are we so sure that 20 is a good
value?

>  static int rt5677_probe(struct snd_soc_codec *codec)
>  {

> +       rt5677_setup_jack_detect(codec, rt5677);
>         return 0;

This sets up GPIO1 as an interrupt without verifying that we have an
interrupt assigned for it successfully and only in the ASoC level probe,
not in the chip level probe.  This means we might not have an interrupt
at all if the board configuration was buggy and means that we'll start
paying attention to the interrupt (which is requested in the i2c level
probe as it should be) without configuring the chip to be in the
appropriate state which means that it might start falsely flagging
interrupts.  I'd expect the configuration to be closer together, and
to have the CODEC set up ready to generate interrupts prior to starting
to listen to them.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] ASoC: rt5677: Include gpio driver header
  2014-10-03 21:37 [PATCH 1/3] ASoC: rt5677: Include gpio driver header Ben Zhang
  2014-10-03 21:37 ` [PATCH 2/3] ASoC: rt5677: Add ACPI device probing Ben Zhang
  2014-10-03 21:37 ` [PATCH 3/3] ASoC: rt5677: Add jack detection support Ben Zhang
@ 2014-10-07 19:36 ` Mark Brown
  2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2014-10-07 19:36 UTC (permalink / raw)
  To: Ben Zhang; +Cc: alsa-devel, Bard Liao, Oder Chiou, linux-kernel

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

On Fri, Oct 03, 2014 at 02:37:24PM -0700, Ben Zhang wrote:
> The header file is needed because struct gpio_chip is
> placed in struct rt5677_priv.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-10-07 19:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-03 21:37 [PATCH 1/3] ASoC: rt5677: Include gpio driver header Ben Zhang
2014-10-03 21:37 ` [PATCH 2/3] ASoC: rt5677: Add ACPI device probing Ben Zhang
2014-10-07 19:12   ` Mark Brown
2014-10-03 21:37 ` [PATCH 3/3] ASoC: rt5677: Add jack detection support Ben Zhang
2014-10-07 19:35   ` Mark Brown
2014-10-07 19:36 ` [PATCH 1/3] ASoC: rt5677: Include gpio driver header Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®