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?