Re: [v4] ASoC: max98520: add max98520 audio amplifier driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +static int max98520_dai_tdm_slot(struct snd_soc_dai *dai,
> +				 unsigned int tx_mask, unsigned int rx_mask,
> +				 int slots, int slot_width)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct max98520_priv *max98520 =
> +		snd_soc_component_get_drvdata(component);
> +	int bsel = 0;

nit-pick: useless initialization....

> +	unsigned int chan_sz = 0;
> +
> +	if (!tx_mask && !rx_mask && !slots && !slot_width)
> +		max98520->tdm_mode = false;
> +	else
> +		max98520->tdm_mode = true;
> +
> +	/* BCLK configuration */
> +	bsel = max98520_get_bclk_sel(slots * slot_width);

... it's overridden here.

> +	if (bsel == 0) {
> +		dev_err(component->dev, "BCLK %d not supported\n",
> +			slots * slot_width);
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(max98520->regmap,
> +			   MAX98520_R2041_PCM_CLK_SETUP,
> +			   MAX98520_PCM_CLK_SETUP_BSEL_MASK,
> +			   bsel);
> +
> +	/* Channel size configuration */
> +	switch (slot_width) {
> +	case 16:
> +		chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_16;
> +		break;
> +	case 24:
> +		chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_24;
> +		break;
> +	case 32:
> +		chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_32;
> +		break;
> +	default:
> +		dev_err(component->dev, "format unsupported %d\n",
> +			slot_width);
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(max98520->regmap,
> +			   MAX98520_R2040_PCM_MODE_CFG,
> +			   MAX98520_PCM_MODE_CFG_CHANSZ_MASK, chan_sz);
> +
> +	/* Rx slot configuration */
> +	regmap_update_bits(max98520->regmap,
> +			   MAX98520_R2044_PCM_RX_SRC2,
> +			   MAX98520_PCM_DMIX_CH0_SRC_MASK,
> +			   rx_mask);
> +	regmap_update_bits(max98520->regmap,
> +			   MAX98520_R2044_PCM_RX_SRC2,
> +			   MAX98520_PCM_DMIX_CH1_SRC_MASK,
> +			   rx_mask << MAX98520_PCM_DMIX_CH1_SHIFT);
> +
> +	return 0;
> +}
> +
> +#define MAX98520_RATES SNDRV_PCM_RATE_8000_192000
> +
> +#define MAX98520_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> +	SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
> +
> +static const struct snd_soc_dai_ops max98520_dai_ops = {
> +	.set_fmt = max98520_dai_set_fmt,
> +	.hw_params = max98520_dai_hw_params,
> +	.set_tdm_slot = max98520_dai_tdm_slot,
> +};
> +
> +static int max98520_dac_event(struct snd_soc_dapm_widget *w,
> +			      struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct max98520_priv *max98520 =
> +		snd_soc_component_get_drvdata(component);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:

is it intentional that you use POST_PMU here? for symmetry with POST_PMD
below, should this be PRE_PMU?

> +		dev_dbg(component->dev, " AMP ON\n");
> +
> +		regmap_write(max98520->regmap, MAX98520_R209F_AMP_EN, 1);
> +		regmap_write(max98520->regmap, MAX98520_R210F_GLOBAL_EN, 1);
> +		usleep_range(30000, 31000);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		dev_dbg(component->dev, " AMP OFF\n");
> +
> +		regmap_write(max98520->regmap, MAX98520_R210F_GLOBAL_EN, 0);
> +		regmap_write(max98520->regmap, MAX98520_R209F_AMP_EN, 0);
> +		usleep_range(30000, 31000);
> +		break;
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
>
> +static void max98520_reset(struct max98520_priv *max98520, struct device *dev)
> +{
> +	int ret, reg, count;
> +
> +	/* Software Reset */
> +	ret = regmap_write(max98520->regmap, MAX98520_R2000_SW_RESET, 1);
> +	if (ret)
> +		dev_err(dev, "Reset command failed. (ret:%d)\n", ret);

return; ?

Trying to verify if the reset worked is the reset command failed seems
unnecessary?

> +
> +	count = 0;
> +	while (count < 3) {
> +		usleep_range(10000, 11000);
> +		/* Software Reset Verification */
> +		ret = regmap_read(max98520->regmap, MAX98520_R21FF_REVISION_ID, &reg);
> +		if (!ret)
> +			return;
> +
> +		count++;
> +	}
> +	dev_err(dev, "Reset failed. (ret:%d)\n", ret);
> +}

> +#ifdef CONFIG_PM_SLEEP

the recommendation is to remove these ifdefs and use __maybe_unused for
pm functions.

> +static int max98520_suspend(struct device *dev)
> +{
> +	struct max98520_priv *max98520 = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(max98520->regmap, true);
> +	regcache_mark_dirty(max98520->regmap);
> +	return 0;
> +}
> +
> +static int max98520_resume(struct device *dev)
> +{
> +	struct max98520_priv *max98520 = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(max98520->regmap, false);
> +	max98520_reset(max98520, dev);
> +	regcache_sync(max98520->regmap);
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops max98520_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(max98520_suspend, max98520_resume)
> +};
> +
> +static const struct snd_soc_component_driver soc_codec_dev_max98520 = {
> +	.probe			= max98520_probe,
> +	.controls		= max98520_snd_controls,
> +	.num_controls		= ARRAY_SIZE(max98520_snd_controls),
> +	.dapm_widgets		= max98520_dapm_widgets,
> +	.num_dapm_widgets	= ARRAY_SIZE(max98520_dapm_widgets),
> +	.dapm_routes		= max98520_audio_map,
> +	.num_dapm_routes	= ARRAY_SIZE(max98520_audio_map),
> +	.idle_bias_on		= 1,
> +	.use_pmdown_time	= 1,
> +	.endianness		= 1,
> +	.non_legacy_dai_naming	= 1,
> +};
> +
> +static const struct regmap_config max98520_regmap = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = MAX98520_R21FF_REVISION_ID,
> +	.reg_defaults  = max98520_reg,
> +	.num_reg_defaults = ARRAY_SIZE(max98520_reg),
> +	.readable_reg = max98520_readable_register,
> +	.volatile_reg = max98520_volatile_reg,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static void max98520_power_on(struct max98520_priv *max98520, bool poweron)
> +{
> +	if (max98520->reset_gpio)
> +		gpiod_set_value_cansleep(max98520->reset_gpio, !poweron);
> +}
> +
> +static int max98520_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
> +{
> +	int ret = 0;

useless init

> +	int reg = 0;
> +	struct max98520_priv *max98520 = NULL;

useless init

> +	struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
> +
> +	ret = i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA);
> +	if (!ret) {
> +		dev_err(&i2c->dev, "I2C check functionality failed\n");
> +		return -ENXIO;
> +	}
> +
> +	max98520 = devm_kzalloc(&i2c->dev, sizeof(*max98520), GFP_KERNEL);
> +
> +	if (!max98520) {
> +		ret = -ENOMEM;
> +		return ret;

return -ENOMEM;

> +	}
> +	i2c_set_clientdata(i2c, max98520);
> +
> +	/* regmap initialization */
> +	max98520->regmap =
> +		devm_regmap_init_i2c(i2c, &max98520_regmap);

one line?

> +	if (IS_ERR(max98520->regmap)) {
> +		ret = PTR_ERR(max98520->regmap);
> +		dev_err(&i2c->dev,
> +			"Failed to allocate regmap: %d\n", ret);

one line?

> +		return ret;
> +	}
> +
> +	/* Power on device */
> +	max98520->reset_gpio = devm_gpiod_get_optional(&i2c->dev, "reset", GPIOD_OUT_HIGH);
> +	if (max98520->reset_gpio) {
> +		if (IS_ERR(max98520->reset_gpio)) {
> +			ret = PTR_ERR(max98520->reset_gpio);
> +			dev_err(&i2c->dev, "Unable to request GPIO pin: %d.\n", ret);
> +			return ret;
> +		}
> +
> +		max98520_power_on(max98520, 1);
> +	}
> +
> +	/* Check Revision ID */
> +	ret = regmap_read(max98520->regmap, MAX98520_R21FF_REVISION_ID, &reg);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev,
> +			"Failed to read: 0x%02X\n", MAX98520_R21FF_REVISION_ID);
> +		return ret;
> +	}
> +	dev_info(&i2c->dev, "MAX98520 revisionID: 0x%02X\n", reg);
> +
> +	/* codec registration */
> +	ret = devm_snd_soc_register_component(&i2c->dev,
> +					      &soc_codec_dev_max98520,
> +		max98520_dai, ARRAY_SIZE(max98520_dai));

alignment?

> +	if (ret < 0)
> +		dev_err(&i2c->dev, "Failed to register codec: %d\n", ret);
> +
> +	return ret;
> +}

> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id max98520_acpi_match[] = {
> +	{ "MX98520", 0 },

MX is not a valid ACPI/PNP vendor identifier but I suppose it's been
used by Maxim for all products...