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:
signature.asc
Description: Digital signature
