On 12/8/2022 1:23 PM, wangweidong.a@xxxxxxxxxx wrote:
> From: Weidong Wang <wangweidong.a@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Bruce zhao <zhaolei@xxxxxxxxxx>
> Signed-off-by: Weidong Wang <wangweidong.a@xxxxxxxxxx>
> ---
> sound/soc/codecs/aw883xx/aw883xx.c | 909 +++++++++++++++++++++++++++++
> sound/soc/codecs/aw883xx/aw883xx.h | 81 +++
> 2 files changed, 990 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..f82e6d8c71a7
> --- /dev/null
> +++ b/sound/soc/codecs/aw883xx/aw883xx.c
> @@ -0,0 +1,909 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * aw883xx.c -- ALSA Soc AW883XX codec support
> Soc -> SoC
I will change it on patch v7
> + *
> + * Copyright (c) 2022 AWINIC Technology CO., LTD
> + *
> + * Author: Bruce zhao <zhaolei@xxxxxxxxxx>
> + */
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/firmware.h>
> +#include <linux/hrtimer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/syscalls.h>
> +#include <linux/timer.h>
> +#include <linux/uaccess.h>
> +#include <linux/version.h>
> +#include <linux/vmalloc.h>
> +#include <linux/workqueue.h>
> Are all those headers really needed? Just picking few, for example
> debugfs.h, version.h and vmalloc.h seems unnecessary to me, and I
> suspect few more can be removed.
I will change it on patch v7
> +#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_bin_parse.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,
> +};
> +
> +/*
> + * aw883xx dsp write/read
> + */
> +static int aw883xx_dsp_write_16bit(struct aw883xx *aw883xx,
> + unsigned short dsp_addr, unsigned int dsp_data)
> +{
> + int ret;
> + struct aw_dsp_mem_desc *desc = &aw883xx->aw_pa->dsp_mem_desc;
> +
> + ret = regmap_write(aw883xx->regmap, desc->dsp_madd_reg, dsp_addr);
> + if (ret < 0) {
> + dev_err(aw883xx->dev, "%s error, ret=%d", __func__, ret);
> + return ret;
> + }
> +
> + ret = regmap_write(aw883xx->regmap, desc->dsp_mdat_reg, (u16)dsp_data);
> + if (ret < 0) {
> + dev_err(aw883xx->dev, "%s error, ret=%d", __func__, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int aw883xx_dsp_write_32bit(struct aw883xx *aw883xx,
> + unsigned short dsp_addr, unsigned int dsp_data)
> +{
> + int ret;
> + u16 temp_data = 0;
> + struct aw_dsp_mem_desc *desc = &aw883xx->aw_pa->dsp_mem_desc;
> +
> + ret = regmap_write(aw883xx->regmap, desc->dsp_madd_reg, dsp_addr);
> + if (ret < 0) {
> + dev_err(aw883xx->dev, "%s error, ret=%d", __func__, ret);
> + return ret;
> + }
> +
> + temp_data = dsp_data & AW_DSP_16_DATA_MASK;
> + ret = regmap_write(aw883xx->regmap, desc->dsp_mdat_reg, temp_data);
> + if (ret < 0) {
> + dev_err(aw883xx->dev, "%s error, ret=%d", __func__, ret);
> + return ret;
> + }
> +
> + temp_data = dsp_data >> 16;
> + ret = regmap_write(aw883xx->regmap, desc->dsp_mdat_reg, temp_data);
> + if (ret < 0) {
> + dev_err(aw883xx->dev, "%s error, ret=%d", __func__, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * aw883xx clear dsp chip select state
> + */
> +static void aw883xx_clear_dsp_sel_st(struct aw883xx *aw883xx)
> +{
> + unsigned int reg_value;
> + u8 reg = aw883xx->aw_pa->soft_rst.reg;
> +
> + regmap_read(aw883xx->regmap, reg, ®_value);
> +}
> Is it enough to just read register to clear it?
Yes, this is because our chip hardware design,
just need to read, the hardware can automatically clear the register value
> +
> +int aw883xx_dsp_write(struct aw883xx *aw883xx,
> + unsigned short dsp_addr, unsigned int dsp_data, unsigned char data_type)
> +{
> + int ret = -1;
> No need to set "-1" value here, as following switch always sets ret and
> -1 == -EPERM, which may lead to some confusion if something is changed
> later and ret is returned. If you want to set it to anything, just set
> it to -EINVAL.
> There is few more places in the patch, where ret is initialized to -1
> only to be overwritten later, I won't mark them all, but it seems weird
> to me and should probably be fixed.
I will change it on patch v7
> +
> + mutex_lock(&aw883xx->dsp_lock);
> + switch (data_type) {
> + case AW_DSP_16_DATA:
> + ret = aw883xx_dsp_write_16bit(aw883xx, dsp_addr, dsp_data);
> + if (ret < 0)
> + dev_err(aw883xx->dev, "write dsp_addr[0x%04x] 16 bit dsp_data[%04x] failed",
> + (u32)dsp_addr, dsp_data);
> + break;
> + case AW_DSP_32_DATA:
> + ret = aw883xx_dsp_write_32bit(aw883xx, dsp_addr, dsp_data);
> remove double space after '='
I will change it on patch v7
> + if (ret < 0)
> + dev_err(aw883xx->dev, "write dsp_addr[0x%04x] 32 bit dsp_data[%08x] failed",
> + (u32)dsp_addr, dsp_data);
> + break;
> + default:
> + dev_err(aw883xx->dev, "data type[%d] unsupported", data_type);
> + ret = -EINVAL;
> + break;
> + }
> +
> + aw883xx_clear_dsp_sel_st(aw883xx);
> + mutex_unlock(&aw883xx->dsp_lock);
> + return ret;
> +}
> +
(...)
> diff --git a/sound/soc/codecs/aw883xx/aw883xx.h b/sound/soc/codecs/aw883xx/aw883xx.h
> new file mode 100644
> index 000000000000..209851cae7ef
> --- /dev/null
> +++ b/sound/soc/codecs/aw883xx/aw883xx.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * aw883xx.c -- ALSA Soc AW883XX codec support
> Soc -> SoC
I will change it on patch v7
> + *
> + * Copyright (c) 2022 AWINIC Technology CO., LTD
> + *
> + * Author: Bruce zhao <zhaolei@xxxxxxxxxx>
> + */
> +
> +#ifndef __AW883XX_H__
> +#define __AW883XX_H__
> +
> +#include <linux/version.h>
> This header should be unnecessary
I will change it on patch v7
> +#include <sound/control.h>
> +#include <sound/soc.h>
> +#include "aw883xx_data_type.h"
> +
> +#define AW_CHIP_ID_REG (0x00)
> +#define AW_START_RETRIES (5)
> +#define AW_START_WORK_DELAY_MS (0)
> +
> +#define AW_DSP_16_DATA_MASK (0x0000ffff)
> +
> +#define AW_I2C_NAME "aw883xx_smartpa"
> +
> +#define AW_RATES (SNDRV_PCM_RATE_8000_48000 | \
> + SNDRV_PCM_RATE_96000)
> +#define AW_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> + SNDRV_PCM_FMTBIT_S24_LE | \
> + SNDRV_PCM_FMTBIT_S32_LE)
> +#define AW_REQUEST_FW_RETRIES 5 /* 5 times */
> Unnecessary comment
I will change it on patch v7
> +#define AW_SYNC_LOAD
> +
> +#define FADE_TIME_MAX 100000
> +#define FADE_TIME_MIN 0
> +
> +#define AW_PROFILE_EXT(xname, profile_info, profile_get, profile_set) \
> +{ \
> + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> + .name = xname, \
> + .info = profile_info, \
> + .get = profile_get, \
> + .put = profile_set, \
> +}
> +
> +enum {
> + AW_SYNC_START = 0,
> + AW_ASYNC_START,
> +};
> +
> +enum {
> + AW883XX_STREAM_CLOSE = 0,
> + AW883XX_STREAM_OPEN,
> +};
> +
> +struct aw883xx {
> + struct i2c_client *i2c;
> + struct device *dev;
> + struct mutex lock;
> + struct mutex dsp_lock;
> + struct snd_soc_component *codec;
> + struct aw_device *aw_pa;
> + struct gpio_desc *reset_gpio;
> + unsigned char phase_sync; /*phase sync*/
> Also unnecessary comment
I will change it on patch v7
> + bool allow_pw;
> + u8 pstream;
> + struct workqueue_struct *work_queue;
> + struct delayed_work start_work;
> + u16 chip_id;
> + struct regmap *regmap;
> + struct aw_container *aw_cfg;
> +};
> +
> +int aw883xx_init(struct aw883xx *aw883xx);
> +
> +int aw883xx_dsp_write(struct aw883xx *aw883xx,
> + unsigned short dsp_addr, unsigned int dsp_data, unsigned char data_type);
> +int aw883xx_dsp_read(struct aw883xx *aw883xx,
> + unsigned short dsp_addr, unsigned int *dsp_data, unsigned char data_type);
> +
> +#endif
Thank you very much for your advice
Best regards,
Weidong Wang
