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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]