[alsa-devel] [PATCH - try2] ASoC: TPA6130A2 amplifier driver
Driver for Texas Instruments TPA6130A2 stereo headphone amplifier.
The driver provides playback gain control and also pre-defined DAPM_HP widgets and DAPM routings for power management.
The DAPM_HP widget names are: "TPA6130A2 Headphone Left" "TPA6130A2 Headphone Right"
_From soc machine drivers to use with the tpa6130a2 amplifier, the tpa6130a2_add_controls has to be called, which adds the alsa controls and the DAPM routing needed for the tpa6130a2. After that the machine driver can connect the codec's output with 'TPA6130A2 Left' and 'TPA6130A2 Right':
{"TPA6130A2 Left", NULL, "CODEC LEFT OUT"}, {"TPA6130A2 Right", NULL, "CODEC RIGHT OUT"},
Internally the left and right channels are powered separately. When none of the channels are needed the amplifier is powered down: hard power: valid GPIO number is passed within platform data soft power: Using the software shutdown of the amplifier
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com ---
The previous version of the tpa6130a2 driver was part of Eduardo's series, to add audio support for RX-51.
We decided, that I'll send the tpa amplifier driver as separate patch to speed up the process.
Comments from Mark has been addressed with this patch: set_power is replaced by GPIO number. DAPM routing added with per channel power management Only warning is printed, when using the driver on untested version of the chip
include/sound/tpa6130a2-plat.h | 30 +++ sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tpa6130a2.c | 463 ++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/tpa6130a2.h | 62 ++++++ 5 files changed, 561 insertions(+), 0 deletions(-) create mode 100644 include/sound/tpa6130a2-plat.h create mode 100644 sound/soc/codecs/tpa6130a2.c create mode 100644 sound/soc/codecs/tpa6130a2.h
diff --git a/include/sound/tpa6130a2-plat.h b/include/sound/tpa6130a2-plat.h new file mode 100644 index 0000000..e8c901e --- /dev/null +++ b/include/sound/tpa6130a2-plat.h @@ -0,0 +1,30 @@ +/* + * TPA6130A2 driver platform header + * + * Copyright (C) Nokia Corporation + * + * Written by Peter Ujfalusi peter.ujfalusi@nokia.com + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + +#ifndef TPA6130A2_PLAT_H +#define TPA6130A2_PLAT_H + +struct tpa6130a2_platform_data { + int power_gpio; +}; + +#endif diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 3c46f34..fab01c9 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -29,6 +29,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TLV320AIC23 if I2C select SND_SOC_TLV320AIC26 if SPI_MASTER select SND_SOC_TLV320AIC3X if I2C + select SND_SOC_TPA6130A2 if I2C select SND_SOC_TWL4030 if TWL4030_CORE select SND_SOC_UDA134X select SND_SOC_UDA1380 if I2C @@ -228,3 +229,6 @@ config SND_SOC_WM9713 # Amp config SND_SOC_MAX9877 tristate + +config SND_SOC_TPA6130A2 + tristate diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index fc1c458..2f14391 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -49,6 +49,7 @@ snd-soc-wm-hubs-objs := wm_hubs.o
# Amp snd-soc-max9877-objs := max9877.o +snd-soc-tpa6130a2-objs := tpa6130a2.o
obj-$(CONFIG_SND_SOC_AC97_CODEC) += snd-soc-ac97.o obj-$(CONFIG_SND_SOC_AD1836) += snd-soc-ad1836.o @@ -101,3 +102,4 @@ obj-$(CONFIG_SND_SOC_WM_HUBS) += snd-soc-wm-hubs.o
# Amp obj-$(CONFIG_SND_SOC_MAX9877) += snd-soc-max9877.o +obj-$(CONFIG_SND_SOC_TPA6130A2) += snd-soc-tpa6130a2.o diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c new file mode 100644 index 0000000..1b77c95 --- /dev/null +++ b/sound/soc/codecs/tpa6130a2.c @@ -0,0 +1,463 @@ +/* + * ALSA SoC Texas Instruments TPA6130A2 headset stereo amplifier driver + * + * Copyright (C) Nokia Corporation + * + * Author: Peter Ujfalusi peter.ujfalusi@nokia.com + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + +#include <linux/module.h> +#include <linux/errno.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/gpio.h> +#include <sound/tpa6130a2-plat.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/tlv.h> + +#include "tpa6130a2.h" + +struct i2c_client *tpa6130a2_client; + +/* This struct is used to save the context */ +struct tpa6130a2_data { + struct mutex mutex; + unsigned char regs[TPA6130A2_CACHEREGNUM]; + int power_gpio; + unsigned char power_state; +}; + +static int tpa6130a2_i2c_read(int reg) +{ + struct tpa6130a2_data *data; + int val; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + /* If powered off, return the cached value */ + if (data->power_state) { + val = i2c_smbus_read_byte_data(tpa6130a2_client, reg); + if (val < 0) + dev_err(&tpa6130a2_client->dev, "Read failed\n"); + else + data->regs[reg] = val; + } else { + val = data->regs[reg]; + } + + return val; +} + +static int tpa6130a2_i2c_write(int reg, u8 value) +{ + struct tpa6130a2_data *data; + int val = 0; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + if (data->power_state) { + val = i2c_smbus_write_byte_data(tpa6130a2_client, reg, value); + if (val < 0) + dev_err(&tpa6130a2_client->dev, "Write failed\n"); + } + + /* Either powered on or off, we save the context */ + data->regs[reg] = value; + + return val; +} + +static u8 tpa6130a2_read(int reg) +{ + struct tpa6130a2_data *data; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + return data->regs[reg]; +} + +static void tpa6130a2_initialize(void) +{ + struct tpa6130a2_data *data; + int i; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + for (i = 1; i < TPA6130A2_REG_VERSION; i++) + tpa6130a2_i2c_write(i, data->regs[i]); +} + +void tpa6130a2_power(int power) +{ + struct tpa6130a2_data *data; + u8 val; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + mutex_lock(&data->mutex); + if (power) { + /* Power on */ + if (data->power_gpio >= 0) { + gpio_set_value(data->power_gpio, 1); + data->power_state = 1; + tpa6130a2_initialize(); + } + /* Clear SWS */ + val = tpa6130a2_read(TPA6130A2_REG_CONTROL); + val &= ~TPA6130A2_SWS; + tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); + } else { + /* set SWS */ + val = tpa6130a2_read(TPA6130A2_REG_CONTROL); + val |= TPA6130A2_SWS; + tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); + /* Power off */ + if (data->power_gpio >= 0) { + gpio_set_value(data->power_gpio, 0); + data->power_state = 0; + } + } + mutex_unlock(&data->mutex); +} + +static int tpa6130a2_get_reg(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct soc_mixer_control *mc = + (struct soc_mixer_control *)kcontrol->private_value; + struct tpa6130a2_data *data; + unsigned int reg = mc->reg; + unsigned int shift = mc->shift; + unsigned int mask = mc->max; + unsigned int invert = mc->invert; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + mutex_lock(&data->mutex); + + ucontrol->value.integer.value[0] = + (tpa6130a2_read(reg) >> shift) & mask; + + if (invert) + ucontrol->value.integer.value[0] = + mask - ucontrol->value.integer.value[0]; + + mutex_unlock(&data->mutex); + return 0; +} + +static int tpa6130a2_set_reg(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct soc_mixer_control *mc = + (struct soc_mixer_control *)kcontrol->private_value; + struct tpa6130a2_data *data; + unsigned int reg = mc->reg; + unsigned int shift = mc->shift; + unsigned int mask = mc->max; + unsigned int invert = mc->invert; + unsigned int val = (ucontrol->value.integer.value[0] & mask); + unsigned int val_reg; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + if (invert) + val = mask - val; + + mutex_lock(&data->mutex); + + val_reg = tpa6130a2_read(reg); + if (((val_reg >> shift) & mask) == val) { + mutex_unlock(&data->mutex); + return 0; + } + + val_reg &= ~(mask << shift); + val_reg |= val << shift; + tpa6130a2_i2c_write(reg, val_reg); + + mutex_unlock(&data->mutex); + + return 1; +} + +/* + * TPA6130 volume. From -59.5 to 4 dB with increasing step size when going + * down in gain. + */ +static const unsigned int tpa6130_tlv[] = { + TLV_DB_RANGE_HEAD(10), + 0, 1, TLV_DB_SCALE_ITEM(-5950, 600, 0), + 2, 3, TLV_DB_SCALE_ITEM(-5000, 250, 0), + 4, 5, TLV_DB_SCALE_ITEM(-4550, 160, 0), + 6, 7, TLV_DB_SCALE_ITEM(-4140, 190, 0), + 8, 9, TLV_DB_SCALE_ITEM(-3650, 120, 0), + 10, 11, TLV_DB_SCALE_ITEM(-3330, 160, 0), + 12, 13, TLV_DB_SCALE_ITEM(-3040, 180, 0), + 14, 20, TLV_DB_SCALE_ITEM(-2710, 110, 0), + 21, 37, TLV_DB_SCALE_ITEM(-1960, 74, 0), + 38, 63, TLV_DB_SCALE_ITEM(-720, 45, 0), +}; + +static const struct snd_kcontrol_new tpa6130a2_controls[] = { + SOC_SINGLE_EXT_TLV("TPA6130A2 Headphone Playback Volume", + TPA6130A2_REG_VOL_MUTE, 0, 0x3f, 0, + tpa6130a2_get_reg, tpa6130a2_set_reg, + tpa6130_tlv), +}; + +/* + * Enable or disable channel (left or right) + * The bit number for mute and amplifier are the same per channel: + * bit 6: Right channel + * bit 7: Left channel + * in both registers. + */ +static void tpa6130a2_channel_enable(u8 channel, int enable) +{ + struct tpa6130a2_data *data; + u8 val; + + BUG_ON(tpa6130a2_client == NULL); + data = i2c_get_clientdata(tpa6130a2_client); + + if (enable) { + /* Enable channel */ + /* Enable amplifier */ + val = tpa6130a2_read(TPA6130A2_REG_CONTROL); + val |= channel; + tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); + + /* Unmute channel */ + val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE); + val &= ~channel; + tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val); + } else { + /* Disable channel */ + /* Mute channel */ + val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE); + val |= channel; + tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val); + + /* Disable amplifier */ + val = tpa6130a2_read(TPA6130A2_REG_CONTROL); + val &= ~channel; + tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); + } +} + +static int tpa6130a2_left_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + switch (event) { + case SND_SOC_DAPM_POST_PMU: + tpa6130a2_channel_enable(TPA6130A2_HP_EN_L, 1); + break; + case SND_SOC_DAPM_POST_PMD: + tpa6130a2_channel_enable(TPA6130A2_HP_EN_L, 0); + break; + } + return 0; +} + +static int tpa6130a2_right_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + switch (event) { + case SND_SOC_DAPM_POST_PMU: + tpa6130a2_channel_enable(TPA6130A2_HP_EN_R, 1); + break; + case SND_SOC_DAPM_POST_PMD: + tpa6130a2_channel_enable(TPA6130A2_HP_EN_R, 0); + break; + } + return 0; +} + +static int tpa6130a2_supply_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + switch (event) { + case SND_SOC_DAPM_POST_PMU: + tpa6130a2_power(1); + break; + case SND_SOC_DAPM_POST_PMD: + tpa6130a2_power(0); + break; + } + return 0; +} + +static const struct snd_soc_dapm_widget tpa6130a2_dapm_widgets[] = { + SND_SOC_DAPM_PGA_E("TPA6130A2 Left", SND_SOC_NOPM, + 0, 0, NULL, 0, tpa6130a2_left_event, + SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PGA_E("TPA6130A2 Right", SND_SOC_NOPM, + 0, 0, NULL, 0, tpa6130a2_right_event, + SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_SUPPLY("TPA6130A2 Enable", SND_SOC_NOPM, + 0, 0, tpa6130a2_supply_event, + SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), + /* Outputs */ + SND_SOC_DAPM_HP("TPA6130A2 Headphone Left", NULL), + SND_SOC_DAPM_HP("TPA6130A2 Headphone Right", NULL), +}; + +static const struct snd_soc_dapm_route audio_map[] = { + {"TPA6130A2 Headphone Left", NULL, "TPA6130A2 Left"}, + {"TPA6130A2 Headphone Right", NULL, "TPA6130A2 Right"}, + + {"TPA6130A2 Headphone Left", NULL, "TPA6130A2 Enable"}, + {"TPA6130A2 Headphone Right", NULL, "TPA6130A2 Enable"}, +}; + +int tpa6130a2_add_controls(struct snd_soc_codec *codec) +{ + snd_soc_dapm_new_controls(codec, tpa6130a2_dapm_widgets, + ARRAY_SIZE(tpa6130a2_dapm_widgets)); + + snd_soc_dapm_add_routes(codec, audio_map, ARRAY_SIZE(audio_map)); + + return snd_soc_add_controls(codec, tpa6130a2_controls, + ARRAY_SIZE(tpa6130a2_controls)); + +} +EXPORT_SYMBOL_GPL(tpa6130a2_add_controls); + +static int tpa6130a2_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct device *dev; + struct tpa6130a2_data *data; + struct tpa6130a2_platform_data *pdata; + int ret; + + dev = &client->dev; + + if (client->dev.platform_data == NULL) { + dev_err(dev, "Platform data not set\n"); + dump_stack(); + return -ENODEV; + } + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (data == NULL) { + dev_err(dev, "Can not allocate memory\n"); + return -ENOMEM; + } + + tpa6130a2_client = client; + + i2c_set_clientdata(tpa6130a2_client, data); + + pdata = (struct tpa6130a2_platform_data *)client->dev.platform_data; + data->power_gpio = pdata->power_gpio; + + mutex_init(&data->mutex); + + /* Set default register values */ + data->regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS; + data->regs[TPA6130A2_REG_VOL_MUTE] = TPA6130A2_MUTE_R | + TPA6130A2_MUTE_L; + + if (data->power_gpio >= 0) { + ret = gpio_request(data->power_gpio, "tpa6130a2 enable"); + if (ret < 0) { + dev_err(dev, "Failed to request power GPIO (%d)\n", + data->power_gpio); + goto fail; + } + gpio_direction_output(data->power_gpio, 0); + } else { + data->power_state = 1; + tpa6130a2_initialize(); + } + + tpa6130a2_power(1); + + /* Read version */ + ret = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) & + TPA6130A2_VERSION_MASK; + if ((ret != 1) && (ret != 2)) + dev_warn(dev, "UNTESTED version detected (%d)\n", ret); + + /* Disable the chip */ + tpa6130a2_power(0); + + return 0; +fail: + kfree(data); + i2c_set_clientdata(tpa6130a2_client, NULL); + tpa6130a2_client = 0; + + return ret; +} + +static int tpa6130a2_remove(struct i2c_client *client) +{ + struct tpa6130a2_data *data = i2c_get_clientdata(client); + + tpa6130a2_power(0); + + if (data->power_gpio >= 0) + gpio_free(data->power_gpio); + kfree(data); + tpa6130a2_client = 0; + + return 0; +} + +static const struct i2c_device_id tpa6130a2_id[] = { + { "tpa6130a2", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, tpa6130a2_id); + +static struct i2c_driver tpa6130a2_i2c_driver = { + .driver = { + .name = "tpa6130a2", + .owner = THIS_MODULE, + }, + .probe = tpa6130a2_probe, + .remove = __devexit_p(tpa6130a2_remove), + .id_table = tpa6130a2_id, +}; + +static int __init tpa6130a2_init(void) +{ + return i2c_add_driver(&tpa6130a2_i2c_driver); +} + +static void __exit tpa6130a2_exit(void) +{ + i2c_del_driver(&tpa6130a2_i2c_driver); +} + +MODULE_AUTHOR("Peter Ujfalusi"); +MODULE_DESCRIPTION("TPA6130A2 Headphone amplifier driver"); +MODULE_LICENSE("GPL"); + +module_init(tpa6130a2_init); +module_exit(tpa6130a2_exit); diff --git a/sound/soc/codecs/tpa6130a2.h b/sound/soc/codecs/tpa6130a2.h new file mode 100644 index 0000000..6a794f1 --- /dev/null +++ b/sound/soc/codecs/tpa6130a2.h @@ -0,0 +1,62 @@ +/* + * ALSA SoC TPA6130A2 amplifier driver + * + * Copyright (C) Nokia Corporation + * + * Author: Peter Ujfalusi peter.ujfalusi@nokia.com + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + * + */ + +#ifndef __TPA6130A2_H__ +#define __TPA6130A2_H__ + +/* Register addresses */ +#define TPA6130A2_REG_CONTROL 0x01 +#define TPA6130A2_REG_VOL_MUTE 0x02 +#define TPA6130A2_REG_OUT_IMPEDANCE 0x03 +#define TPA6130A2_REG_VERSION 0x04 + +#define TPA6130A2_CACHEREGNUM (TPA6130A2_REG_VERSION + 1) + +/* Register bits */ +/* TPA6130A2_REG_CONTROL (0x01) */ +#define TPA6130A2_SWS (0x01 << 0) +#define TPA6130A2_TERMAL (0x01 << 1) +#define TPA6130A2_MODE(x) (x << 4) +#define TPA6130A2_MODE_STEREO (0x00) +#define TPA6130A2_MODE_DUAL_MONO (0x01) +#define TPA6130A2_MODE_BRIDGE (0x02) +#define TPA6130A2_MODE_MASK (0x03) +#define TPA6130A2_HP_EN_R (0x01 << 6) +#define TPA6130A2_HP_EN_L (0x01 << 7) + +/* TPA6130A2_REG_VOL_MUTE (0x02) */ +#define TPA6130A2_VOLUME(x) ((x & 0x3f) << 0) +#define TPA6130A2_MUTE_R (0x01 << 6) +#define TPA6130A2_MUTE_L (0x01 << 7) + +/* TPA6130A2_REG_OUT_IMPEDANCE (0x03) */ +#define TPA6130A2_HIZ_R (0x01 << 0) +#define TPA6130A2_HIZ_L (0x01 << 1) + +/* TPA6130A2_REG_VERSION (0x04) */ +#define TPA6130A2_VERSION_MASK (0x0f) + +extern int tpa6130a2_add_controls(struct snd_soc_codec *codec); +extern void tpa6130a2_power(int power); + +#endif /* __TPA6130A2_H__ */ -- 1.6.5.rc2
On Fri, 2009-10-09 at 14:55 +0200, Ujfalusi Peter (Nokia-D/Tampere) wrote:
+static int tpa6130a2_i2c_write(int reg, u8 value) +{
struct tpa6130a2_data *data;int val = 0;BUG_ON(tpa6130a2_client == NULL);data = i2c_get_clientdata(tpa6130a2_client);if (data->power_state) {val = i2c_smbus_write_byte_data(tpa6130a2_client, reg, value);if (val < 0)dev_err(&tpa6130a2_client->dev, "Write failed\n");}/* Either powered on or off, we save the context */data->regs[reg] = value;return val;+}
<snip>
+static void tpa6130a2_channel_enable(u8 channel, int enable) +{
struct tpa6130a2_data *data;u8 val;BUG_ON(tpa6130a2_client == NULL);data = i2c_get_clientdata(tpa6130a2_client);
mutex_lock()
if (enable) {/* Enable channel *//* Enable amplifier */val = tpa6130a2_read(TPA6130A2_REG_CONTROL);val |= channel;tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val);
During the tpa6130a2_i2c_write, you read the data->power_state, that may change it's state meanwhile (preempted). Thus, I suggest you using the mutex to cover all i2c writes. (and all tpa read <-> write cycles, so that things are consistent?)
A call to tpa6130a2_power() -> preemted over somewhere here, you may have i2c accesses to a chip that's not powered up?
/* Unmute channel */val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE);val &= ~channel;tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val);} else {/* Disable channel *//* Mute channel */val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE);val |= channel;tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val);/* Disable amplifier */val = tpa6130a2_read(TPA6130A2_REG_CONTROL);val &= ~channel;tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val);}
mutex_unlock() ?
+}
On Friday 09 October 2009 16:10:31 Nurkkala Eero.An (EXT-Offcode/Oulu) wrote:
On Fri, 2009-10-09 at 14:55 +0200, Ujfalusi Peter (Nokia-D/Tampere)
During the tpa6130a2_i2c_write, you read the data->power_state, that may change it's state meanwhile (preempted). Thus, I suggest you using the mutex to cover all i2c writes. (and all tpa read <-> write cycles, so that things are consistent?)
A call to tpa6130a2_power() -> preemted over somewhere here, you may have i2c accesses to a chip that's not powered up?
Well it is very unlikely that something like this could ever happen since the tpa6130a2_channel_enable and the tpa6130a2_power is called when DAPM is enabling/disabling the path. I think in a middle of enable or disable there should not be any opposite operation running in DAPM...
But better to be safe: I can add the mutex_lock, unlock pair.
I'll wait for more comments and address this one as well.
During the tpa6130a2_i2c_write, you read the data->power_state, that may change it's state meanwhile (preempted). Thus, I suggest you using the mutex to cover all i2c writes. (and all tpa read <-> write cycles, so that things are consistent?)
A call to tpa6130a2_power() -> preemted over somewhere here, you may have i2c accesses to a chip that's not powered up?
Well it is very unlikely that something like this could ever happen since the tpa6130a2_channel_enable and the tpa6130a2_power is called when DAPM is enabling/disabling the path. I think in a middle of enable or disable there should not be any opposite operation running in DAPM...
But better to be safe: I can add the mutex_lock, unlock pair.
I'll wait for more comments and address this one as well.
-- Péter
Right, sorry I read the patch too fast =) So I guess it's ok like it was. However, I'll put sleep(n)'s in the problematic places and verify it's 100% ok one of these weeks.
- Eero
On Fri, Oct 09, 2009 at 04:36:16PM +0300, Peter Ujfalusi wrote:
Well it is very unlikely that something like this could ever happen since the tpa6130a2_channel_enable and the tpa6130a2_power is called when DAPM is enabling/disabling the path. I think in a middle of enable or disable there should not be any opposite operation running in DAPM...
DAPM guarantees single threadedness - DAPM itself would get confused if there were more than one power update in progress at once.
ext Mark Brown wrote:
On Fri, Oct 09, 2009 at 04:36:16PM +0300, Peter Ujfalusi wrote:
Well it is very unlikely that something like this could ever happen since the tpa6130a2_channel_enable and the tpa6130a2_power is called when DAPM is enabling/disabling the path. I think in a middle of enable or disable there should not be any opposite operation running in DAPM...
DAPM guarantees single threadedness - DAPM itself would get confused if there were more than one power update in progress at once.
I guess there's (or may be) an exception though. I think I've seen some strangeness with the close_delayed_work() -> and simultaneous mixer tweaking:
http://mailman.alsa-project.org/pipermail/alsa-devel/2009-October/021671.htm...
This is just an example:
Userspace thread: snd_soc_dapm_put_volsw() -> mutex_lock(&widget->codec->mutex); -> dapm_mixer_update_power() -> dapm_power_widgets() -> PRE_EMTED in damp_power_widgets()
Workqueue thread: close_delayed_work() snd_soc_dapm_stream_event() mutex_lock(&widget->codec->mutex); mutex_unlock(&widget->codec->mutex); dapm_power_widgets() -> PRE_EMTED back to Userspace thread
So there can and does run two dapm_power_widgets() when stars point to the right direction. That's when delayed work is at damp_power_widgets, and then preempted to the userspace thread. May that isn't related to this TPA though, but I think I'll silently review the code so that the "stars are forced to point to the right direction".
(but yeah, these are just my observations, don't worry about them)
On Fri, Oct 09, 2009 at 08:24:49PM +0200, ext-Eero.Nurkkala@nokia.com wrote:
I guess there's (or may be) an exception though. I think I've seen some strangeness with the close_delayed_work() -> and simultaneous mixer tweaking:
As I said when you posted previously that's a problem anyway and close_delayed_work() needs to be fixed. You had a half-posted patch, did you manage to test it properly (even in normal operation)? I'd been expecting a proper posting of it to appear at some point.
So there can and does run two dapm_power_widgets() when stars point to the right direction. That's when delayed work is at damp_power_widgets, and then preempted to the userspace thread. May that isn't related to this TPA though, but I think I'll silently review the code so that the "stars are forced to point to the right direction".
This is completely unrelated to driver code, the core's data is much more of a problem than the register cache which will generally fix itself the next time DAPM does anything.
ext Mark Brown wrote:
On Fri, Oct 09, 2009 at 08:24:49PM +0200, ext-Eero.Nurkkala@nokia.com wrote:
I guess there's (or may be) an exception though. I think I've seen some strangeness with the close_delayed_work() -> and simultaneous mixer tweaking:
As I said when you posted previously that's a problem anyway and close_delayed_work() needs to be fixed. You had a half-posted patch, did you manage to test it properly (even in normal operation)? I'd been expecting a proper posting of it to appear at some point.
Yes I did test it immediatelly. No apparent problems with that. However, now I use kernel 2.6.28 - and I'm about to update it next week. I wish to take the time with that patch, rather than taking the blame =) Lockdep doesn't seem to find dependencies with the mutex - until the code faces it. (I wish not to make other codecs unusable, locked up)..
So there can and does run two dapm_power_widgets() when stars point to the right direction. That's when delayed work is at damp_power_widgets, and then preempted to the userspace thread. May that isn't related to this TPA though, but I think I'll silently review the code so that the "stars are forced to point to the right direction".
This is completely unrelated to driver code, the core's data is much more of a problem than the register cache which will generally fix itself the next time DAPM does anything.
Yeah, agreed.
On Friday 09 October 2009 15:55:41 Ujfalusi Peter (Nokia-D/Tampere) wrote:
Driver for Texas Instruments TPA6130A2 stereo headphone amplifier.
The patch applies on top of sound-2.6 topic/asoc:
commit 0cce1cfa5c3cc262d9261b59dd7fa6d878775ac9 Merge: 9a6fda9 6f775ba Author: Takashi Iwai tiwai@suse.de Date: Thu Oct 8 11:50:07 2009 +0200
Merge branch 'fix/asoc'
On Fri, Oct 09, 2009 at 03:55:41PM +0300, Peter Ujfalusi wrote:
+struct i2c_client *tpa6130a2_client;
This should be static.
+void tpa6130a2_power(int power) +{
This should either be static or have an EXPORT_SYMBOL_GPL() - I'd expect the former since this is being managed via DAPM.
- pdata = (struct tpa6130a2_platform_data *)client->dev.platform_data;
You should never need to cast away from void, and doing so can mask actual errors.
+fail:
kfree(data);i2c_set_clientdata(tpa6130a2_client, NULL);tpa6130a2_client = 0;
The kernel coding style is to use an explict NULL.
Other than that everything looks good so I've applied this with the fixes I noted above. If you could send a followup patch for the tpa6130a2_power export/static thing that'd be good.
Thanks!
On Friday 09 October 2009 21:15:38 ext Mark Brown wrote:
On Fri, Oct 09, 2009 at 03:55:41PM +0300, Peter Ujfalusi wrote:
+struct i2c_client *tpa6130a2_client;
This should be static.
Yes, it should
+void tpa6130a2_power(int power) +{
This should either be static or have an EXPORT_SYMBOL_GPL() - I'd expect the former since this is being managed via DAPM.
Will be static, since it should be handled by the tpa6130a2 driver internally.
- pdata = (struct tpa6130a2_platform_data *)client->dev.platform_data;
You should never need to cast away from void, and doing so can mask actual errors.
Yes.
+fail:
kfree(data);i2c_set_clientdata(tpa6130a2_client, NULL);tpa6130a2_client = 0;The kernel coding style is to use an explict NULL.
Correct.
Other than that everything looks good so I've applied this with the fixes I noted above. If you could send a followup patch for the tpa6130a2_power export/static thing that'd be good.
I'll send a patch to make the tpa6130a2_power static.
Thanks!
Thanks, Péter
participants (4)
-
Eero Nurkkala -
ext-Eero.Nurkkala＠nokia.com
-
Mark Brown -
Peter Ujfalusi