[PATCH] ASoC: MT6660 update to 1.0.8_G

[PATCH] ASoC: MT6660 update to 1.0.8_G

[Jeff Chang]

From: Jeff Chang <jeff_chang@richtek.com>

1. add parsing register initial table via device tree.
2. add applying initial register value function at component driver.

Signed-off-by: Jeff Chang <jeff_chang@richtek.com>
---
 Documentation/devicetree/bindings/sound/mt6660.txt |  53 ++++++++++
 sound/soc/codecs/mt6660.c                          | 114 ++++++++++++++++++++-
 2 files changed, 164 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/mt6660.txt


Following Mr. Mark's Suggestion, I create another patch for applying our chip INIT SETTING.


diff --git a/Documentation/devicetree/bindings/sound/mt6660.txt b/Documentation/devicetree/bindings/sound/mt6660.txt
new file mode 100644
index 0000000..2a1736b
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/mt6660.txt
@@ -0,0 +1,53 @@
+MT6660 MediaTek Speaker Amplifier
+
+This device supports I2C mode only.
+
+Required properties:
+
+	- compatible : "mediatek,mt6660"
+	
+	- reg : The I2C slave address
+
+Optional properties:
+
+	- rt,init_setting_num : The initial register setting element number.
+
+	- rt,init_setting_addr : the addreses array for INIT Setting table.
+
+	- rt,init_setting_mask : the mask array for INIT Setting table.
+
+	- rt,init_setting_val : the value array for INIT Setting table.
+
+Example:
+
+	mt6660@34 {
+		status = "ok";
+		compatible = "mediatek,mt6660";
+		reg = <0x34>;
+		rt,init_setting_num = <26>;
+		rt,init_setting_addr =
+			<0x20 0x30 0x50 0xB1
+			 0xD3 0xE0 0x98 0xB9
+			 0xB7 0xB6 0x6B 0x07
+			 0xBB 0x69 0xBD 0x70
+			 0x7C 0x46 0x1A 0x1B
+			 0x51 0xA2 0x33 0x4C
+			 0x15 0x68>;
+		rt,init_setting_mask =
+			<0x80 0x01 0x1c 0x0c
+			 0x03 0x01 0x44 0xff
+			 0x7777 0x07 0xe0 0xff
+			 0xff 0xff 0xffff 0xff
+			 0xff 0xff 0xffffffff 0xffffffff
+			 0xff 0xff 0xffff 0xffff
+			 0x1800 0x1f>;
+		rt,init_setting_val =
+			<0x00 0x00 0x04 0x00
+			 0x03 0x00 0x04 0x82
+			 0x7273 0x03 0x20 0x70
+			 0x20 0x40 0x17f8 0x15
+			 0x00 0x1d 0x7fdb7ffe 0x7fdb7ffe
+			 0x58 0xce 0x7fff 0x0116
+			 0x0800 0x07>;
+	};
+
diff --git a/sound/soc/codecs/mt6660.c b/sound/soc/codecs/mt6660.c
index a36c416..5df2780 100644
--- a/sound/soc/codecs/mt6660.c
+++ b/sound/soc/codecs/mt6660.c
@@ -9,7 +9,6 @@
 #include <linux/i2c.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
-#include <linux/debugfs.h>
 #include <sound/soc.h>
 #include <sound/tlv.h>
 #include <sound/pcm_params.h>
@@ -225,14 +224,48 @@ static int _mt6660_chip_power_on(struct mt6660_chip *chip, int on_off)
 				 0x01, on_off ? 0x00 : 0x01);
 }
 
+static int mt6660_apply_plat_data(struct mt6660_chip *chip,
+		struct snd_soc_component *component)
+{
+	size_t i;
+	int num = chip->plat_data.init_setting_num;
+	int ret;
+
+	ret = _mt6660_chip_power_on(chip, 1);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s power on failed\n", __func__);
+		return ret;
+	}
+
+	for (i = 0; i < num; i++) {
+		ret = snd_soc_component_update_bits(component,
+				chip->plat_data.init_setting_addr[i],
+				chip->plat_data.init_setting_mask[i],
+				chip->plat_data.init_setting_val[i]);
+		if (ret < 0)
+			return ret;
+	}
+	ret = _mt6660_chip_power_on(chip, 0);
+	if (ret < 0) {
+		dev_err(chip->dev, "%s power on failed\n", __func__);
+		return ret;
+	}
+	return 0;
+}
+
 static int mt6660_component_probe(struct snd_soc_component *component)
 {
 	struct mt6660_chip *chip = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	dev_dbg(component->dev, "%s\n", __func__);
 	snd_soc_component_init_regmap(component, chip->regmap);
 
-	return 0;
+	ret = mt6660_apply_plat_data(chip, component);
+	if (ret < 0)
+		dev_err(chip->dev, "mt6660 apply plat data failed\n");
+
+	return ret;
 }
 
 static void mt6660_component_remove(struct snd_soc_component *component)
@@ -386,6 +419,75 @@ static int _mt6660_read_chip_revision(struct mt6660_chip *chip)
 	return 0;
 }
 
+static int mt6660_parse_dt(struct mt6660_chip *chip, struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	u32 val;
+	size_t i;
+
+	if (!np) {
+		dev_err(dev, "no device node\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(np, "rt,init_setting_num", &val)) {
+		dev_err(dev, "no init setting\n");
+		chip->plat_data.init_setting_num = 0;
+	} else {
+		chip->plat_data.init_setting_num = val;
+	}
+
+	if (chip->plat_data.init_setting_num) {
+		chip->plat_data.init_setting_addr =
+			devm_kzalloc(dev, sizeof(u32) *
+			chip->plat_data.init_setting_num, GFP_KERNEL);
+		if (!chip->plat_data.init_setting_addr) {
+			dev_err(dev, "%s addr memory alloc failed\n", __func__);
+			return -ENOMEM;
+		}
+		chip->plat_data.init_setting_mask =
+			devm_kzalloc(dev, sizeof(u32) *
+			chip->plat_data.init_setting_num, GFP_KERNEL);
+		if (!chip->plat_data.init_setting_mask) {
+			dev_err(dev, "%s mask memory alloc failed\n", __func__);
+			return -ENOMEM;
+		}
+		chip->plat_data.init_setting_val =
+			devm_kzalloc(dev, sizeof(u32) *
+			chip->plat_data.init_setting_num, GFP_KERNEL);
+		if (!chip->plat_data.init_setting_val) {
+			dev_err(dev, "%s val memory alloc failed\n", __func__);
+			return -ENOMEM;
+		}
+
+		if (of_property_read_u32_array(np, "rt,init_setting_addr",
+					chip->plat_data.init_setting_addr,
+					chip->plat_data.init_setting_num)) {
+			dev_err(dev, "no init setting addr\n");
+		}
+		if (of_property_read_u32_array(np, "rt,init_setting_mask",
+					chip->plat_data.init_setting_mask,
+					chip->plat_data.init_setting_num)) {
+			dev_err(dev, "no init setting mask\n");
+		}
+		if (of_property_read_u32_array(np, "rt,init_setting_val",
+					chip->plat_data.init_setting_val,
+					chip->plat_data.init_setting_num)) {
+			dev_err(dev, "no init setting val\n");
+		}
+	}
+
+	dev_dbg(dev, "%s, init stting table, num = %d\n", __func__,
+		chip->plat_data.init_setting_num);
+	for (i = 0; i < chip->plat_data.init_setting_num; i++) {
+		dev_dbg(dev, "0x%02x, 0x%08x, 0x%08x\n",
+				chip->plat_data.init_setting_addr[i],
+				chip->plat_data.init_setting_mask[i],
+				chip->plat_data.init_setting_val[i]);
+	}
+	return 0;
+}
+
 static int mt6660_i2c_probe(struct i2c_client *client,
 			    const struct i2c_device_id *id)
 {
@@ -401,6 +503,12 @@ static int mt6660_i2c_probe(struct i2c_client *client,
 	mutex_init(&chip->io_lock);
 	i2c_set_clientdata(client, chip);
 
+	ret = mt6660_parse_dt(chip, &client->dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "parsing dts failed\n");
+		return ret;
+	}
+
 	chip->regmap = devm_regmap_init(&client->dev,
 		NULL, chip, &mt6660_regmap_config);
 	if (IS_ERR(chip->regmap)) {
@@ -506,4 +614,4 @@ module_i2c_driver(mt6660_i2c_driver);
 MODULE_AUTHOR("Jeff Chang <jeff_chang@richtek.com>");
 MODULE_DESCRIPTION("MT6660 SPKAMP Driver");
 MODULE_LICENSE("GPL");
-MODULE_VERSION("1.0.7_G");
+MODULE_VERSION("1.0.8_G");
-- 
2.7.4

Re: [PATCH] ASoC: MT6660 update to 1.0.8_G

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Tue, Feb 04, 2020 at 11:41:37AM +0800, Jeff Chang wrote:
> From: Jeff Chang <jeff_chang@richtek.com>
> 
> 1. add parsing register initial table via device tree.
> 2. add applying initial register value function at component driver.

OK, so there's still a problem with the whole concept of putting
"initial register settings" in the device tree - this is clearly not
idiomatic for an ASoC driver.  If there are machine specific settings
that need to be done unconditionally (eg, values controlled by external
passive components) there should be specific properties for them.  If
there are runtime options these should be normal ALSA controls with the
default values being whatever the hardware defaults are.  If there are
things that should just always be set no matter what then they should
just be hard coded into the driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] ASoC: MT6660 update to 1.0.8_G

[jeff_chang(張世佳)]

Dear Mark:

Thanks for your replying.

This INIT SETTING is always be set, and we don't want anyone to modify it.

What should I do is just apply them to be hard code into the driver? I cannot use event a table to descript it like below?

static const struct codec_reg_val e4_reg_inits[] = {
        { MT6660_REG_WDT_CTRL, 0x80, 0x00 },
        { MT6660_REG_SPS_CTRL, 0x01, 0x00 },
        { MT6660_REG_AUDIO_IN2_SEL, 0x1c, 0x04 },
        { MT6660_REG_RESV11, 0x0c, 0x00 },
        { MT6660_REG_RESV31, 0x03, 0x03 },
        { MT6660_REG_RESV40, 0x01, 0x00 },
        { MT6660_REG_RESV0, 0x44, 0x04 },
        { MT6660_REG_RESV19, 0xff, 0x82 },
        { MT6660_REG_RESV17, 0x7777, 0x7273 },
        { MT6660_REG_RESV16, 0x07, 0x03 },
        { MT6660_REG_DRE_CORASE, 0xe0, 0x20 },
        { MT6660_REG_ADDA_CLOCK, 0xff, 0x70 },
        { MT6660_REG_RESV21, 0xff, 0x20 },
        { MT6660_REG_DRE_THDMODE, 0xff, 0x40 },
        { MT6660_REG_RESV23, 0xffff, 0x17f8 },
        { MT6660_REG_PWM_CTRL, 0xff, 0x15 },
        { MT6660_REG_ADC_USB_MODE, 0xff, 0x00 },
        { MT6660_REG_PROTECTION_CFG, 0xff, 0x1d },
        { MT6660_REG_HPF1_COEF, 0xffffffff, 0x7fdb7ffe },
        { MT6660_REG_HPF2_COEF, 0xffffffff, 0x7fdb7ffe },
        { MT6660_REG_SIG_GAIN, 0xff, 0x58 },
        { MT6660_REG_RESV6, 0xff, 0xce },
        { MT6660_REG_SIGMAX, 0xffff, 0x7fff },
        { MT6660_REG_DA_GAIN, 0xffff, 0x0116 },
        { MT6660_REG_TDM_CFG3, 0x1800, 0x0800 },
        { MT6660_REG_DRE_CTRL, 0x1f, 0x07 },
};

Just hard coded??


Thanks & BRs
Jeff Chang
Tel 886-3-5526789 Ext 2357
14F, No. 8, Taiyuen 1st st., Zhubei City,
Hsinchu County, Taiwan 30288



-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Tuesday, February 11, 2020 2:51 AM
To: Jeff Chang <richtek.jeff.chang@gmail.com>
Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.com; matthias.bgg@gmail.com; alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; jeff_chang(張世佳) <jeff_chang@richtek.com>
Subject: Re: [PATCH] ASoC: MT6660 update to 1.0.8_G

On Tue, Feb 04, 2020 at 11:41:37AM +0800, Jeff Chang wrote:
> From: Jeff Chang <jeff_chang@richtek.com>
>
> 1. add parsing register initial table via device tree.
> 2. add applying initial register value function at component driver.

OK, so there's still a problem with the whole concept of putting "initial register settings" in the device tree - this is clearly not idiomatic for an ASoC driver.  If there are machine specific settings that need to be done unconditionally (eg, values controlled by external passive components) there should be specific properties for them.  If there are runtime options these should be normal ALSA controls with the default values being whatever the hardware defaults are.  If there are things that should just always be set no matter what then they should just be hard coded into the driver.
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH] ASoC: MT6660 update to 1.0.8_G

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On Tue, Feb 11, 2020 at 02:04:42AM +0000, jeff_chang(張世佳) wrote:

> What should I do is just apply them to be hard code into the driver? I cannot use event a table to descript it like below?
> 
> static const struct codec_reg_val e4_reg_inits[] = {

Doing what you've got here is hard coding it in the driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]