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 containspeaker 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