Re: [PATCH 2/2] ASoC: max98390: Added Amplifier Driver

[Steve Lee]

On Mon, May 11, 2020 at 8:03 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Sat, May 09, 2020 at 12:19:19PM +0900, Steve Lee wrote:
> > Signed-off-by: Steve Lee <steves.lee@xxxxxxxxxxxxxxxxxxx>
>
> This looks mostly good, a few smallish things below though:
>
> > +++ b/sound/soc/codecs/max98390.c
> > @@ -0,0 +1,1039 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020, Maxim Integrated.
> > + */
>
> Please make the entire comment a C++ one so things look more
> intentional.
Thank you for feedback.
Modified as requested.
>
> > +     dev_info(component->dev, "Tdm mode : %d\n",
> > +                     max98390->tdm_mode);
>
> This is a bit noisy, please make it at most a dev_dbg().
>
> > +static const char * const max98390_analog_gain_text[] = {
> > +     "Mute", "3dB", "6dB", "9dB", "12dB", "15dB", "18dB"};
>
> Use TLV data with regulator Volume controls for volumes, don't make them
> enums.
>
Remove enums and use TLV.
> > +static const char * const max98390_boost_voltage_text[] = {
> > +     "6.5V", "6.625V", "6.75V", "6.875V", "7V", "7.125V", "7.25V", "7.375V",
> > +     "7.5V", "7.625V", "7.75V", "7.875V", "8V", "8.125V", "8.25V", "8.375V",
> > +     "8.5V", "8.625V", "8.75V", "8.875V", "9V", "9.125V", "9.25V", "9.375V",
> > +     "9.5V", "9.625V", "9.75V", "9.875V", "10V"
> > +};
>
> Is this really something that should be configured at runtime rather
> than through DT?
Since this control is needed while running system according to system
battery situation.
I'd keep this mixer for further use.
>
> > +static const char * const max98390_current_limit_text[] = {
> > +     "0.00A", "0.50A", "1.00A", "1.05A", "1.10A", "1.15A", "1.20A", "1.25A",
> > +     "1.30A", "1.35A", "1.40A", "1.45A", "1.50A", "1.55A", "1.60A", "1.65A",
>
> This looks like it should be in DT too.
Since this control  is needed while running system according to system
battery situation.
I'd keep this mixer for further use.
>
> > +static int max98390_dsm_calib_get(struct snd_kcontrol *kcontrol,
> > +             struct snd_ctl_elem_value *ucontrol)
> > +{
> > +     struct snd_soc_component *component =
> > +             snd_soc_kcontrol_component(kcontrol);
> > +
> > +     dev_warn(component->dev, "Get dsm_calib_get not supported\n");
> > +
> > +     return 0;
> > +}
>
> Just don't implement the operation if you can't implement it.
If this not exist as dummy operation and all mixer was not working and
could not implement better idea.
Could you consider it as with warn message ?
>
> > +     dev_info(component->dev,
> > +             "max98390: param fw size %d\n",
> > +             fw->size);
>
> This should probably be a dev_dbg() too.
Modified as requested.
>
> > +     /* Amp Setting */
> > +     regmap_write(max98390->regmap, MAX98390_CLK_MON, 0x6f);
> > +     regmap_write(max98390->regmap, MAX98390_PCM_RX_EN_A, 0x03);
> > +     regmap_write(max98390->regmap, MAX98390_R203D_SPK_GAIN, 0x05);
> > +     regmap_write(max98390->regmap, MAX98390_MEAS_EN, 0x03);
> > +     regmap_write(max98390->regmap, MAX98390_PWR_GATE_CTL, 0x2d);
> > +     regmap_write(max98390->regmap, MAX98390_ENV_TRACK_VOUT_HEADROOM, 0x0e);
> > +     regmap_write(max98390->regmap, MAX98390_BOOST_BYPASS1, 0x46);
> > +     regmap_write(max98390->regmap, MAX98390_FET_SCALING3, 0x03);
>
> Are some of these things that might vary per system?  If so they
> probably shouldn't be hard code but instead in DT.  Things like the
> speaker gain jump out.
I removed hard-coded Volume setting.
>
> > +static int max98390_suspend(struct device *dev)
> > +{
> > +     struct max98390_priv *max98390 = dev_get_drvdata(dev);
> > +
> > +     dev_info(dev, "%s:Enter\n", __func__);
>
> dev_dbg()
Modified as requested.
>
> > +static int max98390_resume(struct device *dev)
> > +{
> > +     struct max98390_priv *max98390 = dev_get_drvdata(dev);
> > +
> > +     dev_info(dev, "%s:Enter\n", __func__);
>
> dev_dbg()
Modified as requested.
>
> > +     dev_info(&i2c->dev, "ASoC: MAX98390 i2c probe\n");
>
> Just drop this.
Removed.
>
> > +     ret = device_property_read_u32(&i2c->dev, "maxim,temperature_calib",
> > +                                    &max98390->ambient_temp_value);
>
> Normally for DT that'd be maxim,temperature-calib.
This is follow as coreboot in Chromium OS case.
I'd follow this name unchanged.