[RFC 0/5] Add a gpio jack device

[RFC 0/5] Add a gpio jack device

[Dylan Reid]

The first three changes add a gpio audio jack device.  This device can
be used on systems that report headphone or mic plug through GPIOS.
There can be 0-N of these devices created per board each can report one
of several events.  For example, this allows for a single jack for
HP/Mic and a separate jack for line out.

The last two patches are an example of the jack being used on Acer's
Tegra Chromebook, those can be considered separately and might not be
worth the churn.

Dylan Reid (5):
  ALSA: Add jack types to dt-bindings
  ASoC: jack - make add_gpiods accept pre-filled descriptors
  ASoC: Add GPIO based jack device
  ASoC: tegra_max98090: Change nyan to use gpio-jack
  ARM: tegra: nyan: specify gpio-audio-jack device

 .../devicetree/bindings/sound/gpio-audio-jack.txt  |  39 +++++
 arch/arm/boot/dts/tegra124-nyan.dtsi               |  22 ++-
 include/dt-bindings/sound/audio-jack-events.h      |   9 +
 sound/soc/codecs/Kconfig                           |   4 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/gpio-audio-jack.c                 | 191 +++++++++++++++++++++
 sound/soc/soc-jack.c                               |   9 +-
 sound/soc/tegra/tegra_max98090.c                   | 107 +++---------
 8 files changed, 291 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/gpio-audio-jack.txt
 create mode 100644 include/dt-bindings/sound/audio-jack-events.h
 create mode 100644 sound/soc/codecs/gpio-audio-jack.c

-- 
2.2.1.62.g3f15098

[RFC 1/5] ALSA: Add jack types to dt-bindings

[Dylan Reid]

Adding the jack type to the dt-bindings directory will allow for device
tree files to specify the type of audio jacks that are present for a
board.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 include/dt-bindings/sound/audio-jack-events.h | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 include/dt-bindings/sound/audio-jack-events.h

diff --git a/include/dt-bindings/sound/audio-jack-events.h b/include/dt-bindings/sound/audio-jack-events.h
new file mode 100644
index 0000000..378349f
--- /dev/null
+++ b/include/dt-bindings/sound/audio-jack-events.h
@@ -0,0 +1,9 @@
+#ifndef __AUDIO_JACK_EVENTS_H
+#define __AUDIO_JACK_EVENTS_H
+
+#define JACK_HEADPHONE		1
+#define JACK_MICROPHONE		2
+#define JACK_LINEOUT		3
+#define JACK_LINEIN		4
+
+#endif /* __AUDIO_JACK_EVENTS_H */
-- 
2.2.1.62.g3f15098

Re: [RFC 1/5] ALSA: Add jack types to dt-bindings

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 232 bytes --]

On Fri, May 22, 2015 at 03:09:19PM -0700, Dylan Reid wrote:
> Adding the jack type to the dt-bindings directory will allow for device
> tree files to specify the type of audio jacks that are present for a
> board.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[RFC 2/5] ASoC: jack - add_gpiods accepts filled descriptors

[Dylan Reid]

Allow for the desc field to be pre-filled when adding gpios to a jack.
This allows drivers to get the gpios and decide if they should be added
to the list or not.  Specifically this will allow the gpio jack driver
to add gpios based on device property specifications.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 sound/soc/soc-jack.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c
index 9f60c25..171c429 100644
--- a/sound/soc/soc-jack.c
+++ b/sound/soc/soc-jack.c
@@ -315,8 +315,11 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
 			goto undo;
 		}
 
-		if (gpios[i].gpiod_dev) {
-			/* GPIO descriptor */
+		if (gpios[i].desc) {
+			/* Already have a GPIO descriptor. */
+			goto got_gpio;
+		} else if (gpios[i].gpiod_dev) {
+			/* Get a GPIO descriptor */
 			gpios[i].desc = gpiod_get_index(gpios[i].gpiod_dev,
 							gpios[i].name,
 							gpios[i].idx, GPIOD_IN);
@@ -344,7 +347,7 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
 
 			gpios[i].desc = gpio_to_desc(gpios[i].gpio);
 		}
-
+got_gpio:
 		INIT_DELAYED_WORK(&gpios[i].work, gpio_work);
 		gpios[i].jack = jack;
 
-- 
2.2.1.62.g3f15098

Re: [RFC 2/5] ASoC: jack - add_gpiods accepts filled descriptors

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 354 bytes --]

On Fri, May 22, 2015 at 03:09:20PM -0700, Dylan Reid wrote:
> Allow for the desc field to be pre-filled when adding gpios to a jack.
> This allows drivers to get the gpios and decide if they should be added
> to the list or not.  Specifically this will allow the gpio jack driver
> to add gpios based on device property specifications.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[RFC 3/5] ASoC: Add GPIO based jack device

[Dylan Reid]

Add a jack device that allows for separate headphone and mic detect
GPIOs.  This device will be used as an aux device and will registered
the jack with the card at init time.

The gpios are each optional allowing for a headphone, microphone, or
combination devices to be created depending on the board configuration.
A board will be able to have several of these jacks, one for each
physical connection.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 .../devicetree/bindings/sound/gpio-audio-jack.txt  |  39 +++++
 sound/soc/codecs/Kconfig                           |   4 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/gpio-audio-jack.c                 | 191 +++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/gpio-audio-jack.txt
 create mode 100644 sound/soc/codecs/gpio-audio-jack.c

diff --git a/Documentation/devicetree/bindings/sound/gpio-audio-jack.txt b/Documentation/devicetree/bindings/sound/gpio-audio-jack.txt
new file mode 100644
index 0000000..17a4501
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/gpio-audio-jack.txt
@@ -0,0 +1,39 @@
+Gpio-Audio-Jack:
+
+GPIO based audio jacks.  This can represent a headphone, mic, or combo jack.
+
+Required properties:
+
+- compatible				: "gpio-audio-jack"
+
+Optional properties:
+
+- gpio-audio-jack,jack-name		: Name of the jack. Normally
+					  'Headphone Jack', 'Mic Jack', or
+					  'Headset Jack'
+- gpio-audio-jack,sw-det-gpios		: Reference to GPIOs that signals when
+					  a jack is plugged.
+- gpio-audio-jack,gpio-names		: Names of the GPIOs. Must provide one
+					  per sw-det-gpio entry.
+- gpio-audio-jack,report-masks		: Jack report mask from
+					  dt-bindings/sound/audio-jack-events.h.
+					  Must have one per sw-det-gpio entry.
+- gpio-audio-jack,debounce-times	: Debounce time for each sw-det-gpio
+					  entry.
+
+Example of Headphone/Mic combo jack:
+
+audio_jack: gpio-audio-jack {
+	compatible = "gpio-audio-jack";
+	gpio-audio-jack,jack-name = "Headset Jack";
+	gpio-audio-jack,sw-det-gpios =
+			<&gpio TEGRA_GPIO(I, 7) GPIO_ACTIVE_HIGH>,
+			<&gpio TEGRA_GPIO(R, 7) GPIO_ACTIVE_LOW>;
+	gpio-audio-jack,gpio-names = "Headphones", "Mic Jack";
+	gpio-audio-jack,report-masks = <JACK_HEADPHONE>, <JACK_MICROPHONE>;
+	gpio-audio-jack,debounce-times = <150>, <150>;
+};
+
+sound {
+	nvidia,headset-dev = <&audio_jack>;
+	...
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 061c465..c9e96a7 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -62,6 +62,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_BT_SCO
 	select SND_SOC_ES8328_SPI if SPI_MASTER
 	select SND_SOC_ES8328_I2C if I2C
+	select SND_SOC_GPIO_AUDIO_JACK
 	select SND_SOC_ISABELLE if I2C
 	select SND_SOC_JZ4740_CODEC
 	select SND_SOC_LM4857 if I2C
@@ -444,6 +445,9 @@ config SND_SOC_ES8328_SPI
 	tristate
 	select SND_SOC_ES8328
 
+config SND_SOC_GPIO_AUDIO_JACK
+        tristate "GPIO based audio jack detection"
+
 config SND_SOC_ISABELLE
         tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index abe2d7e..ae570f5 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -55,6 +55,7 @@ snd-soc-dmic-objs := dmic.o
 snd-soc-es8328-objs := es8328.o
 snd-soc-es8328-i2c-objs := es8328-i2c.o
 snd-soc-es8328-spi-objs := es8328-spi.o
+snd-soc-gpio-audio-jack-objs := gpio-audio-jack.o
 snd-soc-isabelle-objs := isabelle.o
 snd-soc-jz4740-codec-objs := jz4740.o
 snd-soc-l3-objs := l3.o
@@ -240,6 +241,7 @@ obj-$(CONFIG_SND_SOC_DMIC)	+= snd-soc-dmic.o
 obj-$(CONFIG_SND_SOC_ES8328)	+= snd-soc-es8328.o
 obj-$(CONFIG_SND_SOC_ES8328_I2C)+= snd-soc-es8328-i2c.o
 obj-$(CONFIG_SND_SOC_ES8328_SPI)+= snd-soc-es8328-spi.o
+obj-$(CONFIG_SND_SOC_GPIO_AUDIO_JACK)	+= snd-soc-gpio-audio-jack.o
 obj-$(CONFIG_SND_SOC_ISABELLE)	+= snd-soc-isabelle.o
 obj-$(CONFIG_SND_SOC_JZ4740_CODEC)	+= snd-soc-jz4740-codec.o
 obj-$(CONFIG_SND_SOC_L3)	+= snd-soc-l3.o
diff --git a/sound/soc/codecs/gpio-audio-jack.c b/sound/soc/codecs/gpio-audio-jack.c
new file mode 100644
index 0000000..61662cf
--- /dev/null
+++ b/sound/soc/codecs/gpio-audio-jack.c
@@ -0,0 +1,191 @@
+/*
+ * GPIO based audio jack detection
+ *
+ * Copyright (C) 2015 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <dt-bindings/sound/audio-jack-events.h>
+
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/soc.h>
+
+#define JACK_NAME_CON_ID "gpio-audio-jack,jack-name"
+#define GPIO_CON_ID "gpio-audio-jack,sw-det"
+#define NAME_CON_ID "gpio-audio-jack,gpio-names"
+#define REPORT_MASK_CON_ID "gpio-audio-jack,report-masks"
+#define DEBOUNCE_CON_ID "gpio-audio-jack,debounce-times"
+
+struct gpio_audio_jack {
+	struct snd_soc_component_driver component_drv;
+	struct snd_soc_jack jack;
+	struct snd_soc_jack_gpio *gpios;
+	int gpio_count;
+};
+
+static int dt_jack_to_alsa_report(int dt_jack)
+{
+	switch (dt_jack) {
+	case JACK_HEADPHONE:
+		return SND_JACK_HEADPHONE;
+	case JACK_MICROPHONE:
+		return SND_JACK_MICROPHONE;
+	case JACK_LINEOUT:
+		return SND_JACK_LINEOUT;
+	case JACK_LINEIN:
+		return SND_JACK_LINEIN;
+	default:
+		return 0;
+	}
+}
+
+static int gpio_audio_component_probe(struct snd_soc_component *component)
+{
+	struct device *dev = component->dev;
+	struct gpio_audio_jack *priv = container_of(component->driver,
+			struct gpio_audio_jack, component_drv);
+	const char *jack_name;
+	const char **gpio_names;
+	u32 *debounce;
+	u32 *gpio_report;
+	int report_mask = 0;
+	int ret;
+	int i;
+
+	ret = device_property_read_string(dev, JACK_NAME_CON_ID, &jack_name);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read jack name %d\n", ret);
+		return ret;
+	}
+
+	priv->gpio_count = gpiod_count(dev, GPIO_CON_ID);
+	if (priv->gpio_count <= 0)
+		return priv->gpio_count;
+
+	gpio_names = devm_kzalloc(dev, sizeof(*gpio_names) * priv->gpio_count,
+				  GFP_KERNEL);
+	if (!gpio_names)
+		return -ENOMEM;
+	gpio_report = devm_kzalloc(dev, sizeof(*gpio_report) * priv->gpio_count,
+				   GFP_KERNEL);
+	if (!gpio_report)
+		return -ENOMEM;
+	debounce = devm_kzalloc(dev, sizeof(*debounce) * priv->gpio_count,
+				GFP_KERNEL);
+	if (!debounce)
+		return -ENOMEM;
+	priv->gpios = devm_kzalloc(dev,
+			sizeof(*priv->gpios) * priv->gpio_count, GFP_KERNEL);
+	if (!priv->gpios)
+		return -ENOMEM;
+
+	ret = device_property_read_string_array(dev, NAME_CON_ID, gpio_names,
+						priv->gpio_count);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read gpio names %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32_array(dev, REPORT_MASK_CON_ID,
+					     gpio_report, priv->gpio_count);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read gpio masks %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32_array(dev, DEBOUNCE_CON_ID,
+					     debounce, priv->gpio_count);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read gpio debounce times %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < priv->gpio_count; i++) {
+		priv->gpios[i].desc = gpiod_get_index(dev, GPIO_CON_ID,
+						      i, GPIOD_IN);
+		if (IS_ERR(priv->gpios[i].desc)) {
+			ret = PTR_ERR(priv->gpios[i].desc);
+			dev_err(priv->gpios[i].gpiod_dev,
+				"Cannot get jack gpio at index %d: %d",
+				i, ret);
+			goto gpio_err;
+		}
+
+		priv->gpios[i].name = gpio_names[i];
+		priv->gpios[i].report = dt_jack_to_alsa_report(gpio_report[i]);
+		priv->gpios[i].debounce_time = debounce[i];
+		report_mask |= gpio_report[i];
+	}
+
+	snd_soc_card_jack_new(component->card, jack_name, report_mask,
+			      &priv->jack, NULL, 0);
+
+	devm_kfree(dev, gpio_names);
+	devm_kfree(dev, gpio_report);
+	devm_kfree(dev, debounce);
+
+	return snd_soc_jack_add_gpiods(dev, &priv->jack,
+				       priv->gpio_count, priv->gpios);
+
+gpio_err:
+	for (i = 0; i < priv->gpio_count; i++) {
+		if (!IS_ERR(priv->gpios[i].desc))
+			gpiod_put(priv->gpios[i].desc);
+	}
+	return ret;
+}
+
+static void gpio_audio_component_remove(struct snd_soc_component *component)
+{
+	struct gpio_audio_jack *priv = container_of(component->driver,
+			struct gpio_audio_jack, component_drv);
+	int i;
+
+	for (i = 0; i < priv->gpio_count; i++) {
+		if (!IS_ERR(priv->gpios[i].desc))
+			gpiod_put(priv->gpios[i].desc);
+	}
+}
+
+static int gpio_audio_jack_probe(struct platform_device *pdev)
+{
+	struct gpio_audio_jack *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, priv);
+
+	priv->component_drv.probe = gpio_audio_component_probe;
+	priv->component_drv.remove = gpio_audio_component_remove;
+
+	return devm_snd_soc_register_component(&pdev->dev, &priv->component_drv,
+					       NULL, 0);
+}
+
+static const struct of_device_id gpio_audio_of_match[] = {
+	{ .compatible = "gpio-audio-jack", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpio_audio_of_match);
+
+static struct platform_driver gpio_audio_jack = {
+	.driver = {
+		.name = "gpio-audio-jack",
+		.of_match_table = gpio_audio_of_match,
+	},
+	.probe = gpio_audio_jack_probe,
+};
+module_platform_driver(gpio_audio_jack);
+
+MODULE_DESCRIPTION("ASoC GPIO audio jack driver");
+MODULE_AUTHOR("Dylan Reid <dgreid@chromium.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.2.1.62.g3f15098

Re: [RFC 3/5] ASoC: Add GPIO based jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]

On Fri, May 22, 2015 at 03:09:21PM -0700, Dylan Reid wrote:

> Add a jack device that allows for separate headphone and mic detect
> GPIOs.  This device will be used as an aux device and will registered
> the jack with the card at init time.

This looks basically fine, a couple of things below but they're
nitpicks.

> +- gpio-audio-jack,debounce-times	: Debounce time for each sw-det-gpio
> +					  entry.

specified in...?

> +config SND_SOC_GPIO_AUDIO_JACK
> +        tristate "GPIO based audio jack detection"
> +

This should surely depend on GPIOLIB || COMPILE_TEST.

> +	gpio_names = devm_kzalloc(dev, sizeof(*gpio_names) * priv->gpio_count,
> +				  GFP_KERNEL);

We have devm_kcalloc() (not that it makes a huge difference but may as
well be clear about the intent).

> +	if (!gpio_names)
> +		return -ENOMEM;
> +	gpio_report = devm_kzalloc(dev, sizeof(*gpio_report) * priv->gpio_count,
> +				   GFP_KERNEL);

Blank lines between blocks please.

> +	snd_soc_card_jack_new(component->card, jack_name, report_mask,
> +			      &priv->jack, NULL, 0);

Ignoring the return value here (not likely to fail but...).

> +static void gpio_audio_component_remove(struct snd_soc_component *component)
> +{
> +	struct gpio_audio_jack *priv = container_of(component->driver,
> +			struct gpio_audio_jack, component_drv);
> +	int i;
> +
> +	for (i = 0; i < priv->gpio_count; i++) {
> +		if (!IS_ERR(priv->gpios[i].desc))
> +			gpiod_put(priv->gpios[i].desc);
> +	}

I would have expected us to be acquiring and releasing the GPIOs in the
platform device probe, not in the ASoC level probe - that way we handle
deferred probe better.

> +static int gpio_audio_jack_probe(struct platform_device *pdev)
> +{
> +	struct gpio_audio_jack *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, priv);
> +
> +	priv->component_drv.probe = gpio_audio_component_probe;
> +	priv->component_drv.remove = gpio_audio_component_remove;
> +
> +	return devm_snd_soc_register_component(&pdev->dev, &priv->component_drv,
> +					       NULL, 0);

Why do we allocate a component driver structure per device?

> +static const struct of_device_id gpio_audio_of_match[] = {
> +	{ .compatible = "gpio-audio-jack", },

linux,gpio-audio-jack possibly.  Not entirely sure what the current best
practice is on that.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 3/5] ASoC: Add GPIO based jack device

[Dylan Reid]

On Mon, May 25, 2015 at 5:11 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, May 22, 2015 at 03:09:21PM -0700, Dylan Reid wrote:
>
>> Add a jack device that allows for separate headphone and mic detect
>> GPIOs.  This device will be used as an aux device and will registered
>> the jack with the card at init time.
>
> This looks basically fine, a couple of things below but they're
> nitpicks.

Thanks for taking a look Mark, and thanks for catching those things.
I'll send an updated version of just this patch on top of your
topic/gpio-jack branch tomorrow morning pacific time.

>
>> +- gpio-audio-jack,debounce-times     : Debounce time for each sw-det-gpio
>> +                                       entry.
>
> specified in...?

That is important info, added.

>
>> +config SND_SOC_GPIO_AUDIO_JACK
>> +        tristate "GPIO based audio jack detection"
>> +
>
> This should surely depend on GPIOLIB || COMPILE_TEST.

indeed, added.

>
>> +     gpio_names = devm_kzalloc(dev, sizeof(*gpio_names) * priv->gpio_count,
>> +                               GFP_KERNEL);
>
> We have devm_kcalloc() (not that it makes a huge difference but may as
> well be clear about the intent).

Good call, done.

>
>> +     if (!gpio_names)
>> +             return -ENOMEM;
>> +     gpio_report = devm_kzalloc(dev, sizeof(*gpio_report) * priv->gpio_count,
>> +                                GFP_KERNEL);
>
> Blank lines between blocks please.

Added.

>
>> +     snd_soc_card_jack_new(component->card, jack_name, report_mask,
>> +                           &priv->jack, NULL, 0);
>
> Ignoring the return value here (not likely to fail but...).

handled.

>
>> +static void gpio_audio_component_remove(struct snd_soc_component *component)
>> +{
>> +     struct gpio_audio_jack *priv = container_of(component->driver,
>> +                     struct gpio_audio_jack, component_drv);
>> +     int i;
>> +
>> +     for (i = 0; i < priv->gpio_count; i++) {
>> +             if (!IS_ERR(priv->gpios[i].desc))
>> +                     gpiod_put(priv->gpios[i].desc);
>> +     }
>
> I would have expected us to be acquiring and releasing the GPIOs in the
> platform device probe, not in the ASoC level probe - that way we handle
> deferred probe better.

That does work better, I moved all the gpio get/put to the device probe. Thanks.

>
>> +static int gpio_audio_jack_probe(struct platform_device *pdev)
>> +{
>> +     struct gpio_audio_jack *priv;
>> +
>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     dev_set_drvdata(&pdev->dev, priv);
>> +
>> +     priv->component_drv.probe = gpio_audio_component_probe;
>> +     priv->component_drv.remove = gpio_audio_component_remove;
>> +
>> +     return devm_snd_soc_register_component(&pdev->dev, &priv->component_drv,
>> +                                            NULL, 0);
>
> Why do we allocate a component driver structure per device?

I'm not sure how that happened.  I'll fix it =)

>
>> +static const struct of_device_id gpio_audio_of_match[] = {
>> +     { .compatible = "gpio-audio-jack", },
>
> linux,gpio-audio-jack possibly.  Not entirely sure what the current best
> practice is on that.

Changed.

[RFC 4/5] ASoC: tegra_max98090: Change nyan to use gpio-jack

[Dylan Reid]

Use the new gpio jack device instead of handling the gpios in the
machine driver.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 sound/soc/tegra/Kconfig          |   1 +
 sound/soc/tegra/tegra_max98090.c | 107 ++++++++-------------------------------
 2 files changed, 22 insertions(+), 86 deletions(-)

diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
index a6768f8..de42bfb 100644
--- a/sound/soc/tegra/Kconfig
+++ b/sound/soc/tegra/Kconfig
@@ -125,6 +125,7 @@ config SND_SOC_TEGRA_MAX98090
 	select SND_SOC_TEGRA20_I2S if ARCH_TEGRA_2x_SOC
 	select SND_SOC_TEGRA30_I2S if ARCH_TEGRA_3x_SOC
 	select SND_SOC_MAX98090
+	select SND_SOC_GPIO_AUDIO_JACK
 	help
 	  Say Y or M here if you want to add support for SoC audio on Tegra
 	  boards using the MAX98090 codec, such as Venice2.
diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..a8d77ff 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -41,8 +41,6 @@
 
 struct tegra_max98090 {
 	struct tegra_asoc_utils_data util_data;
-	int gpio_hp_det;
-	int gpio_mic_det;
 };
 
 static int tegra_max98090_asoc_hw_params(struct snd_pcm_substream *substream,
@@ -97,38 +95,6 @@ static struct snd_soc_ops tegra_max98090_ops = {
 	.hw_params = tegra_max98090_asoc_hw_params,
 };
 
-static struct snd_soc_jack tegra_max98090_hp_jack;
-
-static struct snd_soc_jack_pin tegra_max98090_hp_jack_pins[] = {
-	{
-		.pin = "Headphones",
-		.mask = SND_JACK_HEADPHONE,
-	},
-};
-
-static struct snd_soc_jack_gpio tegra_max98090_hp_jack_gpio = {
-	.name = "Headphone detection",
-	.report = SND_JACK_HEADPHONE,
-	.debounce_time = 150,
-	.invert = 1,
-};
-
-static struct snd_soc_jack tegra_max98090_mic_jack;
-
-static struct snd_soc_jack_pin tegra_max98090_mic_jack_pins[] = {
-	{
-		.pin = "Mic Jack",
-		.mask = SND_JACK_MICROPHONE,
-	},
-};
-
-static struct snd_soc_jack_gpio tegra_max98090_mic_jack_gpio = {
-	.name = "Mic detection",
-	.report = SND_JACK_MICROPHONE,
-	.debounce_time = 150,
-	.invert = 1,
-};
-
 static const struct snd_soc_dapm_widget tegra_max98090_dapm_widgets[] = {
 	SND_SOC_DAPM_HP("Headphones", NULL),
 	SND_SOC_DAPM_SPK("Speakers", NULL),
@@ -145,50 +111,14 @@ static const struct snd_kcontrol_new tegra_max98090_controls[] = {
 
 static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
 {
-	struct tegra_max98090 *machine = snd_soc_card_get_drvdata(rtd->card);
-
-	if (gpio_is_valid(machine->gpio_hp_det)) {
-		snd_soc_card_jack_new(rtd->card, "Headphones",
-				      SND_JACK_HEADPHONE,
-				      &tegra_max98090_hp_jack,
-				      tegra_max98090_hp_jack_pins,
-				      ARRAY_SIZE(tegra_max98090_hp_jack_pins));
-
-		tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
-		snd_soc_jack_add_gpios(&tegra_max98090_hp_jack,
-					1,
-					&tegra_max98090_hp_jack_gpio);
-	}
-
-	if (gpio_is_valid(machine->gpio_mic_det)) {
-		snd_soc_card_jack_new(rtd->card, "Mic Jack",
-				      SND_JACK_MICROPHONE,
-				      &tegra_max98090_mic_jack,
-				      tegra_max98090_mic_jack_pins,
-				      ARRAY_SIZE(tegra_max98090_mic_jack_pins));
-
-		tegra_max98090_mic_jack_gpio.gpio = machine->gpio_mic_det;
-		snd_soc_jack_add_gpios(&tegra_max98090_mic_jack,
-				       1,
-				       &tegra_max98090_mic_jack_gpio);
-	}
-
-	return 0;
-}
-
-static int tegra_max98090_card_remove(struct snd_soc_card *card)
-{
-	struct tegra_max98090 *machine = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct snd_soc_dapm_context *dapm = &codec->dapm;
 
-	if (gpio_is_valid(machine->gpio_hp_det)) {
-		snd_soc_jack_free_gpios(&tegra_max98090_hp_jack, 1,
-					&tegra_max98090_hp_jack_gpio);
-	}
+	/* Force MICBIAS on because it is needed for mic detection. */
+	snd_soc_dapm_force_enable_pin(dapm, "MICBIAS");
 
-	if (gpio_is_valid(machine->gpio_mic_det)) {
-		snd_soc_jack_free_gpios(&tegra_max98090_mic_jack, 1,
-					&tegra_max98090_mic_jack_gpio);
-	}
+	snd_soc_dapm_sync(dapm);
 
 	return 0;
 }
@@ -203,12 +133,17 @@ static struct snd_soc_dai_link tegra_max98090_dai = {
 			SND_SOC_DAIFMT_CBS_CFS,
 };
 
+static struct snd_soc_aux_dev gpio_headset_jack_dev = {
+		.name = "gpio-headset-jack",
+};
+
 static struct snd_soc_card snd_soc_tegra_max98090 = {
 	.name = "tegra-max98090",
 	.owner = THIS_MODULE,
-	.remove = tegra_max98090_card_remove,
 	.dai_link = &tegra_max98090_dai,
 	.num_links = 1,
+	.aux_dev = &gpio_headset_jack_dev,
+	.num_aux_devs = 1,
 	.controls = tegra_max98090_controls,
 	.num_controls = ARRAY_SIZE(tegra_max98090_controls),
 	.dapm_widgets = tegra_max98090_dapm_widgets,
@@ -234,15 +169,6 @@ static int tegra_max98090_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, card);
 	snd_soc_card_set_drvdata(card, machine);
 
-	machine->gpio_hp_det = of_get_named_gpio(np, "nvidia,hp-det-gpios", 0);
-	if (machine->gpio_hp_det == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
-
-	machine->gpio_mic_det =
-			of_get_named_gpio(np, "nvidia,mic-det-gpios", 0);
-	if (machine->gpio_mic_det == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
-
 	ret = snd_soc_of_parse_card_name(card, "nvidia,model");
 	if (ret)
 		goto err;
@@ -269,6 +195,15 @@ static int tegra_max98090_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	gpio_headset_jack_dev.codec_of_node = of_parse_phandle(np,
+			"nvidia,headset-dev", 0);
+	if (!gpio_headset_jack_dev.codec_of_node) {
+		dev_err(&pdev->dev,
+			"Property 'nvidia,headset-dev' missing or invalid\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
 	tegra_max98090_dai.platform_of_node = tegra_max98090_dai.cpu_of_node;
 
 	ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev);
-- 
2.2.1.62.g3f15098

[RFC 5/5] ARM: tegra: nyan: specify gpio-audio-jack device

[Dylan Reid]

Model the audio jack as a headset jack using the new gpio-audio-jack
device.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 arch/arm/boot/dts/tegra124-nyan.dtsi | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi b/arch/arm/boot/dts/tegra124-nyan.dtsi
index a9aec23..ec33110 100644
--- a/arch/arm/boot/dts/tegra124-nyan.dtsi
+++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
@@ -1,4 +1,5 @@
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/sound/audio-jack-events.h>
 #include "tegra124.dtsi"
 
 / {
@@ -661,6 +662,23 @@
 		};
 	};
 
+	audio_jack: gpio-audio-jack {
+		compatible = "gpio-audio-jack";
+		gpio-audio-jack,jack-name = "Headset Jack";
+		gpio-audio-jack,sw-det-gpios =
+				<&gpio TEGRA_GPIO(I, 7) GPIO_ACTIVE_HIGH>,
+				<&gpio TEGRA_GPIO(R, 7) GPIO_ACTIVE_LOW>;
+		gpio-audio-jack,gpio-names =
+				"Headphones",
+				"Mic Jack";
+		gpio-audio-jack,report-masks =
+				<JACK_HEADPHONE>,
+				<JACK_MICROPHONE>;
+		gpio-audio-jack,debounce-times =
+				<150>,
+				<150>;
+	};
+
 	sound {
 		nvidia,audio-routing =
 			"Headphones", "HPR",
@@ -674,15 +692,13 @@
 
 		nvidia,i2s-controller = <&tegra_i2s1>;
 		nvidia,audio-codec = <&acodec>;
+		nvidia,headset-dev = <&audio_jack>;
 
 		clocks = <&tegra_car TEGRA124_CLK_PLL_A>,
 			 <&tegra_car TEGRA124_CLK_PLL_A_OUT0>,
 			 <&tegra_car TEGRA124_CLK_EXTERN1>;
 		clock-names = "pll_a", "pll_a_out0", "mclk";
 
-		nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(I, 7) GPIO_ACTIVE_HIGH>;
-		nvidia,mic-det-gpios =
-				<&gpio TEGRA_GPIO(R, 7) GPIO_ACTIVE_HIGH>;
 	};
 
 	gpio-restart {
-- 
2.2.1.62.g3f15098

Re: [RFC 0/5] Add a gpio jack device

[Lars-Peter Clausen]

On 05/23/2015 12:09 AM, Dylan Reid wrote:
> The first three changes add a gpio audio jack device.  This device can
> be used on systems that report headphone or mic plug through GPIOS.
> There can be 0-N of these devices created per board each can report one
> of several events.  For example, this allows for a single jack for
> HP/Mic and a separate jack for line out.

I'm not convinced that this series is the right approach and I don't think 
it helps us to solve the problem.

I think what we need to get the layering and encapsulation right is to 
introduce a distinction between jacks and jack detection logic. The jacks 
are part of the fabric and should be registered by the machine driver. The 
jack detection logic can be implemented by either GPIOs, a dedicated jack 
detection chip, like the TS3A227E, or can be part of a more complex CODEC. 
The jack detection logic is a function that is provided by these. The audio 
fabric, which makes up the sound card, is a consumer of this functionality.

To be able to properly abstract this changes to the framework are necessary 
to introduce the concept of jack detection logic providers and consumers.

The chip/driver that implements the jack detection logic register a jack 
detection logic provider, the machine driver registers the jack and 
specifies which jack detection provider is used for each jack.

- Lars

Re: [RFC 0/5] Add a gpio jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2022 bytes --]

On Mon, May 25, 2015 at 05:17:01PM +0200, Lars-Peter Clausen wrote:
> On 05/23/2015 12:09 AM, Dylan Reid wrote:

> >The first three changes add a gpio audio jack device.  This device can
> >be used on systems that report headphone or mic plug through GPIOS.
> >There can be 0-N of these devices created per board each can report one
> >of several events.  For example, this allows for a single jack for
> >HP/Mic and a separate jack for line out.

> I'm not convinced that this series is the right approach and I don't think
> it helps us to solve the problem.

I think it solves the 90% case well enough for simple-card (which is to
the main target user here) and the situation with jack detection is
already fragmented enough that we're not likely to make things
that much worse.  Though now I think about it just taking the gpio out
of the device name would help with binding reuse for other users.

> I think what we need to get the layering and encapsulation right is to
> introduce a distinction between jacks and jack detection logic. The jacks
> are part of the fabric and should be registered by the machine driver. The
> jack detection logic can be implemented by either GPIOs, a dedicated jack
> detection chip, like the TS3A227E, or can be part of a more complex CODEC.
> The jack detection logic is a function that is provided by these. The audio
> fabric, which makes up the sound card, is a consumer of this functionality.

> To be able to properly abstract this changes to the framework are necessary
> to introduce the concept of jack detection logic providers and consumers.

> The chip/driver that implements the jack detection logic register a jack
> detection logic provider, the machine driver registers the jack and
> specifies which jack detection provider is used for each jack.

Yes, this is the complete solution - and it's not an audio specific
thing either, there's a reasonable case to be made for saying that that
this should be resolved in extcon rather than in any one consumer
subsystem.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 0/5] Add a gpio jack device

[Dylan Reid]

On Mon, May 25, 2015 at 10:15 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, May 25, 2015 at 05:17:01PM +0200, Lars-Peter Clausen wrote:
>> On 05/23/2015 12:09 AM, Dylan Reid wrote:
>
>> >The first three changes add a gpio audio jack device.  This device can
>> >be used on systems that report headphone or mic plug through GPIOS.
>> >There can be 0-N of these devices created per board each can report one
>> >of several events.  For example, this allows for a single jack for
>> >HP/Mic and a separate jack for line out.
>
>> I'm not convinced that this series is the right approach and I don't think
>> it helps us to solve the problem.
>
> I think it solves the 90% case well enough for simple-card (which is to
> the main target user here) and the situation with jack detection is
> already fragmented enough that we're not likely to make things
> that much worse.  Though now I think about it just taking the gpio out
> of the device name would help with binding reuse for other users.

Thanks for taking the time to look at this.

My goal was to enable simple-card to be used with a wider arrangement
of jacks.   There is still more that can be done to make this more
general, but hopefully this is a step in the right direction.  The
next step is to add an "audio-jack" binding to simple card which could
be satisfied with this "gpio-audio-jack" or a reference to the
ts3a227e or similar.  That should allow for the rk3288 board to use
simple-card.

>
>> I think what we need to get the layering and encapsulation right is to
>> introduce a distinction between jacks and jack detection logic. The jacks
>> are part of the fabric and should be registered by the machine driver. The
>> jack detection logic can be implemented by either GPIOs, a dedicated jack
>> detection chip, like the TS3A227E, or can be part of a more complex CODEC.
>> The jack detection logic is a function that is provided by these. The audio
>> fabric, which makes up the sound card, is a consumer of this functionality.
>
>> To be able to properly abstract this changes to the framework are necessary
>> to introduce the concept of jack detection logic providers and consumers.
>
>> The chip/driver that implements the jack detection logic register a jack
>> detection logic provider, the machine driver registers the jack and
>> specifies which jack detection provider is used for each jack.
>
> Yes, this is the complete solution - and it's not an audio specific
> thing either, there's a reasonable case to be made for saying that that
> this should be resolved in extcon rather than in any one consumer
> subsystem.

The common use case is also the simplest one.  The framework doesn't
need to know if it is plugged or not because the policy is handled in
user space.  That could work well in extcon as long as there is a way
to map to a device and control.  However I'm not sure that provides
much added value.

Re: [RFC 0/5] Add a gpio jack device

[Lars-Peter Clausen]

On 05/25/2015 07:15 PM, Mark Brown wrote:
> On Mon, May 25, 2015 at 05:17:01PM +0200, Lars-Peter Clausen wrote:
>> On 05/23/2015 12:09 AM, Dylan Reid wrote:
>
>>> The first three changes add a gpio audio jack device.  This device can
>>> be used on systems that report headphone or mic plug through GPIOS.
>>> There can be 0-N of these devices created per board each can report one
>>> of several events.  For example, this allows for a single jack for
>>> HP/Mic and a separate jack for line out.
>
>> I'm not convinced that this series is the right approach and I don't think
>> it helps us to solve the problem.
>
> I think it solves the 90% case well enough for simple-card (which is to
> the main target user here) and the situation with jack detection is
> already fragmented enough that we're not likely to make things
> that much worse.  Though now I think about it just taking the gpio out
> of the device name would help with binding reuse for other users.
>

Yea, but 90% of those 90% are already covered by the existing bindings. The 
existing simple-card bindings and driver support GPIO based jack detection, 
albeit not as flexible as this. But we don't actually gain that much with 
this series and most importantly it does not fix the underlying problem. 
What it does though is introduce a set of bindings which are not proper 
DT-ish bindings which describe the hardware, but which are a workaround for 
the quirks of the current ASoC framework. And once these bindings are 
applied we have to maintain them forever, which also means we have to 
maintain these quirks in the ASoC implementation forever.

>> I think what we need to get the layering and encapsulation right is to
>> introduce a distinction between jacks and jack detection logic. The jacks
>> are part of the fabric and should be registered by the machine driver. The
>> jack detection logic can be implemented by either GPIOs, a dedicated jack
>> detection chip, like the TS3A227E, or can be part of a more complex CODEC.
>> The jack detection logic is a function that is provided by these. The audio
>> fabric, which makes up the sound card, is a consumer of this functionality.
>
>> To be able to properly abstract this changes to the framework are necessary
>> to introduce the concept of jack detection logic providers and consumers.
>
>> The chip/driver that implements the jack detection logic register a jack
>> detection logic provider, the machine driver registers the jack and
>> specifies which jack detection provider is used for each jack.
>
> Yes, this is the complete solution - and it's not an audio specific
> thing either, there's a reasonable case to be made for saying that that
> this should be resolved in extcon rather than in any one consumer
> subsystem.

If the bindings are good it doesn't really matter which framework eventually 
picks them up, but in this case the bindings are awfully ASoC specific and 
leak a lot of the shortcomings of the current implementation.

- Lars

Re: [RFC 0/5] Add a gpio jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1911 bytes --]

On Tue, May 26, 2015 at 08:43:34PM +0200, Lars-Peter Clausen wrote:
> On 05/25/2015 07:15 PM, Mark Brown wrote:

> >I think it solves the 90% case well enough for simple-card (which is to
> >the main target user here) and the situation with jack detection is
> >already fragmented enough that we're not likely to make things
> >that much worse.  Though now I think about it just taking the gpio out
> >of the device name would help with binding reuse for other users.

> Yea, but 90% of those 90% are already covered by the existing bindings. The

I'm not sure what this thing with "yea" is (I've seen some other people
use it too) but the normal word is "yes"...

> existing simple-card bindings and driver support GPIO based jack detection,
> albeit not as flexible as this. But we don't actually gain that much with

Huh, so they do.  Ugh.

> >Yes, this is the complete solution - and it's not an audio specific
> >thing either, there's a reasonable case to be made for saying that that
> >this should be resolved in extcon rather than in any one consumer
> >subsystem.

> If the bindings are good it doesn't really matter which framework eventually
> picks them up, but in this case the bindings are awfully ASoC specific and
> leak a lot of the shortcomings of the current implementation.

Could you expand on the abstraction problems you see please?  It looks
like a fairly direct mapping of GPIOs to a jack to me (like I say I
don't see having GPIOs directly on the jack object as a problem - having
to create a separate node to put the GPIOs in doesn't seem to solve
anything) and we're not likely to have enough GPIOs to make the usual
problems with lists of values too severe.

The only things that concerned me particularly were the name (which I
did agree on once you mentioned it) and the use of a bitmask to describe
what's being reported but it's hard to think of anything much better
than that.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 0/5] Add a gpio jack device

[Dylan Reid]

On Tue, May 26, 2015 at 1:14 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 26, 2015 at 08:43:34PM +0200, Lars-Peter Clausen wrote:
>> On 05/25/2015 07:15 PM, Mark Brown wrote:
>
>> >I think it solves the 90% case well enough for simple-card (which is to
>> >the main target user here) and the situation with jack detection is
>> >already fragmented enough that we're not likely to make things
>> >that much worse.  Though now I think about it just taking the gpio out
>> >of the device name would help with binding reuse for other users.
>
>> Yea, but 90% of those 90% are already covered by the existing bindings. The
>
> I'm not sure what this thing with "yea" is (I've seen some other people
> use it too) but the normal word is "yes"...
>
>> existing simple-card bindings and driver support GPIO based jack detection,
>> albeit not as flexible as this. But we don't actually gain that much with
>
> Huh, so they do.  Ugh.
>
>> >Yes, this is the complete solution - and it's not an audio specific
>> >thing either, there's a reasonable case to be made for saying that that
>> >this should be resolved in extcon rather than in any one consumer
>> >subsystem.
>
>> If the bindings are good it doesn't really matter which framework eventually
>> picks them up, but in this case the bindings are awfully ASoC specific and
>> leak a lot of the shortcomings of the current implementation.
>
> Could you expand on the abstraction problems you see please?  It looks
> like a fairly direct mapping of GPIOs to a jack to me (like I say I
> don't see having GPIOs directly on the jack object as a problem - having
> to create a separate node to put the GPIOs in doesn't seem to solve
> anything) and we're not likely to have enough GPIOs to make the usual
> problems with lists of values too severe.
>
> The only things that concerned me particularly were the name (which I
> did agree on once you mentioned it) and the use of a bitmask to describe
> what's being reported but it's hard to think of anything much better
> than that.

Is just "audio-jack" too generic?  There are a lot of audio jacks that
wouldn't be described by this binding, such as those reported by the
227e or 5650.  The original goal here was to describe a jack that has
one or more gpios, each representing a particular type of device being
attached.  This doesn't overlap with the binding for a jack that is
handled by a headset detect chip.  Does this seem like the right goal,
or is there a benefit to having an "audio-jack" binding that tries to
cover all different types of jacks?

Thanks for taking the time to think about this.

Dylan

Re: [RFC 0/5] Add a gpio jack device

[Lars-Peter Clausen]

On 05/27/2015 06:22 AM, Dylan Reid wrote:
> On Tue, May 26, 2015 at 1:14 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, May 26, 2015 at 08:43:34PM +0200, Lars-Peter Clausen wrote:
>>> On 05/25/2015 07:15 PM, Mark Brown wrote:
>>
>>>> I think it solves the 90% case well enough for simple-card (which is to
>>>> the main target user here) and the situation with jack detection is
>>>> already fragmented enough that we're not likely to make things
>>>> that much worse.  Though now I think about it just taking the gpio out
>>>> of the device name would help with binding reuse for other users.
>>
>>> Yea, but 90% of those 90% are already covered by the existing bindings. The
>>
>> I'm not sure what this thing with "yea" is (I've seen some other people
>> use it too) but the normal word is "yes"...
>>
>>> existing simple-card bindings and driver support GPIO based jack detection,
>>> albeit not as flexible as this. But we don't actually gain that much with
>>
>> Huh, so they do.  Ugh.
>>
>>>> Yes, this is the complete solution - and it's not an audio specific
>>>> thing either, there's a reasonable case to be made for saying that that
>>>> this should be resolved in extcon rather than in any one consumer
>>>> subsystem.
>>
>>> If the bindings are good it doesn't really matter which framework eventually
>>> picks them up, but in this case the bindings are awfully ASoC specific and
>>> leak a lot of the shortcomings of the current implementation.
>>
>> Could you expand on the abstraction problems you see please?  It looks
>> like a fairly direct mapping of GPIOs to a jack to me (like I say I
>> don't see having GPIOs directly on the jack object as a problem - having
>> to create a separate node to put the GPIOs in doesn't seem to solve
>> anything) and we're not likely to have enough GPIOs to make the usual
>> problems with lists of values too severe.
>>
>> The only things that concerned me particularly were the name (which I
>> did agree on once you mentioned it) and the use of a bitmask to describe
>> what's being reported but it's hard to think of anything much better
>> than that.
>
> Is just "audio-jack" too generic?  There are a lot of audio jacks that
> wouldn't be described by this binding, such as those reported by the
> 227e or 5650.  The original goal here was to describe a jack that has
> one or more gpios, each representing a particular type of device being
> attached.  This doesn't overlap with the binding for a jack that is
> handled by a headset detect chip.  Does this seem like the right goal,
> or is there a benefit to having an "audio-jack" binding that tries to
> cover all different types of jacks?

Ideally we'd have a binding which is generic enough to cover not only audio 
jacks but be a bit more generic. I think Laurent already has some thoughts 
on how such a binding should look like.

- Lars

Re: [RFC 0/5] Add a gpio jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1304 bytes --]

On Tue, May 26, 2015 at 09:22:53PM -0700, Dylan Reid wrote:
> On Tue, May 26, 2015 at 1:14 PM, Mark Brown <broonie@kernel.org> wrote:

> > The only things that concerned me particularly were the name (which I
> > did agree on once you mentioned it) and the use of a bitmask to describe
> > what's being reported but it's hard to think of anything much better
> > than that.

> Is just "audio-jack" too generic?  There are a lot of audio jacks that
> wouldn't be described by this binding, such as those reported by the
> 227e or 5650.  The original goal here was to describe a jack that has

I think it's fine - I think we can use this as a jack object and have
other things reference it to supply additional detection mechanism.
Lars' point about jacks not just being for audio is a valid one, though.

> one or more gpios, each representing a particular type of device being
> attached.  This doesn't overlap with the binding for a jack that is
> handled by a headset detect chip.  Does this seem like the right goal,
> or is there a benefit to having an "audio-jack" binding that tries to
> cover all different types of jacks?

So like I say I was thinking that either the jack object has a list of
detection method phandles which point to other devices or the other
devices point at the jack object.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 0/5] Add a gpio jack device

[Dylan Reid]

On Wed, May 27, 2015 at 10:26 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 26, 2015 at 09:22:53PM -0700, Dylan Reid wrote:
>> On Tue, May 26, 2015 at 1:14 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > The only things that concerned me particularly were the name (which I
>> > did agree on once you mentioned it) and the use of a bitmask to describe
>> > what's being reported but it's hard to think of anything much better
>> > than that.
>
>> Is just "audio-jack" too generic?  There are a lot of audio jacks that
>> wouldn't be described by this binding, such as those reported by the
>> 227e or 5650.  The original goal here was to describe a jack that has
>
> I think it's fine - I think we can use this as a jack object and have
> other things reference it to supply additional detection mechanism.
> Lars' point about jacks not just being for audio is a valid one, though.
>
>> one or more gpios, each representing a particular type of device being
>> attached.  This doesn't overlap with the binding for a jack that is
>> handled by a headset detect chip.  Does this seem like the right goal,
>> or is there a benefit to having an "audio-jack" binding that tries to
>> cover all different types of jacks?
>
> So like I say I was thinking that either the jack object has a list of
> detection method phandles which point to other devices or the other
> devices point at the jack object.

I might not be completely following this.  Do we want to create a
binding for the physical plug and another for the method used to
detect the if a device is attached?

What is the benefit in separating the plug from the detection method,
these seem pretty tightly bound to me.  There are three main types of
detection for headsets, gpio, ADC measurement, and offload to a codec
or HP chip.  Are there situations where those would change separate
from how the jack is connected?

Re: [RFC 0/5] Add a gpio jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]

On Wed, May 27, 2015 at 10:38:32PM -0700, Dylan Reid wrote:
> On Wed, May 27, 2015 at 10:26 AM, Mark Brown <broonie@kernel.org> wrote:

> > So like I say I was thinking that either the jack object has a list of
> > detection method phandles which point to other devices or the other
> > devices point at the jack object.

> I might not be completely following this.  Do we want to create a
> binding for the physical plug and another for the method used to
> detect the if a device is attached?

Yes, or at least allow other things to extend what the device is doing.

> What is the benefit in separating the plug from the detection method,
> these seem pretty tightly bound to me.  There are three main types of
> detection for headsets, gpio, ADC measurement, and offload to a codec
> or HP chip.  Are there situations where those would change separate
> from how the jack is connected?

No, but they can be combined onto a single jack - for example have a
GPIO switch that detects mechanical insertion and a CODEC that detects
microphone and button presses.  The GPIO/ADC combination used to be very
common, it's essentially just a non-integrated way of doing what your
accessory detection device does.  If you have non-audio functions on the
jack then things get even richer.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 0/5] Add a gpio jack device

[Lars-Peter Clausen]

On 05/26/2015 10:14 PM, Mark Brown wrote:
[...]
>
>>> Yes, this is the complete solution - and it's not an audio specific
>>> thing either, there's a reasonable case to be made for saying that that
>>> this should be resolved in extcon rather than in any one consumer
>>> subsystem.
>
>> If the bindings are good it doesn't really matter which framework eventually
>> picks them up, but in this case the bindings are awfully ASoC specific and
>> leak a lot of the shortcomings of the current implementation.
>
> Could you expand on the abstraction problems you see please?  It looks
> like a fairly direct mapping of GPIOs to a jack to me (like I say I
> don't see having GPIOs directly on the jack object as a problem - having
> to create a separate node to put the GPIOs in doesn't seem to solve
> anything) and we're not likely to have enough GPIOs to make the usual
> problems with lists of values too severe.

Sorry, I overlooked something which lead to a bit of misunderstanding. I 
though that the gpio-audio-jack driver also sets up the DAPM pins for the 
jack, based on that patch 4 removes the DAPM jack pin setup from 
tegra_max98090 driver. This would have resulted in a weired dependency chain 
where the gpio-audio-jack driver sets references DAPM pins by name which 
belong to the audio fabric that references the gpio-audio-jack node as its 
jack detection chip. But the driver doesn't actually do that so there is no 
real implicit dependency to the audio-fabric in the binding itself and it is 
all internal to the implementation. So that is OK.

I'm still a bit uncomfortable with mixing up jacks and jack detection logic. 
For the GPIO based jack detection logic this isn't that bad since we could 
argue that the DT node represents the jack. But I'm a bit afraid that this 
sets a precedence and we'll soon see bindings where the description of the 
jack becomes part of the description of the jack detection chip or CODEC. 
And the component or CODEC driver is responsible for registering the jack.

And the other problem is how do we match up DAPM widgets to a jack. This is 
functionality from the tegra_max98090 driver that is lost in this series.

- Lars

Re: [RFC 0/5] Add a gpio jack device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2405 bytes --]

On Wed, May 27, 2015 at 01:12:51PM +0200, Lars-Peter Clausen wrote:
> On 05/26/2015 10:14 PM, Mark Brown wrote:

> >Could you expand on the abstraction problems you see please?  It looks
> >like a fairly direct mapping of GPIOs to a jack to me (like I say I
> >don't see having GPIOs directly on the jack object as a problem - having
> >to create a separate node to put the GPIOs in doesn't seem to solve
> >anything) and we're not likely to have enough GPIOs to make the usual
> >problems with lists of values too severe.

> Sorry, I overlooked something which lead to a bit of misunderstanding. I
> though that the gpio-audio-jack driver also sets up the DAPM pins for the
> jack, based on that patch 4 removes the DAPM jack pin setup from
> tegra_max98090 driver. This would have resulted in a weired dependency chain
> where the gpio-audio-jack driver sets references DAPM pins by name which
> belong to the audio fabric that references the gpio-audio-jack node as its
> jack detection chip. But the driver doesn't actually do that so there is no
> real implicit dependency to the audio-fabric in the binding itself and it is
> all internal to the implementation. So that is OK.

Ah, yes - that would indeed have been bad.

> I'm still a bit uncomfortable with mixing up jacks and jack detection logic.
> For the GPIO based jack detection logic this isn't that bad since we could
> argue that the DT node represents the jack. But I'm a bit afraid that this
> sets a precedence and we'll soon see bindings where the description of the
> jack becomes part of the description of the jack detection chip or CODEC.
> And the component or CODEC driver is responsible for registering the jack.

Yes, we do need to make it *the* binding for a jack when things are put
into DT.  Hopefully that's fairly easy to police, and if we can get some
detection drivers using a binding that works with a jack object (ideally
with helpers) then that will provide positive examples too.

> And the other problem is how do we match up DAPM widgets to a jack. This is
> functionality from the tegra_max98090 driver that is lost in this series.

That is going to be an issue when we start hooking things up isn't it.
Off the top if my head if we define some standard set of widgets that
get created then it just becomes another device for the machine to route
to/from (which is the logical extension of making it an auxdev).

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]