Re: [PATCH V7 1/5] ASoC: codecs: Add i2c and codec registration for aw883xx and their associated operation functions

On 12/22/22 06:32, wangweidong.a@awinic.com wrote:

From: Weidong Wang wangweidong.a@awinic.com

The Awinic AW883XX is an I2S/TDM input, high efficiency digital Smart K audio amplifier with an integrated 10.25V smart boost convert

Signed-off-by: Nick Li liweilei@awinic.com Signed-off-by: Bruce zhao zhaolei@awinic.com Signed-off-by: Weidong Wang wangweidong.a@awinic.com

---

sound/soc/codecs/aw883xx/aw883xx.c | 706 +++++++++++++++++++++++++++++ sound/soc/codecs/aw883xx/aw883xx.h | 61 +++ 2 files changed, 767 insertions(+) create mode 100644 sound/soc/codecs/aw883xx/aw883xx.c create mode 100644 sound/soc/codecs/aw883xx/aw883xx.h

diff --git a/sound/soc/codecs/aw883xx/aw883xx.c b/sound/soc/codecs/aw883xx/aw883xx.c new file mode 100644 index 000000000000..0abf8d96d2fe --- /dev/null +++ b/sound/soc/codecs/aw883xx/aw883xx.c @@ -0,0 +1,706 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • aw883xx.c -- ALSA SoC AW883XX codec support
• • Copyright (c) 2022 AWINIC Technology CO., LTD
• • Author: Bruce zhao zhaolei@awinic.com
• • Author: Weidong Wang wangweidong.a@awinic.com
• */

+#include <linux/i2c.h> +#include <linux/firmware.h> +#include <linux/of_gpio.h> +#include <linux/regmap.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/tlv.h> +#include "aw883xx_pid_2049_reg.h" +#include "aw883xx.h" +#include "aw883xx_device.h"

+static const struct regmap_config aw883xx_remap_config = {

• .val_bits = 16,
• .reg_bits = 8,
• .max_register = AW_PID_2049_REG_MAX - 1,
• .reg_format_endian = REGMAP_ENDIAN_LITTLE,
• .val_format_endian = REGMAP_ENDIAN_BIG,

+};

+static void aw883xx_start_pa(struct aw883xx *aw883xx) +{

• int ret, i;
• if (!aw883xx->allow_pw) {

• dev_info(aw883xx->aw_pa->dev, "%s:dev can not allow power", __func__);
  

• return;
  

• }
• if (aw883xx->pstream == AW883XX_STREAM_CLOSE) {

• dev_info(aw883xx->aw_pa->dev, "%s:pstream is close", __func__);
  

• return;
  

• }
• for (i = 0; i < AW_START_RETRIES; i++) {

• ret = aw883xx_dev_start(aw883xx->aw_pa);
  

• if (ret) {
  

• 	dev_err(aw883xx->aw_pa->dev, "aw883xx device start failed. retry = %d", i);
  

• 	ret = aw883xx_dev_fw_update(aw883xx->aw_pa, AW_DSP_FW_UPDATE_ON, true);
  

• 	if (ret < 0) {
  

• 		dev_err(aw883xx->aw_pa->dev, "fw update failed");
  

• 		continue;
  

• 	}
  

• } else {
  

• 	dev_info(aw883xx->aw_pa->dev, "start success\n");
  

• 	break;
  

• }
  

• }

+}

+static void aw883xx_startup_work(struct work_struct *work) +{

• struct aw883xx *aw883xx =

• container_of(work, struct aw883xx, start_work.work);
  

• mutex_lock(&aw883xx->lock);
• aw883xx_start_pa(aw883xx);
• mutex_unlock(&aw883xx->lock);

+}

+static void aw883xx_start(struct aw883xx *aw883xx, bool sync_start) +{

• int ret;
• int i;
• if (aw883xx->aw_pa->fw_status != AW_DEV_FW_OK)

• return;
  

• if (!aw883xx->allow_pw) {

• dev_info(aw883xx->aw_pa->dev, "%s:dev can not allow power", __func__);
  

• return;
  

• }
• if (aw883xx->aw_pa->status == AW_DEV_PW_ON)

• return;
  

• for (i = 0; i < AW_START_RETRIES; i++) {

• ret = aw883xx_dev_fw_update(aw883xx->aw_pa, AW_DSP_FW_UPDATE_OFF, true);
  

• if (ret < 0) {
  

• 	dev_err(aw883xx->aw_pa->dev, "fw update failed. retry = %d", i);
  

• 	continue;
  

• } else {
  

• 	/*firmware update success*/
  

• 	if (sync_start == AW_SYNC_START)
  

• 		aw883xx_start_pa(aw883xx);
  

If I scroll two functions above, I see this:

static void aw883xx_start_pa(struct aw883xx *aw883xx) { int ret, i;

[...0]

for (i = 0; i < AW_START_RETRIES; i++) { ret = aw883xx_dev_start(aw883xx->aw_pa); if (ret) { dev_err(aw883xx->aw_pa->dev, "aw883xx device start failed. retry = %d", i); ret = aw883xx_dev_fw_update(aw883xx->aw_pa, AW_DSP_FW_UPDATE_ON, true);

so your handling of retries is rather convoluted, with two nested retry loops.

• 	else
  

• 		queue_delayed_work(aw883xx->work_queue,
  

• 			&aw883xx->start_work,
  

• 			AW_START_WORK_DELAY_MS);
  

• 	return;
  

• }
  

• }

+}

+/*

• • Digital Audio Interface
• */

+static int aw883xx_startup(struct snd_pcm_substream *substream,

• 	struct snd_soc_dai *dai)
  

+{

• struct snd_soc_component *codec = dai->component;
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• aw883xx->pstream = AW883XX_STREAM_OPEN;
• mutex_lock(&aw883xx->lock);
• aw883xx_start(aw883xx, AW_ASYNC_START);
• mutex_unlock(&aw883xx->lock);
• return 0;

+}

+static void aw883xx_shutdown(struct snd_pcm_substream *substream,

• 		struct snd_soc_dai *dai)
  

+{

• struct snd_soc_component *codec = dai->component;
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• aw883xx->pstream = AW883XX_STREAM_CLOSE;

it's a bit odd that you are setting the state before the transitions happen.

• cancel_delayed_work_sync(&aw883xx->start_work);
• mutex_lock(&aw883xx->lock);
• aw883xx_dev_stop(aw883xx->aw_pa);
• mutex_unlock(&aw883xx->lock);

+}

+static int aw883xx_set_fade_in_time(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)

+{

• unsigned int time = 0;

useless initialization, and move this declaration lower (reverse x-mas tree style).

• struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(component);
• struct aw_device *aw_dev = aw883xx->aw_pa;
• struct soc_mixer_control *mc =

• (struct soc_mixer_control *)kcontrol->private_value;
  

• time = ucontrol->value.integer.value[0];
• if (time < mc->min || time > mc->max)

• return 0;
  

• if (time != aw_dev->fade_in_time) {

• aw_dev->fade_in_time = time;
  

• return 1;
  

• }
• return 0;

+}

+static int aw883xx_get_fade_out_time(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)

+{

• struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(component);
• struct aw_device *aw_dev = aw883xx->aw_pa;
• ucontrol->value.integer.value[0] = aw_dev->fade_out_time;
• return 0;

+}

+static int aw883xx_set_fade_out_time(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)

+{

• struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(component);
• struct soc_mixer_control *mc =

• (struct soc_mixer_control *)kcontrol->private_value;
  

• struct aw_device *aw_dev = aw883xx->aw_pa;
• unsigned int time = 0;

useless init, you override the value on the next line.

• time = ucontrol->value.integer.value[0];
• if (time < mc->min || time > mc->max)

• return 0;
  

• if (time != aw_dev->fade_out_time) {

• aw_dev->fade_out_time = time;
  

• return 1;
  

• }
• return 0;

+}

+static int aw883xx_profile_info(struct snd_kcontrol *kcontrol,

• 	 struct snd_ctl_elem_info *uinfo)
  

+{

• int count;
• char *name = NULL;

useless init, and reverse the declaration order.

• const char *prof_name = NULL;

useless init

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
• uinfo->count = 1;
• count = aw883xx_dev_get_profile_count(aw883xx->aw_pa);
• if (count <= 0) {

• uinfo->value.enumerated.items = 0;
  

• return 0;
  

• }
• uinfo->value.enumerated.items = count;
• if (uinfo->value.enumerated.item >= count)

• uinfo->value.enumerated.item = count - 1;
  

• name = uinfo->value.enumerated.name;
• count = uinfo->value.enumerated.item;
• prof_name = aw883xx_dev_get_prof_name(aw883xx->aw_pa, count);
• if (!prof_name) {

• strscpy(uinfo->value.enumerated.name, "null",
  

• 				strlen("null") + 1);
  

• return 0;
  

• }
• strscpy(name, prof_name, sizeof(uinfo->value.enumerated.name));
• return 0;

+}

+static int aw883xx_profile_get(struct snd_kcontrol *kcontrol,

• 	struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• ucontrol->value.integer.value[0] = aw883xx_dev_get_profile_index(aw883xx->aw_pa);
• return 0;

+}

+static int aw883xx_profile_set(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• int ret;
• /*pa stop or stopping just set profile*/

use spaces after and before /* and */

• mutex_lock(&aw883xx->lock);
• ret = aw883xx_dev_set_profile_index(aw883xx->aw_pa, ucontrol->value.integer.value[0]);
• if (ret < 0) {

• dev_dbg(codec->dev, "profile index does not change");
  

• mutex_unlock(&aw883xx->lock);
  

• return 0;
  

• }
• if (aw883xx->pstream) {

• aw883xx_dev_stop(aw883xx->aw_pa);
  

• aw883xx_start(aw883xx, AW_SYNC_START);
  

• }
• mutex_unlock(&aw883xx->lock);
• return 1;

+}

+static int aw883xx_switch_get(struct snd_kcontrol *kcontrol,

• 	struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• ucontrol->value.integer.value[0] = aw883xx->allow_pw;
• return 0;

+}

+static int aw883xx_switch_set(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• struct soc_mixer_control *mc =

• (struct soc_mixer_control *)kcontrol->private_value;
  

• unsigned int value = 0;

useless init

• value = ucontrol->value.integer.value[0];
• if (value < mc->min || value > mc->max)

• return 0;
  

• if (value == aw883xx->allow_pw) {

• dev_dbg(aw883xx->aw_pa->dev, "PA switch not change");
  

• return 0;
  

• }
• aw883xx->allow_pw = value;
• if (aw883xx->pstream) {

• if (!aw883xx->allow_pw) {
  

• 	cancel_delayed_work_sync(&aw883xx->start_work);
  

• 	mutex_lock(&aw883xx->lock);
  

• 	aw883xx_dev_stop(aw883xx->aw_pa);
  

• 	mutex_unlock(&aw883xx->lock);
  

• } else {
  

• 	cancel_delayed_work_sync(&aw883xx->start_work);
  

the cancel_delayed_work_sync can be moved outside of the test, and so can the mutex_lock and mutex_unlock. What's in the test should be aw883xx_dev_stop() and aw883xx_start()

• 	mutex_lock(&aw883xx->lock);
  

• 	aw883xx_start(aw883xx, AW_SYNC_START);
  

• 	mutex_unlock(&aw883xx->lock);
  

• }
  

• }
• return 1;

+}

+static int aw883xx_volume_get(struct snd_kcontrol *kcontrol,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• struct aw_volume_desc *vol_desc = &aw883xx->aw_pa->volume_desc;
• ucontrol->value.integer.value[0] = vol_desc->ctl_volume;
• return 0;

+}

+static int aw883xx_volume_set(struct snd_kcontrol *kcontrol,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• int value = 0;

useless init and reverse order. same for the rest of the code.

• struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(codec);
• struct aw_volume_desc *vol_desc = &aw883xx->aw_pa->volume_desc;
• struct soc_mixer_control *mc =

• (struct soc_mixer_control *)kcontrol->private_value;
  

• value = ucontrol->value.integer.value[0];
• if (value < mc->min || value > mc->max)

• return 0;
  

• if (vol_desc->ctl_volume != value) {

• vol_desc->ctl_volume = value;
  

• aw883xx_dev_set_volume(aw883xx->aw_pa, vol_desc->ctl_volume);
  

• return 1;
  

• }
• return 0;

+}

+static int aw883xx_codec_probe(struct snd_soc_component *component) +{

• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(component);
• struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
• int ret = 0;
• /*destroy_workqueue(struct workqueue_struct *wq)*/
• aw883xx->work_queue = create_singlethread_workqueue("aw883xx");

why do you need your own workqueue? It's common for codec/amplifiers to use a default one.

• if (!aw883xx->work_queue) {

• dev_err(aw883xx->aw_pa->dev, "create workqueue failed !");
  

• return -EINVAL;
  

• }
• INIT_DELAYED_WORK(&aw883xx->start_work, aw883xx_startup_work);
• /*add widgets*/
• ret = snd_soc_dapm_new_controls(dapm, aw883xx_dapm_widgets,

• 					ARRAY_SIZE(aw883xx_dapm_widgets));
  

• if (ret < 0)

• return ret;
  

• /*add route*/
• ret = snd_soc_dapm_add_routes(dapm, aw883xx_audio_map,

• 					ARRAY_SIZE(aw883xx_audio_map));
  

• if (ret < 0)

• return ret;
  

• ret = snd_soc_add_component_controls(component, aw883xx_controls,

• 					ARRAY_SIZE(aw883xx_controls));
  

• return ret;

+}

+static void aw883xx_codec_remove(struct snd_soc_component *aw_codec) +{

• struct aw883xx *aw883xx = snd_soc_component_get_drvdata(aw_codec);
• cancel_delayed_work_sync(&aw883xx->start_work);
• if (aw883xx->work_queue)

do you need to test this? The probe will not succeed without a workqueue created.

• destroy_workqueue(aw883xx->work_queue);
  

+}

+static const struct snd_soc_component_driver soc_codec_dev_aw883xx = {

• .probe = aw883xx_codec_probe,
• .remove = aw883xx_codec_remove,

+};

+static struct aw883xx *aw883xx_malloc_init(struct i2c_client *i2c) +{

• struct aw883xx *aw883xx = devm_kzalloc(&i2c->dev,

• 	sizeof(struct aw883xx), GFP_KERNEL);
  

• if (!aw883xx)

• return NULL;
  

• aw883xx->aw_pa = NULL;
• aw883xx->allow_pw = true;

what does 'pw' stand for?

• aw883xx->work_queue = NULL;
• mutex_init(&aw883xx->lock);
• return aw883xx;

+}

+static void aw883xx_hw_reset(struct aw883xx *aw883xx) +{

• if (aw883xx->reset_gpio) {

• gpiod_set_value_cansleep(aw883xx->reset_gpio, 0);
  

• usleep_range(AW_1000_US, AW_1000_US + 10);
  

• gpiod_set_value_cansleep(aw883xx->reset_gpio, 1);
  

• usleep_range(AW_1000_US, AW_1000_US + 10);
  

• } else {

• dev_err(aw883xx->aw_pa->dev, "%s failed", __func__);
  

• }

+}

+static int aw883xx_request_firmware_file(struct aw883xx *aw883xx) +{

• const struct firmware *cont = NULL;
• int ret = 0;
• aw883xx->aw_pa->fw_status = AW_DEV_FW_FAILED;
• ret = request_firmware(&cont, AW_ACF_FILE, aw883xx->aw_pa->dev);
• if ((ret < 0) || (!cont)) {

• dev_err(aw883xx->aw_pa->dev, "load [%s] failed!", AW_ACF_FILE);
  

• return ret;
  

• }
• dev_info(aw883xx->aw_pa->dev, "loaded %s - size: %zu\n",

• 	AW_ACF_FILE, cont ? cont->size : 0);
  

• aw883xx->aw_cfg = kzalloc(cont->size + sizeof(int), GFP_KERNEL);
• if (!aw883xx->aw_cfg) {

• release_firmware(cont);
  

• return -ENOMEM;
  

• }
• aw883xx->aw_cfg->len = (int)cont->size;
• memcpy(aw883xx->aw_cfg->data, cont->data, cont->size);
• release_firmware(cont);
• ret = aw883xx_dev_load_acf_check(aw883xx->aw_cfg);
• if (ret < 0) {

• dev_err(aw883xx->aw_pa->dev, "Load [%s] failed ....!", AW_ACF_FILE);
  

• kfree(aw883xx->aw_cfg);
  

• aw883xx->aw_cfg = NULL;
  

• return ret;
  

• }
• dev_info(aw883xx->aw_pa->dev, "%s : bin load success\n", __func__);
• mutex_lock(&aw883xx->lock);
• /*aw device init*/
• ret = aw883xx_dev_init(aw883xx->aw_pa, aw883xx->aw_cfg);
• if (ret < 0) {

• dev_err(aw883xx->aw_pa->dev, "dev init failed");
  

• kfree(aw883xx->aw_cfg);
  

• }
• mutex_unlock(&aw883xx->lock);
• return ret;

+}

+/*

• • i2c driver
• */

+static int aw883xx_i2c_probe(struct i2c_client *i2c) +{

• struct aw883xx *aw883xx = NULL;
• int ret = 0;
• if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {

• dev_err(&i2c->dev, "check_functionality failed");
  

• return -EIO;
  

• }
• aw883xx = aw883xx_malloc_init(i2c);
• if (!aw883xx) {

• dev_err(&i2c->dev, "malloc aw883xx failed");
  

• return -ENOMEM;
  

• }
• i2c_set_clientdata(i2c, aw883xx);
• aw883xx->reset_gpio = devm_gpiod_get_optional(&i2c->dev,

• 						"reset", GPIOD_OUT_LOW);
  

• if (IS_ERR(aw883xx->reset_gpio))

• dev_info(&i2c->dev, "reset gpio not defined\n");
  

• /* hardware reset */
• aw883xx_hw_reset(aw883xx);
• aw883xx->regmap = devm_regmap_init_i2c(i2c, &aw883xx_remap_config);
• if (IS_ERR(aw883xx->regmap)) {

• ret = PTR_ERR(aw883xx->regmap);
  

• dev_err(&i2c->dev, "Failed to init regmap: %d\n", ret);
  

• return ret;
  

• }
• /*aw pa init*/
• ret = aw883xx_init(&aw883xx->aw_pa, i2c, aw883xx->regmap);
• if (ret < 0)

• return ret;
  

• ret = aw883xx_request_firmware_file(aw883xx);
• if (ret < 0) {

• dev_err(&i2c->dev, "%s failed\n", __func__);
  

• return ret;
  

• }
• ret = snd_soc_register_component(&i2c->dev,

• 	&soc_codec_dev_aw883xx,
  

• 	aw883xx_dai, ARRAY_SIZE(aw883xx_dai));
  

• if (ret < 0) {

• dev_err(&i2c->dev, "failed to register aw883xx: %d", ret);
  

• return ret;
  

• }
• return 0;

+}

+static void aw883xx_i2c_remove(struct i2c_client *i2c) +{

• struct aw883xx *aw883xx = i2c_get_clientdata(i2c);
• aw883xx_deinit(aw883xx->aw_pa);
• snd_soc_unregister_component(&i2c->dev);
• if (aw883xx->aw_cfg) {

• kfree(aw883xx->aw_cfg);
  

• aw883xx->aw_cfg = NULL;
  

• }

+}

+static const struct i2c_device_id aw883xx_i2c_id[] = {

• { AW_I2C_NAME, 0 },
• { }

+}; +MODULE_DEVICE_TABLE(i2c, aw883xx_i2c_id);

+static struct i2c_driver aw883xx_i2c_driver = {

• .driver = {

• .name = AW_I2C_NAME,
  

• .owner = THIS_MODULE,
  

• },
• .probe_new = aw883xx_i2c_probe,
• .remove = aw883xx_i2c_remove,
• .id_table = aw883xx_i2c_id,

+}; +module_i2c_driver(aw883xx_i2c_driver);

+MODULE_DESCRIPTION("ASoC AW883XX Smart PA Driver"); +MODULE_LICENSE("GPL v2");

"GPL"