On Sat, May 09, 2020 at 12:19:19PM +0900, Steve Lee wrote:
Signed-off-by: Steve Lee steves.lee@maximintegrated.com
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.