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
+ *
+ * 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.
+#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?
+
+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.
+
+ 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 '='
+ 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
+ *
+ * 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
+#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
+#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
+ 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
