Skip to contentSkip to conversation

asoc tlv320aic3x: add GPIO support

PATCH In review Daniel Mack 4 participants 13 messages Apr 18, 2008 – Apr 22, 2008 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 asoc tlv320aic3x: add GPIO support

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.

sound/soc/codecs/tlv320aic3x.c +83 -0
index dd6d495..2c86283 100644
@@ -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");
sound/soc/codecs/tlv320aic3x.h +7 -1
index d49d001..dc05964 100644
@@ -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 */

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 2 lines · Jarkko Nikula

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

#3 lore raw
Quoted 5 lines · Daniel Mack

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.

#4 lore raw

Hi Mark,

Quoted 9 lines · Mark Brown
Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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

#5 lore raw
Quoted 10 lines · Daniel Mack
Quoted 3 lines · Mark Brown

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.

Quoted 9 lines
Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines
Quoted 4 lines

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.

Quoted 7 lines

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.

#6 lore raw
Quoted 5 lines · Mark Brown
Quoted 4 lines

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?

Quoted 10 lines

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.

Quoted 3 lines

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

sound/soc/codecs/tlv320aic3x.c +46 -0
index dd6d495..75ef421 100644
@@ -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");
sound/soc/codecs/tlv320aic3x.h +47 -1

63 lines not on this page · raw message

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#7 lore raw
Quoted 9 lines · Daniel Mack
Quoted 3 lines · Mark Brown

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.

#8 lore raw
Quoted 12 lines · Mark Brown
Quoted 4 lines · Daniel Mack
Quoted 2 lines · Mark Brown

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

Quoted 5 lines
Quoted 1 line

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?

Quoted 6 lines

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

#9 lore raw
In reply to Daniel Mack
Quoted 3 lines · Daniel Mack

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?

#10 lore raw
Quoted 8 lines · Mark Brown
Quoted 3 lines · Daniel Mack

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.

sound/soc/codecs/tlv320aic3x.c +46 -0
index dd6d495..75ef421 100644
@@ -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");
sound/soc/codecs/tlv320aic3x.h +47 -1

63 lines not on this page · raw message

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#11 lore raw
Quoted 1 line · Daniel Mack

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.

#12 lore raw
In reply to Mark Brown
Quoted 70 lines · Mark Brown
Quoted 11 lines · Daniel Mack
Quoted 3 lines · Mark Brown

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.

Quoted 9 lines
Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines
Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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

#13 lore raw
Quoted 4 lines · pHilipp Zabel

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.