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

[George Song]

> 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...
It will be removed tdm_mode to false line.
> 
> > +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.
It will be modified SND_SOC_NOPM instead of MAX98520_R209F_AMP_EN.
> 
> > +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.
It will be added const casting in front of DECLARE_TLV_DB_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?
It will be removed for logging regarding reset completed.
> 
> > +	/* 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?
It will be removed 30ms sleep after reset function.
> 
> > +	/* 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.
It`s already exposed for 2043 register which is regarding mono mixer for "DAI Sel Mux"
It will be exposed for 2044 register which is regarding pcm input channel selection to dapm mixer.
> 
> > +	/* 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)?
It will be removed to be left as is by default given the name.
> 
> > +	/* 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.
They will be removed.
> 
> > +	/* 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.
It will be modified to gpiod APIs like devm_gpiod_get_optional for getting reset_gpio (struct gpio_desc *reset_gpio).
> 
> > +		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?
It will be modified to gpiod APIs like gpiod_set_value_cansleep function instead of gpio_direction_output.