[PATCH 1/3] ASoC: tas571x: Add DT binding document

[PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

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";
+	};
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

Document the bindings for the soon-to-be-added tas571x driver.

Signed-off-by: Kevin Cernekee <cernekee-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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";
+	};
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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 */
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/3] MAINTAINERS: Add entry for tas571x ASoC codec driver

[Kevin Cernekee]

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)
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/3] MAINTAINERS: Add entry for tas571x ASoC codec driver

[Kevin Cernekee]

Add self as maintainer for the new driver.

Signed-off-by: Kevin Cernekee <cernekee-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 S:	Maintained
 F:	drivers/net/ethernet/ti/netcp*
 
+TI TAS571X FAMILY ASoC CODEC DRIVER
+M:	Kevin Cernekee <cernekee-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
+L:	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
+S:	Odd Fixes
+F:	sound/soc/codecs/tas571x*
+
 TI TWL4030 SERIES SOC CODEC DRIVER
 M:	Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
 L:	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Lars-Peter Clausen]

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);

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Lars-Peter Clausen]

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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * 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);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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.

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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?

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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.14/sound/soc/img/concerto-audio.c#108
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147

Any suggestions on what to change?

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.14/sound/soc/img/concerto-audio.c#108
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147

Any suggestions on what to change?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Andrew Bresticker]

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.14/sound/soc/img/concerto-audio.c#108
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147
>
> 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.

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.14/sound/soc/img/concerto-audio.c#108
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147

> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.14/sound/soc/img/concerto-audio.c#108
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147

> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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.

Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

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?

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

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?

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Mark Brown]

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

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:

> 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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Kevin Cernekee]

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:
>
>> 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
>
> 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?

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] ASoC: tas571x: Add DT binding document

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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.

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

On Sat, Apr 18, 2015 at 9:16 AM, Kevin Cernekee <cernekee-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Kevin Cernekee]

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.

Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers

[Mark Brown]

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

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]