Re: [v3 2/2] ASoC: max98520: add max98520 audio amplifier driver

[Mark Brown <broonie@kernel.org>]

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

On Mon, Oct 18, 2021 at 05:35:54PM +0900, George Song wrote:

> +	case SND_SOC_DAPM_POST_PMD:
> +		dev_dbg(component->dev, " AMP OFF\n");
> +
> +		regmap_write(max98520->regmap, MAX98520_R210F_GLOBAL_EN, 0);
> +		usleep_range(30000, 31000);
> +		max98520->tdm_mode = false;
> +		break;

Why would stopping the DAC put us out of TDM mode?  Not that I can see
anything which ever sets tdm_mode to anything other than false or checks
the value...

> +static const struct snd_soc_dapm_widget max98520_dapm_widgets[] = {
> +	SND_SOC_DAPM_DAC_E("Amp Enable", "HiFi Playback",
> +			   MAX98520_R209F_AMP_EN, 0, 0, max98520_dac_event,
> +	SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),

I can't help think that the global enable ought to be a _SUPPLY widget,
it would get enabled before the DAC rather than after it but it's not
clear that the ordering is critical here.

> +static DECLARE_TLV_DB_SCALE(max98520_digital_tlv, -6300, 50, 1);
> +static const DECLARE_TLV_DB_RANGE(max98520_spk_tlv,
> +	0, 5, TLV_DB_SCALE_ITEM(600, 300, 0),
> +);

Why is _digital_tlv not const?  It's also a bit weird that _spk_tlv is a
range with one entry not a scale.

> +	count = 0;
> +	while (count < 3) {
> +		usleep_range(10000, 11000);
> +		/* Software Reset Verification */
> +		ret = regmap_read(max98520->regmap, MAX98520_R21FF_REVISION_ID, &reg);
> +		if (!ret) {
> +			dev_info(dev, "Reset completed (retry:%d)\n", count);
> +			return;
> +		}
> +		count++;

Does this really need to be logged?

> +	/* Software Reset */
> +	max98520_reset(max98520, component->dev);
> +	usleep_range(30000, 31000);

Shouldn't that delay be in the reset routine?  Perhaps between the
attempts to read the ID register?

> +	/* L/R mix configuration */
> +	regmap_write(max98520->regmap, MAX98520_R2043_PCM_RX_SRC1, 0x2);
> +
> +	regmap_write(max98520->regmap, MAX98520_R2044_PCM_RX_SRC2, 0x10);

These should be exposed to the user, not hard coded - different systems
may need different configurations.

> +	/* Disable Speaker Safe Mode */
> +	regmap_update_bits(max98520->regmap,
> +			   MAX98520_R2092_AMP_DSP_CFG, MAX98520_SPK_SAFE_EN_MASK, 0);

This seems like something that should be left as is by default given the
name (or forced on if it were disabled by default)?

> +	/* Hard coded values for the experiments */
> +	regmap_write(max98520->regmap, MAX98520_R21FF_REVISION_ID, 0x54);
> +	regmap_write(max98520->regmap, MAX98520_R21FF_REVISION_ID, 0x4d);
> +	regmap_write(max98520->regmap, MAX98520_R2161_BOOST_TM1, 0x2);
> +	regmap_write(max98520->regmap, MAX98520_R2095_AMP_CFG, 0xc8);

This doesn't seem suitable for upstream.

> +	/* Power on device */
> +	if (gpio_is_valid(max98520->reset_gpio)) {
> +		ret = devm_gpio_request(&i2c->dev, max98520->reset_gpio,
> +					"MAX98520_RESET");

You should use the gpiod APIs for new code, not the legacy GPIO
interface.  This GPIO wasn't mentioned in the DT bindings but should
have been described there.

> +		if (ret) {
> +			dev_err(&i2c->dev, "%s: Failed to request gpio %d\n",
> +				__func__, max98520->reset_gpio);
> +			return -EINVAL;
> +		}
> +		gpio_direction_output(max98520->reset_gpio, 0);
> +		msleep(50);
> +		gpio_direction_output(max98520->reset_gpio, 1);
> +		msleep(20);
> +	}

Shouldn't the disable/enable of the reset GPIO be in the reset function?

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