[alsa-devel] [PATCH 1/3] ASoC: tas571x: Add DT binding document
Document the bindings for the soon-to-be-added tas571x driver.
Signed-off-by: Kevin Cernekee cernekee@chromium.org --- .../devicetree/bindings/sound/tas571x.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tas571x.txt
diff --git a/Documentation/devicetree/bindings/sound/tas571x.txt b/Documentation/devicetree/bindings/sound/tas571x.txt new file mode 100644 index 000000000000..ac704c1f143d --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tas571x.txt @@ -0,0 +1,34 @@ +Texas Instruments TAS5711/TAS5717/TAS5719 stereo power amplifiers + +The codec is controlled through an I2C interface. It also has two other +signals that can be wired up to GPIOs: reset (strongly recommended), and +powerdown (optional). + +Required properties: + +- compatible: "ti,tas5711", "ti,tas5717", or "ti,tas5719" +- reg: The I2C address of the device +- #sound-dai-cells: must be equal to 0 + +Optional properties: + +- reset-gpios: GPIO specifier for the TAS571x's active low reset line +- pdn-gpios: GPIO specifier for the TAS571x's active low powerdown line +- clocks: clock phandle for the MCLK input +- clock-names: should be "mclk" +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply +- PVDD-supply: regulator phandle for the PVDD supply + +Example: + + tas5717: audio-codec@2a { + compatible = "ti,tas5717"; + reg = <0x2a>; + #sound-dai-cells = <0>; + + reset-gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>; + pdn-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + + clocks = <&clk_core CLK_I2S>; + clock-names = "mclk"; + };
Introduce a new codec driver for the Texas Instruments TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically used to take an I2S digital audio input and drive 10-20W into a pair of speakers.
Signed-off-by: Kevin Cernekee cernekee@chromium.org --- sound/soc/codecs/Kconfig | 5 + sound/soc/codecs/Makefile | 3 +- sound/soc/codecs/tas571x.c | 456 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/tas571x.h | 39 ++++ 4 files changed, 502 insertions(+), 1 deletion(-) create mode 100644 sound/soc/codecs/tas571x.c create mode 100644 sound/soc/codecs/tas571x.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index ea9f0e31f9d4..ef8393926607 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -103,6 +103,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TAS2552 if I2C select SND_SOC_TAS5086 if I2C + select SND_SOC_TAS571X if I2C select SND_SOC_TFA9879 if I2C select SND_SOC_TLV320AIC23_I2C if I2C select SND_SOC_TLV320AIC23_SPI if SPI_MASTER @@ -606,6 +607,10 @@ config SND_SOC_TAS5086 tristate "Texas Instruments TAS5086 speaker amplifier" depends on I2C
+config SND_SOC_TAS571X + tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers" + depends on I2C + config SND_SOC_TFA9879 tristate "NXP Semiconductors TFA9879 amplifier" depends on I2C diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 69b8666d187a..104112f9a486 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -105,6 +105,7 @@ snd-soc-sta350-objs := sta350.o snd-soc-sta529-objs := sta529.o snd-soc-stac9766-objs := stac9766.o snd-soc-tas5086-objs := tas5086.o +snd-soc-tas571x-objs := tas571x.o snd-soc-tfa9879-objs := tfa9879.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o @@ -283,7 +284,7 @@ obj-$(CONFIG_SND_SOC_STA350) += snd-soc-sta350.o obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o -obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o +obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o diff --git a/sound/soc/codecs/tas571x.c b/sound/soc/codecs/tas571x.c new file mode 100644 index 000000000000..25c4deb2342d --- /dev/null +++ b/sound/soc/codecs/tas571x.c @@ -0,0 +1,456 @@ +/* + * TAS571x amplifier audio driver + * + * Copyright (C) 2015 Google, Inc. + * Copyright (c) 2013 Daniel Mack zonque@gmail.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/stddef.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/tlv.h> + +#include "tas571x.h" + +#define TAS571X_NUM_SUPPLIES 2 +static const char * const tas571x_supply_names[TAS571X_NUM_SUPPLIES] = { + "VDD", + "PVDD", +}; + +struct tas571x_private { + struct regmap *regmap; + struct regulator_bulk_data supplies[TAS571X_NUM_SUPPLIES]; + struct clk *mclk; + unsigned int format; + enum snd_soc_bias_level bias_level; + struct gpio_desc *reset_gpio; + struct gpio_desc *pdn_gpio; + unsigned int dev_id; + struct snd_soc_codec_driver codec_driver; +}; + +static int tas571x_set_sysclk(struct snd_soc_dai *dai, + int clk_id, unsigned int freq, int dir) +{ + /* + * TAS5717 datasheet pg 21: "The DAP can autodetect and set the + * internal clock-control logic to the appropriate settings for all + * supported clock rates as defined in the clock control register." + */ + return 0; +} + +static int tas571x_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format) +{ + struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec); + + priv->format = format; + + return 0; +} + +static int tas571x_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec); + u32 val; + + switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_RIGHT_J: + val = 0x00; + break; + case SND_SOC_DAIFMT_I2S: + val = 0x03; + break; + case SND_SOC_DAIFMT_LEFT_J: + val = 0x06; + break; + default: + return -EINVAL; + } + + val += (clamp(params_width(params), 16, 24) >> 2) - 4; + + return regmap_update_bits(priv->regmap, TAS571X_SDI_REG, + TAS571X_SDI_FMT_MASK, val); +} + +static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown) +{ + return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG, + TAS571X_SYS_CTRL_2_SDN_MASK, + is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0); +} + +static int tas571x_set_bias_level(struct snd_soc_codec *codec, + enum snd_soc_bias_level level) +{ + struct tas571x_private *priv = snd_soc_codec_get_drvdata(codec); + int ret, assert_pdn = 0; + + if (priv->bias_level == level) + return 0; + + switch (level) { + case SND_SOC_BIAS_PREPARE: + if (!IS_ERR(priv->mclk)) { + ret = clk_prepare_enable(priv->mclk); + if (ret) { + dev_err(codec->dev, + "Failed to enable master clock\n"); + return ret; + } + } + + ret = tas571x_set_shutdown(priv, false); + if (ret) + return ret; + break; + case SND_SOC_BIAS_STANDBY: + ret = tas571x_set_shutdown(priv, true); + if (ret) + return ret; + + if (!IS_ERR(priv->mclk)) + clk_disable_unprepare(priv->mclk); + break; + case SND_SOC_BIAS_ON: + break; + case SND_SOC_BIAS_OFF: + /* Note that this kills I2C accesses. */ + assert_pdn = 1; + break; + } + + if (!IS_ERR(priv->pdn_gpio)) + gpiod_set_value(priv->pdn_gpio, !assert_pdn); + + priv->bias_level = level; + return 0; +} + +static const struct snd_soc_dai_ops tas571x_dai_ops = { + .set_sysclk = tas571x_set_sysclk, + .set_fmt = tas571x_set_dai_fmt, + .hw_params = tas571x_hw_params, +}; + +static const unsigned int tas5711_volume_tlv[] = { + TLV_DB_RANGE_HEAD(1), + 0, 0xff, TLV_DB_SCALE_ITEM(-10350, 50, 1), +}; + +static const struct snd_kcontrol_new tas5711_controls[] = { + SOC_SINGLE_TLV("Master Volume", + TAS571X_MVOL_REG, + 0, 0xff, 1, tas5711_volume_tlv), + SOC_DOUBLE_R_TLV("Speaker Volume", + TAS571X_CH1_VOL_REG, + TAS571X_CH2_VOL_REG, + 0, 0xff, 1, tas5711_volume_tlv), + SOC_DOUBLE("Speaker Switch", + TAS571X_SOFT_MUTE_REG, + TAS571X_SOFT_MUTE_CH1_SHIFT, TAS571X_SOFT_MUTE_CH2_SHIFT, + 1, 1), +}; + +static const unsigned int tas5717_volume_tlv[] = { + TLV_DB_RANGE_HEAD(1), + 0x000, 0x1ff, TLV_DB_SCALE_ITEM(-10375, 25, 0), +}; + +static const struct snd_kcontrol_new tas5717_controls[] = { + /* MVOL LSB is ignored - see comments in tas571x_i2c_probe() */ + SOC_SINGLE_TLV("Master Volume", + TAS571X_MVOL_REG, 1, 0x1ff, 1, + tas5717_volume_tlv), + SOC_DOUBLE_R_TLV("Speaker Volume", + TAS571X_CH1_VOL_REG, TAS571X_CH2_VOL_REG, + 1, 0x1ff, 1, tas5717_volume_tlv), + SOC_DOUBLE("Speaker Switch", + TAS571X_SOFT_MUTE_REG, + TAS571X_SOFT_MUTE_CH1_SHIFT, TAS571X_SOFT_MUTE_CH2_SHIFT, + 1, 1), +}; + +static const struct snd_soc_dapm_widget tas5717_dapm_widgets[] = { + SND_SOC_DAPM_DAC("DACL", NULL, SND_SOC_NOPM, 0, 0), + SND_SOC_DAPM_DAC("DACR", NULL, SND_SOC_NOPM, 0, 0), + + SND_SOC_DAPM_OUTPUT("OUT_A"), + SND_SOC_DAPM_OUTPUT("OUT_B"), + SND_SOC_DAPM_OUTPUT("OUT_C"), + SND_SOC_DAPM_OUTPUT("OUT_D"), +}; + +static const struct snd_soc_dapm_route tas5717_dapm_routes[] = { + { "DACL", NULL, "Playback" }, + { "DACR", NULL, "Playback" }, + + { "OUT_A", NULL, "DACL" }, + { "OUT_B", NULL, "DACL" }, + { "OUT_C", NULL, "DACR" }, + { "OUT_D", NULL, "DACR" }, +}; + +static const struct snd_soc_codec_driver tas571x_codec = { + .set_bias_level = tas571x_set_bias_level, + .suspend_bias_off = true, + + .dapm_widgets = tas5717_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(tas5717_dapm_widgets), + .dapm_routes = tas5717_dapm_routes, + .num_dapm_routes = ARRAY_SIZE(tas5717_dapm_routes), +}; + +static int tas571x_register_size(struct tas571x_private *priv, unsigned int reg) +{ + switch (reg) { + case TAS571X_MVOL_REG: + case TAS571X_CH1_VOL_REG: + case TAS571X_CH2_VOL_REG: + return priv->dev_id == TAS571X_ID_5711 ? 1 : 2; + default: + return 1; + } +} + +static int tas571x_reg_write(void *context, unsigned int reg, + unsigned int value) +{ + struct i2c_client *client = context; + struct tas571x_private *priv = i2c_get_clientdata(client); + unsigned int i, size; + uint8_t buf[5]; + int ret; + + size = tas571x_register_size(priv, reg); + buf[0] = reg; + + for (i = size; i >= 1; --i) { + buf[i] = value; + value >>= 8; + } + + ret = i2c_master_send(client, buf, size + 1); + if (ret == size + 1) + return 0; + else if (ret < 0) + return ret; + else + return -EIO; +} + +static int tas571x_reg_read(void *context, unsigned int reg, + unsigned int *value) +{ + struct i2c_client *client = context; + struct tas571x_private *priv = i2c_get_clientdata(client); + uint8_t send_buf, recv_buf[4]; + struct i2c_msg msgs[2]; + unsigned int size; + unsigned int i; + int ret; + + size = tas571x_register_size(priv, reg); + send_buf = reg; + + msgs[0].addr = client->addr; + msgs[0].len = sizeof(send_buf); + msgs[0].buf = &send_buf; + msgs[0].flags = 0; + + msgs[1].addr = client->addr; + msgs[1].len = size; + msgs[1].buf = recv_buf; + msgs[1].flags = I2C_M_RD; + + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + if (ret < 0) + return ret; + else if (ret != ARRAY_SIZE(msgs)) + return -EIO; + + *value = 0; + + for (i = 0; i < size; i++) { + *value <<= 8; + *value |= recv_buf[i]; + } + + return 0; +} +static const struct regmap_config tas571x_regmap = { + .reg_bits = 8, + .val_bits = 32, + .reg_read = tas571x_reg_read, + .reg_write = tas571x_reg_write, + .cache_type = REGCACHE_RBTREE, +}; + +static struct snd_soc_dai_driver tas571x_dai = { + .name = "tas571x-hifi", + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_48000, + .formats = SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S16_LE, + }, + .ops = &tas571x_dai_ops, +}; + +static int tas571x_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct tas571x_private *priv; + struct device *dev = &client->dev; + int i; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + i2c_set_clientdata(client, priv); + + priv->mclk = devm_clk_get(dev, "mclk"); + if (PTR_ERR(priv->mclk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + for (i = 0; i < TAS571X_NUM_SUPPLIES; i++) + priv->supplies[i].supply = tas571x_supply_names[i]; + + /* + * This will fall back to the dummy regulator if nothing is specified + * in DT. + */ + if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES, + priv->supplies)) { + dev_err(dev, "Failed to get supplies\n"); + return -EINVAL; + } + if (regulator_bulk_enable(TAS571X_NUM_SUPPLIES, priv->supplies)) { + dev_err(dev, "Failed to enable supplies\n"); + return -EINVAL; + } + + priv->regmap = devm_regmap_init(dev, NULL, client, &tas571x_regmap); + if (IS_ERR(priv->regmap)) + return PTR_ERR(priv->regmap); + + priv->pdn_gpio = devm_gpiod_get(dev, "pdn"); + if (!IS_ERR(priv->pdn_gpio)) { + gpiod_direction_output(priv->pdn_gpio, 1); + } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT && + PTR_ERR(priv->pdn_gpio) != -ENOSYS) { + dev_warn(dev, "error requesting pdn_gpio: %ld\n", + PTR_ERR(priv->pdn_gpio)); + } + + priv->reset_gpio = devm_gpiod_get(dev, "reset"); + if (!IS_ERR(priv->reset_gpio)) { + gpiod_direction_output(priv->reset_gpio, 0); + usleep_range(100, 200); + gpiod_set_value(priv->reset_gpio, 1); + usleep_range(12000, 20000); + } else if (PTR_ERR(priv->reset_gpio) != -ENOENT && + PTR_ERR(priv->reset_gpio) != -ENOSYS) { + dev_warn(dev, "error requesting reset_gpio: %ld\n", + PTR_ERR(priv->reset_gpio)); + } + + priv->bias_level = SND_SOC_BIAS_STANDBY; + + if (regmap_write(priv->regmap, TAS571X_OSC_TRIM_REG, 0)) + return -EIO; + + if (tas571x_set_shutdown(priv, true)) + return -EIO; + + memcpy(&priv->codec_driver, &tas571x_codec, sizeof(priv->codec_driver)); + priv->dev_id = id->driver_data; + + switch (id->driver_data) { + case TAS571X_ID_5711: + priv->codec_driver.controls = tas5711_controls; + priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls); + break; + case TAS571X_ID_5717: + case TAS571X_ID_5719: + priv->codec_driver.controls = tas5717_controls; + priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls); + + /* + * The master volume defaults to 0x3ff (mute), but we ignore + * (zero) the LSB because the hardware step size is 0.125 dB + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. + */ + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) + return -EIO; + + break; + } + + return snd_soc_register_codec(&client->dev, &priv->codec_driver, + &tas571x_dai, 1); +} + +static int tas571x_i2c_remove(struct i2c_client *client) +{ + struct tas571x_private *priv = i2c_get_clientdata(client); + + snd_soc_unregister_codec(&client->dev); + regulator_bulk_disable(TAS571X_NUM_SUPPLIES, priv->supplies); + + return 0; +} + +static const struct of_device_id tas571x_of_match[] = { + { .compatible = "ti,tas5711", }, + { .compatible = "ti,tas5717", }, + { .compatible = "ti,tas5719", }, + { } +}; +MODULE_DEVICE_TABLE(of, tas571x_of_match); + +static const struct i2c_device_id tas571x_i2c_id[] = { + { "tas5711", TAS571X_ID_5711 }, + { "tas5717", TAS571X_ID_5717 }, + { "tas5719", TAS571X_ID_5719 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, tas571x_i2c_id); + +static struct i2c_driver tas571x_i2c_driver = { + .driver = { + .name = "tas571x", + .of_match_table = of_match_ptr(tas571x_of_match), + }, + .probe = tas571x_i2c_probe, + .remove = tas571x_i2c_remove, + .id_table = tas571x_i2c_id, +}; +module_i2c_driver(tas571x_i2c_driver); + +MODULE_DESCRIPTION("ASoC TAS571x driver"); +MODULE_AUTHOR("Kevin Cernekee cernekee@chromium.org"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/tas571x.h b/sound/soc/codecs/tas571x.h new file mode 100644 index 000000000000..965b7cd90a40 --- /dev/null +++ b/sound/soc/codecs/tas571x.h @@ -0,0 +1,39 @@ +/* + * TAS571x amplifier audio driver + * + * Copyright (C) 2015 Google, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef _TAS571X_H +#define _TAS571X_H + +#include <sound/pcm.h> + +#define TAS571X_ID_5711 0x5711 +#define TAS571X_ID_5717 0x5717 +#define TAS571X_ID_5719 0x5719 + +/* device registers */ +#define TAS571X_SDI_REG 0x04 +#define TAS571X_SDI_FMT_MASK 0x0f + +#define TAS571X_SYS_CTRL_2_REG 0x05 +#define TAS571X_SYS_CTRL_2_SDN_MASK 0x40 + +#define TAS571X_SOFT_MUTE_REG 0x06 +#define TAS571X_SOFT_MUTE_CH1_SHIFT 0 +#define TAS571X_SOFT_MUTE_CH2_SHIFT 1 +#define TAS571X_SOFT_MUTE_CH3_SHIFT 2 + +#define TAS571X_MVOL_REG 0x07 +#define TAS571X_CH1_VOL_REG 0x08 +#define TAS571X_CH2_VOL_REG 0x09 + +#define TAS571X_OSC_TRIM_REG 0x1b + +#endif /* _TAS571X_H */
On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
Introduce a new codec driver for the Texas Instruments TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically used to take an I2S digital audio input and drive 10-20W into a pair of speakers.
Signed-off-by: Kevin Cernekee cernekee@chromium.org
Looks pretty good. Comments inlune.
[...]
-obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
Accidentally removed line
+obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o
[...]
+++ b/sound/soc/codecs/tas571x.c @@ -0,0 +1,456 @@ +/*
- TAS571x amplifier audio driver
- Copyright (C) 2015 Google, Inc.
- Copyright (c) 2013 Daniel Mack zonque@gmail.com
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- */
+#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_gpio.h>
There is no of specific GPIO code in the driver.
[...]
+static int tas571x_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level)+{
- struct tas571x_private *priv = snd_soc_codec_get_drvdata(codec);
- int ret, assert_pdn = 0;
- if (priv->bias_level == level)
return 0;
The core already takes care that this function is only called if there is a actual change.
- switch (level) {
- case SND_SOC_BIAS_PREPARE:
if (!IS_ERR(priv->mclk)) {ret = clk_prepare_enable(priv->mclk);if (ret) {dev_err(codec->dev,"Failed to enable master clock\n");return ret;}}ret = tas571x_set_shutdown(priv, false);if (ret)return ret;break;- case SND_SOC_BIAS_STANDBY:
ret = tas571x_set_shutdown(priv, true);if (ret)return ret;if (!IS_ERR(priv->mclk))clk_disable_unprepare(priv->mclk);break;- case SND_SOC_BIAS_ON:
break;- case SND_SOC_BIAS_OFF:
/* Note that this kills I2C accesses. */assert_pdn = 1;break;- }
- if (!IS_ERR(priv->pdn_gpio))
gpiod_set_value(priv->pdn_gpio, !assert_pdn);- priv->bias_level = level;
This should update codec->dapm.bias_level, otherwise the core will become confused about the actual level.
- return 0;
+}
[...]
+static const unsigned int tas5711_volume_tlv[] = {
- TLV_DB_RANGE_HEAD(1),
- 0, 0xff, TLV_DB_SCALE_ITEM(-10350, 50, 1),
+};
For TLVs with a single item use DECLARE_TLV_DB_SCALE()
[...]
+static const unsigned int tas5717_volume_tlv[] = {
- TLV_DB_RANGE_HEAD(1),
- 0x000, 0x1ff, TLV_DB_SCALE_ITEM(-10375, 25, 0),
+};
Same here.
[...]
+static int tas571x_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)+{
- struct tas571x_private *priv;
- struct device *dev = &client->dev;
- int i;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- i2c_set_clientdata(client, priv);
- priv->mclk = devm_clk_get(dev, "mclk");
- if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
If you want to make the clock optional use
if (PTR_ERR(priv->mclk) == -ENOENT) return PTR_ERR(priv->mclk);
This makes sure that the case where the clock is specified, but there is a error with the specification (e.g. incorrect DT cells) is handled properly.
- for (i = 0; i < TAS571X_NUM_SUPPLIES; i++)
priv->supplies[i].supply = tas571x_supply_names[i];- /*
* This will fall back to the dummy regulator if nothing is specified* in DT.*/- if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES,
priv->supplies)) {
Move the function outside the if condition and also pass the error condition to the caller. (And print it)
dev_err(dev, "Failed to get supplies\n");return -EINVAL;- }
- if (regulator_bulk_enable(TAS571X_NUM_SUPPLIES, priv->supplies)) {
Same here.
dev_err(dev, "Failed to enable supplies\n");return -EINVAL;- }
- priv->regmap = devm_regmap_init(dev, NULL, client, &tas571x_regmap);
- if (IS_ERR(priv->regmap))
return PTR_ERR(priv->regmap);- priv->pdn_gpio = devm_gpiod_get(dev, "pdn");
devm_gpiod_get_optional() ?
Using gpiod_get without specifying the direction flags is deprecated. Should be
... = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
and then drop the gpiod_direction_output().
- if (!IS_ERR(priv->pdn_gpio)) {
gpiod_direction_output(priv->pdn_gpio, 1);- } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT &&
PTR_ERR(priv->pdn_gpio) != -ENOSYS) {dev_warn(dev, "error requesting pdn_gpio: %ld\n",PTR_ERR(priv->pdn_gpio));
If the GPIO can't be requested and it is not a optional GPIO that should be treated as an error.
- }
- priv->reset_gpio = devm_gpiod_get(dev, "reset");
Same as for the pdn_gpio.
- if (!IS_ERR(priv->reset_gpio)) {
gpiod_direction_output(priv->reset_gpio, 0);usleep_range(100, 200);gpiod_set_value(priv->reset_gpio, 1);usleep_range(12000, 20000);- } else if (PTR_ERR(priv->reset_gpio) != -ENOENT &&
PTR_ERR(priv->reset_gpio) != -ENOSYS) {dev_warn(dev, "error requesting reset_gpio: %ld\n",PTR_ERR(priv->reset_gpio));
Same as for the pdn_gpio.
- }
- priv->bias_level = SND_SOC_BIAS_STANDBY;
- if (regmap_write(priv->regmap, TAS571X_OSC_TRIM_REG, 0))
return -EIO;- if (tas571x_set_shutdown(priv, true))
return -EIO;- memcpy(&priv->codec_driver, &tas571x_codec, sizeof(priv->codec_driver));
- priv->dev_id = id->driver_data;
- switch (id->driver_data) {
- case TAS571X_ID_5711:
priv->codec_driver.controls = tas5711_controls;priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls);break;- case TAS571X_ID_5717:
- case TAS571X_ID_5719:
priv->codec_driver.controls = tas5717_controls;priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls);/** The master volume defaults to 0x3ff (mute), but we ignore* (zero) the LSB because the hardware step size is 0.125 dB* and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.*/if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))return -EIO;break;- }
Typically when a driver supports multiple chips with different control sets the snd_soc_codec_driver implements a probe callback in which the correct controls are registered.
- return snd_soc_register_codec(&client->dev, &priv->codec_driver,
&tas571x_dai, 1);+}
[...]
+static const struct of_device_id tas571x_of_match[] = {
- { .compatible = "ti,tas5711", },
- { .compatible = "ti,tas5717", },
- { .compatible = "ti,tas5719", },
You should also specify the id data for the of table and get it from the of_data if of_node is non NULL in the probe function. I know that it works without, but that is a bit of a unintentional side-effect and might change in the future.
- { }
+}; +MODULE_DEVICE_TABLE(of, tas571x_of_match);
On Thu, Apr 16, 2015 at 02:57:49PM +0200, Lars-Peter Clausen wrote:
On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
- case TAS571X_ID_5711:
priv->codec_driver.controls = tas5711_controls;priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls);break;- case TAS571X_ID_5717:
- case TAS571X_ID_5719:
priv->codec_driver.controls = tas5717_controls;priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls);
Typically when a driver supports multiple chips with different control sets the snd_soc_codec_driver implements a probe callback in which the correct controls are registered.
I'm fine with doing it with tables (though just having two static CODEC driver structures would be a bit cleaner). The pattern with probe() is usually that there's some base set of controls all the devices have which then gets device specific controls/routes/whatever added to it so you get benefits fromm sharing the table but in this case the table is so tiny anyway that I'm not sure it's worth caring.
On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen lars@metafoo.de wrote:
On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
Introduce a new codec driver for the Texas Instruments TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically used to take an I2S digital audio input and drive 10-20W into a pair of speakers.
Signed-off-by: Kevin Cernekee cernekee@chromium.org
Looks pretty good. Comments inlune.
Thanks for the review. I'm working on a V2 incorporating the feedback from you and Mark.
+static int tas571x_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)+{
struct tas571x_private *priv;struct device *dev = &client->dev;int i;priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);if (!priv)return -ENOMEM;i2c_set_clientdata(client, priv);priv->mclk = devm_clk_get(dev, "mclk");if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)return -EPROBE_DEFER;If you want to make the clock optional use
if (PTR_ERR(priv->mclk) == -ENOENT) return PTR_ERR(priv->mclk);This makes sure that the case where the clock is specified, but there is a error with the specification (e.g. incorrect DT cells) is handled properly.
Did you mean:
if (PTR_ERR(priv->mclk) != -ENOENT) return PTR_ERR(priv->mclk);
I don't see this in other codec drivers, but I do see the explicit EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960.
On Mon, Apr 20, 2015 at 01:56:46PM -0700, Kevin Cernekee wrote:
On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen lars@metafoo.de wrote:
On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
If you want to make the clock optional use
if (PTR_ERR(priv->mclk) == -ENOENT) return PTR_ERR(priv->mclk);
This makes sure that the case where the clock is specified, but there is a error with the specification (e.g. incorrect DT cells) is handled properly.
Did you mean:
if (PTR_ERR(priv->mclk) != -ENOENT) return PTR_ERR(priv->mclk);
I don't see this in other codec drivers, but I do see the explicit EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960.
That's the most correct way of writing a check for an optional clock, best practice on this stuff is evolving over time (as is standardization in the clock API).
On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote:
This looks mostly good but several things below, all of which should be straightforward to fix.
+static int tas571x_set_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)+{
- /*
* TAS5717 datasheet pg 21: "The DAP can autodetect and set the* internal clock-control logic to the appropriate settings for all* supported clock rates as defined in the clock control register."*/- return 0;
+}
Remove empty functions, at best they waste space at worst they break things.
- val += (clamp(params_width(params), 16, 24) >> 2) - 4;
Please write this more clearly or comment it (preferably the former), it's hard to tell what it's supposed to do and therefore hard to tell if it's doing it correctly.
+static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown) +{
- return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG,
TAS571X_SYS_CTRL_2_SDN_MASK,is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0);+}
ret = tas571x_set_shutdown(priv, false);if (ret)return ret;break;- case SND_SOC_BIAS_STANDBY:
ret = tas571x_set_shutdown(priv, true);if (ret)return ret;
This looks like it'd be clearer just as direct register updates, I'm not sure a function to set a single bit is addinng much.
- case SND_SOC_BIAS_OFF:
/* Note that this kills I2C accesses. */assert_pdn = 1;
No, the GPIO set associated with it kills I2C access. I'd also expect to see the regmap being marked cache only before we do this and a resync of the register map when we power back up (assuming that is actually a power down).
+static const struct snd_kcontrol_new tas5711_controls[] = {
- SOC_SINGLE_TLV("Master Volume",
TAS571X_MVOL_REG,0, 0xff, 1, tas5711_volume_tlv),
All these controls will be brokenn if the I2C access goes away.
+static const struct snd_soc_codec_driver tas571x_codec = {
- .set_bias_level = tas571x_set_bias_level,
- .suspend_bias_off = true,
Why not idle_bias_off? It looks like power up takes no meaningful time.
+static int tas571x_register_size(struct tas571x_private *priv, unsigned int reg) +{
- switch (reg) {
- case TAS571X_MVOL_REG:
- case TAS571X_CH1_VOL_REG:
- case TAS571X_CH2_VOL_REG:
return priv->dev_id == TAS571X_ID_5711 ? 1 : 2;
Nest switch statements please, that way things work better if another variant turns up.
- /*
* This will fall back to the dummy regulator if nothing is specified* in DT.*/
The driver doesn't care, it may not even be on a system using DT.
- if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES,
priv->supplies)) {dev_err(dev, "Failed to get supplies\n");return -EINVAL;- }
Don't discard error codes from functions you call, log them and provide them to calllers. The above is broken for probe deferral for example.
- priv->pdn_gpio = devm_gpiod_get(dev, "pdn");
- if (!IS_ERR(priv->pdn_gpio)) {
gpiod_direction_output(priv->pdn_gpio, 1);- } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT &&
PTR_ERR(priv->pdn_gpio) != -ENOSYS) {dev_warn(dev, "error requesting pdn_gpio: %ld\n",PTR_ERR(priv->pdn_gpio));- }
This should at least be handling probe deferral, it's not clear why it doesn't just error out in the cases where it gets an error. Similarly for the reset GPIO.
/** The master volume defaults to 0x3ff (mute), but we ignore* (zero) the LSB because the hardware step size is 0.125 dB* and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.*/if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))return -EIO;
I don't understand this - is the LSB a mute bit or sommething?
+#ifndef _TAS571X_H +#define _TAS571X_H
+#include <sound/pcm.h>
Why is this needed in the header?
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown broonie@kernel.org wrote:
On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote:
+static int tas571x_set_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)+{
/** TAS5717 datasheet pg 21: "The DAP can autodetect and set the* internal clock-control logic to the appropriate settings for all* supported clock rates as defined in the clock control register."*/return 0;+}
Remove empty functions, at best they waste space at worst they break things.
Without the empty function, we run into problems with drivers that abort when they get -ENOTSUPP here:
sound/soc/atmel/atmel_wm8904.c: ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set wm8904 SYSCLK\n", __func__); sound/soc/atmel/atmel_wm8904.c- return ret; sound/soc/atmel/atmel_wm8904.c- }
Is there a stub version that I can use instead? Nothing jumped out at me when looking at the other codec drivers.
+static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown) +{
return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG,TAS571X_SYS_CTRL_2_SDN_MASK,is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0);+}
ret = tas571x_set_shutdown(priv, false);if (ret)return ret;break;case SND_SOC_BIAS_STANDBY:ret = tas571x_set_shutdown(priv, true);if (ret)return ret;This looks like it'd be clearer just as direct register updates, I'm not sure a function to set a single bit is addinng much.
It might be useful if another tas571x variant put the bit somewhere else, but that hasn't happened yet so I can nuke the helper function for now.
case SND_SOC_BIAS_OFF:/* Note that this kills I2C accesses. */assert_pdn = 1;No, the GPIO set associated with it kills I2C access. I'd also expect to see the regmap being marked cache only before we do this and a resync of the register map when we power back up (assuming that is actually a power down).
Hmm, not sure if this actually resets the registers back to power-on defaults, but I'll check.
/** The master volume defaults to 0x3ff (mute), but we ignore* (zero) the LSB because the hardware step size is 0.125 dB* and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.*/if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))return -EIO;I don't understand this - is the LSB a mute bit or sommething?
The 10-bit master volume field on 5717/5719 works like:
0x3ff: MUTE (power-on default) 0x3fe: -103.750 dB 0x3fd: -103.625 dB [lots more options, in 0.125 dB increments] 0x001: 23.875 dB 0x000: 24.000 dB
Since we only have a resolution of 0.01 dB, the driver forces the LSB to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled through the dedicated per-channel soft mute register bits instead of the 0x3ff volume setting.
On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown broonie@kernel.org wrote:
+static int tas571x_set_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)
Remove empty functions, at best they waste space at worst they break things.
Without the empty function, we run into problems with drivers that abort when they get -ENOTSUPP here:
sound/soc/atmel/atmel_wm8904.c: ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set wm8904 SYSCLK\n", __func__); sound/soc/atmel/atmel_wm8904.c- return ret; sound/soc/atmel/atmel_wm8904.c- }
Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince...
Is there a stub version that I can use instead? Nothing jumped out at me when looking at the other codec drivers.
No, such a stub would make no sense - why would we put a stub in all the drivers rather than just making the core do the right thing?
/** The master volume defaults to 0x3ff (mute), but we ignore* (zero) the LSB because the hardware step size is 0.125 dB* and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.*/if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))return -EIO;
I don't understand this - is the LSB a mute bit or sommething?
The 10-bit master volume field on 5717/5719 works like:
0x3ff: MUTE (power-on default) 0x3fe: -103.750 dB 0x3fd: -103.625 dB [lots more options, in 0.125 dB increments] 0x001: 23.875 dB 0x000: 24.000 dB
Since we only have a resolution of 0.01 dB, the driver forces the LSB to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled through the dedicated per-channel soft mute register bits instead of the 0x3ff volume setting.
It's not entirely clear to me why we need to reset the bit, or why if we're just trying to update that one bit we write the entire register value rather than use _update_bits(). If the goal is just to change that one bit then _update_bits() would be a lot clearer.
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown broonie@kernel.org wrote:
On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown broonie@kernel.org wrote:
+static int tas571x_set_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)Remove empty functions, at best they waste space at worst they break things.
Without the empty function, we run into problems with drivers that abort when they get -ENOTSUPP here:
sound/soc/atmel/atmel_wm8904.c: ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set wm8904 SYSCLK\n", __func__); sound/soc/atmel/atmel_wm8904.c- return ret; sound/soc/atmel/atmel_wm8904.c- }
Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince...
The same check shows up in numerous other drivers, including the one for the audio controller on my board.
Is there a stub version that I can use instead? Nothing jumped out at me when looking at the other codec drivers.
No, such a stub would make no sense - why would we put a stub in all the drivers rather than just making the core do the right thing?
AFAICT, implementing the set_sysclk callback is mandatory, even if it is a no-op on the codec side. If I delete the stub function, audio playback fails.
/** The master volume defaults to 0x3ff (mute), but we ignore* (zero) the LSB because the hardware step size is 0.125 dB* and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.*/if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))return -EIO;I don't understand this - is the LSB a mute bit or sommething?
The 10-bit master volume field on 5717/5719 works like:
0x3ff: MUTE (power-on default) 0x3fe: -103.750 dB 0x3fd: -103.625 dB [lots more options, in 0.125 dB increments] 0x001: 23.875 dB 0x000: 24.000 dB
Since we only have a resolution of 0.01 dB, the driver forces the LSB to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled through the dedicated per-channel soft mute register bits instead of the 0x3ff volume setting.
It's not entirely clear to me why we need to reset the bit, or why if we're just trying to update that one bit we write the entire register value rather than use _update_bits(). If the goal is just to change that one bit then _update_bits() would be a lot clearer.
The default master volume setting on the TAS5717/5719 is 0x3ff (bits 9:0). We only ever update bits 9:1 when the volume changes, because the driver is only able to work with 0.25 dB resolution rather than the hardware's native 0.125 dB resolution. So this register write sets the master volume to an even number, which we know we can handle.
Clearing just the LSB would accomplish the same thing, but would be less obvious IMO. It would also require an extra read, and the code is less concise.
Is there a better way to handle this situation?
On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown broonie@kernel.org wrote:
Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince...
The same check shows up in numerous other drivers, including the one for the audio controller on my board.
Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately.
Is there a stub version that I can use instead? Nothing jumped out at me when looking at the other codec drivers.
No, such a stub would make no sense - why would we put a stub in all the drivers rather than just making the core do the right thing?
AFAICT, implementing the set_sysclk callback is mandatory, even if it is a no-op on the codec side. If I delete the stub function, audio playback fails.
For the reasons I mentioned above having a set_sysclk() function is not mandatory and your driver will not be merged with a stub such as is currently present. As far as I can tell you are trying to bodge around some problem elsewhere in either the code or your usage of it.
Clearing just the LSB would accomplish the same thing, but would be less obvious IMO. It would also require an extra read, and the code is less concise.
I don't think anyone is going to care about an extra read on system init, and in any case if the driver followed best practice and provided register defaults that read would be satisfied from cache.
On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown broonie@kernel.org wrote:
On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown broonie@kernel.org wrote:
Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince...
The same check shows up in numerous other drivers, including the one for the audio controller on my board.
Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately.
I don't think this driver has been posted on the list yet, but the checks show up in these two functions:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3... https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
Any suggestions on what to change?
Hi Kevin,
On Mon, Apr 20, 2015 at 8:12 AM, Kevin Cernekee cernekee@chromium.org wrote:
On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown broonie@kernel.org wrote:
On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown broonie@kernel.org wrote:
Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince...
The same check shows up in numerous other drivers, including the one for the audio controller on my board.
Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately.
I don't think this driver has been posted on the list yet, but the checks show up in these two functions:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3... https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
Any suggestions on what to change?
I think the card driver should be ignoring a return value of -ENOTSUPP from snd_soc_dai_set_sysclk() in that case.
On Mon, Apr 20, 2015 at 08:12:36AM -0700, Kevin Cernekee wrote:
On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown broonie@kernel.org wrote:
The same check shows up in numerous other drivers, including the one for the audio controller on my board.
Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately.
I don't think this driver has been posted on the list yet, but the checks show up in these two functions:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3... https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
Any suggestions on what to change?
That driver has not been posted previously (and looks like it could use some rework for mainline). If it doesn't care if it set the clock and is just trying for information it should be handling -ENOTSUPP, that's why the core returns that value.
On Sat, Apr 18, 2015 at 9:16 AM, Kevin Cernekee cernekee@chromium.org wrote:
case SND_SOC_BIAS_OFF:/* Note that this kills I2C accesses. */assert_pdn = 1;No, the GPIO set associated with it kills I2C access. I'd also expect to see the regmap being marked cache only before we do this and a resync of the register map when we power back up (assuming that is actually a power down).
Hmm, not sure if this actually resets the registers back to power-on defaults, but I'll check.
Hi Mark,
I have reworked the driver to do the following:
- set appropriate regmap default values on probe - enable idle_bias_off - use regcache_cache_only() to prevent accesses to I2C when in SND_SOC_BIAS_OFF state (pdn asserted) - use regcache_sync() when transitioning from SND_SOC_BIAS_OFF -> SND_SOC_BIAS_STANDBY
This is mostly working OK, but regcache_sync() assumes that the hardware registers have been reset back to the default values. The "pdn" GPIO doesn't actually reset the state of the tas571x; it just makes I2C inaccessible and inhibits audio output. So if the factory default for mute is 0, corner cases like this fail:
- enter SND_SOC_BIAS_ON (e.g. play a wav file) - set mute to 1 - enter SND_SOC_BIAS_OFF (e.g. playback ends) - set mute to 0 - re-enter SND_SOC_BIAS_ON - regcache_sync() incorrectly assumes that the hardware register is already 0, but in fact it needs to be refreshed from the cache
Aside from unnecessarily pulsing the reset GPIO when transitioning back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can you think of a way to work around this?
Thanks.
On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote:
This is mostly working OK, but regcache_sync() assumes that the hardware registers have been reset back to the default values. The "pdn" GPIO doesn't actually reset the state of the tas571x; it just makes I2C inaccessible and inhibits audio output. So if the factory default for mute is 0, corner cases like this fail:
...
Aside from unnecessarily pulsing the reset GPIO when transitioning back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can you think of a way to work around this?
Do you need to work around it? If the register map is being perserved you don't need to sync so just don't do it - it's just that the normal expectation would be that power down would cause the register map to be reset.
On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown broonie@kernel.org wrote:
On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote:
This is mostly working OK, but regcache_sync() assumes that the hardware registers have been reset back to the default values. The "pdn" GPIO doesn't actually reset the state of the tas571x; it just makes I2C inaccessible and inhibits audio output. So if the factory default for mute is 0, corner cases like this fail:
...
Aside from unnecessarily pulsing the reset GPIO when transitioning back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can you think of a way to work around this?
Do you need to work around it? If the register map is being perserved you don't need to sync so just don't do it - it's just that the normal expectation would be that power down would cause the register map to be reset.
How do I tell regcache to write out any updates that happened while the hardware was inaccessible? I see that regmap->cache_dirty is 1, but nothing flushes it automatically when exiting cache_only mode.
On Fri, Apr 24, 2015 at 06:52:01AM -0700, Kevin Cernekee wrote:
On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown broonie@kernel.org wrote:
Do you need to work around it? If the register map is being perserved you don't need to sync so just don't do it - it's just that the normal expectation would be that power down would cause the register map to be reset.
How do I tell regcache to write out any updates that happened while the hardware was inaccessible? I see that regmap->cache_dirty is 1, but nothing flushes it automatically when exiting cache_only mode.
Oh, I see. A sync will be required, yes. Probably resetting the device is easiest.
Add self as maintainer for the new driver.
Signed-off-by: Kevin Cernekee cernekee@chromium.org --- MAINTAINERS | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index c72a7baec55c..d7d848f840d0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9847,6 +9847,12 @@ L: netdev@vger.kernel.org S: Maintained F: drivers/net/ethernet/ti/netcp*
+TI TAS571X FAMILY ASoC CODEC DRIVER +M: Kevin Cernekee cernekee@chromium.org +L: alsa-devel@alsa-project.org (moderated for non-subscribers) +S: Odd Fixes +F: sound/soc/codecs/tas571x* + TI TWL4030 SERIES SOC CODEC DRIVER M: Peter Ujfalusi peter.ujfalusi@ti.com L: alsa-devel@alsa-project.org (moderated for non-subscribers)
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
+- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical supplies there should be three separate regulators requested. They may all resolve to one physical regulator on the board you are working with but that might not be true on other boards.
Resending from the correct account.
On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown broonie@kernel.org wrote:
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
+- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical supplies there should be three separate regulators requested. They may all resolve to one physical regulator on the board you are working with but that might not be true on other boards.
In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply:
http://www.ti.com/lit/ds/symlink/tas5717.pdf#2
Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD.
But this combines a number of separate pins. On 5711 we have dedicated pins for:
PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD
On 5717 we have dedicated pins for:
PVDD_AB PVDD_CD AVDD DVDD HPVDD
I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD).
I can add as many regulator entries as appropriate. What do you recommend?
On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown broonie@kernel.org wrote:
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
+- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical supplies there should be three separate regulators requested. They may all resolve to one physical regulator on the board you are working with but that might not be true on other boards.
In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply:
Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD.
But this combines a number of separate pins. On 5711 we have dedicated pins for:
PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD
Yes, those are three separate supplies that are typically tied together - it looks like PVDD has high current draw so is tied through multiple pins. I'd not be surprised to see systems with AVDD tied to a separate pin, analogue supplies often benefit from low noise supplies separate to digital ones. Indeed if you look at the pin descriptions the analogue and digital supplies even have separate grounds.
I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD).
That's more likely to go wrong if they're not controlled separately than if they are since we will only be able to turn a single supply on or off..
I can add as many regulator entries as appropriate. What do you recommend?
Have the driver accurately reflect the hardware, request one regulator per supply.
On Mon, Apr 20, 2015 at 3:03 PM, Mark Brown broonie@kernel.org wrote:
On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote:
On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown broonie@kernel.org wrote:
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
+- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical supplies there should be three separate regulators requested. They may all resolve to one physical regulator on the board you are working with but that might not be true on other boards.
In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply:
Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD.
But this combines a number of separate pins. On 5711 we have dedicated pins for:
PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD
Yes, those are three separate supplies that are typically tied together
- it looks like PVDD has high current draw so is tied through multiple
pins.
Each half-bridge output OUT_{A,B,C,D} has its own dedicated PVDD_{A,B,C,D} supply on 5711:
http://www.ti.com/lit/ds/symlink/tas5711.pdf#7
I don't understand the statement: "those are three separate supplies that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can never be driven by the same regulator output as PVDD. Perhaps you were thinking of HPVDD/AVDD/DVDD on 5717/5719?
The simplified application diagram on page 2 does show that AVDD/DVDD are typically tied together, and that PVDD_* are also typically tied together. But as you pointed out, there may be situations where that isn't the case, so that leaves us with up to 6 separate supplies on 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or 3, though.
I'd not be surprised to see systems with AVDD tied to a separate pin, analogue supplies often benefit from low noise supplies separate to digital ones. Indeed if you look at the pin descriptions the analogue and digital supplies even have separate grounds.
I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD).
That's more likely to go wrong if they're not controlled separately than if they are since we will only be able to turn a single supply on or off..
I can add as many regulator entries as appropriate. What do you recommend?
Have the driver accurately reflect the hardware, request one regulator per supply.
So, request one regulator per VDD pin? Or group all of PVDD_* into a single regulator?
On Mon, Apr 20, 2015 at 03:48:19PM -0700, Kevin Cernekee wrote:
I don't understand the statement: "those are three separate supplies that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can never be driven by the same regulator output as PVDD. Perhaps you were thinking of HPVDD/AVDD/DVDD on 5717/5719?
Yes, all the supplies. I'd misremembered which ones you were saying were tied together on your board.
The simplified application diagram on page 2 does show that AVDD/DVDD are typically tied together, and that PVDD_* are also typically tied together. But as you pointed out, there may be situations where that isn't the case, so that leaves us with up to 6 separate supplies on 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or 3, though.
To repeat, the typical case is *not* *relevant*. All that matters is the set of supplies the device has.
Have the driver accurately reflect the hardware, request one regulator per supply.
So, request one regulator per VDD pin? Or group all of PVDD_* into a single regulator?
Please group them into supplies as documented in the datasheet. If they are documented as distinct things they should be distinct things, if they are just the same supply coming in through multiple pins for power reasons they should be one supply.
participants (4)
-
Andrew Bresticker -
Kevin Cernekee -
Lars-Peter Clausen -
Mark Brown