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.
> + 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.
> +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?
> +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.
> +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.
> + dev_info(component->dev,
> + "max98390: param fw size %d\n",
> + fw->size);
This should probably be a dev_dbg() too.
> + /* 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.
> +static int max98390_suspend(struct device *dev)
> +{
> + struct max98390_priv *max98390 = dev_get_drvdata(dev);
> +
> + dev_info(dev, "%s:Enter\n", __func__);
dev_dbg()
> +static int max98390_resume(struct device *dev)
> +{
> + struct max98390_priv *max98390 = dev_get_drvdata(dev);
> +
> + dev_info(dev, "%s:Enter\n", __func__);
dev_dbg()
> + dev_info(&i2c->dev, "ASoC: MAX98390 i2c probe\n");
Just drop this.
> + ret = device_property_read_u32(&i2c->dev, "maxim,temperature_calib",
> + &max98390->ambient_temp_value);
Normally for DT that'd be maxim,temperature-calib.
Attachment:
signature.asc
Description: PGP signature
