[PATCH] asoc tlv320aic3x: add GPIO support

[PATCH] asoc tlv320aic3x: add GPIO support

[Daniel Mack]

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

This patch adds support for AIC3x GPIO lines. They can be configured for
many possible functions as well as be driven manually. I also introduced
i2c read functionality since the GPIO state register has to be read from
hardware every time and can not be served from cache.

Signed-off-by: Daniel Mack <daniel@caiaq.de>


[-- Attachment #2: alsa-tlv320aic33-gpio.diff --]
[-- Type: text/x-diff, Size: 6099 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index dd6d495..2c86283 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -138,6 +138,42 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 		return -EIO;
 }
 
+/*
+ * read from the aic3x register space
+ */
+static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
+		      u8 *value)
+{
+	*value = reg & 0xff;
+	if (codec->hw_read(codec->control_data, value, 1) != 1)
+		return -EIO;
+
+	aic3x_write_reg_cache(codec, reg, *value);
+	return 0;
+}
+
+#define SOC_SINGLE_AIC3X_GPIO(xname, reg, shift, mask, invert) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
+	.info = snd_soc_info_volsw, \
+	.get = snd_soc_get_gpio_aic3x, .put = snd_soc_put_volsw, \
+	.private_value =  SOC_SINGLE_VALUE(reg, shift, mask, invert) }
+
+static int snd_soc_get_gpio_aic3x(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	u8 val = 0;
+
+	aic3x_read(codec, reg, &val);
+	if (reg == AIC3X_GPIO1_REG)
+		ucontrol->value.integer.value[0] = (val >> 1) & 1;
+	else
+		ucontrol->value.integer.value[0] = (val >> 2) & 1;
+
+	return 0;
+}
+
 #define SOC_DAPM_SINGLE_AIC3X(xname, reg, shift, mask, invert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_info_volsw, \
@@ -210,6 +246,25 @@ static const char *aic3x_right_hpcom_mux[] =
       "differential of HPLCOM", "external feedback" };
 static const char *aic3x_linein_mode_mux[] = { "single-ended", "differential" };
 
+static const char *aic3x_gpio1_modes[] = {
+	"Disabled", "Audio word clock",	"Clock mux/1",	"Clock mux/2",
+	"Clock mux/4", "Clock mux/8", "Short circuit IRQ", "AGC noise IRQ",
+	"Input", "Output", "Digital microphone modulator clock",
+	"Audio word clock", "Button press IRQ", "Headset detect IRQ",
+	"Button press or headset detect IRQ", "All IRQ"
+};
+
+static const char *aic3x_gpio2_modes[] = {
+	"Disabled", "Invalid", "Headset detect IRQ", "Input",
+	"Output", "Digital microphone input", "Audio bitclock",
+	"Audio bitclock", "Audio bitclock",
+	"Button press or headset detect IRQ",
+	"All IRQ", "Short-circuit detect or AGC noise IRQ",
+	"Headset detect or button press or short-circuit IRQ",
+	"Short-circuit detect IRQ", "AGC noise detect IRQ",
+	"Button press IRQ"
+};
+
 #define LDAC_ENUM	0
 #define RDAC_ENUM	1
 #define LHPCOM_ENUM	2
@@ -218,6 +273,8 @@ static const char *aic3x_linein_mode_mux[] = { "single-ended", "differential" };
 #define LINE1R_ENUM	5
 #define LINE2L_ENUM	6
 #define LINE2R_ENUM	7
+#define GPIO1_ENUM	8
+#define GPIO2_ENUM	9
 
 static const struct soc_enum aic3x_enum[] = {
 	SOC_ENUM_SINGLE(DAC_LINE_MUX, 6, 3, aic3x_left_dac_mux),
@@ -228,6 +285,8 @@ static const struct soc_enum aic3x_enum[] = {
 	SOC_ENUM_SINGLE(LINE1R_2_RADC_CTRL, 7, 2, aic3x_linein_mode_mux),
 	SOC_ENUM_SINGLE(LINE2L_2_LADC_CTRL, 7, 2, aic3x_linein_mode_mux),
 	SOC_ENUM_SINGLE(LINE2R_2_RADC_CTRL, 7, 2, aic3x_linein_mode_mux),
+	SOC_ENUM_SINGLE(AIC3X_GPIO1_REG, 4, 16, aic3x_gpio1_modes),
+	SOC_ENUM_SINGLE(AIC3X_GPIO2_REG, 4, 16, aic3x_gpio2_modes),
 };
 
 static const struct snd_kcontrol_new aic3x_snd_controls[] = {
@@ -278,6 +337,10 @@ static const struct snd_kcontrol_new aic3x_snd_controls[] = {
 	/* Input */
 	SOC_DOUBLE_R("PGA Capture Volume", LADC_VOL, RADC_VOL, 0, 0x7f, 0),
 	SOC_DOUBLE_R("PGA Capture Switch", LADC_VOL, RADC_VOL, 7, 0x01, 1),
+
+	/* GPIOs */
+	SOC_SINGLE_AIC3X_GPIO("GPIO1 value", AIC3X_GPIO1_REG, 0, 0x01, 0),
+	SOC_SINGLE_AIC3X_GPIO("GPIO2 value", AIC3X_GPIO2_REG, 3, 0x01, 0),
 };
 
 /* add non dapm controls */
@@ -358,6 +421,14 @@ SOC_DAPM_ENUM("Route", aic3x_enum[LINE2L_ENUM]);
 static const struct snd_kcontrol_new aic3x_right_line2_mux_controls =
 SOC_DAPM_ENUM("Route", aic3x_enum[LINE2R_ENUM]);
 
+/* GPIO1 mode */
+static const struct snd_kcontrol_new aic3x_gpio1_controls =
+SOC_DAPM_ENUM("GPIO1", aic3x_enum[GPIO1_ENUM]);
+
+/* GPIO2 mode */
+static const struct snd_kcontrol_new aic3x_gpio2_controls =
+SOC_DAPM_ENUM("GPIO2", aic3x_enum[GPIO2_ENUM]);
+
 /* Left PGA Bypass Mixer */
 static const struct snd_kcontrol_new aic3x_left_pga_bp_mixer_controls[] = {
 	SOC_DAPM_SINGLE("Line Switch", PGAL_2_LLOPM_VOL, 7, 1, 0),
@@ -481,6 +552,11 @@ static const struct snd_soc_dapm_widget aic3x_dapm_widgets[] = {
 	SND_SOC_DAPM_INPUT("LINE1R"),
 	SND_SOC_DAPM_INPUT("LINE2L"),
 	SND_SOC_DAPM_INPUT("LINE2R"),
+
+	SND_SOC_DAPM_MUX("GPIO1 mode", SND_SOC_NOPM, 0, 0,
+			 &aic3x_gpio1_controls),
+	SND_SOC_DAPM_MUX("GPIO2 mode", SND_SOC_NOPM, 0, 0,
+			 &aic3x_gpio2_controls),
 };
 
 static const char *intercon[][3] = {
@@ -1235,6 +1311,12 @@ static struct i2c_client client_template = {
 	.name = "AIC3X",
 	.driver = &aic3x_i2c_driver,
 };
+
+static int aic3x_i2c_read(struct i2c_client *client, u8 *value, int len)
+{
+	value[0] = i2c_smbus_read_byte_data(client, value[0]);
+	return (len == 1);
+}
 #endif
 
 static int aic3x_probe(struct platform_device *pdev)
@@ -1269,6 +1351,7 @@ static int aic3x_probe(struct platform_device *pdev)
 	if (setup->i2c_address) {
 		normal_i2c[0] = setup->i2c_address;
 		codec->hw_write = (hw_write_t) i2c_master_send;
+		codec->hw_read = (hw_read_t) aic3x_i2c_read;
 		ret = i2c_add_driver(&aic3x_i2c_driver);
 		if (ret != 0)
 			printk(KERN_ERR "can't add i2c driver");
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index d49d001..dc05964 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -108,8 +108,14 @@
 #define DACR1_2_RLOPM_VOL		92
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
-/* Clock generation control register */
+/* GPIO/IRQ registers */
+#define AIC3X_STICKY_IRQ_FLAGS_REG	96
+#define AIC3X_REALTIME_IRQ_FLAGS_REG	97
+#define AIC3X_GPIO1_REG			98
+#define AIC3X_GPIO2_REG			99
+#define AIC3X_GPIOA_REG			100
 #define AIC3X_GPIOB_REG			101
+/* Clock generation control register */
 #define AIC3X_CLKGEN_CTRL_REG		102
 
 /* Page select register bits */

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Daniel Mack]

On Fri, Apr 18, 2008 at 11:26:28PM +0300, Jarkko Nikula wrote:
> No any show stopper either but I just started to think about gpiolib if
> AIC3x GPIOs are used as a GPIO.

I agree in general and thought about alternatives as well, but in this
case, it turns out to be a thing the aic3x driver is responsible to as
the registers in questions are purely in its scope. So the idea is to
set up the GPIO's functions via alsa controls and handle its events in
some board-related code as this is really not abstractable. One could,
however, do this setup in machine-related code within the asoc
environment.

My changes don't cast any problems on the existing implementation
either, so I would suggest to just take this patch until someone comes
up with a more suitable solution :)

Best regards,
Daniel

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Sat, Apr 19, 2008 at 02:41:03AM +0200, Daniel Mack wrote:

> the registers in questions are purely in its scope. So the idea is to
> set up the GPIO's functions via alsa controls and handle its events in
> some board-related code as this is really not abstractable. One could,
> however, do this setup in machine-related code within the asoc
> environment.

The usual approach is to have multi-function pins on CODECs are
controlled by the ASoC codec driver based on configuration supplied by
the machine driver.  Their use tends to be heavily constrained by the
board design so it usually makes more sense to let the machine driver
set things up and deal with exposing controls for any configurability
that user space can exercise.

Most of the time when GPIO lines on CODECs are used they tend to be
controlling other bits of the audio subsystem anyway (eg, power for
simple amplifiers) - most SoCs have plenty of GPIOs on-board and they're
usually much easier to work with due to being memory mapped on the CPU.

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Daniel Mack]

Hi Mark,

On Sat, Apr 19, 2008 at 12:24:26PM +0100, Mark Brown wrote:
> > the registers in questions are purely in its scope. So the idea is to
> > set up the GPIO's functions via alsa controls and handle its events in
> > some board-related code as this is really not abstractable. One could,
> > however, do this setup in machine-related code within the asoc
> > environment.
> 
> The usual approach is to have multi-function pins on CODECs are
> controlled by the ASoC codec driver based on configuration supplied by
> the machine driver. 

I was looking for a mechanism to pass such information around but didn't
manage to find any. Which way would you suggest? The approach in my
patch causes as less overhead as possible at it simply reuses the asoc's
widgets, I though.

> Their use tends to be heavily constrained by the
> board design so it usually makes more sense to let the machine driver
> set things up and deal with exposing controls for any configurability
> that user space can exercise.

Ok, but why not let the codec driver expose all the functionality it can
offer for setting up a certain GPIO and use this alsa control either
from user space or from the asoc machine part? I think that's the most
flexible way at all, no?

And when it comes to setting or getting a GPIO's state I see no way to
access such special purpose functions of the codec driver. At least,
'struct snd_soc_codec' doesn't give me any hints how to achive that.

> Most of the time when GPIO lines on CODECs are used they tend to be
> controlling other bits of the audio subsystem anyway (eg, power for
> simple amplifiers) - most SoCs have plenty of GPIOs on-board and they're
> usually much easier to work with due to being memory mapped on the CPU.

Well, I didn't implement that just because it wasn't done yet ;) In the
setup I'm working on, one GPIO is needed to control peripherals next to
it in the board layout where routing constraints become part of the
game. And the other one is connected to one of the processor's GPIO to
catch an interrupt from the AIC3x when a headset is plugged in. Thus, we
really need to support them both. Others may have different use for
them, just have a look at the possible functions of those GPIOs.

Best regards,
Daniel

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Sat, Apr 19, 2008 at 02:18:38PM +0200, Daniel Mack wrote:
> On Sat, Apr 19, 2008 at 12:24:26PM +0100, Mark Brown wrote:

> > The usual approach is to have multi-function pins on CODECs are
> > controlled by the ASoC codec driver based on configuration supplied by
> > the machine driver. 

> I was looking for a mechanism to pass such information around but didn't
> manage to find any. Which way would you suggest? The approach in my
> patch causes as less overhead as possible at it simply reuses the asoc's
> widgets, I though.

The usual thing would be to have your CODEC driver provide its own API
which users can call directly.

> > Their use tends to be heavily constrained by the
> > board design so it usually makes more sense to let the machine driver
> > set things up and deal with exposing controls for any configurability
> > that user space can exercise.

> Ok, but why not let the codec driver expose all the functionality it can
> offer for setting up a certain GPIO and use this alsa control either
> from user space or from the asoc machine part? I think that's the most
> flexible way at all, no?

The CODEC driver can't safely give user space control of multi-function
pins in general, including GPIOs, since their usage depends very
strongly on how they have been connected to the system.  At best much of
the functionality won't be useful on a given board due to a lack of
appropriate connections.  At worst the consequences of misusing them can
include things like causing one or more chips in the system to fall
over, perhaps even the entire system.  For example, if you've got two
chips each trying to drive a signal to different voltages then it is
possible that one or both of them will do something like attempt to
consume too much power and cause supplies to collapse.

It's OK for the kernel to be able to misconfigure things and control of
these features is very useful so it's good to provide it.  The issue is
letting user space get involved unless the machine driver has said that
it's safe to do so.

> And when it comes to setting or getting a GPIO's state I see no way to
> access such special purpose functions of the codec driver. At least,
> 'struct snd_soc_codec' doesn't give me any hints how to achive that.

Like I say, export a CODEC specific API for it.  The machine driver
already needs to know which CODEC and SoC it is working with and how
they are connected in a given system in order to set up the audio
connections.  In some cases the API calls can be avoided - for example,
if there is an I2S port on multi-function pins then configuring that I2S
port could implicitly configure the multi-function pins to enable that
port.

> > Most of the time when GPIO lines on CODECs are used they tend to be
> > controlling other bits of the audio subsystem anyway (eg, power for
> > simple amplifiers) - most SoCs have plenty of GPIOs on-board and they're
> > usually much easier to work with due to being memory mapped on the CPU.

> Well, I didn't implement that just because it wasn't done yet ;) In the
> setup I'm working on, one GPIO is needed to control peripherals next to
> it in the board layout where routing constraints become part of the
> game. And the other one is connected to one of the processor's GPIO to
> catch an interrupt from the AIC3x when a headset is plugged in. Thus, we
> really need to support them both. Others may have different use for
> them, just have a look at the possible functions of those GPIOs.

Those sound like exactly the sort of audio subsystem usages that I was
talking about - things that have fixed functions on a given board and
which the machine driver should be able to know how to set up in a way
that makes sense for the system.

In the case of jack detect I'm actually currently working on a
generalisation of that since it's such a common feature.  I hope to have
something in the next week or two but it depends on my other work.

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Daniel Mack]

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

On Sat, Apr 19, 2008 at 03:50:06PM +0100, Mark Brown wrote:
> > Ok, but why not let the codec driver expose all the functionality it can
> > offer for setting up a certain GPIO and use this alsa control either
> > from user space or from the asoc machine part? I think that's the most
> > flexible way at all, no?
> 
> The CODEC driver can't safely give user space control of multi-function
> pins in general, including GPIOs, since their usage depends very
> strongly on how they have been connected to the system.  At best much of
> the functionality won't be useful on a given board due to a lack of
> appropriate connections.  At worst the consequences of misusing them can
> include things like causing one or more chips in the system to fall
> over, perhaps even the entire system.  For example, if you've got two
> chips each trying to drive a signal to different voltages then it is
> possible that one or both of them will do something like attempt to
> consume too much power and cause supplies to collapse.

Ok, agreed to this point. I wanted to implement this feature as generic
as possbile, but you're right, that might cause some trouble.

> In the case of jack detect I'm actually currently working on a
> generalisation of that since it's such a common feature.  I hope to have
> something in the next week or two but it depends on my other work.

I attached a patch for a proprietary interface - is that what you were
talking about? Would that approach be mergeable with your generic jack
detect code?

Thanks and best regards,
Daniel



[-- Attachment #2: alsa-tlv320aic33-gpio.diff --]
[-- Type: text/x-diff, Size: 5239 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index dd6d495..75ef421 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -138,6 +138,20 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 		return -EIO;
 }
 
+/*
+ * read from the aic3x register space
+ */
+static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
+		      u8 *value)
+{
+	*value = reg & 0xff;
+	if (codec->hw_read(codec->control_data, value, 1) != 1)
+		return -EIO;
+
+	aic3x_write_reg_cache(codec, reg, *value);
+	return 0;
+}
+
 #define SOC_DAPM_SINGLE_AIC3X(xname, reg, shift, mask, invert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_info_volsw, \
@@ -972,6 +986,26 @@ static int aic3x_dapm_event(struct snd_soc_codec *codec, int event)
 	return 0;
 }
 
+void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state)
+{
+	u8 reg = gpio ? AIC3X_GPIO2_REG : AIC3X_GPIO1_REG;
+	u8 bit = gpio ? 3: 0;
+	u8 val = aic3x_read_reg_cache(codec, reg) & ~(1 << bit);
+	aic3x_write(codec, reg, val | (!!state << bit));
+}
+EXPORT_SYMBOL_GPL(aic3x_set_gpio);
+
+int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio)
+{
+	u8 reg = gpio ? AIC3X_GPIO2_REG : AIC3X_GPIO1_REG;
+	u8 bit = gpio ? 2: 1;
+	u8 val = 0;
+
+	aic3x_read(codec, reg, &val);
+	return !!(val & (1 << bit));
+}
+EXPORT_SYMBOL_GPL(aic3x_get_gpio);
+
 #define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
 #define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
 			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
@@ -1038,6 +1072,7 @@ static int aic3x_resume(struct platform_device *pdev)
 static int aic3x_init(struct snd_soc_device *socdev)
 {
 	struct snd_soc_codec *codec = socdev->codec;
+	struct aic3x_setup_data *setup = socdev->codec_data;
 	int reg, ret = 0;
 
 	codec->name = "aic3x";
@@ -1128,6 +1163,10 @@ static int aic3x_init(struct snd_soc_device *socdev)
 	/* off, with power on */
 	aic3x_dapm_event(codec, SNDRV_CTL_POWER_D3hot);
 
+	/* setup GPIO functions */
+	aic3x_write(codec, AIC3X_GPIO1_REG, (setup->gpio_func[0] & 0xf) << 4);
+	aic3x_write(codec, AIC3X_GPIO2_REG, (setup->gpio_func[1] & 0xf) << 4);
+
 	aic3x_add_controls(codec);
 	aic3x_add_widgets(codec);
 	ret = snd_soc_register_card(socdev);
@@ -1235,6 +1274,12 @@ static struct i2c_client client_template = {
 	.name = "AIC3X",
 	.driver = &aic3x_i2c_driver,
 };
+
+static int aic3x_i2c_read(struct i2c_client *client, u8 *value, int len)
+{
+	value[0] = i2c_smbus_read_byte_data(client, value[0]);
+	return (len == 1);
+}
 #endif
 
 static int aic3x_probe(struct platform_device *pdev)
@@ -1269,6 +1314,7 @@ static int aic3x_probe(struct platform_device *pdev)
 	if (setup->i2c_address) {
 		normal_i2c[0] = setup->i2c_address;
 		codec->hw_write = (hw_write_t) i2c_master_send;
+		codec->hw_read = (hw_read_t) aic3x_i2c_read;
 		ret = i2c_add_driver(&aic3x_i2c_driver);
 		if (ret != 0)
 			printk(KERN_ERR "can't add i2c driver");
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index d49d001..c72867b 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -108,8 +108,14 @@
 #define DACR1_2_RLOPM_VOL		92
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
-/* Clock generation control register */
+/* GPIO/IRQ registers */
+#define AIC3X_STICKY_IRQ_FLAGS_REG	96
+#define AIC3X_RT_IRQ_FLAGS_REG		97
+#define AIC3X_GPIO1_REG			98
+#define AIC3X_GPIO2_REG			99
+#define AIC3X_GPIOA_REG			100
 #define AIC3X_GPIOB_REG			101
+/* Clock generation control register */
 #define AIC3X_CLKGEN_CTRL_REG		102
 
 /* Page select register bits */
@@ -175,8 +181,48 @@
 /* Default input volume */
 #define DEFAULT_GAIN    0x20
 
+/* GPIO API */
+enum {
+	AIC3X_GPIO1_FUNC_DISABLED		= 0,
+	AIC3X_GPIO1_FUNC_AUDIO_WORDCLK_ADC	= 1,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX		= 2,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV2		= 3,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV4		= 4,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV8		= 5,
+	AIC3X_GPIO1_FUNC_SHORT_CIRCUIT_IRQ	= 6,
+	AIC3X_GPIO1_FUNC_AGC_NOISE_IRQ		= 7,
+	AIC3X_GPIO1_FUNC_INPUT			= 8,
+	AIC3X_GPIO1_FUNC_OUTPUT			= 9,
+	AIC3X_GPIO1_FUNC_DIGITAL_MIC_MODCLK	= 10,
+	AIC3X_GPIO1_FUNC_AUDIO_WORDCLK		= 11,
+	AIC3X_GPIO1_FUNC_BUTTON_IRQ		= 12,
+	AIC3X_GPIO1_FUNC_HEADSET_DETECT_IRQ	= 13,
+	AIC3X_GPIO1_FUNC_HEADSET_DETECT_OR_BUTTON_IRQ	= 14,
+	AIC3X_GPIO1_FUNC_ALL_IRQ		= 16
+};
+
+enum {
+	AIC3X_GPIO2_FUNC_DISABLED		= 0,
+	AIC3X_GPIO2_FUNC_HEADSET_DETECT_IRQ	= 2,
+	AIC3X_GPIO2_FUNC_INPUT			= 3,
+	AIC3X_GPIO2_FUNC_OUTPUT			= 4,
+	AIC3X_GPIO2_FUNC_DIGITAL_MIC_INPUT	= 5,
+	AIC3X_GPIO2_FUNC_AUDIO_BITCLK		= 8,
+	AIC3X_GPIO2_FUNC_HEADSET_DETECT_OR_BUTTON_IRQ = 9,
+	AIC3X_GPIO2_FUNC_ALL_IRQ		= 10,
+	AIC3X_GPIO2_FUNC_SHORT_CIRCUIT_OR_AGC_IRQ = 11,
+	AIC3X_GPIO2_FUNC_HEADSET_OR_BUTTON_PRESS_OR_SHORT_CIRCUIT_IRQ = 12,
+	AIC3X_GPIO2_FUNC_SHORT_CIRCUIT_IRQ	= 13,
+	AIC3X_GPIO2_FUNC_AGC_NOISE_IRQ		= 14,
+	AIC3X_GPIO2_FUNC_BUTTON_PRESS_IRQ	= 15
+};
+
+void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state);
+int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio);
+
 struct aic3x_setup_data {
 	unsigned short i2c_address;
+	unsigned int gpio_func[2];
 };
 
 extern struct snd_soc_codec_dai aic3x_dai;

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Sat, Apr 19, 2008 at 06:24:18PM +0200, Daniel Mack wrote:
> On Sat, Apr 19, 2008 at 03:50:06PM +0100, Mark Brown wrote:

> > In the case of jack detect I'm actually currently working on a
> > generalisation of that since it's such a common feature.  I hope to have
> > something in the next week or two but it depends on my other work.

> I attached a patch for a proprietary interface - is that what you were
> talking about? Would that approach be mergeable with your generic jack
> detect code?

Yes, that was the sort of thing I was thinking about - looks good from
an API perspective, though I've not looked at the code properly yet.  It
should fit in with the jack detect code, the idea is more to let drivers
push notifications of jack detect status - it doesn't really care how
the driver detects the jack status.

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Jarkko Nikula]

On Sat, Apr 19, 2008 at 9:51 PM, Mark Brown <
broonie@opensource.wolfsonmicro.com> wrote:

> On Sat, Apr 19, 2008 at 06:24:18PM +0200, Daniel Mack wrote:
> > On Sat, Apr 19, 2008 at 03:50:06PM +0100, Mark Brown wrote:
>
> > > In the case of jack detect I'm actually currently working on a
> > > generalisation of that since it's such a common feature.  I hope to
> have
> > > something in the next week or two but it depends on my other work.
>
> > I attached a patch for a proprietary interface - is that what you were
> > talking about? Would that approach be mergeable with your generic jack
> > detect code?
>
> Yes, that was the sort of thing I was thinking about - looks good from
> an API perspective, though I've not looked at the code properly yet.  It
> should fit in with the jack detect code, the idea is more to let drivers
> push notifications of jack detect status - it doesn't really care how
> the driver detects the jack status.
>

This latest version looks ok to me. IMO we can use this as a base for
further development.


Jarkko

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Sat, Apr 19, 2008 at 06:24:18PM +0200, Daniel Mack wrote:

> I attached a patch for a proprietary interface - is that what you were
> talking about? Would that approach be mergeable with your generic jack
> detect code?

The patch looks good but you've not signed off on it - is that just an
oversight or do you think more updates are required?

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Daniel Mack]

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

On Tue, Apr 22, 2008 at 10:52:04AM +0100, Mark Brown wrote:
> On Sat, Apr 19, 2008 at 06:24:18PM +0200, Daniel Mack wrote:
> 
> > I attached a patch for a proprietary interface - is that what you were
> > talking about? Would that approach be mergeable with your generic jack
> > detect code?
> 
> The patch looks good but you've not signed off on it - is that just an
> oversight or do you think more updates are required?

No, that's actually final from my side. Here we go.

---

Subject: [PATCH] asoc tlv320aic3x: add GPIO support

This patch adds support for GPIO pins on TI's TLV320AIC3x via a
proprietary API. I2C read was also implemented as the actual state of
GPIOs must be read from hardware and can not be served from the register
cache.

Signed-off-by: Daniel Mack <daniel@caiaq.de>



[-- Attachment #2: alsa-tlv320aic33-gpio.diff --]
[-- Type: text/x-diff, Size: 5239 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index dd6d495..75ef421 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -138,6 +138,20 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 		return -EIO;
 }
 
+/*
+ * read from the aic3x register space
+ */
+static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
+		      u8 *value)
+{
+	*value = reg & 0xff;
+	if (codec->hw_read(codec->control_data, value, 1) != 1)
+		return -EIO;
+
+	aic3x_write_reg_cache(codec, reg, *value);
+	return 0;
+}
+
 #define SOC_DAPM_SINGLE_AIC3X(xname, reg, shift, mask, invert) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_info_volsw, \
@@ -972,6 +986,26 @@ static int aic3x_dapm_event(struct snd_soc_codec *codec, int event)
 	return 0;
 }
 
+void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state)
+{
+	u8 reg = gpio ? AIC3X_GPIO2_REG : AIC3X_GPIO1_REG;
+	u8 bit = gpio ? 3: 0;
+	u8 val = aic3x_read_reg_cache(codec, reg) & ~(1 << bit);
+	aic3x_write(codec, reg, val | (!!state << bit));
+}
+EXPORT_SYMBOL_GPL(aic3x_set_gpio);
+
+int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio)
+{
+	u8 reg = gpio ? AIC3X_GPIO2_REG : AIC3X_GPIO1_REG;
+	u8 bit = gpio ? 2: 1;
+	u8 val = 0;
+
+	aic3x_read(codec, reg, &val);
+	return !!(val & (1 << bit));
+}
+EXPORT_SYMBOL_GPL(aic3x_get_gpio);
+
 #define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
 #define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
 			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
@@ -1038,6 +1072,7 @@ static int aic3x_resume(struct platform_device *pdev)
 static int aic3x_init(struct snd_soc_device *socdev)
 {
 	struct snd_soc_codec *codec = socdev->codec;
+	struct aic3x_setup_data *setup = socdev->codec_data;
 	int reg, ret = 0;
 
 	codec->name = "aic3x";
@@ -1128,6 +1163,10 @@ static int aic3x_init(struct snd_soc_device *socdev)
 	/* off, with power on */
 	aic3x_dapm_event(codec, SNDRV_CTL_POWER_D3hot);
 
+	/* setup GPIO functions */
+	aic3x_write(codec, AIC3X_GPIO1_REG, (setup->gpio_func[0] & 0xf) << 4);
+	aic3x_write(codec, AIC3X_GPIO2_REG, (setup->gpio_func[1] & 0xf) << 4);
+
 	aic3x_add_controls(codec);
 	aic3x_add_widgets(codec);
 	ret = snd_soc_register_card(socdev);
@@ -1235,6 +1274,12 @@ static struct i2c_client client_template = {
 	.name = "AIC3X",
 	.driver = &aic3x_i2c_driver,
 };
+
+static int aic3x_i2c_read(struct i2c_client *client, u8 *value, int len)
+{
+	value[0] = i2c_smbus_read_byte_data(client, value[0]);
+	return (len == 1);
+}
 #endif
 
 static int aic3x_probe(struct platform_device *pdev)
@@ -1269,6 +1314,7 @@ static int aic3x_probe(struct platform_device *pdev)
 	if (setup->i2c_address) {
 		normal_i2c[0] = setup->i2c_address;
 		codec->hw_write = (hw_write_t) i2c_master_send;
+		codec->hw_read = (hw_read_t) aic3x_i2c_read;
 		ret = i2c_add_driver(&aic3x_i2c_driver);
 		if (ret != 0)
 			printk(KERN_ERR "can't add i2c driver");
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index d49d001..c72867b 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -108,8 +108,14 @@
 #define DACR1_2_RLOPM_VOL		92
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
-/* Clock generation control register */
+/* GPIO/IRQ registers */
+#define AIC3X_STICKY_IRQ_FLAGS_REG	96
+#define AIC3X_RT_IRQ_FLAGS_REG		97
+#define AIC3X_GPIO1_REG			98
+#define AIC3X_GPIO2_REG			99
+#define AIC3X_GPIOA_REG			100
 #define AIC3X_GPIOB_REG			101
+/* Clock generation control register */
 #define AIC3X_CLKGEN_CTRL_REG		102
 
 /* Page select register bits */
@@ -175,8 +181,48 @@
 /* Default input volume */
 #define DEFAULT_GAIN    0x20
 
+/* GPIO API */
+enum {
+	AIC3X_GPIO1_FUNC_DISABLED		= 0,
+	AIC3X_GPIO1_FUNC_AUDIO_WORDCLK_ADC	= 1,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX		= 2,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV2		= 3,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV4		= 4,
+	AIC3X_GPIO1_FUNC_CLOCK_MUX_DIV8		= 5,
+	AIC3X_GPIO1_FUNC_SHORT_CIRCUIT_IRQ	= 6,
+	AIC3X_GPIO1_FUNC_AGC_NOISE_IRQ		= 7,
+	AIC3X_GPIO1_FUNC_INPUT			= 8,
+	AIC3X_GPIO1_FUNC_OUTPUT			= 9,
+	AIC3X_GPIO1_FUNC_DIGITAL_MIC_MODCLK	= 10,
+	AIC3X_GPIO1_FUNC_AUDIO_WORDCLK		= 11,
+	AIC3X_GPIO1_FUNC_BUTTON_IRQ		= 12,
+	AIC3X_GPIO1_FUNC_HEADSET_DETECT_IRQ	= 13,
+	AIC3X_GPIO1_FUNC_HEADSET_DETECT_OR_BUTTON_IRQ	= 14,
+	AIC3X_GPIO1_FUNC_ALL_IRQ		= 16
+};
+
+enum {
+	AIC3X_GPIO2_FUNC_DISABLED		= 0,
+	AIC3X_GPIO2_FUNC_HEADSET_DETECT_IRQ	= 2,
+	AIC3X_GPIO2_FUNC_INPUT			= 3,
+	AIC3X_GPIO2_FUNC_OUTPUT			= 4,
+	AIC3X_GPIO2_FUNC_DIGITAL_MIC_INPUT	= 5,
+	AIC3X_GPIO2_FUNC_AUDIO_BITCLK		= 8,
+	AIC3X_GPIO2_FUNC_HEADSET_DETECT_OR_BUTTON_IRQ = 9,
+	AIC3X_GPIO2_FUNC_ALL_IRQ		= 10,
+	AIC3X_GPIO2_FUNC_SHORT_CIRCUIT_OR_AGC_IRQ = 11,
+	AIC3X_GPIO2_FUNC_HEADSET_OR_BUTTON_PRESS_OR_SHORT_CIRCUIT_IRQ = 12,
+	AIC3X_GPIO2_FUNC_SHORT_CIRCUIT_IRQ	= 13,
+	AIC3X_GPIO2_FUNC_AGC_NOISE_IRQ		= 14,
+	AIC3X_GPIO2_FUNC_BUTTON_PRESS_IRQ	= 15
+};
+
+void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state);
+int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio);
+
 struct aic3x_setup_data {
 	unsigned short i2c_address;
+	unsigned int gpio_func[2];
 };
 
 extern struct snd_soc_codec_dai aic3x_dai;

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Tue, Apr 22, 2008 at 12:04:30PM +0200, Daniel Mack wrote:

> No, that's actually final from my side. Here we go.

Looks like you've generated this against a kernel with the clocking
stuff applied:

Applying asoc tlv320aic3x: add GPIO support
error: patch failed: sound/soc/codecs/tlv320aic3x.h:108
error: sound/soc/codecs/tlv320aic3x.h: patch does not apply
Patch failed at 0001.

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[pHilipp Zabel]

On Sat, Apr 19, 2008 at 4:50 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Apr 19, 2008 at 02:18:38PM +0200, Daniel Mack wrote:
>  > On Sat, Apr 19, 2008 at 12:24:26PM +0100, Mark Brown wrote:
>
>
> > > The usual approach is to have multi-function pins on CODECs are
>  > > controlled by the ASoC codec driver based on configuration supplied by
>  > > the machine driver.
>
>  > I was looking for a mechanism to pass such information around but didn't
>  > manage to find any. Which way would you suggest? The approach in my
>  > patch causes as less overhead as possible at it simply reuses the asoc's
>  > widgets, I though.
>
>  The usual thing would be to have your CODEC driver provide its own API
>  which users can call directly.
>
>
>  > > Their use tends to be heavily constrained by the
>  > > board design so it usually makes more sense to let the machine driver
>  > > set things up and deal with exposing controls for any configurability
>  > > that user space can exercise.
>
>  > Ok, but why not let the codec driver expose all the functionality it can
>  > offer for setting up a certain GPIO and use this alsa control either
>  > from user space or from the asoc machine part? I think that's the most
>  > flexible way at all, no?
>
>  The CODEC driver can't safely give user space control of multi-function
>  pins in general, including GPIOs, since their usage depends very
>  strongly on how they have been connected to the system.  At best much of
>  the functionality won't be useful on a given board due to a lack of
>  appropriate connections.  At worst the consequences of misusing them can
>  include things like causing one or more chips in the system to fall
>  over, perhaps even the entire system.  For example, if you've got two
>  chips each trying to drive a signal to different voltages then it is
>  possible that one or both of them will do something like attempt to
>  consume too much power and cause supplies to collapse.
>
>  It's OK for the kernel to be able to misconfigure things and control of
>  these features is very useful so it's good to provide it.  The issue is
>  letting user space get involved unless the machine driver has said that
>  it's safe to do so.
>
>
>  > And when it comes to setting or getting a GPIO's state I see no way to
>  > access such special purpose functions of the codec driver. At least,
>  > 'struct snd_soc_codec' doesn't give me any hints how to achive that.
>
>  Like I say, export a CODEC specific API for it.  The machine driver
>  already needs to know which CODEC and SoC it is working with and how
>  they are connected in a given system in order to set up the audio
>  connections.  In some cases the API calls can be avoided - for example,
>  if there is an I2S port on multi-function pins then configuring that I2S
>  port could implicitly configure the multi-function pins to enable that
>  port.
>
>
>  > > Most of the time when GPIO lines on CODECs are used they tend to be
>  > > controlling other bits of the audio subsystem anyway (eg, power for
>  > > simple amplifiers) - most SoCs have plenty of GPIOs on-board and they're
>  > > usually much easier to work with due to being memory mapped on the CPU.
>
>  > Well, I didn't implement that just because it wasn't done yet ;) In the
>  > setup I'm working on, one GPIO is needed to control peripherals next to
>  > it in the board layout where routing constraints become part of the
>  > game. And the other one is connected to one of the processor's GPIO to
>  > catch an interrupt from the AIC3x when a headset is plugged in. Thus, we
>  > really need to support them both. Others may have different use for
>  > them, just have a look at the possible functions of those GPIOs.
>
>  Those sound like exactly the sort of audio subsystem usages that I was
>  talking about - things that have fixed functions on a given board and
>  which the machine driver should be able to know how to set up in a way
>  that makes sense for the system.
>
>  In the case of jack detect I'm actually currently working on a
>  generalisation of that since it's such a common feature.  I hope to have
>  something in the next week or two but it depends on my other work.

This is interesting, what is the generic jack detect feature supposed
to do exactly? I ask because I just export the jack detect interrupt
to userspace via input subsystem (SW_HEADPHONE_INSERT). Will it
replace this switch in functionality?

regards
Philipp

Re: [PATCH] asoc tlv320aic3x: add GPIO support

[Mark Brown]

On Mon, Apr 21, 2008 at 04:38:58PM +0200, pHilipp Zabel wrote:

> This is interesting, what is the generic jack detect feature supposed
> to do exactly? I ask because I just export the jack detect interrupt
> to userspace via input subsystem (SW_HEADPHONE_INSERT). Will it
> replace this switch in functionality?

As far as user space is concerned I'd expect it to turn out as pretty
much exactly that, wrapping it up so that sound drivers don't need to
reproduce the code to register an input device and so on.  Devices that
can detect the type of inserted will need some additional switch types
adding.