Re: [PATCH v6] ASoC: Add MediaTek MT6660 Speaker Amp Driver

[Takashi Iwai <tiwai@suse.de>]

On Tue, 14 Jan 2020 03:22:06 +0100,
Jeff Chang wrote:
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 229cc89..f135fbb 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -1465,6 +1466,16 @@ config SND_SOC_MT6358
>  	  Enable support for the platform which uses MT6358 as
>  	  external codec device.
>  
> +config SND_SOC_MT6660
> +	tristate "Mediatek MT6660 Speaker Amplifier"
> +	depends on I2C
> +	help
> +	  MediaTek MT6660 is a smart power amplifier which contain
> +	  speaker protection, multi-band DRC, equalizer functions.
> +	  Select N if you don't have MT6660 on board.
> +	  Select M to build this as module.
> +
> +

One blank line too much here.

> --- /dev/null
> +++ b/sound/soc/codecs/mt6660.c
> @@ -0,0 +1,533 @@
> +// SPDX-License-Identifier: GPL-2.0 //
> +
> +// Copyright (c) 2019 MediaTek Inc.
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>
> +#include <sound/pcm_params.h>
> +#include <linux/debugfs.h>

Move linux/*.h above sound/*.h inclusion.

> +
> +#include "mt6660.h"
> +
> +#pragma pack(push, 1)

Actually packing makes little sense for those use cases.
As I mentioned earlier, packing is useful only for either saving some
memory (e.g. for a large array) or a strict size definition like ABI.

> +struct codec_reg_val {
> +	u32 addr;
> +	u32 mask;
> +	u32 data;
> +};

Is this struct used anywhere?  If not, kill it.

> +static struct regmap_config mt6660_regmap_config = {

This can be const.

> +static int mt6660_codec_dac_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +

A superfluous blank line.

> +static int mt6660_component_get_volsw(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct mt6660_chip *chip = (struct mt6660_chip *)
> +		snd_soc_component_get_drvdata(component);
> +	int ret = -EINVAL;
> +
> +	if (!strcmp(kcontrol->id.name, "Chip Rev")) {
> +		ucontrol->value.integer.value[0] = chip->chip_rev & 0x0f;
> +		ret = 0;
> +	}
> +	return ret;

So, "T0 SEL" control gets always an error when reading?
Then can't we pass simply NULL for get ops instead?

> +static int _mt6660_chip_power_on(struct mt6660_chip *chip, int on_off)
> +{
> +	u8 reg_data;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(chip->i2c, MT6660_REG_SYSTEM_CTRL);
> +	if (ret < 0)
> +		return ret;
> +	reg_data = (u8)ret;
> +	if (on_off)
> +		reg_data &= (~0x01);
> +	else
> +		reg_data |= 0x01;
> +	return regmap_write(chip->regmap, MT6660_REG_SYSTEM_CTRL, reg_data);

Hm, this looks like an open-code of forced update bits via regmap.
But interestingly there is no corresponding standard helper for that.
Essentially it should be regmap_update_bits_base() with force=1.

Mark?

> +static int mt6660_component_aif_hw_params(struct snd_pcm_substream *substream,
> +	struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> +	int word_len = params_physical_width(hw_params);
> +	int aud_bit = params_width(hw_params);
....
> +	switch (aud_bit) {
> +	case 16:
> +		reg_data = 3;
> +		break;
> +	case 18:
> +		reg_data = 2;
> +		break;
> +	case 20:
> +		reg_data = 1;
> +		break;
> +	case 24:
> +	case 32:
> +		reg_data = 0;
> +		break;

So here both 24 and 32 bits data are handled equally, and...

....
> +	ret = snd_soc_component_update_bits(dai->component,
> +		MT6660_REG_TDM_CFG3, 0x3f0, word_len << 4);

... word_len is same for both S32 and S24 formats, so there can be no
difference between S24 and S32 format handling in the code.
Meanwhile, the supported formats are:

> +#define STUB_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
> +			SNDRV_PCM_FMTBIT_U16_LE | \
> +			SNDRV_PCM_FMTBIT_S24_LE | \
> +			SNDRV_PCM_FMTBIT_U24_LE | \
> +			SNDRV_PCM_FMTBIT_S32_LE | \
> +			SNDRV_PCM_FMTBIT_U32_LE)

Are you sure that S24_* formats really work properly?

Also, the code has no check / setup of the format signedness.
Do unsigned formats (U16, U24, etc) really work as expected, too?

> +static inline int _mt6660_chip_id_check(struct mt6660_chip *chip)

Drop unnecessary inline (here and other places).
Compiler optimizes well by itself.

> +static inline int _mt6660_chip_sw_reset(struct mt6660_chip *chip)
> +{
> +	int ret;
> +
> +	/* turn on main pll first, then trigger reset */
> +	ret = regmap_write(chip->regmap, 0x03, 0x00);

It's MT6660_REG_SYSTEM_CTRL, right?

> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(chip->regmap, MT6660_REG_SYSTEM_CTRL, 0x80);
> +	if (ret < 0)
> +		return ret;
> +	msleep(30);
> +	return 0;
> +}
> +
> +static inline int _mt6660_read_chip_revision(struct mt6660_chip *chip)
> +{
> +	u8 reg_data[2];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(
> +		chip->i2c, MT6660_REG_DEVID, 2, reg_data);

Why avoiding regmap here?  This and chip_id_check() use the raw access
by some reason...


thanks,

Takashi