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, ®);
> + 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:
signature.asc
Description: PGP signature
