lkml.org 
[lkml]   [2015]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
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);


\
 
 \ /
  Last update: 2015-04-16 15:21    [W:0.549 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site