lkml.org 
[lkml]   [2015]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
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?
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2015-04-18 14:01    [W:0.121 / U:0.780 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site