[PATCH 0/3] Add support for AW8898 Amplifier

[PATCH 0/3] Add support for AW8898 Amplifier

[Luca Weiss]

Add devicetree bindings and a driver for the AW8898 Amplifier, and add
it to the devicetree for the Fairphone 3 smartphone.

Note, that this is the first audio driver I've worked on, so please
complain if something doesn't quite look right, it probably wasn't done
on purpose.

Unfortunately we can't wire up the full audio stack yet since the SoC
side is not upstream yet, the last revision from July 2024 can be found
here:

https://lore.kernel.org/linux-arm-msm/20240731-msm8953-msm8976-asoc-v3-0-163f23c3a28d@gmail.com/

Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
---
Luca Weiss (3):
      ASoC: dt-bindings: Document Awinic AW8898 amplifier
      ASoC: codecs: Add aw8898 amplifier driver
      arm64: dts: qcom: sdm632-fairphone-fp3: Add AW8898 amplifier

 .../devicetree/bindings/sound/awinic,aw8898.yaml   |  75 +++
 arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts  |  20 +
 sound/soc/codecs/Kconfig                           |  10 +
 sound/soc/codecs/Makefile                          |   1 +
 sound/soc/codecs/snd-soc-aw8898.c                  | 583 +++++++++++++++++++++
 5 files changed, 689 insertions(+)
---
base-commit: 287e43781488fd6ca5f4306dbb4f2a88c5564d79
change-id: 20250406-aw8898-06988582d2e0

Best regards,
-- 
Luca Weiss <luca@lucaweiss.eu>

[PATCH 1/3] ASoC: dt-bindings: Document Awinic AW8898 amplifier

[Luca Weiss]

Add the bindings for the Awinic AW8898 amplifier.

Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
---
 .../devicetree/bindings/sound/awinic,aw8898.yaml   | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/awinic,aw8898.yaml b/Documentation/devicetree/bindings/sound/awinic,aw8898.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..2be084e77253410fd66ded5242fc75174dbfc4f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/awinic,aw8898.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/awinic,aw8898.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW8898 Audio Amplifier
+
+maintainers:
+  - Luca Weiss <luca@lucaweiss.eu>
+
+description:
+  The AW8898 is an I2S/TDM input, high efficiency digital Smart K audio
+  amplifier with an integrated 9.5V smart boost converter, sound quality
+  enhancement algorithms and speaker protection.
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    const: awinic,aw8898
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  dvdd-supply:
+    description:
+      Digital power supply
+
+  vddio-supply:
+    description:
+      Digital IO power supply
+
+  vdd-supply:
+    description:
+      Battery power supply
+
+  "#sound-dai-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - "#sound-dai-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        audio-codec@34 {
+            compatible = "awinic,aw8898";
+            reg = <0x34>;
+            reset-gpios = <&tlmm 21 GPIO_ACTIVE_LOW>;
+            interrupts-extended = <&tlmm 20 IRQ_TYPE_LEVEL_HIGH>;
+            dvdd-supply = <&pm8953_l5>;
+            vddio-supply = <&pm8953_l5>;
+            vdd-supply = <&vph_pwr>;
+            #sound-dai-cells = <0>;
+        };
+    };

-- 
2.49.0

Re: [PATCH 1/3] ASoC: dt-bindings: Document Awinic AW8898 amplifier

[Krzysztof Kozlowski]

On 06/04/2025 15:03, Luca Weiss wrote:
> +    description:
> +      Digital IO power supply
> +
> +  vdd-supply:
> +    description:
> +      Battery power supply
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios

Some or all of the supplies should be required.

> +  - "#sound-dai-cells"
> +
> +additionalProperties: false

and this 'unevaluatedProperties: false' instead.


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>



Best regards,
Krzysztof

[PATCH 2/3] ASoC: codecs: Add aw8898 amplifier driver

[Luca Weiss]

Add a driver for the AW8898 Audio Amplifier.

Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
---
 sound/soc/codecs/Kconfig          |  10 +
 sound/soc/codecs/Makefile         |   1 +
 sound/soc/codecs/snd-soc-aw8898.c | 583 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 594 insertions(+)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 40bb7a1d44bcfa4c386f2eba0f475efeeedf1bdb..8351853d3c21522b91639ddfe862613e2cde5be3 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -61,6 +61,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_AW88166
 	imply SND_SOC_AW88261
 	imply SND_SOC_AW88399
+	imply SND_SOC_AW8898
 	imply SND_SOC_BT_SCO
 	imply SND_SOC_BD28623
 	imply SND_SOC_CHV3_CODEC
@@ -739,6 +740,15 @@ config SND_SOC_AW88399
 	  digital Smart K audio amplifier and SKTune speaker
 	  protection algorithms.
 
+config SND_SOC_AW8898
+	tristate "Awinic AW8898 Audio Amplifier"
+	depends on I2C
+	select REGMAP_I2C
+	select GPIOLIB
+	help
+	  Enable support for the Awinic AW8898 audio amplifier with I2C
+	  control.
+
 config SND_SOC_BD28623
 	tristate "ROHM BD28623 CODEC"
 	help
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 10f726066b6c63736a4f533cf992c8e55a02036d..04691deb218262e2352d21d8bb231613ad1655d7 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -474,6 +474,7 @@ obj-$(CONFIG_SND_SOC_AW88395)	+=snd-soc-aw88395.o
 obj-$(CONFIG_SND_SOC_AW88166)	+=snd-soc-aw88166.o
 obj-$(CONFIG_SND_SOC_AW88261)	+=snd-soc-aw88261.o
 obj-$(CONFIG_SND_SOC_AW88399)	+= snd-soc-aw88399.o
+obj-$(CONFIG_SND_SOC_AW8898)	+= snd-soc-aw8898.o
 obj-$(CONFIG_SND_SOC_BD28623)	+= snd-soc-bd28623.o
 obj-$(CONFIG_SND_SOC_BT_SCO)	+= snd-soc-bt-sco.o
 obj-$(CONFIG_SND_SOC_CHV3_CODEC) += snd-soc-chv3-codec.o
diff --git a/sound/soc/codecs/snd-soc-aw8898.c b/sound/soc/codecs/snd-soc-aw8898.c
new file mode 100644
index 0000000000000000000000000000000000000000..204c39709f17e904f656cb6a945e6dfd969d1569
--- /dev/null
+++ b/sound/soc/codecs/snd-soc-aw8898.c
@@ -0,0 +1,583 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 AWINIC Technology CO., LTD
+ * Author: Nick Li <liweilei@awinic.com.cn>
+ *
+ * Copyright (c) 2025 Luca Weiss <luca@lucaweiss.eu>
+ */
+
+#include <linux/device.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+
+/* Chip ID */
+#define AW8898_ID				0x00
+#define AW8898_CHIP_ID				0x1702
+
+/* System Status */
+#define AW8898_SYSST				0x01
+#define AW8898_SYSST_PLLS			GENMASK(0, 0)
+
+/* System Control */
+#define AW8898_SYSCTRL				0x04
+#define AW8898_SYSCTRL_MODE_MASK		GENMASK(7, 7)
+#define AW8898_SYSCTRL_RCV_MODE			(1)
+#define AW8898_SYSCTRL_SPK_MODE			(0)
+#define AW8898_SYSCTRL_PW_MASK			GENMASK(0, 0)
+#define AW8898_SYSCTRL_PW_PDN			(1)
+#define AW8898_SYSCTRL_PW_ACTIVE		(0)
+
+/* I2S Interface Control */
+#define AW8898_I2SCTRL				0x05
+#define AW8898_I2SCTRL_FMS_MASK			GENMASK(7, 6)
+#define AW8898_I2SCTRL_FMS_32BIT		(3)
+#define AW8898_I2SCTRL_FMS_24BIT		(2)
+#define AW8898_I2SCTRL_FMS_20BIT		(1)
+#define AW8898_I2SCTRL_FMS_16BIT		(0)
+#define AW8898_I2SCTRL_SR_MASK			GENMASK(3, 0)
+#define AW8898_I2SCTRL_SR_48K			(8)
+#define AW8898_I2SCTRL_SR_44P1K			(7)
+#define AW8898_I2SCTRL_SR_32K			(6)
+#define AW8898_I2SCTRL_SR_16K			(3)
+#define AW8898_I2SCTRL_SR_8K			(0)
+
+/* PWM Control */
+#define AW8898_PWMCTRL				0x08
+#define AW8898_PWMCTRL_HMUTE_MASK		GENMASK(0, 0)
+#define AW8898_PWMCTRL_HMUTE_ENABLE		(1)
+#define AW8898_PWMCTRL_HMUTE_DISABLE		(0)
+
+/* Hardware AGC Configuration 7 */
+#define AW8898_HAGCCFG7				0x0f
+#define AW8898_HAGCCFG7_VOL_REG_SHIFT		(8) /* bits 15-8 */
+#define AW8898_VOLUME_MAX			(255)
+#define AW8898_VOLUME_MIN			(0)
+
+#define AW8898_MAX_REGISTER			0xff
+
+#define AW8898_CFG_NAME				"aw8898_cfg.bin"
+
+#define AW8898_NUM_SUPPLIES	3
+static const char *aw8898_supply_names[AW8898_NUM_SUPPLIES] = {
+	"vdd",		/* Battery power */
+	"vddio",	/* Digital IO power */
+	"dvdd",		/* Digital power */
+};
+
+static const char * const aw8898_dev_mode_text[] = {
+	"Speaker",
+	"Receiver",
+};
+
+enum aw8898_mode {
+	AW8898_SPEAKER,
+	AW8898_RECEIVER,
+};
+
+static const struct soc_enum aw8898_dev_mode_enum =
+	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(aw8898_dev_mode_text), aw8898_dev_mode_text);
+
+struct aw8898 {
+	struct snd_soc_component *component;
+	struct regmap *regmap;
+	struct i2c_client *client;
+	struct mutex cfg_lock;
+	struct gpio_desc *reset;
+	struct regulator_bulk_data supplies[AW8898_NUM_SUPPLIES];
+	enum aw8898_mode dev_mode;
+	bool cfg_loaded;
+};
+
+struct aw8898_cfg {
+	int len;
+	struct {
+		__le16 addr;
+		__le16 val;
+	} __packed data[];
+};
+
+static void aw8898_set_mute(struct aw8898 *aw8898, bool mute)
+{
+	unsigned int val = AW8898_PWMCTRL_HMUTE_DISABLE;
+
+	if (mute)
+		val = AW8898_PWMCTRL_HMUTE_ENABLE;
+
+	regmap_update_bits(aw8898->regmap, AW8898_PWMCTRL,
+			   AW8898_PWMCTRL_HMUTE_MASK,
+			   FIELD_PREP(AW8898_PWMCTRL_HMUTE_MASK, val));
+}
+
+static void aw8898_set_power(struct aw8898 *aw8898, bool on)
+{
+	unsigned int val = AW8898_SYSCTRL_PW_PDN;
+
+	if (on)
+		val = AW8898_SYSCTRL_PW_ACTIVE;
+
+	regmap_update_bits(aw8898->regmap, AW8898_SYSCTRL,
+			   AW8898_SYSCTRL_PW_MASK,
+			   FIELD_PREP(AW8898_SYSCTRL_PW_MASK, val));
+}
+
+static void aw8898_update_dev_mode(struct aw8898 *aw8898)
+{
+	unsigned int mode = AW8898_SYSCTRL_SPK_MODE;
+
+	if (aw8898->dev_mode == AW8898_RECEIVER)
+		mode = AW8898_SYSCTRL_RCV_MODE;
+
+	regmap_update_bits(aw8898->regmap, AW8898_SYSCTRL,
+			   AW8898_SYSCTRL_MODE_MASK,
+			   FIELD_PREP(AW8898_SYSCTRL_MODE_MASK, mode));
+}
+
+static void aw8898_start(struct aw8898 *aw8898)
+{
+	unsigned int val;
+	int err;
+
+	aw8898_set_power(aw8898, true);
+
+	fsleep(2000);
+
+	err = regmap_read_poll_timeout(aw8898->regmap, AW8898_SYSST,
+				       val, val & AW8898_SYSST_PLLS,
+				       2000, 1 * USEC_PER_SEC);
+	if (err) {
+		dev_err(&aw8898->client->dev, "iis signal check error: %d\n", err);
+		aw8898_set_power(aw8898, false);
+		return;
+	}
+
+	aw8898_set_mute(aw8898, false);
+}
+
+static void aw8898_stop(struct aw8898 *aw8898)
+{
+	aw8898_set_mute(aw8898, true);
+	aw8898_set_power(aw8898, false);
+}
+
+static void aw8898_cfg_write(struct aw8898 *aw8898,
+			      struct aw8898_cfg *aw8898_cfg)
+{
+	for (int i = 0; i < aw8898_cfg->len; i++) {
+		unsigned int addr = __le32_to_cpu(aw8898_cfg->data[i].addr);
+		unsigned int val = __le32_to_cpu(aw8898_cfg->data[i].val);
+
+		dev_dbg(&aw8898->client->dev, "cfg reg = 0x%04x, val = 0x%04x\n", addr, val);
+		regmap_write(aw8898->regmap, addr, val);
+	}
+}
+
+static void aw8898_fw_loaded(const struct firmware *fw, void *context)
+{
+	struct aw8898 *aw8898 = context;
+	struct aw8898_cfg *aw8898_cfg;
+
+	if (!fw) {
+		dev_err(&aw8898->client->dev, "Failed to load firmware\n");
+		return;
+	}
+
+	dev_dbg(&aw8898->client->dev, "Loaded %s - size: %zu\n", AW8898_CFG_NAME, fw->size);
+
+	if (fw->size % 4 != 0) {
+		dev_err(&aw8898->client->dev, "Invalid firmware size %zu\n", fw->size);
+		release_firmware(fw);
+		return;
+	}
+
+	aw8898_cfg = kzalloc(sizeof(int) + fw->size, GFP_KERNEL);
+	if (!aw8898_cfg) {
+		release_firmware(fw);
+		return;
+	}
+
+	aw8898_cfg->len = fw->size / 4;
+	memcpy(aw8898_cfg->data, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	aw8898_cfg_write(aw8898, aw8898_cfg);
+
+	kfree(aw8898_cfg);
+
+	aw8898->cfg_loaded = true;
+
+	aw8898_update_dev_mode(aw8898);
+	aw8898_start(aw8898);
+}
+
+static void aw8898_cold_start(struct aw8898 *aw8898)
+{
+	int err;
+
+	err = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
+				      AW8898_CFG_NAME, &aw8898->client->dev, GFP_KERNEL,
+				      aw8898, aw8898_fw_loaded);
+	if (err)
+		dev_err(&aw8898->client->dev, "Firmware load request failed: %d\n", err);
+}
+
+static int aw8898_dev_mode_get(struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.enumerated.item[0] = aw8898->dev_mode;
+
+	return 0;
+}
+
+static int aw8898_dev_mode_put(struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
+
+	if (aw8898->dev_mode == ucontrol->value.enumerated.item[0])
+		return 0;
+
+	aw8898->dev_mode = ucontrol->value.enumerated.item[0];
+
+	aw8898_update_dev_mode(aw8898);
+
+	return 1;
+}
+
+/*
+ * -127.5 dB min, 0.5 dB steps, no mute
+ * Note: The official datasheet claims to be able to attenuate between 0 dB and
+ * -96 dB with 0.5 dB/step, but the register values are 0-255 so this doesn't
+ * really line up. It's a best guess.
+ */
+static const DECLARE_TLV_DB_SCALE(vol_tlv, -12750, 50, 0);
+
+static struct snd_kcontrol_new aw8898_controls[] = {
+	SOC_ENUM_EXT("Amp Mode", aw8898_dev_mode_enum,
+		     aw8898_dev_mode_get, aw8898_dev_mode_put),
+	SOC_SINGLE_RANGE_TLV("RX Volume", AW8898_HAGCCFG7,
+			     AW8898_HAGCCFG7_VOL_REG_SHIFT,
+			     AW8898_VOLUME_MIN, AW8898_VOLUME_MAX, 1, vol_tlv),
+};
+
+static int aw8898_startup(struct snd_pcm_substream *substream,
+			  struct snd_soc_dai *dai)
+{
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
+
+	aw8898_set_power(aw8898, true);
+
+	return 0;
+}
+
+static int aw8898_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = dai->component;
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		if ((fmt & SND_SOC_DAIFMT_MASTER_MASK)
+				!= SND_SOC_DAIFMT_CBC_CFC) {
+			dev_err(component->dev, "Invalid codec master mode: %d\n",
+				fmt & SND_SOC_DAIFMT_MASTER_MASK);
+			return -EINVAL;
+		}
+		break;
+	default:
+		dev_err(component->dev, "Unsupported DAI format %d\n",
+			fmt & SND_SOC_DAIFMT_FORMAT_MASK);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int aw8898_hw_params(struct snd_pcm_substream *substream,
+			    struct snd_pcm_hw_params *params,
+			    struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
+	unsigned int val;
+
+	switch (params_rate(params)) {
+	case 8000:
+		val = AW8898_I2SCTRL_SR_8K;
+		break;
+	case 16000:
+		val = AW8898_I2SCTRL_SR_16K;
+		break;
+	case 32000:
+		val = AW8898_I2SCTRL_SR_32K;
+		break;
+	case 44100:
+		val = AW8898_I2SCTRL_SR_44P1K;
+		break;
+	case 48000:
+		val = AW8898_I2SCTRL_SR_48K;
+		break;
+	default:
+		dev_err(component->dev, "Unsupported sample rate: %d\n",
+			params_rate(params));
+		return -EINVAL;
+	}
+
+	regmap_update_bits(aw8898->regmap, AW8898_I2SCTRL,
+			   AW8898_I2SCTRL_SR_MASK,
+			   FIELD_PREP(AW8898_I2SCTRL_SR_MASK, val));
+
+	switch (params_width(params)) {
+	case 16:
+		val = AW8898_I2SCTRL_FMS_16BIT;
+		break;
+	case 20:
+		val = AW8898_I2SCTRL_FMS_20BIT;
+		break;
+	case 24:
+		val = AW8898_I2SCTRL_FMS_24BIT;
+		break;
+	case 32:
+		val = AW8898_I2SCTRL_FMS_32BIT;
+		break;
+	default:
+		dev_err(component->dev, "Unsupported sample size: %d\n",
+			params_width(params));
+		return -EINVAL;
+	}
+
+	regmap_update_bits(aw8898->regmap, AW8898_I2SCTRL,
+			   AW8898_I2SCTRL_FMS_MASK,
+			   FIELD_PREP(AW8898_I2SCTRL_FMS_MASK, val));
+
+	return 0;
+}
+
+static int aw8898_mute(struct snd_soc_dai *dai, int mute, int stream)
+{
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
+
+	mutex_lock(&aw8898->cfg_lock);
+
+	if (mute) {
+		aw8898_stop(aw8898);
+	} else {
+		if (!aw8898->cfg_loaded) {
+			aw8898_cold_start(aw8898);
+		} else {
+			aw8898_update_dev_mode(aw8898);
+			aw8898_start(aw8898);
+		}
+	}
+
+	mutex_unlock(&aw8898->cfg_lock);
+
+	return 0;
+}
+
+static void aw8898_shutdown(struct snd_pcm_substream *substream,
+			    struct snd_soc_dai *dai)
+{
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
+
+	aw8898_set_power(aw8898, false);
+}
+
+static const struct snd_soc_dai_ops aw8898_dai_ops = {
+	.startup	= aw8898_startup,
+	.set_fmt	= aw8898_set_fmt,
+	.hw_params	= aw8898_hw_params,
+	.mute_stream	= aw8898_mute,
+	.shutdown	= aw8898_shutdown,
+};
+
+#define AW8898_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
+			SNDRV_PCM_FMTBIT_S24_LE | \
+			SNDRV_PCM_FMTBIT_S32_LE)
+
+static struct snd_soc_dai_driver aw8898_dai[] = {
+	{
+		.name = "aw8898-amplifier",
+		.playback = {
+			.stream_name = "Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_8000_48000,
+			.formats = AW8898_FORMATS,
+		},
+		.capture = {
+			.stream_name = "Capture",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_8000_48000,
+			.formats = AW8898_FORMATS,
+		},
+		.ops = &aw8898_dai_ops,
+		.symmetric_rate = 1,
+		.symmetric_channels = 1,
+		.symmetric_sample_bits = 1,
+	},
+};
+
+static int aw8898_component_probe(struct snd_soc_component *component)
+{
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	aw8898->component = component;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(aw8898->supplies),
+				    aw8898->supplies);
+	if (ret) {
+		dev_err(component->dev, "Failed to enable supplies: %d\n",
+			ret);
+		return ret;
+	}
+
+	snd_soc_add_component_controls(component, aw8898_controls,
+				       ARRAY_SIZE(aw8898_controls));
+
+	return 0;
+}
+
+static void aw8898_component_remove(struct snd_soc_component *component)
+{
+	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(aw8898->supplies),
+				     aw8898->supplies);
+	if (ret)
+		dev_err(component->dev, "Failed to disable supplies: %d\n",
+			ret);
+};
+
+static const struct snd_soc_component_driver soc_component_dev_aw8898 = {
+	.probe = aw8898_component_probe,
+	.remove = aw8898_component_remove,
+};
+
+static const struct regmap_config aw8898_regmap = {
+	.reg_bits = 8,
+	.val_bits = 16,
+
+	.max_register = AW8898_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static void aw8898_reset(struct aw8898 *aw8898)
+{
+	gpiod_set_value_cansleep(aw8898->reset, 1);
+	fsleep(1000);
+	gpiod_set_value_cansleep(aw8898->reset, 0);
+	fsleep(1000);
+}
+
+static int aw8898_check_chipid(struct aw8898 *aw8898)
+{
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(aw8898->regmap, AW8898_ID, &reg);
+	if (ret < 0) {
+		dev_err(&aw8898->client->dev,
+			"Failed to read register AW8898_ID: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(&aw8898->client->dev, "Read chip ID 0x%x\n", reg);
+
+	if (reg != AW8898_CHIP_ID) {
+		dev_err(&aw8898->client->dev, "Unexpected chip ID: 0x%x\n",
+			reg);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int aw8898_probe(struct i2c_client *client)
+{
+	struct aw8898 *aw8898;
+	int ret;
+
+	aw8898 = devm_kzalloc(&client->dev, sizeof(*aw8898), GFP_KERNEL);
+	if (!aw8898)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, aw8898);
+	aw8898->client = client;
+
+	aw8898->regmap = devm_regmap_init_i2c(client, &aw8898_regmap);
+	if (IS_ERR(aw8898->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(aw8898->regmap),
+				     "Failed to allocate register map\n");
+
+	ret = devm_mutex_init(&client->dev, &aw8898->cfg_lock);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to init mutex\n");
+
+	for (int i = 0; i < ARRAY_SIZE(aw8898->supplies); i++)
+		aw8898->supplies[i].supply = aw8898_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(aw8898->supplies),
+				      aw8898->supplies);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to get regulators\n");
+
+	aw8898->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(aw8898->reset))
+		return dev_err_probe(&client->dev, PTR_ERR(aw8898->reset),
+				     "Failed to get reset GPIO\n");
+
+	aw8898_reset(aw8898);
+
+	ret = aw8898_check_chipid(aw8898);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Chip ID check failed\n");
+
+	dev_set_drvdata(&client->dev, aw8898);
+
+	ret = devm_snd_soc_register_component(&client->dev, &soc_component_dev_aw8898,
+					      aw8898_dai, ARRAY_SIZE(aw8898_dai));
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret, "Failed to register component\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id aw8898_id[] = {
+	{ "aw8898" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, aw8898_id);
+
+static const struct of_device_id aw8898_of_match[] = {
+	{ .compatible = "awinic,aw8898" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, aw8898_of_match);
+
+static struct i2c_driver aw8898_driver = {
+	.driver = {
+		.name = "aw8898",
+		.of_match_table = of_match_ptr(aw8898_of_match),
+	},
+	.probe = aw8898_probe,
+	.id_table = aw8898_id,
+};
+
+module_i2c_driver(aw8898_driver);
+
+MODULE_DESCRIPTION("AW8898 Audio amplifier driver");
+MODULE_LICENSE("GPL");

-- 
2.49.0

Re: [PATCH 2/3] ASoC: codecs: Add aw8898 amplifier driver

[Krzysztof Kozlowski]

On 06/04/2025 15:03, Luca Weiss wrote:
> +static const struct i2c_device_id aw8898_id[] = {
> +	{ "aw8898" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, aw8898_id);
> +
> +static const struct of_device_id aw8898_of_match[] = {
> +	{ .compatible = "awinic,aw8898" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, aw8898_of_match);
> +
> +static struct i2c_driver aw8898_driver = {
> +	.driver = {
> +		.name = "aw8898",
> +		.of_match_table = of_match_ptr(aw8898_of_match),


Drop of_match_ptr.

Best regards,
Krzysztof

Re: [PATCH 2/3] ASoC: codecs: Add aw8898 amplifier driver

[Mark Brown]

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

On Sun, Apr 06, 2025 at 03:03:16PM +0200, Luca Weiss wrote:

Overall this driver feels like it's not terribly well integrated into
the subsystem - it's not using the standard framework features for
things.  The code itself looks broadly fine but things need moving about
a bit to feel more like a standard driver.

> @@ -0,0 +1,583 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2018 AWINIC Technology CO., LTD
> + * Author: Nick Li <liweilei@awinic.com.cn>
> + *
> + * Copyright (c) 2025 Luca Weiss <luca@lucaweiss.eu>
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static void aw8898_set_mute(struct aw8898 *aw8898, bool mute)
> +{
> +	unsigned int val = AW8898_PWMCTRL_HMUTE_DISABLE;
> +
> +	if (mute)
> +		val = AW8898_PWMCTRL_HMUTE_ENABLE;
> +
> +	regmap_update_bits(aw8898->regmap, AW8898_PWMCTRL,
> +			   AW8898_PWMCTRL_HMUTE_MASK,
> +			   FIELD_PREP(AW8898_PWMCTRL_HMUTE_MASK, val));
> +}

This should either be the standard DAI mute operation, user controlled
or there should be a clear explanation of what's going on.  Or just
inlined into callers given how trivial it is.

> +static void aw8898_set_power(struct aw8898 *aw8898, bool on)
> +{
> +	unsigned int val = AW8898_SYSCTRL_PW_PDN;
> +
> +	if (on)
> +		val = AW8898_SYSCTRL_PW_ACTIVE;
> +
> +	regmap_update_bits(aw8898->regmap, AW8898_SYSCTRL,
> +			   AW8898_SYSCTRL_PW_MASK,
> +			   FIELD_PREP(AW8898_SYSCTRL_PW_MASK, val));
> +}

This should be in the standard framework power management flows
(probably either a DAPM widget or set_bias_level(), it looks closer to
the latter).

> +static int aw8898_dev_mode_put(struct snd_kcontrol *kcontrol,
> +			       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
> +
> +	if (aw8898->dev_mode == ucontrol->value.enumerated.item[0])
> +		return 0;
> +
> +	aw8898->dev_mode = ucontrol->value.enumerated.item[0];
> +
> +	aw8898_update_dev_mode(aw8898);
> +
> +	return 1;
> +}

There is no validation of the written value here.  Please use the
mixer-test selftest to vaidate things.

> +/*
> + * -127.5 dB min, 0.5 dB steps, no mute
> + * Note: The official datasheet claims to be able to attenuate between 0 dB and
> + * -96 dB with 0.5 dB/step, but the register values are 0-255 so this doesn't
> + * really line up. It's a best guess.
> + */

It is common for registers to have out of spec values which should never
be written, they might behave in undesirable ways.  The controls should
only expose whatever the documented values are.

> +static int aw8898_startup(struct snd_pcm_substream *substream,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
> +
> +	aw8898_set_power(aw8898, true);
> +
> +	return 0;
> +}

As noted above the power should be managed from power management
operations, either set_bias_level() or DAPM.

> +static int aw8898_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct snd_soc_component *component = dai->component;
> +
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		if ((fmt & SND_SOC_DAIFMT_MASTER_MASK)

_CLOCK_PROVIDER_MASK.

> +static int aw8898_mute(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(dai->component);
> +
> +	mutex_lock(&aw8898->cfg_lock);
> +
> +	if (mute) {
> +		aw8898_stop(aw8898);
> +	} else {
> +		if (!aw8898->cfg_loaded) {
> +			aw8898_cold_start(aw8898);
> +		} else {
> +			aw8898_update_dev_mode(aw8898);
> +			aw8898_start(aw8898);
> +		}
> +	}
> +
> +	mutex_unlock(&aw8898->cfg_lock);

A mute operation should not be doing anything as expensive as this, it
should just be something more like a single register write (eg, just the
mute).  This looks more like power management operations.  

> +static int aw8898_component_probe(struct snd_soc_component *component)
> +{
> +	struct aw8898 *aw8898 = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	aw8898->component = component;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(aw8898->supplies),
> +				    aw8898->supplies);
> +	if (ret) {
> +		dev_err(component->dev, "Failed to enable supplies: %d\n",
> +			ret);
> +		return ret;
> +	}

It seems random to do this separately to the rest of the power
management for the device?

> +	snd_soc_add_component_controls(component, aw8898_controls,
> +				       ARRAY_SIZE(aw8898_controls));

Just specify the controls in the driver struct when registering.

> +static const struct regmap_config aw8898_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +
> +	.max_register = AW8898_MAX_REGISTER,
> +	.cache_type = REGCACHE_RBTREE,
> +};

Use _MAPLE unless you have a particular reason not to, it's a more
modern data structure.

> +static int aw8898_probe(struct i2c_client *client)
> +{
> +	struct aw8898 *aw8898;
> +	int ret;

> +	aw8898->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(aw8898->reset))
> +		return dev_err_probe(&client->dev, PTR_ERR(aw8898->reset),
> +				     "Failed to get reset GPIO\n");
> +
> +	aw8898_reset(aw8898);
> +
> +	ret = aw8898_check_chipid(aw8898);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Chip ID check failed\n");

We didn't power on the regulators before trying to validate the chip ID
which probably isn't going to go terribly well if the regulators are
controllable...

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

[PATCH 3/3] arm64: dts: qcom: sdm632-fairphone-fp3: Add AW8898 amplifier

[Luca Weiss]

Add a node for the amplifier found on Fairphone 3, receiving sound via
I2S from the SoC and being connected to the speaker.

Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
---
 arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts b/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
index 31ed26c31e6ea381a8942ccf569513df3300cdeb..ef6f8c2bb4a5eb8e93aba0a4cbfce5f8ae2ba21a 100644
--- a/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
+++ b/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
@@ -81,6 +81,26 @@ nfc@28 {
 	};
 };
 
+&i2c_6 {
+	status = "okay";
+
+	aw8898: audio-amplifier@34 {
+		compatible = "awinic,aw8898";
+		reg = <0x34>;
+
+		reset-gpios = <&tlmm 21 GPIO_ACTIVE_LOW>;
+		interrupts-extended = <&tlmm 20 IRQ_TYPE_LEVEL_HIGH>;
+
+		dvdd-supply = <&pm8953_l5>;
+		vddio-supply = <&pm8953_l5>;
+		vdd-supply = <&vph_pwr>;
+
+		#sound-dai-cells = <0>;
+	};
+
+	/* Fairphone 3+ only: Speaker amp TAS2557 @ 4c */
+};
+
 &lpass {
 	firmware-name = "qcom/msm8953/fairphone/fp3/adsp.mbn";
 

-- 
2.49.0

Re: [PATCH 3/3] arm64: dts: qcom: sdm632-fairphone-fp3: Add AW8898 amplifier

[Krzysztof Kozlowski]

On 06/04/2025 15:03, Luca Weiss wrote:
> Add a node for the amplifier found on Fairphone 3, receiving sound via
> I2S from the SoC and being connected to the speaker.
> 
> Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/3] arm64: dts: qcom: sdm632-fairphone-fp3: Add AW8898 amplifier

[Konrad Dybcio]

On 4/6/25 3:03 PM, Luca Weiss wrote:
> Add a node for the amplifier found on Fairphone 3, receiving sound via
> I2S from the SoC and being connected to the speaker.
> 
> Signed-off-by: Luca Weiss <luca@lucaweiss.eu>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: (subset) [PATCH 0/3] Add support for AW8898 Amplifier

[Bjorn Andersson]

On Sun, 06 Apr 2025 15:03:14 +0200, Luca Weiss wrote:
> Add devicetree bindings and a driver for the AW8898 Amplifier, and add
> it to the devicetree for the Fairphone 3 smartphone.
> 
> Note, that this is the first audio driver I've worked on, so please
> complain if something doesn't quite look right, it probably wasn't done
> on purpose.
> 
> [...]

Applied, thanks!

[3/3] arm64: dts: qcom: sdm632-fairphone-fp3: Add AW8898 amplifier
      commit: 87ab84799a90bb73f3f42ae636b213ac4564e237

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>