lkml.org 
[lkml]   [2021]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [v3 2/2] ASoC: max98520: add max98520 audio amplifier driver
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?
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-10-18 14:39    [W:0.098 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site