Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
From: Lars-Peter Clausen
Date: Thu Apr 16 2015 - 08:58:00 EST
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@xxxxxxxxxxxx>
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@xxxxxxxxx>
+ *
+ * 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/