mbox series

[V7,0/5] ASoC: codecs: Add Awinic AW883XX audio amplifier driver

Message ID 20221222123205.106353-1-wangweidong.a@awinic.com
Headers show
Series ASoC: codecs: Add Awinic AW883XX audio amplifier driver | expand

Message

wangweidong.a@awinic.com Dec. 22, 2022, 12:32 p.m. UTC
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

Add a DT schema for describing Awinic AW883xx audio amplifiers. They are
controlled using I2C

v6 -> v7: Change name-prefix.yaml to dai-common.yaml in awinic,aw883xx.yaml file
          Delete redundant header files
          Use EINVAL and so on instead of custom error return values
          Remove unnecessary comment
          No longer assign NULL to pointer
          Change the way the if statement is written
          Use devm_kcalloc instead of devm_kzalloc
          Use crc8 and crc32 that come with linux          

Weidong Wang (5):
  ASoC: codecs: Add i2c and codec registration for aw883xx and their
    associated operation functions
  ASoC: codecs: Aw883xx function for ACF file parse and check
  ASoC: codecs: Aw883xx common function for ALSA Audio Driver
  ASoC: codecs: Aw883xx chip register file, data type file and Kconfig
    Makefile
  ASoC: dt-bindings: Add schema for "awinic,aw883xx"

 .../bindings/sound/awinic,aw883xx.yaml        |   49 +
 sound/soc/codecs/Kconfig                      |   10 +
 sound/soc/codecs/Makefile                     |    6 +
 sound/soc/codecs/aw883xx/aw883xx.c            |  706 +++++++
 sound/soc/codecs/aw883xx/aw883xx.h            |   61 +
 sound/soc/codecs/aw883xx/aw883xx_bin_parse.c  | 1138 ++++++++++
 sound/soc/codecs/aw883xx/aw883xx_bin_parse.h  |  123 ++
 sound/soc/codecs/aw883xx/aw883xx_data_type.h  |  143 ++
 sound/soc/codecs/aw883xx/aw883xx_device.c     | 1840 +++++++++++++++++
 sound/soc/codecs/aw883xx/aw883xx_device.h     |  201 ++
 .../soc/codecs/aw883xx/aw883xx_pid_2049_reg.h |  490 +++++
 11 files changed, 4767 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/awinic,aw883xx.yaml
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx.c
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx.h
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.c
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_bin_parse.h
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_data_type.h
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.c
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_device.h
 create mode 100644 sound/soc/codecs/aw883xx/aw883xx_pid_2049_reg.h


base-commit: 9d2f6060fe4c3b49d0cdc1dce1c99296f33379c8

Comments

Pierre-Louis Bossart Dec. 22, 2022, 3:09 p.m. UTC | #1
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"
Pierre-Louis Bossart Dec. 22, 2022, 3:50 p.m. UTC | #2
> +static int aw_dev_dsp_write_32bit(struct aw_device *aw_dev,
> +		unsigned short dsp_addr, unsigned int dsp_data)
> +{
> +	int ret;
> +	u16 temp_data = 0;

useless init

> +
> +	ret = regmap_write(aw_dev->regmap, AW_PID_2049_DSPMADD_REG, dsp_addr);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s write addr error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	temp_data = dsp_data & AW_DSP_16_DATA_MASK;
> +	ret = regmap_write(aw_dev->regmap, AW_PID_2049_DSPMDAT_REG, (u16)temp_data);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s write datal error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	temp_data = dsp_data >> 16;
> +	ret = regmap_write(aw_dev->regmap, AW_PID_2049_DSPMDAT_REG, (u16)temp_data);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s write datah error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int aw_dev_dsp_write(struct aw_device *aw_dev,
> +		unsigned short dsp_addr, unsigned int dsp_data, unsigned char data_type)
> +{
> +	int ret = 0;
> +	u32 reg_value;
> +
> +	mutex_lock(&aw_dev->dsp_lock);
> +	switch (data_type) {
> +	case AW_DSP_16_DATA:
> +		ret = aw_dev_dsp_write_16bit(aw_dev, dsp_addr, dsp_data);
> +		if (ret < 0)
> +			dev_err(aw_dev->dev, "write dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed",
> +					(u32)dsp_addr, dsp_data);
> +		break;
> +	case AW_DSP_32_DATA:
> +		ret = aw_dev_dsp_write_32bit(aw_dev, dsp_addr, dsp_data);
> +		if (ret < 0)
> +			dev_err(aw_dev->dev, "write dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed",
> +					(u32)dsp_addr, dsp_data);
> +		break;
> +	default:
> +		dev_err(aw_dev->dev, "data type[%d] unsupported", data_type);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	/* clear dsp chip select state*/
> +	regmap_read(aw_dev->regmap, AW_PID_2049_ID_REG, &reg_value);
> +	mutex_unlock(&aw_dev->dsp_lock);
> +	return ret;
> +}
> +
> +static int aw_dev_dsp_read_16bit(struct aw_device *aw_dev,
> +		unsigned short dsp_addr, unsigned int *dsp_data)
> +{
> +	int ret;
> +	unsigned int temp_data = 0;
> +
> +	ret = regmap_write(aw_dev->regmap, AW_PID_2049_DSPMADD_REG, dsp_addr);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s write error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(aw_dev->regmap, AW_PID_2049_DSPMDAT_REG, &temp_data);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s read error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	*dsp_data = temp_data;
> +
> +	return 0;
> +}
> +
> +static int aw_dev_dsp_read_32bit(struct aw_device *aw_dev,
> +		unsigned short dsp_addr, unsigned int *dsp_data)
> +{
> +	int ret;
> +	unsigned int temp_data = 0;
> +
> +	ret = regmap_write(aw_dev->regmap, AW_PID_2049_DSPMADD_REG, dsp_addr);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s write error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(aw_dev->regmap, AW_PID_2049_DSPMDAT_REG, &temp_data);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s read error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	*dsp_data = temp_data;
> +
> +	ret = regmap_read(aw_dev->regmap, AW_PID_2049_DSPMDAT_REG, &temp_data);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "%s read error, ret=%d", __func__, ret);
> +		return ret;
> +	}
> +
> +	*dsp_data |= (temp_data << 16);
> +
> +	return 0;
> +}
> +
> +static int aw_dev_dsp_read(struct aw_device *aw_dev,
> +		unsigned short dsp_addr, unsigned int *dsp_data, unsigned char data_type)
> +{
> +	int ret = 0;
> +	u32 reg_value;
> +
> +	mutex_lock(&aw_dev->dsp_lock);
> +	switch (data_type) {
> +	case AW_DSP_16_DATA:
> +		ret = aw_dev_dsp_read_16bit(aw_dev, dsp_addr, dsp_data);
> +		if (ret < 0)
> +			dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed",
> +					(u32)dsp_addr, *dsp_data);
> +		break;
> +	case AW_DSP_32_DATA:
> +		ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data);
> +		if (ret < 0)
> +			dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed",
> +					(u32)dsp_addr, *dsp_data);
> +		break;
> +	default:
> +		dev_err(aw_dev->dev, "data type[%d] unsupported", data_type);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	/* clear dsp chip select state*/
> +	regmap_read(aw_dev->regmap, AW_PID_2049_ID_REG, &reg_value);
> +	mutex_unlock(&aw_dev->dsp_lock);
> +	return ret;
> +}
> +
> +
> +static int aw_dev_read_chipid(struct aw_device *aw_dev, u16 *chip_id)
> +{
> +	int ret = 0;

useless init

> +	int reg_val = 0;
> +
> +	ret = regmap_read(aw_dev->regmap, AW_CHIP_ID_REG, &reg_val);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	dev_info(aw_dev->dev, "chip id = %x\n", reg_val);
> +	*chip_id = reg_val;
> +
> +	return 0;
> +}
> +
> +static void aw_dev_set_cfg_f0_fs(struct aw_device *aw_dev, unsigned int *f0_fs)
> +{
> +	unsigned int rate_data = 0;
> +	u32 fs = 0;
> +
> +	regmap_read(aw_dev->regmap, AW_PID_2049_I2SCTRL_REG, &rate_data);

usually you test the regmap results and here you don't, is it intentional?

> +
> +	switch (rate_data & (~AW_PID_2049_I2SSR_MASK)) {
> +	case AW_PID_2049_I2SSR_8_KHZ_VALUE:
> +		fs = 8000;
> +		break;
> +	case AW_PID_2049_I2SSR_16_KHZ_VALUE:
> +		fs = 16000;
> +		break;
> +	case AW_PID_2049_I2SSR_32_KHZ_VALUE:
> +		fs = 32000;
> +		break;
> +	case AW_PID_2049_I2SSR_44_KHZ_VALUE:
> +		fs = 44000;

did you mean 44100?

> +		break;
> +	case AW_PID_2049_I2SSR_48_KHZ_VALUE:
> +		fs = 48000;
> +		break;
> +	case AW_PID_2049_I2SSR_96_KHZ_VALUE:
> +		fs = 96000;
> +		break;
> +	case AW_PID_2049_I2SSR_192KHZ_VALUE:
> +		fs = 192000;
> +		break;
> +	default:
> +		fs = 48000;
> +		dev_err(aw_dev->dev,
> +			"rate can not support, use default 48kHz");
> +		break;
> +	}
> +
> +	dev_dbg(aw_dev->dev, "i2s fs:%d Hz", fs);
> +	*f0_fs = fs / 8;
> +
> +	aw_dev_dsp_write(aw_dev, AW_PID_2049_DSP_REG_CFGF0_FS, *f0_fs, AW_DSP_32_DATA);
> +}
> +
> +/*[9 : 6]: -6DB ; [5 : 0]: -0.125DB  real_value = value * 8 : 0.125db --> 1*/
> +static unsigned int reg_val_to_db(unsigned int value)
> +{
> +	return (((value >> AW_PID_2049_VOL_6DB_START) * AW_PID_2049_VOLUME_STEP_DB) +
> +			((value & 0x3f) % AW_PID_2049_VOLUME_STEP_DB));
> +}
> +
> +/*[9 : 6]: -6DB ; [5 : 0]: -0.125DB reg_value = value / step << 6 + value % step ; step = 6 * 8*/
> +static unsigned short db_to_reg_val(unsigned short value)
> +{
> +	return (((value / AW_PID_2049_VOLUME_STEP_DB) << AW_PID_2049_VOL_6DB_START) +
> +			(value % AW_PID_2049_VOLUME_STEP_DB));
> +}
> +
> +static int aw_dev_dsp_fw_check(struct aw_device *aw_dev)
> +{
> +	struct aw_prof_desc *set_prof_desc = NULL;
> +	struct aw_sec_data_desc *dsp_fw_desc = NULL;
> +	u16 base_addr = AW_PID_2049_DSP_FW_ADDR;
> +	u16 addr = base_addr;
> +	int ret, i;
> +	u32 dsp_val;
> +	u16 bin_val;
> +
> +	ret = aw883xx_dev_get_prof_data(aw_dev, aw_dev->prof_cur, &set_prof_desc);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*update reg*/
> +	dsp_fw_desc = &set_prof_desc->sec_desc[AW_DATA_TYPE_DSP_FW];
> +
> +	for (i = 0; i < AW_FW_CHECK_PART; i++) {
> +		ret = aw_dev_dsp_read(aw_dev, addr, &dsp_val, AW_DSP_16_DATA);
> +		if (ret  < 0) {
> +			dev_err(aw_dev->dev, "dsp read failed");
> +			return ret;
> +		}
> +
> +		bin_val = be16_to_cpup((void *)&dsp_fw_desc->data[2 * (addr - base_addr)]);
> +
> +		if (dsp_val != bin_val) {
> +			dev_err(aw_dev->dev, "fw check failed, addr[0x%x], read[0x%x] != bindata[0x%x]",
> +					addr, dsp_val, bin_val);
> +			return -EINVAL;
> +		}
> +
> +		addr += (dsp_fw_desc->len / 2) / AW_FW_CHECK_PART;
> +		if ((addr - base_addr) > dsp_fw_desc->len) {
> +			dev_err(aw_dev->dev, "fw check failed, addr[0x%x] too large", addr);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int aw_dev_set_volume(struct aw_device *aw_dev, unsigned int value)
> +{
> +	struct aw_volume_desc *vol_desc = &aw_dev->volume_desc;
> +	unsigned int reg_value = 0;
> +	u16 real_value = 0;
> +	u16 volume = 0;
> +
> +	volume = min((value + vol_desc->init_volume), (unsigned int)AW_PID_2049_MUTE_VOL);
> +	real_value = db_to_reg_val(volume);
> +
> +	/* cal real value */
> +	regmap_read(aw_dev->regmap, AW_PID_2049_SYSCTRL2_REG, &reg_value);
> +
> +	dev_dbg(aw_dev->dev, "value 0x%x , reg:0x%x", value, real_value);
> +
> +	/*[15 : 6] volume*/
> +	real_value = (real_value << AW_PID_2049_VOL_START_BIT) | (reg_value & AW_PID_2049_VOL_MASK);
> +
> +	/* write value */
> +	regmap_write(aw_dev->regmap, AW_PID_2049_SYSCTRL2_REG, real_value);
> +
> +	return 0;
> +}
> +
> +void aw883xx_dev_set_volume(struct aw_device *aw_dev, unsigned short set_vol)
> +{
> +	int ret = 0;
> +
> +	ret = aw_dev_set_volume(aw_dev, set_vol);
> +	if (ret < 0)
> +		dev_dbg(aw_dev->dev, "set volume failed");
> +}
> +EXPORT_SYMBOL_GPL(aw883xx_dev_set_volume);
> +
> +static void aw_dev_fade_in(struct aw_device *aw_dev)
> +{
> +	int i = 0;

useless init

> +	struct aw_volume_desc *desc = &aw_dev->volume_desc;
> +	int fade_step = aw_dev->fade_step;
> +	u16 fade_in_vol = desc->ctl_volume;
> +
> +	if (!aw_dev->fade_en)
> +		return;
> +
> +	if (fade_step == 0 || aw_dev->fade_in_time == 0) {
> +		aw_dev_set_volume(aw_dev, fade_in_vol);
> +		return;
> +	}
> +	/*volume up*/
> +	for (i = AW_PID_2049_MUTE_VOL; i >= fade_in_vol; i -= fade_step) {
> +		aw_dev_set_volume(aw_dev, i);
> +		usleep_range(aw_dev->fade_in_time, aw_dev->fade_in_time + 10);
> +	}
> +	if (i != fade_in_vol)
> +		aw_dev_set_volume(aw_dev, fade_in_vol);
> +}
> +
> +static void aw_dev_fade_out(struct aw_device *aw_dev)
> +{
> +	int i = 0;

useless init

> +	struct aw_volume_desc *desc = &aw_dev->volume_desc;
> +	int fade_step = aw_dev->fade_step;
> +
> +	if (!aw_dev->fade_en)
> +		return;
> +
> +	if (fade_step == 0 || aw_dev->fade_out_time == 0) {
> +		aw_dev_set_volume(aw_dev, AW_PID_2049_MUTE_VOL);
> +		return;
> +	}
> +
> +	for (i = desc->ctl_volume; i <= AW_PID_2049_MUTE_VOL; i += fade_step) {
> +		aw_dev_set_volume(aw_dev, i);
> +		usleep_range(aw_dev->fade_out_time, aw_dev->fade_out_time + 10);
> +	}
> +	if (i != AW_PID_2049_MUTE_VOL) {
> +		aw_dev_set_volume(aw_dev, AW_PID_2049_MUTE_VOL);
> +		usleep_range(aw_dev->fade_out_time, aw_dev->fade_out_time + 10);
> +	}
> +}
> +
> +static int aw_dev_modify_dsp_cfg(struct aw_device *aw_dev,
> +			unsigned int addr, unsigned int dsp_data, unsigned char data_type)
> +{
> +	u32 addr_offset = 0;

useless init

> +	u16 data1 = 0;
> +	u32 data2 = 0;

useless init and could be moved to lower scopes

> +	struct aw_sec_data_desc *crc_dsp_cfg = &aw_dev->crc_dsp_cfg;
> +
> +	dev_dbg(aw_dev->dev, "addr:0x%x, dsp_data:0x%x", addr, dsp_data);
> +
> +	addr_offset = (addr - AW_PID_2049_DSP_CFG_ADDR) * 2;
> +	if (addr_offset > crc_dsp_cfg->len) {
> +		dev_err(aw_dev->dev, "addr_offset[%d] > crc_dsp_cfg->len[%d]",
> +				addr_offset, crc_dsp_cfg->len);
> +		return -EINVAL;
> +	}
> +	switch (data_type) {
> +	case AW_DSP_16_DATA:
> +		data1 = cpu_to_le16((u16)dsp_data);
> +		memcpy(crc_dsp_cfg->data + addr_offset, (u8 *)&data1, 2);
> +		break;
> +	case AW_DSP_32_DATA:
> +		data2 = cpu_to_le32(dsp_data);
> +		memcpy(crc_dsp_cfg->data + addr_offset, (u8 *)&data2, 4);
> +		break;
> +	default:
> +		dev_err(aw_dev->dev, "data type[%d] unsupported", data_type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int aw_dev_dsp_set_cali_re(struct aw_device *aw_dev)
> +{
> +	u32 cali_re = 0;
> +	int ret = 0;

useless inits

> +
> +	cali_re = AW_SHOW_RE_TO_DSP_RE((aw_dev->cali_desc.cali_re +
> +		aw_dev->cali_desc.ra), AW_PID_2049_DSP_RE_SHIFT);
> +
> +	/* set cali re to device */
> +	ret = aw_dev_dsp_write(aw_dev,
> +			AW_PID_2049_DSP_REG_CFG_ADPZ_RE, cali_re, AW_DSP_32_DATA);
> +	if (ret < 0) {
> +		dev_err(aw_dev->dev, "set cali re error");
> +		return ret;
> +	}
> +
> +	ret = aw_dev_modify_dsp_cfg(aw_dev, AW_PID_2049_DSP_REG_CFG_ADPZ_RE,
> +				cali_re, AW_DSP_32_DATA);
> +	if (ret < 0)
> +		dev_err(aw_dev->dev, "modify dsp cfg failed");
> +
> +	return ret;
> +
> +}

this file needs style fixes, stopping the review here.