Dear Mark:
Thanks for your reply.
I have some questions for your comment. Please check the red comment.
Mark Brown 於 2019/9/4 上午12:38 寫道:
On Tue, Sep 03, 2019 at 03:08:21PM +0800, richtek.jeff.chang@xxxxxxxxx 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 :/
Actually this is our hardware Design for MT6660. We cannot modify
hardware this time.
We will design well in next chip.
+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.
Due to our hardware design, it is hard to implement regmap for MT6660.
+
+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?
Because MT6660 needs this initial setting for working well.
+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.
In our experience, some Customer platform doesn't support runtime PM.
Do you have any suggestion about a compatible driver for support and not
support runtime PM platform?
+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.
I will modify it for a error message.
+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.
I will modify it.
+ 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?
i will modify kcontrol names for more detail description.
Thanks & BRs
Richtek Jeff Chang.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
