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

[Mark Brown <broonie@kernel.org>]

[-- 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 --]