[PATCH 0/6] ASoC: Add NTP8918 and NTP8835 codecs support

[PATCH 0/6] ASoC: Add NTP8918 and NTP8835 codecs support

[Igor Prusov]

This series adds support for two NeoFidelity amplifiers. For both
amplifiers vendor provides software for equalizer and filters
configuration, which generates firmware files with registers values.
Since in both cases those files have same encoding, a common helper
module is added to get firmware via request_firmware() API and set
registers values.

Igor Prusov (6):
  dt-bindings: vendor-prefixes: Add NeoFidelity, Inc
  ASoC: codecs: Add NeoFidelity Firmware helpers
  ASoC: dt-bindings: Add bindings for NeoFidelity NTP8918
  ASoC: codecs: Add NeoFidelity NTP8918 codec
  ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835
  ASoC: codecs: Add NeoFidelity NTP8835 codec

 .../bindings/sound/neofidelity,ntp8835.yaml   |  65 +++
 .../bindings/sound/neofidelity,ntp8918.yaml   |  63 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 sound/soc/codecs/Kconfig                      |  13 +
 sound/soc/codecs/Makefile                     |   6 +
 sound/soc/codecs/ntp8835.c                    | 432 ++++++++++++++++++
 sound/soc/codecs/ntp8918.c                    | 356 +++++++++++++++
 sound/soc/codecs/ntpfw.c                      | 137 ++++++
 sound/soc/codecs/ntpfw.h                      |  23 +
 9 files changed, 1097 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml
 create mode 100644 sound/soc/codecs/ntp8835.c
 create mode 100644 sound/soc/codecs/ntp8918.c
 create mode 100644 sound/soc/codecs/ntpfw.c
 create mode 100644 sound/soc/codecs/ntpfw.h

-- 
2.34.1

[PATCH 1/6] dt-bindings: vendor-prefixes: Add NeoFidelity, Inc

[Igor Prusov]

Add vendor prefix for NeoFidelity, Inc

Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index fbf47f0bacf1..be62a59788b5 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -997,6 +997,8 @@ patternProperties:
     description: National Semiconductor
   "^nec,.*":
     description: NEC LCD Technologies, Ltd.
+  "^neofidelity,.*":
+    description: Neofidelity Inc.
   "^neonode,.*":
     description: Neonode Inc.
   "^netgear,.*":
-- 
2.34.1

[PATCH 2/6] ASoC: codecs: Add NeoFidelity Firmware helpers

[Igor Prusov]

Add support for loading firmware for NeoFidelity amplifiers.

Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 sound/soc/codecs/Kconfig  |   3 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/ntpfw.c  | 137 ++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/ntpfw.h  |  23 +++++++
 4 files changed, 165 insertions(+)
 create mode 100644 sound/soc/codecs/ntpfw.c
 create mode 100644 sound/soc/codecs/ntpfw.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 4afc43d3f71f..9583243f1966 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -2492,6 +2492,9 @@ config SND_SOC_NAU8825
 	tristate
 	depends on I2C
 
+config SND_SOC_NTPFW
+	tristate
+
 config SND_SOC_TPA6130A2
 	tristate "Texas Instruments TPA6130A2 headphone amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index b4df22186e25..eae4ab047c72 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -183,6 +183,7 @@ snd-soc-nau8821-y := nau8821.o
 snd-soc-nau8822-y := nau8822.o
 snd-soc-nau8824-y := nau8824.o
 snd-soc-nau8825-y := nau8825.o
+snd-soc-ntpfw-y := ntpfw.o
 snd-soc-hdmi-codec-y := hdmi-codec.o
 snd-soc-pcm1681-y := pcm1681.o
 snd-soc-pcm1789-codec-y := pcm1789.o
@@ -575,6 +576,7 @@ obj-$(CONFIG_SND_SOC_NAU8821)   += snd-soc-nau8821.o
 obj-$(CONFIG_SND_SOC_NAU8822)   += snd-soc-nau8822.o
 obj-$(CONFIG_SND_SOC_NAU8824)   += snd-soc-nau8824.o
 obj-$(CONFIG_SND_SOC_NAU8825)   += snd-soc-nau8825.o
+obj-$(CONFIG_SND_SOC_NTPFW)	+= snd-soc-ntpfw.o
 obj-$(CONFIG_SND_SOC_HDMI_CODEC)	+= snd-soc-hdmi-codec.o
 obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
 obj-$(CONFIG_SND_SOC_PCM179X)	+= snd-soc-pcm179x-codec.o
diff --git a/sound/soc/codecs/ntpfw.c b/sound/soc/codecs/ntpfw.c
new file mode 100644
index 000000000000..beaf300fe59c
--- /dev/null
+++ b/sound/soc/codecs/ntpfw.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/**
+ * ntpfw.c - Firmware helper functions for Neofidelity codecs
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ */
+
+#include <linux/i2c.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+
+#include "ntpfw.h"
+
+struct ntpfw_chunk {
+	__be16 length;
+	u8 step;
+	u8 data[];
+} __packed;
+
+struct ntpfw_header {
+	__be32 magic;
+} __packed;
+
+static bool ntpfw_verify(struct device *dev, const u8 *buf, size_t buf_size, u32 magic)
+{
+	const struct ntpfw_header *header = (struct ntpfw_header *)buf;
+	u32 buf_magic;
+
+	if (buf_size <= sizeof(*header)) {
+		dev_err(dev, "Failed to load firmware: image too small\n");
+		return false;
+	}
+
+	buf_magic = be32_to_cpu(header->magic);
+	if (buf_magic != magic) {
+		dev_err(dev, "Failed to load firmware: invalid magic 0x%x:\n", buf_magic);
+		return false;
+	}
+
+	return true;
+}
+
+static bool ntpfw_verify_chunk(struct device *dev, const struct ntpfw_chunk *chunk, size_t buf_size)
+{
+	size_t chunk_size;
+
+	if (buf_size <= sizeof(*chunk)) {
+		dev_err(dev, "Failed to load firmware: chunk size too big\n");
+		return false;
+	}
+
+	if (chunk->step != 2 && chunk->step != 5) {
+		dev_err(dev, "Failed to load firmware: invalid chunk step: %d\n", chunk->step);
+		return false;
+	}
+
+	chunk_size = be16_to_cpu(chunk->length);
+	if (chunk_size > buf_size) {
+		dev_err(dev, "Failed to load firmware: invalid chunk length\n");
+		return false;
+	}
+
+	if (chunk_size % chunk->step) {
+		dev_err(dev, "Failed to load firmware: chunk length and step mismatch\n");
+		return false;
+	}
+
+	return true;
+}
+
+static int ntpfw_send_chunk(struct i2c_client *i2c, const struct ntpfw_chunk *chunk)
+{
+	int ret;
+	size_t i;
+	size_t length = be16_to_cpu(chunk->length);
+
+	for (i = 0; i < length; i += chunk->step) {
+		ret = i2c_master_send(i2c, &chunk->data[i], chunk->step);
+		if (ret != chunk->step) {
+			dev_err(&i2c->dev, "I2C send failed: %d\n", ret);
+			return ret < 0 ? ret : -EIO;
+		}
+	}
+
+	return 0;
+}
+
+int ntpfw_load(struct i2c_client *i2c, const char *name, u32 magic)
+{
+	struct device *dev = &i2c->dev;
+	const struct ntpfw_chunk *chunk;
+	const struct firmware *fw;
+	const u8 *data;
+	size_t leftover;
+	int ret;
+
+	ret = request_firmware(&fw, name, dev);
+	if (ret) {
+		dev_warn(dev, "request_firmware '%s' failed with %d\n",
+			 name, ret);
+		return ret;
+	}
+
+	if (!ntpfw_verify(dev, fw->data, fw->size, magic)) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	data = fw->data + sizeof(struct ntpfw_header);
+	leftover = fw->size - sizeof(struct ntpfw_header);
+
+	while (leftover) {
+		chunk = (struct ntpfw_chunk *)data;
+
+		if (!ntpfw_verify_chunk(dev, chunk, leftover)) {
+			ret = -EINVAL;
+			goto done;
+		}
+
+		ret = ntpfw_send_chunk(i2c, chunk);
+		if (ret)
+			goto done;
+
+		data += be16_to_cpu(chunk->length) + sizeof(*chunk);
+		leftover -= be16_to_cpu(chunk->length) + sizeof(*chunk);
+	}
+
+done:
+	release_firmware(fw);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ntpfw_load);
+
+MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
+MODULE_DESCRIPTION("Helper for loading Neofidelity amplifiers firmware");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/ntpfw.h b/sound/soc/codecs/ntpfw.h
new file mode 100644
index 000000000000..1cf10d5480ee
--- /dev/null
+++ b/sound/soc/codecs/ntpfw.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/**
+ * ntpfw.h - Firmware helper functions for Neofidelity codecs
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ */
+
+#ifndef __NTPFW_H__
+#define __NTPFW_H__
+#include <linux/i2c.h>
+#include <linux/firmware.h>
+
+/**
+ * ntpfw_load - load firmware to amplifier over i2c interface.
+ *
+ * @i2c		Pointer to amplifier's I2C client.
+ * @name	Firmware file name.
+ * @magic	Magic number to validate firmware.
+ * @return	0 or error code upon error.
+ */
+int ntpfw_load(struct i2c_client *i2c, const char *name, const u32 magic);
+
+#endif /* __NTPFW_H__ */
-- 
2.34.1

[PATCH 3/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8918

[Igor Prusov]

Add dt-bindings for NeoFidelity NTP8918 Amplifier

Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 .../bindings/sound/neofidelity,ntp8918.yaml   | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml

diff --git a/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml b/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml
new file mode 100644
index 000000000000..4ca8957309d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/neofidelity,ntp8918.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NeoFidelity NTP8918 Amplifier
+
+maintainers:
+  - Igor Prusov <ivprusov@salutedevices.com>
+
+description: |
+  The NTP8918 is a single chip full digital audio amplifier
+  including power stage for stereo amplifier system.
+  The NTP8918 is integrated with versatile digital audio signal
+  processing functions, high-performance, high-fidelity fully
+  digital PWM modulator and two high-power full-bridge MOSFET
+  power stages.
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - neofidelity,ntp8918
+
+  reg:
+    maxItems: 1
+    enum:
+      - 0x2a
+      - 0x2b
+      - 0x2c
+      - 0x2d
+    description: |
+      I2C address of the device.
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO used to control the state of the device.
+
+  '#sound-dai-cells':
+    enum: [0]
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/gpio/gpio.h>
+   i2c {
+     #address-cells = <1>;
+     #size-cells = <0>;
+     ntp8918@54 {
+       compatible = "neofidelity,ntp8918";
+       #sound-dai-cells = <0>;
+       reg = <0x54>;
+       reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+    };
+   };
-- 
2.34.1

[PATCH 4/6] ASoC: codecs: Add NeoFidelity NTP8918 codec

[Igor Prusov]

The NeoFidelity NTP8918 is a two channel amplifier with mixer and
biquad filters.

Datasheet: https://datasheetspdf.com/pdf-down/N/T/P/NTP8918-NeoFidelity.pdf
Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 sound/soc/codecs/Kconfig   |   5 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/ntp8918.c | 356 +++++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 sound/soc/codecs/ntp8918.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9583243f1966..d16c983fcb7a 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -2495,6 +2495,11 @@ config SND_SOC_NAU8825
 config SND_SOC_NTPFW
 	tristate
 
+config SND_SOC_NTP8918
+	select SND_SOC_NTPFW
+	tristate "NeoFidelity NTP8918 amplifier"
+	depends on I2C
+
 config SND_SOC_TPA6130A2
 	tristate "Texas Instruments TPA6130A2 headphone amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index eae4ab047c72..a49ab11a98ec 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -183,6 +183,7 @@ snd-soc-nau8821-y := nau8821.o
 snd-soc-nau8822-y := nau8822.o
 snd-soc-nau8824-y := nau8824.o
 snd-soc-nau8825-y := nau8825.o
+snd-soc-ntp8918-y := ntp8918.o
 snd-soc-ntpfw-y := ntpfw.o
 snd-soc-hdmi-codec-y := hdmi-codec.o
 snd-soc-pcm1681-y := pcm1681.o
@@ -576,6 +577,7 @@ obj-$(CONFIG_SND_SOC_NAU8821)   += snd-soc-nau8821.o
 obj-$(CONFIG_SND_SOC_NAU8822)   += snd-soc-nau8822.o
 obj-$(CONFIG_SND_SOC_NAU8824)   += snd-soc-nau8824.o
 obj-$(CONFIG_SND_SOC_NAU8825)   += snd-soc-nau8825.o
+obj-$(CONFIG_SND_SOC_NTP8918)	+= snd-soc-ntp8918.o
 obj-$(CONFIG_SND_SOC_NTPFW)	+= snd-soc-ntpfw.o
 obj-$(CONFIG_SND_SOC_HDMI_CODEC)	+= snd-soc-hdmi-codec.o
 obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
diff --git a/sound/soc/codecs/ntp8918.c b/sound/soc/codecs/ntp8918.c
new file mode 100644
index 000000000000..185f8b235138
--- /dev/null
+++ b/sound/soc/codecs/ntp8918.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the NTP8918 Audio Amplifier
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ *
+ * Author: Igor Prusov <ivprusov@salutedevices.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/reset.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+
+#include <sound/initval.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-component.h>
+#include <sound/tlv.h>
+
+#include "ntpfw.h"
+
+#define NTP8918_RATES   (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
+			 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
+
+#define NTP8918_FORMATS     (SNDRV_PCM_FMTBIT_S16_LE | \
+			     SNDRV_PCM_FMTBIT_S20_3LE | \
+			     SNDRV_PCM_FMTBIT_S24_LE | \
+			     SNDRV_PCM_FMTBIT_S32_LE)
+
+#define NTP8918_INPUT_FMT			0x0
+#define  NTP8918_INPUT_FMT_MASTER_MODE		BIT(0)
+#define  NTP8918_INPUT_FMT_GSA_MODE		BIT(1)
+#define NTP8918_GSA_FMT				0x1
+#define  NTP8918_GSA_BS_MASK			GENMASK(3, 2)
+#define  NTP8918_GSA_BS(x)			((x) << 2)
+#define  NTP8918_GSA_RIGHT_J			BIT(0)
+#define  NTP8918_GSA_LSB			BIT(1)
+#define NTP8918_MASTER_VOL			0x0C
+#define NTP8918_CHNL_A_VOL			0x17
+#define NTP8918_CHNL_B_VOL			0x18
+#define NTP8918_SOFT_MUTE			0x33
+#define REG_MAX					NTP8918_SOFT_MUTE
+
+#define NTP8918_FW_NAME		"eq_8918.bin"
+#define NTP8918_FW_MAGIC	0x38393138	/* "8918" */
+
+struct ntp8918_priv {
+	struct i2c_client *i2c;
+	struct reset_control *reset;
+	unsigned int format;
+};
+
+static const DECLARE_TLV_DB_SCALE(ntp8918_master_vol_scale, -12550, 50, 0);
+
+static const struct snd_kcontrol_new ntp8918_vol_control[] = {
+	SOC_SINGLE_RANGE_TLV("Playback Volume", NTP8918_MASTER_VOL, 0,
+			     0x04, 0xff, 0, ntp8918_master_vol_scale),
+	SOC_DOUBLE("Playback Switch", NTP8918_SOFT_MUTE, 1, 0, 1, 1),
+};
+
+static void ntp8918_reset_gpio(struct ntp8918_priv *ntp8918, bool active)
+{
+	if (active) {
+		/*
+		 * According to NTP8918 datasheet, 6.2 Timing Sequence 1:
+		 * Deassert for T2 >= 1ms...
+		 */
+		reset_control_deassert(ntp8918->reset);
+		fsleep(1000);
+
+		/* ...Assert for T3 >= 0.1us... */
+		reset_control_assert(ntp8918->reset);
+		fsleep(1);
+
+		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
+		reset_control_deassert(ntp8918->reset);
+		fsleep(500);
+	} else {
+		reset_control_assert(ntp8918->reset);
+	}
+}
+
+static const struct reg_sequence ntp8918_sound_off[] = {
+	{ NTP8918_MASTER_VOL, 0 },
+};
+
+static const struct reg_sequence ntp8918_sound_on[] = {
+	{ NTP8918_MASTER_VOL, 0b11 },
+};
+
+static int ntp8918_load_firmware(struct ntp8918_priv *ntp8918)
+{
+	int ret;
+
+	ret = ntpfw_load(ntp8918->i2c, NTP8918_FW_NAME, NTP8918_FW_MAGIC);
+	if (ret == -ENOENT) {
+		dev_warn_once(&ntp8918->i2c->dev, "Could not find firmware %s\n",
+			      NTP8918_FW_NAME);
+		return 0;
+	}
+
+	return ret;
+}
+
+static int ntp8918_snd_suspend(struct snd_soc_component *component)
+{
+	struct ntp8918_priv *ntp8918 = snd_soc_component_get_drvdata(component);
+
+	regcache_cache_only(component->regmap, true);
+
+	regmap_multi_reg_write_bypassed(component->regmap,
+					ntp8918_sound_off,
+					ARRAY_SIZE(ntp8918_sound_off));
+
+	/*
+	 * According to NTP8918 datasheet, 6.2 Timing Sequence 1:
+	 * wait after sound off for T6 >= 0.5ms
+	 */
+	fsleep(500);
+	ntp8918_reset_gpio(ntp8918, false);
+
+	regcache_mark_dirty(component->regmap);
+
+	return 0;
+}
+
+static int ntp8918_snd_resume(struct snd_soc_component *component)
+{
+	struct ntp8918_priv *ntp8918 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ntp8918_reset_gpio(ntp8918, true);
+
+	regmap_multi_reg_write_bypassed(component->regmap,
+					ntp8918_sound_on,
+					ARRAY_SIZE(ntp8918_sound_on));
+
+	ret = ntp8918_load_firmware(ntp8918);
+	if (ret) {
+		dev_err(&ntp8918->i2c->dev, "Failed to load firmware\n");
+		return ret;
+	}
+
+	regcache_cache_only(component->regmap, false);
+	snd_soc_component_cache_sync(component);
+
+	return 0;
+}
+
+static int ntp8918_probe(struct snd_soc_component *component)
+{
+	int ret;
+	struct ntp8918_priv *ntp8918 = snd_soc_component_get_drvdata(component);
+	struct device *dev = component->dev;
+
+	ret = snd_soc_add_component_controls(component, ntp8918_vol_control,
+					     ARRAY_SIZE(ntp8918_vol_control));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add controls\n");
+
+	ret = ntp8918_load_firmware(ntp8918);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to load firmware\n");
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget ntp8918_dapm_widgets[] = {
+	SND_SOC_DAPM_DAC("AIFIN", "Playback", SND_SOC_NOPM, 0, 0),
+
+	SND_SOC_DAPM_OUTPUT("OUT1"),
+	SND_SOC_DAPM_OUTPUT("OUT2"),
+};
+
+static const struct snd_soc_dapm_route ntp8918_dapm_routes[] = {
+	{ "OUT1", NULL, "AIFIN" },
+	{ "OUT2", NULL, "AIFIN" },
+};
+
+static const struct snd_soc_component_driver soc_component_ntp8918 = {
+	.probe = ntp8918_probe,
+	.suspend = ntp8918_snd_suspend,
+	.resume = ntp8918_snd_resume,
+	.dapm_widgets = ntp8918_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(ntp8918_dapm_widgets),
+	.dapm_routes = ntp8918_dapm_routes,
+	.num_dapm_routes = ARRAY_SIZE(ntp8918_dapm_routes),
+};
+
+static int ntp8918_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 ntp8918_priv *ntp8918 = snd_soc_component_get_drvdata(component);
+	unsigned int input_fmt = 0;
+	unsigned int gsa_fmt = 0;
+	unsigned int gsa_fmt_mask;
+	int ret;
+
+	switch (ntp8918->format) {
+	case SND_SOC_DAIFMT_I2S:
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		input_fmt |= NTP8918_INPUT_FMT_GSA_MODE;
+		gsa_fmt |= NTP8918_GSA_RIGHT_J;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		input_fmt |= NTP8918_INPUT_FMT_GSA_MODE;
+		break;
+	}
+
+	ret = snd_soc_component_update_bits(component, NTP8918_INPUT_FMT,
+					    NTP8918_INPUT_FMT_MASTER_MODE |
+					    NTP8918_INPUT_FMT_GSA_MODE,
+					    input_fmt);
+
+	if (!(input_fmt & NTP8918_INPUT_FMT_GSA_MODE) || ret < 0)
+		return ret;
+
+	switch (params_width(params)) {
+	case 24:
+		gsa_fmt |= NTP8918_GSA_BS(0);
+		break;
+	case 20:
+		gsa_fmt |= NTP8918_GSA_BS(1);
+		break;
+	case 18:
+		gsa_fmt |= NTP8918_GSA_BS(2);
+		break;
+	case 16:
+		gsa_fmt |= NTP8918_GSA_BS(3);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	gsa_fmt_mask = NTP8918_GSA_BS_MASK |
+		       NTP8918_GSA_RIGHT_J |
+		       NTP8918_GSA_LSB;
+	return snd_soc_component_update_bits(component, NTP8918_GSA_FMT,
+					     gsa_fmt_mask, gsa_fmt);
+}
+
+static int ntp8918_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = dai->component;
+	struct ntp8918_priv *ntp8918 = snd_soc_component_get_drvdata(component);
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+	case SND_SOC_DAIFMT_RIGHT_J:
+	case SND_SOC_DAIFMT_LEFT_J:
+		ntp8918->format = fmt & SND_SOC_DAIFMT_FORMAT_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+struct snd_soc_dai_ops ntp8918_dai_ops = {
+	.hw_params = ntp8918_hw_params,
+	.set_fmt = ntp8918_set_fmt,
+};
+
+static struct snd_soc_dai_driver ntp8918_dai = {
+	.name = "ntp8918-amplifier",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = NTP8918_RATES,
+		.formats = NTP8918_FORMATS,
+	},
+	.ops = &ntp8918_dai_ops,
+};
+
+static struct regmap_config ntp8918_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_MAX,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int ntp8918_i2c_probe(struct i2c_client *i2c)
+{
+	struct ntp8918_priv *ntp8918;
+	int ret;
+	struct regmap *regmap;
+
+	ntp8918 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8918_priv), GFP_KERNEL);
+	if (!ntp8918)
+		return -ENOMEM;
+
+	ntp8918->i2c = i2c;
+
+	ntp8918->reset = devm_reset_control_get_shared(&i2c->dev, NULL);
+	if (IS_ERR(ntp8918->reset))
+		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8918->reset), "Failed to get reset\n");
+
+	dev_set_drvdata(&i2c->dev, ntp8918);
+
+	ntp8918_reset_gpio(ntp8918, true);
+
+	regmap = devm_regmap_init_i2c(i2c, &ntp8918_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
+				     "Failed to allocate regmap\n");
+
+	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8918,
+					      &ntp8918_dai, 1);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to register component\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id ntp8918_i2c_id[] = {
+	{ "ntp8918", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ntp8918_i2c_id);
+
+static const struct of_device_id ntp8918_of_match[] = {
+	{.compatible = "neofidelity,ntp8918"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ntp8918_of_match);
+
+static struct i2c_driver ntp8918_i2c_driver = {
+	.probe = ntp8918_i2c_probe,
+	.id_table = ntp8918_i2c_id,
+	.driver = {
+		.name = "NTP8918",
+		.of_match_table = ntp8918_of_match,
+	},
+};
+module_i2c_driver(ntp8918_i2c_driver);
+
+MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
+MODULE_DESCRIPTION("NTP8918 Audio Amplifier Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH 5/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835

[Igor Prusov]

Add dt-bindings for NeoFidelity NTP8835C/NTP8835C Amplifiers

Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 .../bindings/sound/neofidelity,ntp8835.yaml   | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml

diff --git a/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml b/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml
new file mode 100644
index 000000000000..6dbe0eb2e93d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/neofidelity,ntp8835.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NeoFidelity NTP8835/NTP8835C Amplifiers
+
+maintainers:
+  - Igor Prusov <ivprusov@salutedevices.com>
+
+description: |
+  The NTP8835 is a single chip full digital audio amplifier
+  including power stages for stereo amplifier systems.
+  NTP8835 is integrated with versatile digital audio signal
+  processing functions, high-performance, high-fidelity fully
+  digital PWM modulator and two high-power full-bridge MOSFET
+  power stages. NTP8835C has identical programming interface,
+  but has different output signal characteristics.
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - neofidelity,ntp8835
+      - neofidelity,ntp8835c
+
+  reg:
+    enum:
+      - 0x2a
+      - 0x2b
+      - 0x2c
+      - 0x2d
+    maxItems: 1
+    description: |
+      I2C address of the device.
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO used to control the state of the device.
+
+  '#sound-dai-cells':
+    enum: [0]
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/gpio/gpio.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ntp8835@56 {
+        compatible = "neofidelity,ntp8835";
+        #sound-dai-cells = <0>;
+        reg = <0x56>;
+        reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+      };
+    };
-- 
2.34.1

[PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Igor Prusov]

The NeoFidelity NTP8835 adn NTP8835C are 2.1 channel amplifiers with
mixer and biquad filters. Both amplifiers have identical programming
interfaces but differ in output signal characteristics.

Datasheet: https://www.cpbay.com/Uploads/20210225/6037116a3ea91.pdf
Datasheet: https://www.cpbay.com/Uploads/20210918/61458b2f2631e.pdf
Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
---
 sound/soc/codecs/Kconfig   |   5 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/ntp8835.c | 432 +++++++++++++++++++++++++++++++++++++
 3 files changed, 439 insertions(+)
 create mode 100644 sound/soc/codecs/ntp8835.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index d16c983fcb7a..2235727f0d41 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -2500,6 +2500,11 @@ config SND_SOC_NTP8918
 	tristate "NeoFidelity NTP8918 amplifier"
 	depends on I2C
 
+config SND_SOC_NTP8835
+	select SND_SOC_NTPFW
+	tristate "NeoFidelity NTP8835 and NTP8835C amplifiers"
+	depends on I2C
+
 config SND_SOC_TPA6130A2
 	tristate "Texas Instruments TPA6130A2 headphone amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index a49ab11a98ec..49fc78bc631e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -183,6 +183,7 @@ snd-soc-nau8821-y := nau8821.o
 snd-soc-nau8822-y := nau8822.o
 snd-soc-nau8824-y := nau8824.o
 snd-soc-nau8825-y := nau8825.o
+snd-soc-ntp8835-y := ntp8835.o
 snd-soc-ntp8918-y := ntp8918.o
 snd-soc-ntpfw-y := ntpfw.o
 snd-soc-hdmi-codec-y := hdmi-codec.o
@@ -577,6 +578,7 @@ obj-$(CONFIG_SND_SOC_NAU8821)   += snd-soc-nau8821.o
 obj-$(CONFIG_SND_SOC_NAU8822)   += snd-soc-nau8822.o
 obj-$(CONFIG_SND_SOC_NAU8824)   += snd-soc-nau8824.o
 obj-$(CONFIG_SND_SOC_NAU8825)   += snd-soc-nau8825.o
+obj-$(CONFIG_SND_SOC_NTP8835)	+= snd-soc-ntp8835.o
 obj-$(CONFIG_SND_SOC_NTP8918)	+= snd-soc-ntp8918.o
 obj-$(CONFIG_SND_SOC_NTPFW)	+= snd-soc-ntpfw.o
 obj-$(CONFIG_SND_SOC_HDMI_CODEC)	+= snd-soc-hdmi-codec.o
diff --git a/sound/soc/codecs/ntp8835.c b/sound/soc/codecs/ntp8835.c
new file mode 100644
index 000000000000..ec16660478c2
--- /dev/null
+++ b/sound/soc/codecs/ntp8835.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the NTP8835/NTP8835C Audio Amplifiers
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ *
+ * Author: Igor Prusov <ivprusov@salutedevices.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/bits.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/reset.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#include <sound/initval.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-component.h>
+#include <sound/tlv.h>
+
+#include "ntpfw.h"
+
+#define NTP8835_FORMATS     (SNDRV_PCM_FMTBIT_S16_LE | \
+			     SNDRV_PCM_FMTBIT_S20_3LE | \
+			     SNDRV_PCM_FMTBIT_S24_LE | \
+			     SNDRV_PCM_FMTBIT_S32_LE)
+
+#define NTP8835_INPUT_FMT			0x0
+#define  NTP8835_INPUT_FMT_MASTER_MODE		BIT(0)
+#define  NTP8835_INPUT_FMT_GSA_MODE		BIT(1)
+#define NTP8835_GSA_FMT				0x1
+#define  NTP8835_GSA_BS_MASK			GENMASK(3, 2)
+#define  NTP8835_GSA_BS(x)			((x) << 2)
+#define  NTP8835_GSA_RIGHT_J			BIT(0)
+#define  NTP8835_GSA_LSB			BIT(1)
+#define NTP8835_SOFT_MUTE			0x26
+#define  NTP8835_SOFT_MUTE_SM1			BIT(0)
+#define  NTP8835_SOFT_MUTE_SM2			BIT(1)
+#define  NTP8835_SOFT_MUTE_SM3			BIT(2)
+#define NTP8835_PWM_SWITCH			0x27
+#define  NTP8835_PWM_SWITCH_POF1		BIT(0)
+#define  NTP8835_PWM_SWITCH_POF2		BIT(1)
+#define  NTP8835_PWM_SWITCH_POF3		BIT(2)
+#define NTP8835_PWM_MASK_CTRL0			0x28
+#define  NTP8835_PWM_MASK_CTRL0_OUT_LOW		BIT(1)
+#define  NTP8835_PWM_MASK_CTRL0_FPMLD		BIT(2)
+#define NTP8835_MASTER_VOL			0x2e
+#define NTP8835_CHNL_A_VOL			0x2f
+#define NTP8835_CHNL_B_VOL			0x30
+#define NTP8835_CHNL_C_VOL			0x31
+#define REG_MAX					NTP8835_CHNL_C_VOL
+
+#define NTP8835_FW_NAME				"eq_8835.bin"
+#define NTP8835_FW_MAGIC			0x38383335	/* "8835" */
+
+struct ntp8835_priv {
+	struct i2c_client *i2c;
+	struct reset_control *reset;
+	unsigned int format;
+};
+
+static const DECLARE_TLV_DB_RANGE(ntp8835_vol_scale,
+	0, 1, TLV_DB_SCALE_ITEM(-15000, 0, 0),
+	2, 6, TLV_DB_SCALE_ITEM(-15000, 1000, 0),
+	7, 0xff, TLV_DB_SCALE_ITEM(-10000, 50, 0),
+);
+
+static int ntp8835_mute_info(struct snd_kcontrol *kcontrol,
+			     struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
+	uinfo->count = 1;
+
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 1;
+	uinfo->value.integer.step = 1;
+
+	return 0;
+}
+
+static int ntp8835_mute_get(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	unsigned int val;
+
+	val = snd_soc_component_read(component, NTP8835_SOFT_MUTE);
+
+	ucontrol->value.integer.value[0] = val ? 0 : 1;
+	return 0;
+}
+
+static int ntp8835_mute_put(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	unsigned int val;
+
+	val = ucontrol->value.integer.value[0] ? 0 : 7;
+
+	snd_soc_component_write(component, NTP8835_SOFT_MUTE, val);
+
+	return 0;
+}
+
+static const struct snd_kcontrol_new ntp8835_vol_control[] = {
+	SOC_SINGLE_TLV("Playback Volume", NTP8835_MASTER_VOL, 0,
+		       0xff, 0, ntp8835_vol_scale),
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "Playback Switch",
+		.info = ntp8835_mute_info,
+		.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_READWRITE,
+		.get = ntp8835_mute_get,
+		.put = ntp8835_mute_put,
+	},
+};
+
+static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
+{
+	if (active) {
+		/*
+		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
+		 * Deassert for T2 >= 1ms...
+		 */
+		reset_control_deassert(ntp8835->reset);
+		fsleep(1000);
+
+		/* ...Assert for T3 >= 0.1us... */
+		reset_control_assert(ntp8835->reset);
+		fsleep(1);
+
+		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
+		reset_control_deassert(ntp8835->reset);
+		fsleep(500);
+	} else {
+		reset_control_assert(ntp8835->reset);
+	}
+}
+
+static const struct reg_sequence ntp8835_sound_on[] = {
+	{ NTP8835_PWM_MASK_CTRL0,	NTP8835_PWM_MASK_CTRL0_FPMLD },
+	{ NTP8835_PWM_SWITCH,		0x00 },
+	{ NTP8835_SOFT_MUTE,		0x00 },
+};
+
+static const struct reg_sequence ntp8835_sound_off[] = {
+	{ NTP8835_SOFT_MUTE,		NTP8835_SOFT_MUTE_SM1 |
+					NTP8835_SOFT_MUTE_SM2 |
+					NTP8835_SOFT_MUTE_SM3 },
+
+	{ NTP8835_PWM_SWITCH,		NTP8835_PWM_SWITCH_POF1 |
+					NTP8835_PWM_SWITCH_POF2 |
+					NTP8835_PWM_SWITCH_POF3 },
+
+	{ NTP8835_PWM_MASK_CTRL0,	NTP8835_PWM_MASK_CTRL0_OUT_LOW |
+					NTP8835_PWM_MASK_CTRL0_FPMLD },
+};
+
+static int ntp8835_load_firmware(struct ntp8835_priv *ntp8835)
+{
+	int ret;
+
+	ret = ntpfw_load(ntp8835->i2c, NTP8835_FW_NAME, NTP8835_FW_MAGIC);
+	if (ret == -ENOENT) {
+		dev_warn_once(&ntp8835->i2c->dev,
+			      "Could not find firmware %s\n", NTP8835_FW_NAME);
+		return 0;
+	}
+
+	return ret;
+}
+
+static int ntp8835_snd_suspend(struct snd_soc_component *component)
+{
+	struct ntp8835_priv *ntp8835 = snd_soc_component_get_drvdata(component);
+
+	regcache_cache_only(component->regmap, true);
+
+	regmap_multi_reg_write_bypassed(component->regmap,
+					ntp8835_sound_off,
+					ARRAY_SIZE(ntp8835_sound_off));
+
+	/*
+	 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
+	 * wait after sound off for T6 >= 0.5ms
+	 */
+	fsleep(500);
+	ntp8835_reset_gpio(ntp8835, false);
+
+	regcache_mark_dirty(component->regmap);
+
+	return 0;
+}
+
+static int ntp8835_snd_resume(struct snd_soc_component *component)
+{
+	struct ntp8835_priv *ntp8835 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	ntp8835_reset_gpio(ntp8835, true);
+
+	regmap_multi_reg_write_bypassed(component->regmap,
+					ntp8835_sound_on,
+					ARRAY_SIZE(ntp8835_sound_on));
+
+	ret = ntp8835_load_firmware(ntp8835);
+	if (ret) {
+		dev_err(&ntp8835->i2c->dev, "Failed to load firmware\n");
+		return ret;
+	}
+
+	regcache_cache_only(component->regmap, false);
+	snd_soc_component_cache_sync(component);
+
+	return 0;
+}
+
+static int ntp8835_probe(struct snd_soc_component *component)
+{
+	int ret;
+	struct ntp8835_priv *ntp8835 = snd_soc_component_get_drvdata(component);
+	struct device *dev = component->dev;
+
+	ret = snd_soc_add_component_controls(component, ntp8835_vol_control,
+					     ARRAY_SIZE(ntp8835_vol_control));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add controls\n");
+
+	ret = ntp8835_load_firmware(ntp8835);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to load firmware\n");
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget ntp8835_dapm_widgets[] = {
+	SND_SOC_DAPM_DAC("AIFIN", "Playback", SND_SOC_NOPM, 0, 0),
+
+	SND_SOC_DAPM_OUTPUT("OUT1"),
+	SND_SOC_DAPM_OUTPUT("OUT2"),
+	SND_SOC_DAPM_OUTPUT("OUT3"),
+};
+
+static const struct snd_soc_dapm_route ntp8835_dapm_routes[] = {
+	{ "OUT1", NULL, "AIFIN" },
+	{ "OUT2", NULL, "AIFIN" },
+	{ "OUT3", NULL, "AIFIN" },
+};
+
+static const struct snd_soc_component_driver soc_component_ntp8835 = {
+	.probe = ntp8835_probe,
+	.suspend = ntp8835_snd_suspend,
+	.resume = ntp8835_snd_resume,
+	.dapm_widgets = ntp8835_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(ntp8835_dapm_widgets),
+	.dapm_routes = ntp8835_dapm_routes,
+	.num_dapm_routes = ARRAY_SIZE(ntp8835_dapm_routes),
+};
+
+static int ntp8835_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 ntp8835_priv *ntp8835 = snd_soc_component_get_drvdata(component);
+	unsigned int input_fmt = 0;
+	unsigned int gsa_fmt = 0;
+	unsigned int gsa_fmt_mask;
+	int ret;
+
+	switch (ntp8835->format) {
+	case SND_SOC_DAIFMT_I2S:
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		input_fmt |= NTP8835_INPUT_FMT_GSA_MODE;
+		gsa_fmt |= NTP8835_GSA_RIGHT_J;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		input_fmt |= NTP8835_INPUT_FMT_GSA_MODE;
+		break;
+	}
+
+	ret = snd_soc_component_update_bits(component, NTP8835_INPUT_FMT,
+					    NTP8835_INPUT_FMT_MASTER_MODE |
+					    NTP8835_INPUT_FMT_GSA_MODE,
+					    input_fmt);
+
+	if (!(input_fmt & NTP8835_INPUT_FMT_GSA_MODE) || ret < 0)
+		return ret;
+
+	switch (params_width(params)) {
+	case 24:
+		gsa_fmt |= NTP8835_GSA_BS(0);
+		break;
+	case 20:
+		gsa_fmt |= NTP8835_GSA_BS(1);
+		break;
+	case 18:
+		gsa_fmt |= NTP8835_GSA_BS(2);
+		break;
+	case 16:
+		gsa_fmt |= NTP8835_GSA_BS(3);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	gsa_fmt_mask = NTP8835_GSA_BS_MASK |
+		       NTP8835_GSA_RIGHT_J |
+		       NTP8835_GSA_LSB;
+	return snd_soc_component_update_bits(component, NTP8835_GSA_FMT,
+					     gsa_fmt_mask, gsa_fmt);
+}
+
+static int ntp8835_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = dai->component;
+	struct ntp8835_priv *ntp8835 = snd_soc_component_get_drvdata(component);
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+	case SND_SOC_DAIFMT_RIGHT_J:
+	case SND_SOC_DAIFMT_LEFT_J:
+		ntp8835->format = fmt & SND_SOC_DAIFMT_FORMAT_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+};
+
+static const struct snd_soc_dai_ops ntp8835_dai_ops = {
+	.hw_params = ntp8835_hw_params,
+	.set_fmt = ntp8835_set_fmt,
+};
+
+static struct snd_soc_dai_driver ntp8835_dai = {
+	.name = "ntp8835-amplifier",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 3,
+		.rates = SNDRV_PCM_RATE_8000_192000,
+		.formats = NTP8835_FORMATS,
+	},
+	.ops = &ntp8835_dai_ops,
+};
+
+static struct regmap_config ntp8835_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_MAX,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int ntp8835_i2c_probe(struct i2c_client *i2c)
+{
+	struct ntp8835_priv *ntp8835;
+	struct regmap *regmap;
+	int ret;
+
+	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);
+	if (!ntp8835)
+		return -ENOMEM;
+
+	ntp8835->i2c = i2c;
+
+	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);
+	if (IS_ERR(ntp8835->reset))
+		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
+				     "Failed to get reset\n");
+
+	ret = reset_control_deassert(ntp8835->reset);
+	if (ret)
+		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
+				     "Failed to deassert reset\n");
+
+	dev_set_drvdata(&i2c->dev, ntp8835);
+
+	ntp8835_reset_gpio(ntp8835, true);
+
+	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
+				     "Failed to allocate regmap\n");
+
+	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
+					      &ntp8835_dai, 1);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to register component\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id ntp8835_i2c_id[] = {
+	{ "ntp8835", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
+
+static const struct of_device_id ntp8835_of_match[] = {
+	{.compatible = "neofidelity,ntp8835",},
+	{.compatible = "neofidelity,ntp8835c",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ntp8835_of_match);
+
+static struct i2c_driver ntp8835_i2c_driver = {
+	.probe = ntp8835_i2c_probe,
+	.id_table = ntp8835_i2c_id,
+	.driver = {
+		.name = "NTP8835",
+		.of_match_table = ntp8835_of_match,
+	},
+};
+module_i2c_driver(ntp8835_i2c_driver);
+
+MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
+MODULE_DESCRIPTION("NTP8835 Audio Amplifier Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 3/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8918

[Rob Herring (Arm)]

On Tue, 09 Jul 2024 20:28:31 +0300, Igor Prusov wrote:
> Add dt-bindings for NeoFidelity NTP8918 Amplifier
> 
> Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> ---
>  .../bindings/sound/neofidelity,ntp8918.yaml   | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml: properties:reg: 'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
	hint: Scalar and array keywords cannot be mixed
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml: properties:reg: 'anyOf' conditional failed, one must be fixed:
	'enum' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	Additional properties are not allowed ('enum' was unexpected)
		hint: Arrays must be described with a combination of minItems/maxItems/items
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of minItems/maxItems/items
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.example.dtb: ntp8918@54: reg:0:0: 84 is not one of [42, 43, 44, 45]
	from schema $id: http://devicetree.org/schemas/sound/neofidelity,ntp8918.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8918.example.dtb: ntp8918@54: Unevaluated properties are not allowed ('reg' was unexpected)
	from schema $id: http://devicetree.org/schemas/sound/neofidelity,ntp8918.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240709172834.9785-4-ivprusov@salutedevices.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 5/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835

[Rob Herring (Arm)]

On Tue, 09 Jul 2024 20:28:33 +0300, Igor Prusov wrote:
> Add dt-bindings for NeoFidelity NTP8835C/NTP8835C Amplifiers
> 
> Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> ---
>  .../bindings/sound/neofidelity,ntp8835.yaml   | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml: properties:reg: 'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
	hint: Scalar and array keywords cannot be mixed
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.yaml: properties:reg: 'anyOf' conditional failed, one must be fixed:
	'enum' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	Additional properties are not allowed ('enum' was unexpected)
		hint: Arrays must be described with a combination of minItems/maxItems/items
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of minItems/maxItems/items
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.example.dtb: ntp8835@56: reg:0:0: 86 is not one of [42, 43, 44, 45]
	from schema $id: http://devicetree.org/schemas/sound/neofidelity,ntp8835.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/neofidelity,ntp8835.example.dtb: ntp8835@56: Unevaluated properties are not allowed ('reg' was unexpected)
	from schema $id: http://devicetree.org/schemas/sound/neofidelity,ntp8835.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240709172834.9785-6-ivprusov@salutedevices.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/6] dt-bindings: vendor-prefixes: Add NeoFidelity, Inc

[Krzysztof Kozlowski]

On 09/07/2024 19:28, Igor Prusov wrote:
> Add vendor prefix for NeoFidelity, Inc
> 
> Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>

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

Best regards,
Krzysztof

Re: [PATCH 3/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8918

[Krzysztof Kozlowski]

On 09/07/2024 19:28, Igor Prusov wrote:
> Add dt-bindings for NeoFidelity NTP8918 Amplifier
> 
> Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> ---
>  .../bindings/sound/neofidelity,ntp8918.yaml   | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/neofidelity,ntp8918.yaml
> 

This looks like not tested, so limited review follows.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> +> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The NTP8918 is a single chip full digital audio amplifier
> +  including power stage for stereo amplifier system.
> +  The NTP8918 is integrated with versatile digital audio signal
> +  processing functions, high-performance, high-fidelity fully
> +  digital PWM modulator and two high-power full-bridge MOSFET
> +  power stages.
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - neofidelity,ntp8918
> +
> +  reg:
> +    maxItems: 1
> +    enum:
> +      - 0x2a
> +      - 0x2b
> +      - 0x2c
> +      - 0x2d
> +    description: |
> +      I2C address of the device.

Drop description, redundant.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO used to control the state of the device.

Drop description, redundant.

> +
> +  '#sound-dai-cells':
> +    enum: [0]

That's a const

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +   #include <dt-bindings/gpio/gpio.h>
> +   i2c {
> +     #address-cells = <1>;
> +     #size-cells = <0>;
> +     ntp8918@54 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



Best regards,
Krzysztof

Re: [PATCH 5/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835

[Krzysztof Kozlowski]

On 09/07/2024 19:28, Igor Prusov wrote:
> Add dt-bindings for NeoFidelity NTP8835C/NTP8835C Amplifiers
> 
> Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> ---
>  .../bindings/sound/neofidelity,ntp8835.yaml   | 65 +++++++++++++++++++

No need for new schema. Just put it - after testing - into previous
bindings file.

Best regards,
Krzysztof

Re: [PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Krzysztof Kozlowski]

On 09/07/2024 19:28, Igor Prusov wrote:
> The NeoFidelity NTP8835 adn NTP8835C are 2.1 channel amplifiers with
> mixer and biquad filters. Both amplifiers have identical programming
> interfaces but differ in output signal characteristics.
> 


> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>

Where do you use moduleparam?

> +#include <linux/bits.h>
> +#include <linux/gpio.h>

And gpio?

> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>

And this?

Please clean up the driver first.

> +#include <linux/reset.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#include <sound/initval.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/soc-component.h>
> +#include <sound/tlv.h>
> +
> +#include "ntpfw.h"
> +
> +#define NTP8835_FORMATS     (SNDRV_PCM_FMTBIT_S16_LE | \
> +			     SNDRV_PCM_FMTBIT_S20_3LE | \
> +			     SNDRV_PCM_FMTBIT_S24_LE | \
> +			     SNDRV_PCM_FMTBIT_S32_LE)
> +
> +#define NTP8835_INPUT_FMT			0x0
> +#define  NTP8835_INPUT_FMT_MASTER_MODE		BIT(0)
> +#define  NTP8835_INPUT_FMT_GSA_MODE		BIT(1)
> +#define NTP8835_GSA_FMT				0x1
> +#define  NTP8835_GSA_BS_MASK			GENMASK(3, 2)
> +#define  NTP8835_GSA_BS(x)			((x) << 2)
> +#define  NTP8835_GSA_RIGHT_J			BIT(0)
> +#define  NTP8835_GSA_LSB			BIT(1)
> +#define NTP8835_SOFT_MUTE			0x26
> +#define  NTP8835_SOFT_MUTE_SM1			BIT(0)
> +#define  NTP8835_SOFT_MUTE_SM2			BIT(1)
> +#define  NTP8835_SOFT_MUTE_SM3			BIT(2)
> +#define NTP8835_PWM_SWITCH			0x27
> +#define  NTP8835_PWM_SWITCH_POF1		BIT(0)
> +#define  NTP8835_PWM_SWITCH_POF2		BIT(1)
> +#define  NTP8835_PWM_SWITCH_POF3		BIT(2)
> +#define NTP8835_PWM_MASK_CTRL0			0x28
> +#define  NTP8835_PWM_MASK_CTRL0_OUT_LOW		BIT(1)
> +#define  NTP8835_PWM_MASK_CTRL0_FPMLD		BIT(2)
> +#define NTP8835_MASTER_VOL			0x2e
> +#define NTP8835_CHNL_A_VOL			0x2f
> +#define NTP8835_CHNL_B_VOL			0x30
> +#define NTP8835_CHNL_C_VOL			0x31
> +#define REG_MAX					NTP8835_CHNL_C_VOL
> +
> +#define NTP8835_FW_NAME				"eq_8835.bin"
> +#define NTP8835_FW_MAGIC			0x38383335	/* "8835" */
> +


...


> +
> +static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
> +{
> +	if (active) {
> +		/*
> +		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
> +		 * Deassert for T2 >= 1ms...
> +		 */
> +		reset_control_deassert(ntp8835->reset);

Explain in comment why do you need to power up device to perform
reset... This sounds odd.

> +		fsleep(1000);
> +
> +		/* ...Assert for T3 >= 0.1us... */
> +		reset_control_assert(ntp8835->reset);
> +		fsleep(1);
> +
> +		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
> +		reset_control_deassert(ntp8835->reset);
> +		fsleep(500);
> +	} else {
> +		reset_control_assert(ntp8835->reset);

This function is confusing. It is supposed to perform reset and leave
the device in active state, but here it leaves the device in reset.



> +
> +static struct snd_soc_dai_driver ntp8835_dai = {

Not const?

> +	.name = "ntp8835-amplifier",
> +	.playback = {
> +		.stream_name = "Playback",
> +		.channels_min = 1,
> +		.channels_max = 3,
> +		.rates = SNDRV_PCM_RATE_8000_192000,
> +		.formats = NTP8835_FORMATS,
> +	},
> +	.ops = &ntp8835_dai_ops,
> +};
> +
> +static struct regmap_config ntp8835_regmap = {

Not const?

Judging by weird includes and such simple issues, it looks like you try
to upstream downstream or old code. That's not how you are supposed to
bring new devices. You expect us to perform review on the same issues we
fixed already. Work on newest drivers - take them as template - so you
will not repeat the same issues we already fixed.

> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_MAX,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +static int ntp8835_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct ntp8835_priv *ntp8835;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);

sizeof(*)

> +	if (!ntp8835)
> +		return -ENOMEM;
> +
> +	ntp8835->i2c = i2c;
> +
> +	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);

shared is on purpose?

> +	if (IS_ERR(ntp8835->reset))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> +				     "Failed to get reset\n");
> +
> +	ret = reset_control_deassert(ntp8835->reset);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> +				     "Failed to deassert reset\n");
> +
> +	dev_set_drvdata(&i2c->dev, ntp8835);
> +
> +	ntp8835_reset_gpio(ntp8835, true);
> +
> +	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> +				     "Failed to allocate regmap\n");
> +
> +	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
> +					      &ntp8835_dai, 1);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to register component\n");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ntp8835_i2c_id[] = {
> +	{ "ntp8835", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
> +
> +static const struct of_device_id ntp8835_of_match[] = {
> +	{.compatible = "neofidelity,ntp8835",},
> +	{.compatible = "neofidelity,ntp8835c",},

This does not match your i2c IDs, which leads to troubles when matching
variants.

Anyway, aren't they compatible?


> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ntp8835_of_match);
> +
> +static struct i2c_driver ntp8835_i2c_driver = {
> +	.probe = ntp8835_i2c_probe,
> +	.id_table = ntp8835_i2c_id,
> +	.driver = {
> +		.name = "NTP8835",

Driver names are lowercase

> +		.of_match_table = ntp8835_of_match,
> +	},
> +};
> +module_i2c_driver(ntp8835_i2c_driver);
> +
> +MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
> +MODULE_DESCRIPTION("NTP8835 Audio Amplifier Driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Re: [PATCH 4/6] ASoC: codecs: Add NeoFidelity NTP8918 codec

[kernel test robot]

Hi Igor,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on robh/for-next linus/master v6.10-rc7 next-20240710]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Igor-Prusov/dt-bindings-vendor-prefixes-Add-NeoFidelity-Inc/20240710-145725
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20240709172834.9785-5-ivprusov%40salutedevices.com
patch subject: [PATCH 4/6] ASoC: codecs: Add NeoFidelity NTP8918 codec
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240711/202407111044.RZFnMlVh-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240711/202407111044.RZFnMlVh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407111044.RZFnMlVh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/soc/codecs/ntpfw.c:14: warning: cannot understand function prototype: 'struct ntpfw_chunk '


vim +14 sound/soc/codecs/ntpfw.c

fa3c817751bfd3 Igor Prusov 2024-07-09  13  
fa3c817751bfd3 Igor Prusov 2024-07-09 @14  struct ntpfw_chunk {
fa3c817751bfd3 Igor Prusov 2024-07-09  15  	__be16 length;
fa3c817751bfd3 Igor Prusov 2024-07-09  16  	u8 step;
fa3c817751bfd3 Igor Prusov 2024-07-09  17  	u8 data[];
fa3c817751bfd3 Igor Prusov 2024-07-09  18  } __packed;
fa3c817751bfd3 Igor Prusov 2024-07-09  19  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/6] ASoC: codecs: Add NeoFidelity NTP8918 codec

[kernel test robot]

Hi Igor,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on robh/for-next linus/master v6.10-rc7 next-20240712]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Igor-Prusov/dt-bindings-vendor-prefixes-Add-NeoFidelity-Inc/20240710-145725
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20240709172834.9785-5-ivprusov%40salutedevices.com
patch subject: [PATCH 4/6] ASoC: codecs: Add NeoFidelity NTP8918 codec
config: sh-randconfig-r111-20240712 (https://download.01.org/0day-ci/archive/20240713/202407130520.NOxTmD3N-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240713/202407130520.NOxTmD3N-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407130520.NOxTmD3N-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> sound/soc/codecs/ntp8918.c:274:24: sparse: sparse: symbol 'ntp8918_dai_ops' was not declared. Should it be static?

vim +/ntp8918_dai_ops +274 sound/soc/codecs/ntp8918.c

   273	
 > 274	struct snd_soc_dai_ops ntp8918_dai_ops = {
   275		.hw_params = ntp8918_hw_params,
   276		.set_fmt = ntp8918_set_fmt,
   277	};
   278	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Igor Prusov]

Hello Krzysztof,

Thank you for review and apologies for late response. I'm about to send
next version with all comments addressed, just wanted to ask some
questions before.

On Wed, Jul 10, 2024 at 12:31:53PM +0200, Krzysztof Kozlowski wrote:
> On 09/07/2024 19:28, Igor Prusov wrote:
> > The NeoFidelity NTP8835 adn NTP8835C are 2.1 channel amplifiers with
> > mixer and biquad filters. Both amplifiers have identical programming
> > interfaces but differ in output signal characteristics.
> > 
> 
> 
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> 
> Where do you use moduleparam?
> 
> > +#include <linux/bits.h>
> > +#include <linux/gpio.h>
> 
> And gpio?
> 
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> 
> And this?
> 
> Please clean up the driver first.
> 
> > +#include <linux/reset.h>
> > +#include <linux/init.h>
> > +#include <linux/i2c.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <sound/initval.h>
> > +#include <sound/core.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <sound/soc-component.h>
> > +#include <sound/tlv.h>
> > +
> > +#include "ntpfw.h"
> > +
> > +#define NTP8835_FORMATS     (SNDRV_PCM_FMTBIT_S16_LE | \
> > +			     SNDRV_PCM_FMTBIT_S20_3LE | \
> > +			     SNDRV_PCM_FMTBIT_S24_LE | \
> > +			     SNDRV_PCM_FMTBIT_S32_LE)
> > +
> > +#define NTP8835_INPUT_FMT			0x0
> > +#define  NTP8835_INPUT_FMT_MASTER_MODE		BIT(0)
> > +#define  NTP8835_INPUT_FMT_GSA_MODE		BIT(1)
> > +#define NTP8835_GSA_FMT				0x1
> > +#define  NTP8835_GSA_BS_MASK			GENMASK(3, 2)
> > +#define  NTP8835_GSA_BS(x)			((x) << 2)
> > +#define  NTP8835_GSA_RIGHT_J			BIT(0)
> > +#define  NTP8835_GSA_LSB			BIT(1)
> > +#define NTP8835_SOFT_MUTE			0x26
> > +#define  NTP8835_SOFT_MUTE_SM1			BIT(0)
> > +#define  NTP8835_SOFT_MUTE_SM2			BIT(1)
> > +#define  NTP8835_SOFT_MUTE_SM3			BIT(2)
> > +#define NTP8835_PWM_SWITCH			0x27
> > +#define  NTP8835_PWM_SWITCH_POF1		BIT(0)
> > +#define  NTP8835_PWM_SWITCH_POF2		BIT(1)
> > +#define  NTP8835_PWM_SWITCH_POF3		BIT(2)
> > +#define NTP8835_PWM_MASK_CTRL0			0x28
> > +#define  NTP8835_PWM_MASK_CTRL0_OUT_LOW		BIT(1)
> > +#define  NTP8835_PWM_MASK_CTRL0_FPMLD		BIT(2)
> > +#define NTP8835_MASTER_VOL			0x2e
> > +#define NTP8835_CHNL_A_VOL			0x2f
> > +#define NTP8835_CHNL_B_VOL			0x30
> > +#define NTP8835_CHNL_C_VOL			0x31
> > +#define REG_MAX					NTP8835_CHNL_C_VOL
> > +
> > +#define NTP8835_FW_NAME				"eq_8835.bin"
> > +#define NTP8835_FW_MAGIC			0x38383335	/* "8835" */
> > +
> 
> 
> ...
> 
> 
> > +
> > +static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
> > +{
> > +	if (active) {
> > +		/*
> > +		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
> > +		 * Deassert for T2 >= 1ms...
> > +		 */
> > +		reset_control_deassert(ntp8835->reset);
> 
> Explain in comment why do you need to power up device to perform
> reset... This sounds odd.
> 

This sequence comes from device datasheet, for some reason vendor
recommends to drive /RESET low for 0.1us during initialization.
Datasheet also describes (section 6.3) init sequence with simple reset
deassert, but it's called legacy, though it works fine on my board. Do
you mean to add more verbose comment than linking to a datasheet?

> > +		fsleep(1000);
> > +
> > +		/* ...Assert for T3 >= 0.1us... */
> > +		reset_control_assert(ntp8835->reset);
> > +		fsleep(1);
> > +
> > +		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
> > +		reset_control_deassert(ntp8835->reset);
> > +		fsleep(500);
> > +	} else {
> > +		reset_control_assert(ntp8835->reset);
> 
> This function is confusing. It is supposed to perform reset and leave
> the device in active state, but here it leaves the device in reset.
> 
> 
> 
> > +
> > +static struct snd_soc_dai_driver ntp8835_dai = {
> 
> Not const?
> 

ntp8835_dai is passed to devm_snd_soc_register_component(), which takes
non-const parameter.

> > +	.name = "ntp8835-amplifier",
> > +	.playback = {
> > +		.stream_name = "Playback",
> > +		.channels_min = 1,
> > +		.channels_max = 3,
> > +		.rates = SNDRV_PCM_RATE_8000_192000,
> > +		.formats = NTP8835_FORMATS,
> > +	},
> > +	.ops = &ntp8835_dai_ops,
> > +};
> > +
> > +static struct regmap_config ntp8835_regmap = {
> 
> Not const?
> 
> Judging by weird includes and such simple issues, it looks like you try
> to upstream downstream or old code. That's not how you are supposed to
> bring new devices. You expect us to perform review on the same issues we
> fixed already. Work on newest drivers - take them as template - so you
> will not repeat the same issues we already fixed.
> 
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = REG_MAX,
> > +	.cache_type = REGCACHE_MAPLE,
> > +};
> > +
> > +static int ntp8835_i2c_probe(struct i2c_client *i2c)
> > +{
> > +	struct ntp8835_priv *ntp8835;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);
> 
> sizeof(*)
> 
> > +	if (!ntp8835)
> > +		return -ENOMEM;
> > +
> > +	ntp8835->i2c = i2c;
> > +
> > +	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);
> 
> shared is on purpose?
> 

Yes, we have a board with two amplifiers sharing same reset line, so
shared allows to work around this hardware issue. Is it the wrong
approach?

> > +	if (IS_ERR(ntp8835->reset))
> > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > +				     "Failed to get reset\n");
> > +
> > +	ret = reset_control_deassert(ntp8835->reset);
> > +	if (ret)
> > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > +				     "Failed to deassert reset\n");
> > +
> > +	dev_set_drvdata(&i2c->dev, ntp8835);
> > +
> > +	ntp8835_reset_gpio(ntp8835, true);
> > +
> > +	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > +				     "Failed to allocate regmap\n");
> > +
> > +	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
> > +					      &ntp8835_dai, 1);
> > +	if (ret)
> > +		return dev_err_probe(&i2c->dev, ret,
> > +				     "Failed to register component\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ntp8835_i2c_id[] = {
> > +	{ "ntp8835", 0 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
> > +
> > +static const struct of_device_id ntp8835_of_match[] = {
> > +	{.compatible = "neofidelity,ntp8835",},
> > +	{.compatible = "neofidelity,ntp8835c",},
> 
> This does not match your i2c IDs, which leads to troubles when matching
> variants.
> 
> Anyway, aren't they compatible?
> 
> 

They have identical programming interface and only differ in some output
signal characteristics. Is it OK use single compatible string in such
case?

> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, ntp8835_of_match);
> > +
> > +static struct i2c_driver ntp8835_i2c_driver = {
> > +	.probe = ntp8835_i2c_probe,
> > +	.id_table = ntp8835_i2c_id,
> > +	.driver = {
> > +		.name = "NTP8835",
> 
> Driver names are lowercase
> 
> > +		.of_match_table = ntp8835_of_match,
> > +	},
> > +};
> > +module_i2c_driver(ntp8835_i2c_driver);
> > +
> > +MODULE_AUTHOR("Igor Prusov <ivprusov@salutedevices.com>");
> > +MODULE_DESCRIPTION("NTP8835 Audio Amplifier Driver");
> > +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof
> 

-- 
Best Regards,
Igor Prusov

Re: [PATCH 5/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835

[Igor Prusov]

On Wed, Jul 10, 2024 at 12:24:33PM +0200, Krzysztof Kozlowski wrote:
> On 09/07/2024 19:28, Igor Prusov wrote:
> > Add dt-bindings for NeoFidelity NTP8835C/NTP8835C Amplifiers
> > 
> > Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> > ---
> >  .../bindings/sound/neofidelity,ntp8835.yaml   | 65 +++++++++++++++++++
> 
> No need for new schema. Just put it - after testing - into previous
> bindings file.

I am going to add some clocks in next version and there are some
differences between amplifiers. 8835 uses separate master clock, 8918 is
clocked by BCLK. Is it still better to use same schema with anyOf, or
keep them in different files?

> 
> Best regards,
> Krzysztof
> 

-- 
Best Regards,
Igor Prusov

Re: [PATCH 5/6] ASoC: dt-bindings: Add bindings for NeoFidelity NTP8835

[Krzysztof Kozlowski]

On Fri, Sep 20, 2024 at 08:42:33PM +0300, Igor Prusov wrote:
> On Wed, Jul 10, 2024 at 12:24:33PM +0200, Krzysztof Kozlowski wrote:
> > On 09/07/2024 19:28, Igor Prusov wrote:
> > > Add dt-bindings for NeoFidelity NTP8835C/NTP8835C Amplifiers
> > > 
> > > Signed-off-by: Igor Prusov <ivprusov@salutedevices.com>
> > > ---
> > >  .../bindings/sound/neofidelity,ntp8835.yaml   | 65 +++++++++++++++++++
> > 
> > No need for new schema. Just put it - after testing - into previous
> > bindings file.
> 
> I am going to add some clocks in next version and there are some
> differences between amplifiers. 8835 uses separate master clock, 8918 is
> clocked by BCLK. Is it still better to use same schema with anyOf, or
> keep them in different files?

No clue, that was 2 months ago so I don't have that email in my inbox at
all.

Please post complete bindings (regardless whether driver implement it or
not), so we can see the differences.

Best regards,
Krzysztof

Re: [PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Krzysztof Kozlowski]

On Fri, Sep 20, 2024 at 08:33:12PM +0300, Igor Prusov wrote:
 > 
> > 
> > > +
> > > +static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
> > > +{
> > > +	if (active) {
> > > +		/*
> > > +		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
> > > +		 * Deassert for T2 >= 1ms...
> > > +		 */
> > > +		reset_control_deassert(ntp8835->reset);
> > 
> > Explain in comment why do you need to power up device to perform
> > reset... This sounds odd.
> > 
> 
> This sequence comes from device datasheet, for some reason vendor
> recommends to drive /RESET low for 0.1us during initialization.
> Datasheet also describes (section 6.3) init sequence with simple reset
> deassert, but it's called legacy, though it works fine on my board. Do
> you mean to add more verbose comment than linking to a datasheet?

I think verbose comment would be better.

> 
> > > +		fsleep(1000);
> > > +
> > > +		/* ...Assert for T3 >= 0.1us... */
> > > +		reset_control_assert(ntp8835->reset);
> > > +		fsleep(1);
> > > +
> > > +		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
> > > +		reset_control_deassert(ntp8835->reset);
> > > +		fsleep(500);
> > > +	} else {
> > > +		reset_control_assert(ntp8835->reset);
> > 
> > This function is confusing. It is supposed to perform reset and leave
> > the device in active state, but here it leaves the device in reset.
> > 
> > 
> > 
> > > +
> > > +static struct snd_soc_dai_driver ntp8835_dai = {
> > 
> > Not const?
> > 
> 
> ntp8835_dai is passed to devm_snd_soc_register_component(), which takes
> non-const parameter.

Right, indeed.

> 
> > > +	.name = "ntp8835-amplifier",
> > > +	.playback = {
> > > +		.stream_name = "Playback",
> > > +		.channels_min = 1,
> > > +		.channels_max = 3,
> > > +		.rates = SNDRV_PCM_RATE_8000_192000,
> > > +		.formats = NTP8835_FORMATS,
> > > +	},
> > > +	.ops = &ntp8835_dai_ops,
> > > +};
> > > +
> > > +static struct regmap_config ntp8835_regmap = {
> > 
> > Not const?
> > 
> > Judging by weird includes and such simple issues, it looks like you try
> > to upstream downstream or old code. That's not how you are supposed to
> > bring new devices. You expect us to perform review on the same issues we
> > fixed already. Work on newest drivers - take them as template - so you
> > will not repeat the same issues we already fixed.
> > 
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +	.max_register = REG_MAX,
> > > +	.cache_type = REGCACHE_MAPLE,
> > > +};
> > > +
> > > +static int ntp8835_i2c_probe(struct i2c_client *i2c)
> > > +{
> > > +	struct ntp8835_priv *ntp8835;
> > > +	struct regmap *regmap;
> > > +	int ret;
> > > +
> > > +	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);
> > 
> > sizeof(*)
> > 
> > > +	if (!ntp8835)
> > > +		return -ENOMEM;
> > > +
> > > +	ntp8835->i2c = i2c;
> > > +
> > > +	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);
> > 
> > shared is on purpose?
> > 
> 
> Yes, we have a board with two amplifiers sharing same reset line, so
> shared allows to work around this hardware issue. Is it the wrong
> approach?

No, it's ok, I just want to be sure you added this consciously.

> 
> > > +	if (IS_ERR(ntp8835->reset))
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > > +				     "Failed to get reset\n");
> > > +
> > > +	ret = reset_control_deassert(ntp8835->reset);
> > > +	if (ret)
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > > +				     "Failed to deassert reset\n");
> > > +
> > > +	dev_set_drvdata(&i2c->dev, ntp8835);
> > > +
> > > +	ntp8835_reset_gpio(ntp8835, true);
> > > +
> > > +	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
> > > +	if (IS_ERR(regmap))
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > > +				     "Failed to allocate regmap\n");
> > > +
> > > +	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
> > > +					      &ntp8835_dai, 1);
> > > +	if (ret)
> > > +		return dev_err_probe(&i2c->dev, ret,
> > > +				     "Failed to register component\n");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ntp8835_i2c_id[] = {
> > > +	{ "ntp8835", 0 },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
> > > +
> > > +static const struct of_device_id ntp8835_of_match[] = {
> > > +	{.compatible = "neofidelity,ntp8835",},
> > > +	{.compatible = "neofidelity,ntp8835c",},
> > 
> > This does not match your i2c IDs, which leads to troubles when matching
> > variants.
> > 
> > Anyway, aren't they compatible?
> > 
> > 
> 
> They have identical programming interface and only differ in some output
> signal characteristics. Is it OK use single compatible string in such
> case?

Driver should have one compatible (and one entry in i2c device ID). You
add the second in the bindings as one followed by fallback.
Like this:
https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31

Best regards,
Krzysztof