On Tue, Sep 03, 2019 at 03:08:21PM +0800, richtek.jeff.chang@gmail.com wrote:
--- /dev/null +++ b/sound/soc/codecs/mt6660.c @@ -0,0 +1,802 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2019 MediaTek Inc.
- */
Please make the entire comment block a C++ comment so things look more consistent.
+struct reg_size_table {
- u32 addr;
- u8 size;
+};
+static const struct reg_size_table mt6660_reg_size_table[] = {
- { MT6660_REG_HPF1_COEF, 4 },
- { MT6660_REG_HPF2_COEF, 4 },
- { MT6660_REG_TDM_CFG3, 2 },
- { MT6660_REG_RESV17, 2 },
- { MT6660_REG_RESV23, 2 },
- { MT6660_REG_SIGMAX, 2 },
- { MT6660_REG_DEVID, 2},
- { MT6660_REG_TDM_CFG3, 2},
- { MT6660_REG_HCLIP_CTRL, 2},
- { MT6660_REG_DA_GAIN, 2},
+};
Please talk to your hardware designers about the benefits of consistent register sizes :/
+static int32_t mt6660_i2c_update_bits(struct mt6660_chip *chip,
- uint32_t addr, uint32_t mask, uint32_t data)
+{
It would be good to implement a regmap rather than open coding *everything* - it'd give you things like this without needing to open code them. Providing you don't use the cache code it should cope fine with variable register sizes.
+static int mt6660_i2c_init_setting(struct mt6660_chip *chip) +{
- int i, len, ret;
- const struct codec_reg_val *init_table;
- init_table = e4_reg_inits;
- len = ARRAY_SIZE(e4_reg_inits);
- for (i = 0; i < len; i++) {
ret = mt6660_i2c_update_bits(chip, init_table[i].addr,init_table[i].mask, init_table[i].data);if (ret < 0)return ret;
Why are we not using the chip defaults here?
+static int mt6660_chip_power_on(
- struct snd_soc_component *component, int on_off)
+{
- struct mt6660_chip *chip = (struct mt6660_chip *)
snd_soc_component_get_drvdata(component);- int ret = 0;
- unsigned int val;
- dev_dbg(component->dev, "%s: on_off = %d\n", __func__, on_off);
- mutex_lock(&chip->var_lock);
- if (on_off) {
if (chip->pwr_cnt == 0) {ret = mt6660_i2c_update_bits(chip,MT6660_REG_SYSTEM_CTRL, 0x01, 0x00);val = mt6660_i2c_read(chip, MT6660_REG_IRQ_STATUS1);dev_info(chip->dev,"%s reg0x05 = 0x%x\n", __func__, val);}chip->pwr_cnt++;
This looks like you're open coding runtime PM stuff? AFAICT the issue is that you need to write to this register to do any I/O. Just implement this via the standard runtime PM framework, you'll need to do something about the register I/O in the controls (ideally in the framework, it'd be a lot easier if you did have a cache) but you could cut out this bit.
+static int mt6660_component_set_bias_level(struct snd_soc_component *component,
- enum snd_soc_bias_level level)
+{
- struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(component);- int ret;
- unsigned int val;
- struct mt6660_chip *chip = snd_soc_component_get_drvdata(component);
- if (dapm->bias_level == level) {
dev_warn(component->dev, "%s: repeat level change\n", __func__);
This shouldn't be a warning.
+static const struct snd_kcontrol_new mt6660_component_snd_controls[] = {
- SOC_SINGLE_EXT_TLV("Volume_Ctrl", MT6660_REG_VOL_CTRL, 0, 255,
1, snd_soc_get_volsw, mt6660_component_put_volsw,vol_ctl_tlv),- SOC_SINGLE_EXT("WDT_Enable", MT6660_REG_WDT_CTRL, 7, 1, 0,
snd_soc_get_volsw, mt6660_component_put_volsw),
These control names should all use standard ALSA control names - on/off switches should end in Switch, volume controls in Volume.
- SOC_SINGLE_EXT("I2SLRS", MT6660_REG_DATAO_SEL, 6, 3, 0,
snd_soc_get_volsw, mt6660_component_put_volsw),- SOC_SINGLE_EXT("I2SDOLS", MT6660_REG_DATAO_SEL, 3, 7, 0,
snd_soc_get_volsw, mt6660_component_put_volsw),- SOC_SINGLE_EXT("I2SDORS", MT6660_REG_DATAO_SEL, 0, 7, 0,
snd_soc_get_volsw, mt6660_component_put_volsw),
What do these controls do?