Re: [PATCH 1/2] ASoC: add support for TAS5805M digital amplifier

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4805 bytes --]

On Tue, Jan 11, 2022 at 12:53:11PM +1300, Daniel Beer wrote:

> +++ b/sound/soc/codecs/tas5805m.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the TAS5805M Audio Amplifier
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static void tas5805m_refresh_unlocked(struct snd_soc_component *component)
> +{
> +	struct tas5805m_priv *tas5805m =
> +		snd_soc_component_get_drvdata(component);
> +	uint8_t v[4];
> +	unsigned int i;
> +	uint32_t x;

> +	snd_soc_component_write(component, REG_PAGE, 0x00);
> +	snd_soc_component_write(component, REG_BOOK, 0x8c);
> +	snd_soc_component_write(component, REG_PAGE, 0x2a);

This isn't using the regmap paging support and I'm not seeing anything
that resets the page here.

> +	for (i = 0; i < 4; i++)
> +		snd_soc_component_write(component, 0x24 + i, v[i]);
> +	for (i = 0; i < 4; i++)
> +		snd_soc_component_write(component, 0x28 + i, v[i]);

This looks like it's potentially a stereo control but we're not allowing
the two channels to be configured separately?

> +	/* Volume controls */
> +	snd_soc_component_write(component, REG_DEVICE_CTRL_2,
> +		tas5805m->is_muted ?  0x0b : 0x03);

This comment doesn't seem terribly descriptive and this is very much a
magic number.

> +static int tas5805m_vol_get(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct tas5805m_priv *tas5805m =
> +		snd_soc_component_get_drvdata(component);
> +
> +	mutex_lock(&tas5805m->lock);
> +	ucontrol->value.integer.value[0] = tas5805m->vol;
> +	mutex_unlock(&tas5805m->lock);

What is this lock intended to protect?  We take the lock, do a read of a
single value and then unlock which doesn't seem like it's adding a huge
amount.

> +
> +	mutex_lock(&tas5805m->lock);
> +	tas5805m->vol = clamp((int)ucontrol->value.integer.value[0],
> +			      TAS5805M_VOLUME_MIN, TAS5805M_VOLUME_MAX);

It would be better to reject out of bounds values with an error.

> +static void send_cfg(struct snd_soc_component *component,
> +		     const uint8_t *s, unsigned int len)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i + 1 < len; i += 2)
> +		snd_soc_component_write(component, s[i], s[i + 1]);

This looks like the driver might be happier using regmap directly, this
looks like an open coded _multi_reg_write().

> +/* The TAS5805M can't be configured or brought out of power-down without
> + * an I2S clock. In power-down, registers are reset.
> + *
> + * We rely on DAPM not powering up the DAC widget until the source for
> + * it is ready, which we think implies that the I2S clock is present and
> + * stable.
> + */

That's not true, we run DAPM in prepare() prior to calling trigger() and
controllers might not start clocking the bus until trigger() has been
called.  There was some other device with similar issues which was
scheduling things from the trigger() callback IIRC, if you search around
in the CODEC directory you should be able to find something.

> +	dev_set_drvdata(dev, tas5805m);
> +	tas5805m->regmap = regmap;
> +	tas5805m->gpio_pdn_n = of_get_named_gpio(dev->of_node, "pdn-gpio", 0);
> +	if (!gpio_is_valid(tas5805m->gpio_pdn_n)) {
> +		dev_err(dev, "power-down GPIO not specified\n");
> +		return -EINVAL;
> +	}

Use the gpiod APIs, you shouldn't need to do anything specific to OF
here.  Also GPIO properties need to have names ending -gpios even if
they're for a single GPIO.

> +	ret = devm_snd_soc_register_component(dev, &soc_codec_dev_tas5805m,
> +					      &tas5805m_dai, 1);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register codec: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(tas5805m->pvdd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable pvdd: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(100000, 150000);
> +	gpio_set_value(tas5805m->gpio_pdn_n, 1);
> +	usleep_range(10000, 15000);

This seems broken - the card might be instantiated and userspace start
using it as soon as the component is registered but we don't power on
the device until after registering it and there's no runtime power up or
down.  For power management like this you need to complete the initial
power on prior to registering the component.

> +static int tas5805m_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct tas5805m_priv *tas5805m = dev_get_drvdata(&i2c->dev);
> +
> +	gpio_set_value(tas5805m->gpio_pdn_n, 0);
> +	usleep_range(10000, 15000);
> +	regulator_disable(tas5805m->pvdd);

This will power down the device prior to unregistering the component so
userspace could still be running.  You need to not use devm_ for
component registration and manually unregister here to avoid that issue
(or register a custom devm action).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]