[alsa-devel] [PATCH v2 0/4] Tegra: Harmony: Add internal speaker support
Harmony has headers on the board for an internal speaker and internal mic. This patch series adds various infra-structure, and enables usage of the internal speaker.
The patch series includes both changes to code within the ASoC subsystem and Tegra subsystem, the latter provided only for context when reading the ASoC changes. Would you prefer I separated these so that the ASoC maintainers can grab a whole series and apply it, rather than just a subset of the patches?
While patch 2 is physically located in the Tegra subsystem, I'd consider it part of the ASoC driver. Can such a change be checked into the ASoC tree? This new header file will be required to build to ASoC driver once the following patches are in. I was considering placing this in include/sound, following the example of various codecs, but it doesn't seem a good idea to pollute that directory with platform-specific files. The current location for the file follows the example of s3c24xx/simtec. Perhaps it should be in sound/soc/tegra?
Stephen Warren (4): ASoC: WM8903: Expose GPIOs through gpiolib ARM: tegra: Add Harmony sound platform data type ARM: tegra: Platform data fixes for ASoC driver updates ASoC: tegra: Harmony: Support the internal speaker
arch/arm/mach-tegra/board-harmony.c | 31 ++++++ arch/arm/mach-tegra/gpio-names.h | 2 + arch/arm/mach-tegra/include/mach/harmony_audio.h | 19 ++++ include/sound/wm8903.h | 7 +- sound/soc/codecs/wm8903.c | 117 +++++++++++++++++++++- sound/soc/tegra/harmony.c | 89 ++++++++++++++-- 6 files changed, 251 insertions(+), 14 deletions(-) create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
Also, fix platform_data GPIO handling to have an explicit "don't touch this pin" option.
Signed-off-by: Stephen Warren swarren@nvidia.com --- include/sound/wm8903.h | 7 ++- sound/soc/codecs/wm8903.c | 117 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 3 deletions(-)
diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h index b4a0db2..c835f2c 100644 --- a/include/sound/wm8903.h +++ b/include/sound/wm8903.h @@ -12,7 +12,7 @@ #define __LINUX_SND_WM8903_H
/* Used to enable configuration of a GPIO to all zeros */ -#define WM8903_GPIO_NO_CONFIG 0x8000 +#define WM8903_GPIO_NO_CONFIG 0x10000
/* * R6 (0x06) - Mic Bias Control 0 @@ -231,6 +231,8 @@ #define WM8903_GP5_DB_SHIFT 0 /* GP5_DB */ #define WM8903_GP5_DB_WIDTH 1 /* GP5_DB */
+#define WM8903_NUM_GPIO 5 + struct wm8903_platform_data { bool irq_active_low; /* Set if IRQ active low, default high */
@@ -243,7 +245,8 @@ struct wm8903_platform_data {
int micdet_delay; /* Delay after microphone detection (ms) */
- u32 gpio_cfg[5]; /* Default register values for GPIO pin mux */ + int gpio_base; + u32 gpio_cfg[WM8903_NUM_GPIO]; /* Default register values for GPIO pin mux */ };
#endif diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index a2a446c..db641c5 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -2,6 +2,7 @@ * wm8903.c -- WM8903 ALSA SoC Audio driver * * Copyright 2008 Wolfson Microelectronics + * Copyright 2011 NVIDIA, Inc. * * Author: Mark Brown broonie@opensource.wolfsonmicro.com * @@ -19,6 +20,7 @@ #include <linux/init.h> #include <linux/completion.h> #include <linux/delay.h> +#include <linux/gpio.h> #include <linux/pm.h> #include <linux/i2c.h> #include <linux/platform_device.h> @@ -213,6 +215,7 @@ static u16 wm8903_reg_defaults[] = { };
struct wm8903_priv { + struct snd_soc_codec *codec;
int sysclk; int irq; @@ -230,6 +233,10 @@ struct wm8903_priv { int mic_short; int mic_last_report; int mic_delay; + +#ifdef CONFIG_GPIOLIB + struct gpio_chip gpio_chip; +#endif };
static int wm8903_volatile_register(struct snd_soc_codec *codec, unsigned int reg) @@ -1635,6 +1642,110 @@ static int wm8903_resume(struct snd_soc_codec *codec) return 0; }
+#ifdef CONFIG_GPIOLIB +static inline struct wm8903_priv *gpio_to_wm8903(struct gpio_chip *chip) +{ + return container_of(chip, struct wm8903_priv, gpio_chip); +} + +static int wm8903_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + if (offset >= WM8903_NUM_GPIO) + return -EINVAL; + + return 0; +} + +static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) +{ + struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); + struct snd_soc_codec *codec = wm8903->codec; + + return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset, + WM8903_GP1_DIR_MASK, WM8903_GP1_DIR); +} + +static int wm8903_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); + struct snd_soc_codec *codec = wm8903->codec; + int reg; + + reg = snd_soc_read(codec, WM8903_GPIO_CONTROL_1 + offset); + + return (reg & WM8903_GP1_LVL_MASK) >> WM8903_GP1_LVL_SHIFT; +} + +static int wm8903_gpio_direction_out(struct gpio_chip *chip, + unsigned offset, int value) +{ + struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); + struct snd_soc_codec *codec = wm8903->codec; + + return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset, + WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK, + value << WM8903_GP2_LVL_SHIFT); +} + +static void wm8903_gpio_set(struct gpio_chip *chip, unsigned offset, int value) +{ + struct wm8903_priv *wm8903 = gpio_to_wm8903(chip); + struct snd_soc_codec *codec = wm8903->codec; + + snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset, + WM8903_GP1_LVL_MASK, value << WM8903_GP1_LVL_SHIFT); +} + +static struct gpio_chip wm8903_template_chip = { + .label = "wm8903", + .owner = THIS_MODULE, + .request = wm8903_gpio_request, + .direction_input = wm8903_gpio_direction_in, + .get = wm8903_gpio_get, + .direction_output = wm8903_gpio_direction_out, + .set = wm8903_gpio_set, + .can_sleep = 1, +}; + +static void wm8903_init_gpio(struct snd_soc_codec *codec) +{ + struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); + struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); + int ret; + + wm8903->gpio_chip = wm8903_template_chip; + wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO; + wm8903->gpio_chip.dev = codec->dev; + + if (pdata && pdata->gpio_base) + wm8903->gpio_chip.base = pdata->gpio_base; + else + wm8903->gpio_chip.base = -1; + + ret = gpiochip_add(&wm8903->gpio_chip); + if (ret != 0) + dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret); +} + +static void wm8903_free_gpio(struct snd_soc_codec *codec) +{ + struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); + int ret; + + ret = gpiochip_remove(&wm8903->gpio_chip); + if (ret != 0) + dev_err(codec->dev, "Failed to remove GPIOs: %d\n", ret); +} +#else +static void wm8903_init_gpio(struct snd_soc_codec *codec) +{ +} + +static void wm8903_free_gpio(struct snd_soc_codec *codec) +{ +} +#endif + static int wm8903_probe(struct snd_soc_codec *codec) { struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); @@ -1643,6 +1754,7 @@ static int wm8903_probe(struct snd_soc_codec *codec) int trigger, irq_pol; u16 val;
+ wm8903->codec = codec; init_completion(&wm8903->wseq);
ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_I2C); @@ -1667,7 +1779,7 @@ static int wm8903_probe(struct snd_soc_codec *codec) /* Set up GPIOs and microphone detection */ if (pdata) { for (i = 0; i < ARRAY_SIZE(pdata->gpio_cfg); i++) { - if (!pdata->gpio_cfg[i]) + if (pdata->gpio_cfg[i] == WM8903_GPIO_NO_CONFIG) continue;
snd_soc_write(codec, WM8903_GPIO_CONTROL_1 + i, @@ -1749,12 +1861,15 @@ static int wm8903_probe(struct snd_soc_codec *codec) ARRAY_SIZE(wm8903_snd_controls)); wm8903_add_widgets(codec);
+ wm8903_init_gpio(codec); + return ret; }
/* power down chip */ static int wm8903_remove(struct snd_soc_codec *codec) { + wm8903_free_gpio(codec); wm8903_set_bias_level(codec, SND_SOC_BIAS_OFF); return 0; }
On Wed, Jan 19, 2011 at 01:50:02PM -0700, Stephen Warren wrote:
This looks good, one query and one minor omission though.
/* Used to enable configuration of a GPIO to all zeros */ -#define WM8903_GPIO_NO_CONFIG 0x8000 +#define WM8903_GPIO_NO_CONFIG 0x10000
Why this change? The top bit in the registers is never actually used, really they're all 15 bit.
+static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) +{
- struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
- struct snd_soc_codec *codec = wm8903->codec;
- return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
WM8903_GP1_DIR_MASK, WM8903_GP1_DIR);+}
+static int wm8903_gpio_direction_out(struct gpio_chip *chip,
unsigned offset, int value)+{
- struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
- struct snd_soc_codec *codec = wm8903->codec;
- return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK,value << WM8903_GP2_LVL_SHIFT);+}
These should also set GPn_FN - to zero for GPIO output, 3 for GPIO input. Otherwise changing between input and output mode at runtime won't do the right thing. As a side effect of that you probably wouldn't need to specify the GPIO configuration in platform data.
Mark Brown wrote on Thursday, January 20, 2011 4:53 AM:
On Wed, Jan 19, 2011 at 01:50:02PM -0700, Stephen Warren wrote:
This looks good, one query and one minor omission though.
/* Used to enable configuration of a GPIO to all zeros */ -#define WM8903_GPIO_NO_CONFIG 0x8000 +#define WM8903_GPIO_NO_CONFIG 0x10000
Why this change? The top bit in the registers is never actually used, really they're all 15 bit.
Some other registers appear to have bit 15 defined, so I made sure this flag was completely outside any valid register bits.
Although mainly, I just made it consistent with wm8962.h, which I assume picked that value for the same reason.
That said, the original value was OK for the GPIO registers, so I can revert that if you want.
+static int wm8903_gpio_direction_in(struct gpio_chip *chip, unsigned offset) +{
- struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
- struct snd_soc_codec *codec = wm8903->codec;
- return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
WM8903_GP1_DIR_MASK, WM8903_GP1_DIR);+}
+static int wm8903_gpio_direction_out(struct gpio_chip *chip,
unsigned offset, int value)+{
- struct wm8903_priv *wm8903 = gpio_to_wm8903(chip);
- struct snd_soc_codec *codec = wm8903->codec;
- return snd_soc_update_bits(codec, WM8903_GPIO_CONTROL_1 + offset,
WM8903_GP1_DIR_MASK | WM8903_GP1_LVL_MASK,value << WM8903_GP2_LVL_SHIFT);+}
These should also set GPn_FN - to zero for GPIO output, 3 for GPIO input. Otherwise changing between input and output mode at runtime won't do the right thing. As a side effect of that you probably wouldn't need to specify the GPIO configuration in platform data.
Ooops. I didn't notice that. How does this interact with bit 7; GPn_DIR? I assume both need to be set appropriately since they're both defined bits.
On Thu, Jan 20, 2011 at 09:23:45AM -0800, Stephen Warren wrote:
Although mainly, I just made it consistent with wm8962.h, which I assume picked that value for the same reason.
8962 has a very different register map as it has compatiblity with some very old devices.
That said, the original value was OK for the GPIO registers, so I can revert that if you want.
It did stick out as a bit random.
These should also set GPn_FN - to zero for GPIO output, 3 for GPIO input. Otherwise changing between input and output mode at runtime won't do the right thing. As a side effect of that you probably wouldn't need to specify the GPIO configuration in platform data.
Ooops. I didn't notice that. How does this interact with bit 7; GPn_DIR? I assume both need to be set appropriately since they're both defined bits.
Yes, one controls the function and the other controls the pad mode.
Signed-off-by: Stephen Warren swarren@nvidia.com --- arch/arm/mach-tegra/include/mach/harmony_audio.h | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
diff --git a/arch/arm/mach-tegra/include/mach/harmony_audio.h b/arch/arm/mach-tegra/include/mach/harmony_audio.h new file mode 100644 index 0000000..5c46391 --- /dev/null +++ b/arch/arm/mach-tegra/include/mach/harmony_audio.h @@ -0,0 +1,19 @@ +/* + * arch/arm/mach-tegra/include/mach/harmony_audio.h + * + * Copyright 2011 NVIDIA, Inc. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * 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. + * + */ + +struct harmony_audio_platform_data { + int gpio_spkr_en; +};
The WM8903 driver now needs platform data in order to correctly expose its GPIOs through gpiolib.
The top-level ASoC driver now needs a platform device in order to provide infra-structure to pass in platform data.
The top-level ASoC driver now needs platform data in order to be informed which GPIO IDs to use.
This is all needed to allow the top-level ASoC driver to configure the WM8903's GPIO3 as an output GPIO, and enable it as appropriate. This is the enable signal for the external speaker amplifier chips.
Signed-off-by: Stephen Warren swarren@nvidia.com --- arch/arm/mach-tegra/board-harmony.c | 31 +++++++++++++++++++++++++++++++ arch/arm/mach-tegra/gpio-names.h | 2 ++ 2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c index 2696a20..c5a4ae0 100644 --- a/arch/arm/mach-tegra/board-harmony.c +++ b/arch/arm/mach-tegra/board-harmony.c @@ -2,6 +2,7 @@ * arch/arm/mach-tegra/board-harmony.c * * Copyright (C) 2010 Google, Inc. + * Copyright (C) 2011 NVIDIA, Inc. * * This software is licensed under the terms of the GNU General Public * License version 2, as published by the Free Software Foundation, and @@ -24,12 +25,15 @@ #include <linux/i2c.h> #include <linux/io.h>
+#include <sound/wm8903.h> + #include <asm/mach-types.h> #include <asm/mach/arch.h> #include <asm/mach/time.h> #include <asm/setup.h>
#include <mach/gpio.h> +#include <mach/harmony_audio.h> #include <mach/i2c.h> #include <mach/iomap.h> #include <mach/irqs.h> @@ -53,6 +57,8 @@ #define ATAG_NVIDIA_PRESERVED_MEM_N 2 #define ATAG_NVIDIA_FORCE_32 0x7fffffff
+#define GPIO_WM8903(_x_) (TEGRA_MAX_GPIO + 1 + (_x_)) + struct tag_tegra { __u32 bootarg_key; __u32 bootarg_len; @@ -112,9 +118,33 @@ static struct tegra_i2c_platform_data harmony_i2c1_platform_data = { .bus_clk_rate = { 400000, 0 }, };
+static struct wm8903_platform_data harmony_wm8903_pdata = { + .gpio_base = GPIO_WM8903(0), + .gpio_cfg = { + WM8903_GPIO_NO_CONFIG, + WM8903_GPIO_NO_CONFIG, + 0, + WM8903_GPIO_NO_CONFIG, + WM8903_GPIO_NO_CONFIG, + }, +}; + static struct i2c_board_info __initdata harmony_i2c_bus1_board_info[] = { { I2C_BOARD_INFO("wm8903", 0x1a), + .platform_data = &harmony_wm8903_pdata, + }, +}; + +static struct harmony_audio_platform_data harmony_audio_pdata = { + .gpio_spkr_en = GPIO_WM8903(2), +}; + +static struct platform_device harmony_audio_device = { + .name = "tegra-snd-harmony", + .id = 0, + .dev = { + .platform_data = &harmony_audio_pdata, }, };
@@ -127,6 +157,7 @@ static struct platform_device *harmony_devices[] __initdata = { &tegra_i2s_device1, &tegra_das_device, &tegra_pcm_device, + &harmony_audio_device, };
static void __init tegra_harmony_fixup(struct machine_desc *desc, diff --git a/arch/arm/mach-tegra/gpio-names.h b/arch/arm/mach-tegra/gpio-names.h index f28220a..e8b5cf3 100644 --- a/arch/arm/mach-tegra/gpio-names.h +++ b/arch/arm/mach-tegra/gpio-names.h @@ -244,4 +244,6 @@ #define TEGRA_GPIO_PBB6 222 #define TEGRA_GPIO_PBB7 223
+#define TEGRA_MAX_GPIO TEGRA_GPIO_PBB7 + #endif
Add DAPM widget definitions, and code to control the speaker amplifier GPIO to the Harmony machine driver. Currently, this path is always enabled, while playback is active, since jack detection isn't yet implemented.
The driver initialization path has been reworked a little, and a new driver/device introduced, in order to allow platform data to be passed in, currently containing just the speaker enable GPIO ID.
The GPIO is only requested during _init, since that's the first time it is guaranteed that the WM8903 module is loaded, probed, and hence has exported its GPIO chip.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 89 +++++++++++++++++++++++++++++++++++++++------ 1 files changed, 78 insertions(+), 11 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index b160b71..0e44dd6 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -2,7 +2,7 @@ * harmony.c - Harmony machine ASoC driver * * Author: Stephen Warren swarren@nvidia.com - * Copyright (C) 2010 - NVIDIA, Inc. + * Copyright (C) 2010-2011 - NVIDIA, Inc. * * Based on code copyright/by: * @@ -29,7 +29,14 @@ */
#include <asm/mach-types.h> + +#include <linux/gpio.h> #include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#include <mach/harmony_audio.h> + #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -40,9 +47,12 @@ #include "tegra_pcm.h" #include "tegra_asoc_utils.h"
-#define PREFIX "ASoC Harmony: " +#define DRV_NAME "tegra-snd-harmony" +#define PREFIX DRV_NAME ": "
+static struct harmony_audio_platform_data *pdata; static struct platform_device *harmony_snd_device; +static int gpio_spkr_en_requested;
static int harmony_asoc_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) @@ -107,7 +117,19 @@ static struct snd_soc_ops harmony_asoc_ops = { .hw_params = harmony_asoc_hw_params, };
+static int harmony_event_int_spk(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + if (SND_SOC_DAPM_EVENT_ON(event)) + gpio_set_value_cansleep(pdata->gpio_spkr_en, 1); + else + gpio_set_value_cansleep(pdata->gpio_spkr_en, 0); + + return 0; +} + static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = { + SND_SOC_DAPM_SPK("Int Spk", harmony_event_int_spk), SND_SOC_DAPM_HP("Headphone Jack", NULL), SND_SOC_DAPM_MIC("Mic Jack", NULL), }; @@ -115,6 +137,10 @@ static const struct snd_soc_dapm_widget harmony_dapm_widgets[] = { static const struct snd_soc_dapm_route harmony_audio_map[] = { {"Headphone Jack", NULL, "HPOUTR"}, {"Headphone Jack", NULL, "HPOUTL"}, + {"Int Spk", NULL, "ROP"}, + {"Int Spk", NULL, "RON"}, + {"Int Spk", NULL, "LOP"}, + {"Int Spk", NULL, "LON"}, {"Mic Bias", NULL, "Mic Jack"}, {"IN1L", NULL, "Mic Bias"}, }; @@ -123,6 +149,7 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) { struct snd_soc_codec *codec = rtd->codec; struct snd_soc_dapm_context *dapm = &codec->dapm; + int ret;
snd_soc_dapm_new_controls(dapm, harmony_dapm_widgets, ARRAY_SIZE(harmony_dapm_widgets)); @@ -131,9 +158,19 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) ARRAY_SIZE(harmony_audio_map));
snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); + snd_soc_dapm_enable_pin(dapm, "Int Spk"); snd_soc_dapm_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_sync(dapm);
+ ret = gpio_request(pdata->gpio_spkr_en, "spkr_en"); + if (ret) { + pr_err(PREFIX "cannot get spkr_en gpio\n"); + return ret; + } + gpio_spkr_en_requested = 1; + + gpio_direction_output(pdata->gpio_spkr_en, 0); + return 0; }
@@ -154,13 +191,19 @@ static struct snd_soc_card snd_soc_harmony = { .num_links = 1, };
-static int __init harmony_soc_modinit(void) +static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev) { int ret;
- if (!machine_is_harmony()) { - pr_err(PREFIX "Not running on Tegra Harmony!\n"); - return -ENODEV; + if (pdev->id != 0) { + dev_err(&pdev->dev, "ID %d out of range\n", pdev->id); + return -EINVAL; + } + + pdata = pdev->dev.platform_data; + if (!pdata) { + dev_err(&pdev->dev, "no platform data supplied\n"); + return -EINVAL; }
ret = tegra_asoc_utils_init(); @@ -173,7 +216,7 @@ static int __init harmony_soc_modinit(void) */ harmony_snd_device = platform_device_alloc("soc-audio", -1); if (harmony_snd_device == NULL) { - pr_err(PREFIX "platform_device_alloc failed\n"); + dev_err(&pdev->dev, "platform_device_alloc failed\n"); ret = -ENOMEM; goto err_clock_utils; } @@ -182,7 +225,7 @@ static int __init harmony_soc_modinit(void)
ret = platform_device_add(harmony_snd_device); if (ret) { - pr_err(PREFIX "platform_device_add failed (%d)\n", + dev_err(&pdev->dev, "platform_device_add failed (%d)\n", ret); goto err_device_put; } @@ -195,15 +238,39 @@ err_clock_utils: tegra_asoc_utils_fini(); return ret; } -module_init(harmony_soc_modinit);
-static void __exit harmony_soc_modexit(void) +static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev) { platform_device_unregister(harmony_snd_device);
+ if (gpio_spkr_en_requested) + gpio_free(pdata->gpio_spkr_en); + tegra_asoc_utils_fini(); + + return 0; +} + +static struct platform_driver tegra_snd_harmony_driver = { + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + }, + .probe = tegra_snd_harmony_probe, + .remove = __devexit_p(tegra_snd_harmony_remove), +}; + +static int __init snd_tegra_harmony_init(void) +{ + return platform_driver_register(&tegra_snd_harmony_driver); +} +module_init(snd_tegra_harmony_init); + +static void __exit snd_tegra_harmony_exit(void) +{ + platform_driver_unregister(&tegra_snd_harmony_driver); } -module_exit(harmony_soc_modexit); +module_exit(snd_tegra_harmony_exit);
MODULE_AUTHOR("Stephen Warren swarren@nvidia.com"); MODULE_DESCRIPTION("Harmony machine ASoC driver");
On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:
- if (SND_SOC_DAPM_EVENT_ON(event))
gpio_set_value_cansleep(pdata->gpio_spkr_en, 1);- else
gpio_set_value_cansleep(pdata->gpio_spkr_en, 0);
Very minor stylistic thing but you could say:
gpio_set_value_cansleep(pdata->gpio_spkr_en, SND_SOC_DAPM_EVENT_ON(event));
instead.
On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:
-static int __init harmony_soc_modinit(void) +static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
Unrelated change...
- if (!machine_is_harmony()) {
pr_err(PREFIX "Not running on Tegra Harmony!\n");return -ENODEV;
- if (pdev->id != 0) {
dev_err(&pdev->dev, "ID %d out of range\n", pdev->id);return -EINVAL;- }
- pdata = pdev->dev.platform_data;
- if (!pdata) {
dev_err(&pdev->dev, "no platform data supplied\n");return -EINVAL;
Unless you have more than one sound card your device ID should be -1. I'd also be inclined to keep the machine_is_harmony() check here for paranoia.
With the recently added exposure of snd_soc_register_card() you *should* just be able allocate a regular platform device with a regular name in your arch/arm code and then register that directly with the ASoC core - something like:
int __devinit harmony_audio_probe(struct platform_device *pdev) { /* Do GPIO stuff */
card->dev = &pdev->dev; snd_soc_register_card(&snd_soc_harmony);
/* Error handling */ }
ought to do the trick, and is much neater and more idiomatic than the soc-audio stuff.
Mark Brown wrote on Tuesday, January 25, 2011 1:30 PM:
On Wed, Jan 19, 2011 at 01:50:05PM -0700, Stephen Warren wrote:
-static int __init harmony_soc_modinit(void) +static __devinit int tegra_snd_harmony_probe(struct platform_device
*pdev)
Unrelated change...
Yeah, I guess I should separate the device probing rework into a separate patch.
... With the recently added exposure of snd_soc_register_card() you *should* just be able allocate a regular platform device with a regular name in your arch/arm code and then register that directly with the ASoC core - something like:
int __devinit harmony_audio_probe(struct platform_device *pdev) { /* Do GPIO stuff */
card->dev = &pdev->dev; snd_soc_register_card(&snd_soc_harmony);
/* Error handling */ }
ought to do the trick, and is much neater and more idiomatic than the soc-audio stuff.
With the existing soc-audio structure, one has to:
platform_set_drvdata(harmony_snd_device, &snd_soc_harmony);
I assume there's no need for this when registering via snd_soc_register_card; In other words, I'm free to use dev_set_drvdata on the platform_device/device so I can get rid of all the globals in harmony.y while I'm at it?
I do see some internal use of set_drvdata/get_drvdata in soc-core.c. It looks like that's restricted to when the soc-audio platform_device is used, but I don't know if that's just co-incidence, or if it's a guarantee of the API. Can you confirm this?
Thanks.
-- nvpublic
On Tue, Jan 25, 2011 at 07:46:57PM -0800, Stephen Warren wrote:
With the existing soc-audio structure, one has to:
platform_set_drvdata(harmony_snd_device, &snd_soc_harmony);
I assume there's no need for this when registering via snd_soc_register_card; In other words, I'm free to use dev_set_drvdata on the platform_device/device so I can get rid of all the globals in harmony.y while I'm at it?
I do see some internal use of set_drvdata/get_drvdata in soc-core.c. It looks like that's restricted to when the soc-audio platform_device is used, but I don't know if that's just co-incidence, or if it's a guarantee of the API. Can you confirm this?
Oh, bah. The code is currently broken - suspend and resume are still using the platform device. I'll post a patch later today which removes this requirement and adds callbacks for you to use.
On Wed, 2011-01-19 at 13:50 -0700, Stephen Warren wrote:
Harmony has headers on the board for an internal speaker and internal mic. This patch series adds various infra-structure, and enables usage of the internal speaker.
The patch series includes both changes to code within the ASoC subsystem and Tegra subsystem, the latter provided only for context when reading the ASoC changes. Would you prefer I separated these so that the ASoC maintainers can grab a whole series and apply it, rather than just a subset of the patches?
While patch 2 is physically located in the Tegra subsystem, I'd consider it part of the ASoC driver. Can such a change be checked into the ASoC tree? This new header file will be required to build to ASoC driver once the following patches are in. I was considering placing this in include/sound, following the example of various codecs, but it doesn't seem a good idea to pollute that directory with platform-specific files. The current location for the file follows the example of s3c24xx/simtec. Perhaps it should be in sound/soc/tegra?
Stephen Warren (4): ASoC: WM8903: Expose GPIOs through gpiolib ARM: tegra: Add Harmony sound platform data type ARM: tegra: Platform data fixes for ASoC driver updates ASoC: tegra: Harmony: Support the internal speaker
arch/arm/mach-tegra/board-harmony.c | 31 ++++++ arch/arm/mach-tegra/gpio-names.h | 2 + arch/arm/mach-tegra/include/mach/harmony_audio.h | 19 ++++ include/sound/wm8903.h | 7 +- sound/soc/codecs/wm8903.c | 117 +++++++++++++++++++++- sound/soc/tegra/harmony.c | 89 ++++++++++++++-- 6 files changed, 251 insertions(+), 14 deletions(-) create mode 100644 arch/arm/mach-tegra/include/mach/harmony_audio.h
All
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
participants (3)
-
Liam Girdwood -
Mark Brown -
Stephen Warren